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 :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 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 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 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 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 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 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 :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 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 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 Ed Morley [:emorley] 2012-05-08 03:17:27 PDT
https://hg.mozilla.org/mozilla-central/rev/991d6c9dc2d9
Comment 13 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 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 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.