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

fix($location): hijack area links too #5933

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

gion
Copy link

@gion gion commented Jan 22, 2014

the $location service only hijacks anchor () links,
but there is one more element that has a href property and acts like a link:
the area child element () of an image map element ()

@gion
Copy link
Author

gion commented Jan 22, 2014

Hi,
In a recent project my team and I have stumbled across the famous ie 10 digest iteration loop error, but none of the posted issues, fixes or SO answers did the trick for us.
Our use case is somewhat different than all the other already spotted issues (we don't mess with the history pushState api by setting $locationProvider.html5Mode(true);):
On internet explorer 10 and 11, when you use both an image map's area elements and normal [anchor elements] as nav items, IE>=10 breaks (with this error message) and the view-route binding isn't updated: the url is changing in the browser, but the view isn't.
After digging into the raw code, we realized that using the <area href="#"></area> as a link was triggering these problems.
Please address this issue and provide any feedback on our pull request that fixes this issue.

@gion
Copy link
Author

gion commented Jan 22, 2014

We made 2 public plunkr links to reveal our problem and our fix:

Steps to reproduce this issue:

Here's a screenshot of the error:

angular-bug

@mary-poppins
Copy link

I'm sorry, but I wasn't able to verify your Contributor License Agreement (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 us 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.

@gion
Copy link
Author

gion commented Jan 22, 2014

Hi,
I've already singed it earlyer today, but I probably had my work email as the primary email for my account.
Could you please check again now?

@caitp
Copy link
Contributor

caitp commented Jan 22, 2014

It's a robot, @gion, don't worry about it =) So long as the email address matches the email you authored the commit as, should be fine

@gion
Copy link
Author

gion commented Jan 22, 2014

He he :) silly me

@caitp
Copy link
Contributor

caitp commented Jan 23, 2014

So I don't necessarily think this is a bad change, the fix looks pretty tiny, so we might be able to merge this.

I am not positive that the changes to ngScenario are totally necessary, as it's deprecated. Anyways, get this cleaned up and we can probably merge this into 1.2.10

@tbosch tbosch modified the milestones: 1.2.12, 1.2.11 Feb 3, 2014
@jeffbcross jeffbcross self-assigned this Feb 3, 2014
@jeffbcross
Copy link
Contributor

I'll review this for 1.2.12. I think we can remove some of the tests.

Can you make sure you signed the CLA with the email address contained in this patch? https://github.com/angular/angular.js/pull/5933.patch

@mary-poppins
Copy link

CLA signature verified! Thank you!

Someone from the team will now triage your PR and it will be processed based on the determined priority (doc updates and fixes with tests are prioritized over other changes).

@gion
Copy link
Author

gion commented Feb 4, 2014

Yeah, using another element as an anchor has duplicated some test scenarios.
I previously signed the agreement with my other email address, but all should be good now.

@jeffbcross jeffbcross removed their assignment Feb 6, 2014
@jeffbcross jeffbcross modified the milestones: Backlog, 1.2.12 Feb 6, 2014
@gion
Copy link
Author

gion commented Mar 3, 2014

@jeffbcross any updates on this thread?

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

1 similar comment
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

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.

8 participants