Last Comment Bug 743421 - Anchor navigation resets click-to-play state
: Anchor navigation resets click-to-play state
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 15
Assigned To: Jared Wein [:jaws] (please needinfo? me)
:
:
Mentors:
Depends on: 788584
Blocks: 711552
  Show dependency treegraph
 
Reported: 2012-04-06 22:40 PDT by :Felipe Gomes (needinfo me!)
Modified: 2012-09-05 10:39 PDT (History)
5 users (show)
jaws: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Patch for bug (2.51 KB, patch)
2012-04-24 12:26 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Tests for patch (4.50 KB, patch)
2012-05-03 12:27 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch for bug v2 (1.37 KB, patch)
2012-05-03 12:28 PDT, Jared Wein [:jaws] (please needinfo? me)
felipc: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Tests for patch v2 (6.99 KB, patch)
2012-05-03 13:41 PDT, Jared Wein [:jaws] (please needinfo? me)
felipc: review+
Details | Diff | Splinter Review

Description User image :Felipe Gomes (needinfo me!) 2012-04-06 22:40:58 PDT
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
Comment 1 User image Jared Wein [:jaws] (please needinfo? me) 2012-04-24 12:26:08 PDT
Created attachment 617990 [details] [diff] [review]
Patch for bug

This patch fixes the case where anchor navigation and history.push/pop/replaceState.
Comment 2 User image Jared Wein [:jaws] (please needinfo? me) 2012-04-26 07:34:34 PDT
Comment on attachment 617990 [details] [diff] [review]
Patch for bug

New patches on the way that will come with tests.
Comment 3 User image Jared Wein [:jaws] (please needinfo? me) 2012-05-03 12:27:05 PDT
Created attachment 620810 [details] [diff] [review]
Tests for patch

Pushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=c7eb352d46c5
Comment 4 User image Jared Wein [:jaws] (please needinfo? me) 2012-05-03 12:28:46 PDT
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.
Comment 5 User image Jared Wein [:jaws] (please needinfo? me) 2012-05-03 13:19:24 PDT
Comment on attachment 620810 [details] [diff] [review]
Tests for patch

forgot to hg add
Comment 6 User image Jared Wein [:jaws] (please needinfo? me) 2012-05-03 13:41:20 PDT
Created attachment 620835 [details] [diff] [review]
Tests for patch v2

Repushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=745fe786c531
Comment 9 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-07 14:23:43 PDT
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 10 User image Jared Wein [:jaws] (please needinfo? me) 2012-05-07 14:27:52 PDT
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
Comment 11 User image Jared Wein [:jaws] (please needinfo? me) 2012-05-07 14:41:42 PDT
(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
Comment 12 User image Ed Morley [:emorley] 2012-05-08 03:17:27 PDT
https://hg.mozilla.org/mozilla-central/rev/991d6c9dc2d9
Comment 13 User image Alex Keybl [:akeybl] 2012-05-09 16:23:22 PDT
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.
Comment 15 User image neil@parkwaycc.co.uk 2012-09-05 09:29:54 PDT
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.
Comment 16 User image Jared Wein [:jaws] (please needinfo? me) 2012-09-05 10:39:46 PDT
Thanks Neil.

Note You need to log in before you can comment on or make changes to this bug.