1561900 - Scroll-to-line when going back or restoring closed tab is broken

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Start at a URL with a line number, e.g. https://searchfox.org/mozilla-central/source/dom/events/PendingFullscreenEvent.h#27

Click on things and navigate forward ~5 pages (enough that the original URL is no longer in the bfcache, I think). In my case I clicked on Document function and did a symbol search, then clicked on the single use of it, then clicked on the CancelPendingFullscreenEvents function name and did another symbol search, and then clicked on the single use of that.

Then hit back repeatedly until you're back at the starting URL.

Expected: when the starting URL loads (not from bfcache), it scrolls down to show the appropriate line number

Actual: it doesn't.

You can then repeat hit "forward" repeatedly to go the "last" URL (in my example it's at Document.cpp#5986) and the same thing happens, because that page isn't in the bfcache any more.

Pretty sure this is a regression from one of the hashchange/scroll-to-line patches that went along with bug 1533802

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Easier STR:

• Go to https://searchfox.org/mozilla-central/source/gfx/2d/BaseRect.h#62
• Close tab
• Restore tab from recently closed tabs (or ctrl+shift+t)

Expected:

• page scrolls down to show line number

Actual:

• page is at the top

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

This might be a load race, possibly due to how sessionstore reloads tabs?

I just did the steps in your excellent comment 1 STR and easily reproduced. I then brought up devtools and did:

document.getElementById('62')
<div id="62" class="goto">

That "goto" thing is our synthetic anchor which is what makes the browser able to jump to the right position including the scrolling position padding (which we can switch to using https://developer.mozilla.org/en-US/docs/Web/CSS/scroll-padding now that 68 is in release). This magic is more essential for cases like "#68-70" where the browser could never do that even if we changed our anchors so that the line numbers were "68" instead of "l68".

So the question is why we're not jumping to that anchor given that the magic does seem to be trying to do the right thing. I'd guess we're creating the div too late. Or sessionstore is betraying us.

I'm cc'ing bz for his interest and wisdom in this scenario previously in case there's something more subtle we should know about with how the anchor seeking works.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So there are four things that may happen during the history load here, presumably:

1. Gecko trying to restore the scroll position from the history entry. This generally works, but seems to be broken on searchfox, probably because one of the other things that are going on stomps on it. That part is annoying in its own right, actually: if I got back/forward to a searchfox tab it loses my scroll position if I've scrolled it, again modulo bfcache. :(
2. Gecko trying to scroll to the anchor position from the URL. This is meant to happen before #1, so #1 should win.
3. The page trying to scroll to somewhere manually from the searchfox scripts
4. The page adding that goto div.

Presumably the intent is that #4 happens before #3, right? Is that the case during the history load?

Comment 5

[Emilio Cobos Álvarez [:emilio]]

I had a patch here that seemed to work btw, and involved using pageshow and tweaking the anchor scrolling code in searchfox...

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

I had a patch here that seemed to work btw, and involved using pageshow and tweaking the anchor scrolling code in searchfox...

That sounds good! Assigning to you!

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Attachment: Reduced test-case.

STR:

• Scroll to some position.
• Click on the link.
• Navigate back.

ER:

• Restored scroll position.

AR:

• Scroll position is lost.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

So this is a Gecko bug?

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1561900 - Fix scroll state restoration when not coming from the bfcache on the initial frame construction. r=bzbarsky

There is no way this ever properly worked, as we always passed null for
aFrameState.

So it'd only work if we reframed the document element or such... Which is not
amazing.

For simpler test-cases, when we don't construct the scrollframe via
PresShell::Initialize, but via the regular frame constructor updates
(ContentAppended, etc...), those end up working because we go through lazy frame
construction, which ends up in RecreateFramesForContent, which passes
mTempFrameTreeState.

Comment 11

[Emilio Cobos Álvarez [:emilio]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #9)

So this is a Gecko bug?

Yes ;)

Comment 12

[Emilio Cobos Álvarez [:emilio]]

(So I'm glad I did this instead of my searchfox wallpaper)

Comment 13

[Emilio Cobos Álvarez [:emilio]]

https://github.com/mozsearch/mozsearch/pull/269 is the only part that's worth landing for searchfox, I think.

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Here's a green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fa52306675d5e082e2a4a74da3f2f412e0fc6f6

And I verified that the test fails without the patch applied.

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #9)

So this is a Gecko bug?

Yes ;)

\o/

Amazing work! (Although I'm a little worried that I'll learn the wrong lesson here... make sure when I next regress things to speak up if you're confident I just regressed things ;)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)

(So I'm glad I did this instead of my searchfox wallpaper)

Can we still have this too? :)

Comment 16

[Emilio Cobos Álvarez [:emilio]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #15)

Can we still have this too? :)

Well the searchfox wallpaper would basically scroll the anchor into view on pageshow. That's better / more likely to be correct than the current behavior, but it's worse than the restoration behavior. We could try to detect it if scrollTop == 0 and then scroll into view... But that's not always great.

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

Oh, I thought you meant like you were making a desktop background wallpaper. Like http://blog.seanmartell.com/wp-content/uploads/2012/03/logo_0013_14.png writ large or what not. I get your meaning now, though!

Comment 18

[Emilio Cobos Álvarez [:emilio]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #17)

Oh, I thought you meant like you were making a desktop background wallpaper. Like http://blog.seanmartell.com/wp-content/uploads/2012/03/logo_0013_14.png writ large or what not. I get your meaning now, though!

Oh! Whoops nope, my art skills are probably terrible anyway... But now I wonder how a Searchfox wallpaper would look like! :D

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/802afff4c1de
Fix scroll state restoration when not coming from the bfcache on the initial frame construction. r=bzbarsky

Comment 20

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/802afff4c1de

Comment 21

[Haik Aftandilian [:haik]]

This is still reproducible for me on the latest Nightly. I'm using the URL below[1] to test with. Restoring a closed tab, or a browser restart with session restore results in the page loading at the top of the page and not automatically scrolling to the URL anchor. I tried mozregression, but could not find a good revision to test with.

Was bug 1569820 erroneously closed as a duplicate? Or should we file a new bug to tackle this again?

1. https://searchfox.org/mozilla-central/rev/40e889be8ff926e32f7567957f4c316f14f6fbef/widget/cocoa/nsToolkit.mm#205

Comment 22

[Emilio Cobos Álvarez [:emilio]]

Yes, that bug looks different. This is about the bfcache. There's no bfcache involved when restoring the session afaict. It may be a sessionrestore bug, or a different layout bug, or a searchfox bug (the anchors for line numbers are dynamic).