1546027 - # anchor jumps to inconsistent position. it is depending on cache.

Status: VERIFIED FIXED

(Core :: Layout: Scrolling and Overflow, defect)

Description

[Alice0775 White]

Attachment: anchor.html

[Tracking Requested - why for this release]: broken scroll position

Reproducible: always

Steps To Reproduce:

1. Save the attached html to local file system as anchor.html

2. Clear Cache

3. Open file://path_to_file/anchor.html#anc2 in new tab
   --- observe scroll position. the scroll position is incorrect!!

4. Open same url file://path_to_file/anchor.html#anc2 in new tab
   --- observe scroll position. the scroll position is correct.

Actual Results:
The scroll position is wrong at first page load.
The Dev logo is out of view port. if cache is empty.

Expected Results:
The scroll position should be consistent.
The Dev logo should be fully visible

Comment 1

[Pascal Chevrel:pascalc (PTO until April 26)]

Sean, could you prioritize this regression with regards to our 67 release? thanks

Comment 2

[Maire Reavy [:mreavy]]

Sean's out on parental leave until May 6th. So I'm taking this needinfo for him.

Ryan -- Can you take a look at this? I'm interested in your opinion of how bad a regression this is and what the effort would be to fix it. Is this something we could fix and uplift into 67? Thanks!

Comment 3

[Ryan Hunt [:rhunt] (on leave until early May)]

The issue is that scroll anchoring is being applied in an undesirable way during page-load. We have another very similar issue in bug 1524281. Comment 6 is relevant.

Loosely, it looks like the order of events for this page are:

1. We load the HTML for the page
2. We initiate async loads for the images
3. The page has layout run (unsure why so far)
   a. This causes us to restore our scroll position from history
   b. Restoring a scroll position causes us to choose a scroll anchor which will be one of the div's
4. The async loads for the images finish
5. We run layout again
   a. The images now take up space and push down the content we are anchored to
   b. We apply scroll anchoring and scroll down

The step to clear the cache just forces us to always async load the images.

There are some questions here still. Like why we run the layout in step (3). Chrome doesn't seem to have this issue, but I haven't had a chance to read their code enough to understand why. This behavior is under-specified.

I'm not sure how complicated the fix would be. It might involve finding a scope during loading that we can disable scroll anchoring, or it might involve something more fundamental to the scroll anchoring spec.

I'd like to take a holistic look at these interop issues sometime soon, but have been busy with fission work.

Comment 4

[Pascal Chevrel:pascalc (PTO until April 26)]

Unprioritized and unassigned 1 week before RC week, too late for 67.

Comment 5

[Maire Reavy [:mreavy]]

My apologies that I didn't follow up here immediately after Ryan gave his assessment. This is not a simple fix. We may have to re-evaluate the spec and how it decides to apply scroll anchoring -- or figure out how to apply heuristics to avoid bad cases. While we don't know why this layout occurred, layouts can occur during load so we can't avoid them entirely. Ryan is working this week on Fission which is a higher priority project. When Sean is back next week, I'll ask him to work with Ryan to figure out a plan for this follow up work.

Comment 6

[Julien Cristau [:jcristau]]

Thanks Maire. Sounds like this won't make 68 either.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1546027 - Don't apply scroll anchor adjustments if we're restoring our scroll position. r=rhunt,dholbert

Since scroll position restoration is absolute, and we'll lose it as soon as we
apply any adjustment or do any other sort of scrolling.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1546027 - Don't apply scroll anchor adjustments if we're processing an async scroll animation. r=rhunt,dholbert

Comment 10

[Phabricator Automation]

Comment on attachment 9078147 [details]
Bug 1546027 - Don't apply scroll anchor adjustments if we're processing an async scroll animation. r=rhunt,dholbert

Revision D38077 was moved to bug 1554572. Setting attachment 9078147 [details] to obsolete.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da82282ba536
Don't apply scroll anchor adjustments if we're restoring our scroll position. r=dholbert

Comment 12

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/da82282ba536

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 15

[Djfe]

Will this be ported to firefox-esr68?

Comment 16

[socratisk]

Originally coming from bug 1555229, which was deemed a duplicate of bug 1524281, which was deemed a duplicate of this bug 1546027 (no wonder we're at the 1.5 Mbugs with so many duplicates :) ).

Just wanted to confirm (and subscribe) that Firefox Nightly 70.0a1 (2019-07-16) (64-bit) seems to have fixed this issue on several sites that I've tried. Host: OSX 10.11.6 and 10.9.5.

Thanks everyone, back to DEFCON 1!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for backport?

Comment 18

[Cristian Fogel [:cfogel]]

In an attempt to verify the fix with 70.0a1 (2019-07-16) on Windows 10(1803) with the attached TC, however:

• step 3 still has issues with positioning;
• step 4 is ok.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

It's better to verify this in terms of the STR of bug 1524281 or one of their many dupes. I'll file a bug for the still-not-great interaction of scroll anchoring with scrolling-to-an-anchor, which is only partially fixed.

And yes, I think we should uplift this.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9077853 [details]
Bug 1546027 - Don't apply scroll anchor adjustments if we're restoring our scroll position. r=rhunt,dholbert

Beta/Release Uplift Approval Request

• User impact if declined: Scroll position is not kept properly upon navigation / reloads.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: see bug 1524281 or many others.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's a patch that disables scroll anchoring until we've effectively restored the scroll position. Relatively simple and when the branch is taken the behavior should be the same thing we've been shipping until FF66.
• String changes made/needed: none

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Pretty annoying bug with tons of duplicates.
• User impact if declined: Scroll position restoration remains broken in a lot of pages.
• Fix Landed on Version: 70
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): See above.
• String or UUID changes made by this patch: none

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9077853 [details]
Bug 1546027 - Don't apply scroll anchor adjustments if we're restoring our scroll position. r=rhunt,dholbert

Fixes a commonly-duped scroll anchoring regression. Approved for 69.0b6 and 68.1esr.

Comment 22

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/releases/mozilla-beta/rev/74cd260c31d7

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/f0c9647660d5

Comment 24

[Cristian Fogel [:cfogel]]

Verified with 68.0.1 ESR 69.0b6, as per comment 20.
(scroll position properly restored with - https://www.rhs.org.uk/)

Updating status for 70 as well.

Comment 25

[socratisk]

Verified as fixed with FF 69.0 final (2019-09-03), thanks to everyone involved!