Last Comment Bug 680257 - :target pseudo-class not updating when pressing back button
: :target pseudo-class not updating when pressing back button
Status: VERIFIED FIXED
: qawanted, regression
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
: 682935 (view as bug list)
Depends on:
Blocks: 653741
  Show dependency treegraph
 
Reported: 2011-08-18 14:30 PDT by Matt Cosentino
Modified: 2013-12-27 14:32 PST (History)
9 users (show)
justin.lebar+bug: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
wontfix
-
fixed
-
fixed
-
fixed


Attachments
Patch v1 (2.61 KB, patch)
2011-08-22 13:34 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Review
Patch v2 (with tests) (4.82 KB, patch)
2011-08-22 13:46 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review+
bugzilla: approval‑mozilla‑aurora+
bugzilla: approval‑mozilla‑beta+
Details | Diff | Review

Description Matt Cosentino 2011-08-18 14:30:25 PDT
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0
Build ID: 20110811165603

Steps to reproduce:

Here is a jsfiddle that demonstrates the problem.

http://jsfiddle.net/gvFPv/2/


Actual results:

Clicking on the "a" link correctly applies the :target pseudo-class to the "a" link.  Clicking on the "b" link then applies the pseudo-class to the "b" link and removes it from the "a" link.  Pressing the back button does not update the pseudo-class on either link.


Expected results:

When pressing the back button it should remove the pseudo-class from the "b" link and apply it to the "a" link.  This works in Firefox 3.6 and it works in Chrome, but it does not work in Firefox 6.  I haven't tested 4 or 5, so I'm not sure when this regressed.
Comment 1 XtC4UaLL [:xtc4uall] 2011-08-18 16:07:07 PDT
This regressed with Version 6. 5 is WFM.
Comment 2 Alice0775 White 2011-08-18 17:43:43 PDT
Regression window(cached m-c hourly):
Works:
http://hg.mozilla.org/mozilla-central/rev/837f762860af
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110524 Firefox/6.0a1 ID:20110524085016
Fails:
http://hg.mozilla.org/mozilla-central/rev/0fad9fa9fa3f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110524 Firefox/7.0a1 ID:20110524105711
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=837f762860af&tochange=0fad9fa9fa3f

Suspected:
575362d9b92e	Justin Lebar — Bug 653741 - You should be able to scoll back to the current anchor by focusing the location bar and pressing <enter>. r=bz
Comment 3 Justin Lebar (not reading bugmail) 2011-08-22 12:05:05 PDT
I guess ScrollToAnchor also updates the :target?  That would be unfortunate.
Comment 4 Justin Lebar (not reading bugmail) 2011-08-22 13:34:07 PDT
Created attachment 554947 [details] [diff] [review]
Patch v1
Comment 5 Justin Lebar (not reading bugmail) 2011-08-22 13:36:32 PDT
Nominating for tracking 7, 8, 9.  :target isn't widely used AFAIK, but this bug breaks it pretty hard.
Comment 6 Justin Lebar (not reading bugmail) 2011-08-22 13:42:43 PDT
Pushed patch v1 to try:

http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=2be0bae7f070
Comment 7 Justin Lebar (not reading bugmail) 2011-08-22 13:46:54 PDT
Created attachment 554949 [details] [diff] [review]
Patch v2 (with tests)
Comment 8 Justin Lebar (not reading bugmail) 2011-08-22 13:47:19 PDT
Forgot to include the tests last time.

http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=fd83538ee308
Comment 9 Justin Lebar (not reading bugmail) 2011-08-22 16:35:25 PDT
Comment on attachment 554949 [details] [diff] [review]
Patch v2 (with tests)

Looks good on try so far.

I think this change is safe, assuming

  !aSHEntry iff aLoadType == LOAD_HISTORY  (1),

because in ScrollToAnchor, we always pass FALSE as the second argument to shell->GoToAnchor (indicating that we should not scroll to the anchor) if aLoadType == LOAD_HISTORY, and because ScrollToAnchor doesn't do anything other than call shell->GoToAnchor and, if the hash is empty, call SetCurScrollPosEx(0,0).

If (1) does not hold, then I'm confused about why I wrote the condition around ScrollToAnchor as |!aSHEntry| in the first place.
Comment 10 Justin Lebar (not reading bugmail) 2011-08-22 16:50:13 PDT
...and because SetCurScrollPosEx is not called if aLoadType is HISTORY or RELOAD.  But this is all probably clearer if you just look at the code.  :)
Comment 11 Boris Zbarsky [:bz] 2011-08-22 19:24:45 PDT
(1) as written above is certainly false.  !aSHEntry is true for non-history loads (e.g. LOAD_NORMAL, LOAD_LINK, etc).

I believe that |aSHEntry iff (aLoadType == LOAD_HISTORY || aLoadType == LOAD_RELOAD_NORMAL)| is true, however, and I would think that's what you really care about here, no?
Comment 12 Boris Zbarsky [:bz] 2011-08-22 19:25:33 PDT
Comment on attachment 554949 [details] [diff] [review]
Patch v2 (with tests)

