Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

ngAttr* directive issue with href attribute in IE #5479

Closed
wants to merge 2 commits into from
Closed

ngAttr* directive issue with href attribute in IE #5479

wants to merge 2 commits into from

Conversation

plalx
Copy link
Contributor

@plalx plalx commented Dec 19, 2013

This fixes an issue where any form of ng-attr-href directive wasn't correctly processed in IE.

I am not sure why there was the special IE condition for the href attribute but node.getAttribute(name, 2) doesn't make any sense since it's trying to fetch the binding value from the attribute we are actually trying to populate.

I have removed the special case handling and every unit tests are still passing.

Issue Plunkr

@IgorMinar
Copy link
Contributor

I'm sorry, but I wasn't able to verify your CLA signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let me know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

@petebacondarwin
Copy link
Member

From looking at the history there appears to have been an issue with an old IE (possibly IE7) regarding accessing the href attribute through the element.attributes collection. You had to get it via element.getAttribute instead and decode it, too.

Since we are no longer testing on IE7, this may be why the tests are no longer failing...

@petebacondarwin
Copy link
Member

See 259c2bb

@petebacondarwin
Copy link
Member

From http://msdn.microsoft.com/en-us/library/ie/ms536429(v=vs.85).aspx:

Internet Explorer 8 and later. In IE7 Standards mode and IE5 (Quirks) mode, attributes that represent URLs and file paths return the same value whether you retrieve them as a DOM property or as a content attribute (using getAttribute). Some attributes return relative URLs, whereas others always return a fully qualified URL. In IE8 mode, DOM attributes always return fully qualified URLs for URL attributes; to retrieve the attribute value as specified in the content, use getAttribute. The following attributes return URLs: action, background, BaseHref, cite, codeBase, data, dynsrc, href, longDesc, lowsrc, pluginspage, profile, src, url, and vrml.

So it seems that this is for IE8 support. Perhaps the check should be (msie<9)?

@petebacondarwin
Copy link
Member

Here is a fork that does this: http://plnkr.co/edit/xtAaQJYBFr7LDZad7onQ?p=preview

@ghost ghost assigned petebacondarwin Dec 19, 2013
@petebacondarwin
Copy link
Member

I can confirm that this works on IE11 and IE9. I'll knock up a branch and send it to the CI server for further testing.

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this pull request Dec 19, 2013
There is a special case for getting hold of the href value for old versions
of IE.  But it turns out that this actually breaks newer versions of IE.

Closes angular#5479
@petebacondarwin
Copy link
Member

Hmm. So the new spec fails on IE8 too! @plalx I think you are right. We should just drop the whole exceptional case!

@plalx plalx deleted the ngAttrHref_bugfix branch December 19, 2013 13:39
@plalx
Copy link
Contributor Author

plalx commented Dec 19, 2013

@IgorMinar I signed it for my next contribution ;)

@petebacondarwin Thanks for fixing. In theory it shouldn't have been much of a big concern since there's the ng-href directive which didin't suffer that issue. But now that we can bind any attributes using ngAttr* we may ask why we need these attribute-specific directives (other than backward compatibility). I haven't investigated but if there are other valid reasons like execution priority, perhaps a warning should be logged when using the generic ngAttr* form for these?

jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
It appears that this exceptional case was only valid for IE<8 and that for IE>=8 it
was actually causing a bug with the `ng-href-attr` directive on `<a>` elements.

Closes angular#5479
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
It appears that this exceptional case was only valid for IE<8 and that for IE>=8 it
was actually causing a bug with the `ng-href-attr` directive on `<a>` elements.

Closes angular#5479
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants