:Felipe Gomes (needinfo for replies!) Reporter
	Description • 12 years ago

Anchor navigation or history.pushState resets the click-to-play state because OnLocationChange gets called. That causes subsequent plugins added to the page to not be activated and the doorhanger to show again for the same page

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 1 • 12 years ago

This patch fixes the case where anchor navigation and history.push/pop/replaceState.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 2 • 12 years ago

Comment on attachment 617990 [details] [diff] [review]
Patch for bug

New patches on the way that will come with tests.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 3 • 12 years ago

Pushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=c7eb352d46c5

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 4 • 12 years ago

I haven't found a scenario where parsing the URL for location changes does a better job than checking for a null aRequest. Running the browser-chrome tests locally didn't show any issues with it, and the push to try should shake out anything potentially wrong with this change.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 5 • 12 years ago

Comment on attachment 620810 [details] [diff] [review]
Tests for patch

forgot to hg add

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 6 • 12 years ago

Repushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=745fe786c531

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 7 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c002e752d3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/375f54071008

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 8 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/0c002e752d3e
https://hg.mozilla.org/mozilla-central/rev/375f54071008

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 9 • 12 years ago

Comment on attachment 620811 [details] [diff] [review]
Patch for bug v2

It might be nice to add a comment explaining why you're null-checking aRequest (to filter out onLocationChanges triggered by anchor navigation, AIUI), since that isn't obvious from quickly glancing at the code.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 10 • 12 years ago

Comment on attachment 620811 [details] [diff] [review]
Patch for bug v2

[Approval Request Comment]
Regression caused by (bug #): bug 711552

User impact if declined: 
Clicking on an anchor link within the page or if the page uses history.pushState API then the state of click-to-play plugins is reset. This is not a super high priority to get on Aurora, but it would be a nice-to-have since the feature is being introduced (preffed-off) on Aurora.

Testing completed (on m-c, etc.): Baked on m-c for a couple days
Risk to taking this patch (and alternatives if risky): None expected, there are tests that accompany this patch.
String changes made by this patch: none

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 11 • 12 years ago

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> Comment on attachment 620811 [details] [diff] [review]
> Patch for bug v2
> 
> It might be nice to add a comment explaining why you're null-checking
> aRequest (to filter out onLocationChanges triggered by anchor navigation,
> AIUI), since that isn't obvious from quickly glancing at the code.

Thanks for the feedback Gavin. I've added a comment and pushed it to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/991d6c9dc2d9

	Ed Morley [:emorley]
	Comment 12 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/991d6c9dc2d9

	Alex Keybl [:akeybl]
	Comment 13 • 12 years ago

Comment on attachment 620811 [details] [diff] [review]
Patch for bug v2

[Triage Comment]
We don't typically take fixes for pref'd off features, but since we've widely communicated how to enable it and this shouldn't cause any new regressions, approving for Aurora 14.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 14 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/ffdecf936969
https://hg.mozilla.org/releases/mozilla-aurora/rev/2955020b4cea

	neil@parkwaycc.co.uk
	Comment 15 • 12 years ago

Comment on attachment 620811 [details] [diff] [review]
Patch for bug v2

>   onLocationChange: function (aBrowser, aWebProgress, aRequest, aLocationURI,
>                               aFlags) {
>     // Filter out any sub-frame loads
>     if (aBrowser.contentWindow == aWebProgress.DOMWindow) {
>+      if (aRequest) {
I'm not sure that checking aRequest was ever a good way to detect an anchor scroll but for your information bug 311007 added a flag to do it properly.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 16 • 12 years ago

Thanks Neil.