r=me if you fix the commit message to not have try gunk in it.

Thank you for adding tests for this!
Comment 13 Justin Lebar (not reading bugmail) 2011-08-22 19:43:15 PDT
> I believe that |aSHEntry iff (aLoadType == LOAD_HISTORY || aLoadType == 
> LOAD_RELOAD_NORMAL)| is true, however, and I would think that's what you really 
> care about here, no?

Yes, indeed!

> Thank you for adding tests for this!

I didn't realize I had a choice.  :)

Inbound - http://hg.mozilla.org/integration/mozilla-inbound/rev/ea8d99353056
Comment 14 Justin Lebar (not reading bugmail) 2011-08-22 19:51:00 PDT
WRT branches, we can't just back out bug 653741, because that itself was a regression fix.  I think we definitely want this on Aurora.  Beta is a tougher call.  This code is obviously fragile, and although it seems from comment 9 and comment 11 that this change is safe, it could be that one of our assumptions is wrong in an edge case.

The likely failure mode is that we'd regress some other edge case, so maybe this known fix outweighs the possibility of creating another, similar bug.
Comment 15 Mounir Lamouri (:mounir) 2011-08-23 01:43:16 PDT
http://hg.mozilla.org/mozilla-central/rev/ea8d99353056
Comment 16 Johnathan Nightingale [:johnath] 2011-08-23 14:51:26 PDT
Comment on attachment 554949 [details] [diff] [review]
Patch v2 (with tests)

Discussed in triage - approving this for both, but it would help if you could give QA some information about how to watch for regressions here. We'll be eager to back this out of beta if those regressions show up, but we need to know what to look for.
Comment 17 Justin Lebar (not reading bugmail) 2011-08-23 16:12:20 PDT
Regressions would likely be apparent when navigating between two history entries which share a document.  For instance, when navigating between foo.html and foo.html#bar, or if foo.html calls history.pushState, navigating between those two history entries.
Comment 18 Justin Lebar (not reading bugmail) 2011-08-23 16:26:14 PDT
Pushed to branches: Aurora (FF8) and Beta (FF7):

http://hg.mozilla.org/releases/mozilla-aurora/rev/636ee9539d54
http://hg.mozilla.org/releases/mozilla-beta/rev/fbcd27571662
Comment 19 Justin Lebar (not reading bugmail) 2011-08-23 16:27:39 PDT
Thank you for the bug report, Matt!
Comment 20 Dão Gottwald [:dao] 2011-08-24 02:18:14 PDT
(In reply to Justin Lebar [:jlebar] from comment #18)
> http://hg.mozilla.org/releases/mozilla-beta/rev/fbcd27571662

Backed out because of mochitest-plain-2 perma-orange.
Comment 21 Justin Lebar (not reading bugmail) 2011-08-24 06:35:15 PDT
In particular, the test for this bug is timing out, beta only.

http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Beta/1314145114.1314147541.7504.gz
Comment 22 Justin Lebar (not reading bugmail) 2011-08-25 13:02:13 PDT
I think the test failure on beta is because my mochitest doesn't include Mochikit/packed.js, which is gone in trunk but not yet in beta.

Try: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=4b67045e0865
Comment 23 Justin Lebar (not reading bugmail) 2011-08-26 06:44:40 PDT
With test fix this was green on try.  Pushed to beta:

http://hg.mozilla.org/releases/mozilla-beta/rev/dddb008734ee
Comment 24 Alice0775 White 2011-08-29 13:16:43 PDT
*** Bug 682935 has been marked as a duplicate of this bug. ***
Comment 25 Ioana (away) 2011-09-02 04:24:21 PDT
Verified as fixed on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0a2) Gecko/20110831 Firefox/8.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a1) Gecko/20110831 Firefox/9.0a1

Mozilla/5.0 (X11; Linux i686; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (X11; Linux i686; rv:8.0a2) Gecko/20110901 Firefox/8.0a2
Mozilla/5.0 (X11; Linux i686; rv:9.0a1) Gecko/20110831 Firefox/9.0a1

Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 5.1; rv:8.0a2) Gecko/20110828 Firefox/8.0a2
Mozilla/5.0 (Windows NT 5.1; rv:9.0a1) Gecko/20110829 Firefox/9.0a1

Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 6.1; rv:8.0a2) Gecko/20110825 Firefox/8.0a2
Mozilla/5.0 (Windows NT 6.1; rv:9.0a1) Gecko/20110825 Firefox/9.0a1

Steps:
1. Open http://jsfiddle.net/gvFPv/2/
2. Click on the "a" link, then click on the "b" link.
3. Click on the back button.
Clicking on the "a" link applies the :target pseudo-class to the "a" link.  Clicking on the "b" link applies the pseudo-class to the "b" link and removes it from the "a" link. When clicking the back button the pseudo-class is applied to the "a" link and it is removed from the "b" link.

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