Session-restored searchfox tabs don't automatically scroll to their anchor
Categories
(Webtools :: Searchfox, defect)
Tracking
(Not tracked)
People
(Reporter: asuth, Unassigned)
References
(Depends on 1 open bug)
Details
:emilio fixed a platform issue in bug 1561900 with https://hg.mozilla.org/mozilla-central/rev/802afff4c1de about scroll state restoration, but based on https://bugzilla.mozilla.org/show_bug.cgi?id=1561900#c21 and :emilio's response in https://bugzilla.mozilla.org/show_bug.cgi?id=1561900#c22 I think session restore was always a separate issue we need to address (and not a regression from that platform fix).
:bzbarsky's points at https://bugzilla.mozilla.org/show_bug.cgi?id=1561900#c3 likely still stand, which is that our dynamic anchor generation may be coming too late for the platform's attempt to navigate to the anchor. (Context: we dynamically generate anchors because we use the hash syntax to support highlighting (multiple) ranges of lines, which inherently can't be pre-generated. And in bug 1604613 I propose letting us provide semantic anchor points.)
|
||
Comment 1•3 years ago
|
||
I think the remaining issue is probably just bug 1569820.
|
|
||
Comment 2•3 years ago
|
||
In particular, session-restore doesn't scroll to the anchor at all, it only restores the previous scroll position.
|
Reporter | |
Comment 3•3 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)
I think the remaining issue is probably just bug 1569820.
Ah! Thanks, yes, I'd lost track of that one and that seems right.
It seems like the likelihood of people hitting this could justify searchfox implementing a mitigation, but I'm a little worried about that potentially messing with the already fixed bug 1561900 case. Maybe it's time for searchfox to grow some keyboard navigation commands...
|
||
Comment 4•2 years ago
|
||
Ran across this in Slack. Here are some trivial STR (just to be explicit):
- Open https://searchfox.org/mozilla-central/rev/a156a65ced2dae5913ae35a68e9445b8ee7ca457/layout/base/nsPresContext.cpp#526
- (optional) Manually scroll up or down as far as you like.
- Close the tab. (Ctrl+W)
- Undo-close-tab (ctrl+shift+T)
EXPECTED RESULTS:
Scroll position should have been restored so the page looks as-it-did before you closed the tab.
ACTUAL RESULTS:
The page is scrolled to the very top (as if there were no anchor & no manual scroll position to restore)
This reproduces in both Firefox and Chrome.
Other pages like e.g. https://wiki.mozilla.org/Release_Management/Calendar#Past_branch_dates give EXPECTED RESULTS if I perform the same STR there.
Side note: in Firefox, after performing the STR, you can force Searchfox to seek to the anchor by reloading the page (Ctrl+R). However, in Chrome, reloading always seems to cause searchfox to seek to the very top of the page.
|
|
||
Comment 5•2 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #4)
However, in Chrome, reloading always seems to cause searchfox to seek to the very top of the page.
I spun off bug 1809566 for this^ afterthought, since it seems like a distinct issue.
|
|
Reporter | |
Comment 6•2 years ago
|
||
Thanks for the reproduction steps! I was able to reproduce the failure to scroll using your STR.
It's probably also worth noting that if we use the non-dynamically generated line-526 identifier, we will scroll appropriately on undo-close-tab via ctrl-shift-T. For line 526 that's https://searchfox.org/mozilla-central/rev/a156a65ced2dae5913ae35a68e9445b8ee7ca457/layout/base/nsPresContext.cpp#line-526. I did confirm that when using #526 and we fail to scroll that devtools did find that we had generated the synthetic anchor by pasting document.getElementById("526") in devtools (although I don't know when it was created relative to anything else, although I do know we put the script tags near the end of the HTML body).
(Just to reiterate, we dynamically generate the element with the 526 identifier, and that's a legal HTML identifier but it's not a legal CSS identifier, so document.querySelector("#526") will fail but document.getElementById("526") will not.)
Description
•