The default bug view has changed. See this FAQ.

Anchor navigation resets click-to-play state

RESOLVED FIXED in Firefox 14

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Felipe, Assigned: jaws)

Tracking

Trunk
Firefox 15
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox14 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 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
Created attachment 617990 [details] [diff] [review]
Patch for bug

This patch fixes the case where anchor navigation and history.push/pop/replaceState.
Assignee: nobody → jwein
Status: NEW → ASSIGNED
Attachment #617990 - Flags: review?(felipc)
(Reporter)

Updated

5 years ago
Attachment #617990 - Flags: review?(felipc) → review+
Comment on attachment 617990 [details] [diff] [review]
Patch for bug

New patches on the way that will come with tests.
Attachment #617990 - Flags: review+
Created attachment 620810 [details] [diff] [review]
Tests for patch

Pushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=c7eb352d46c5
Attachment #620810 - Flags: review?(felipc)
Created attachment 620811 [details] [diff] [review]
Patch for bug v2

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.
Attachment #617990 - Attachment is obsolete: true
Attachment #620811 - Flags: review?(felipc)
Comment on attachment 620810 [details] [diff] [review]
Tests for patch

forgot to hg add
Attachment #620810 - Flags: review?(felipc)
Created attachment 620835 [details] [diff] [review]
Tests for patch v2

Repushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=745fe786c531
Attachment #620810 - Attachment is obsolete: true
Attachment #620835 - Flags: review?
Attachment #620835 - Flags: review? → review?(felipc)
(Reporter)

Updated

5 years ago
Attachment #620811 - Flags: review?(felipc) → review+
(Reporter)

Updated

5 years ago
Attachment #620835 - Flags: review?(felipc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c002e752d3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/375f54071008
Flags: in-testsuite+
Target Milestone: --- → Firefox 15
https://hg.mozilla.org/mozilla-central/rev/0c002e752d3e
https://hg.mozilla.org/mozilla-central/rev/375f54071008
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.
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
Attachment #620811 - Flags: approval-mozilla-aurora?
(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
https://hg.mozilla.org/mozilla-central/rev/991d6c9dc2d9
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.
Attachment #620811 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/ffdecf936969
https://hg.mozilla.org/releases/mozilla-aurora/rev/2955020b4cea
status-firefox14: --- → fixed
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.
Thanks Neil.
Depends on: 788584
You need to log in before you can comment on or make changes to this bug.