Closed Bug 1546027 Opened 1 year ago Closed 9 months ago

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

Categories

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

66 Branch
x86_64
Windows 10
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- verified
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 - wontfix
firefox69 --- verified
firefox70 --- verified

People

(Reporter: alice0775, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: nightly-community, regression)

Attachments

(2 files, 1 obsolete file)

Attached file 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

Component: General → Layout: Scrolling and Overflow

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

Flags: needinfo?(svoisen)

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!

Flags: needinfo?(svoisen) → needinfo?(rhunt)

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.

Flags: needinfo?(rhunt)

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

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.

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

Duplicate of this bug: 1556118

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.

Assignee: nobody → emilio
Blocks: 1524281
No longer depends on: 1524281

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.

Attachment #9078147 - Attachment is obsolete: true
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da82282ba536
Don't apply scroll anchor adjustments if we're restoring our scroll position. r=dholbert
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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

Duplicate of this bug: 1524281

Will this be ported to firefox-esr68?

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!

Is this something we should consider for backport?

Flags: needinfo?(emilio)

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.

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.

Flags: needinfo?(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
Attachment #9077853 - Flags: approval-mozilla-esr68?
Attachment #9077853 - Flags: approval-mozilla-beta?
Flags: qe-verify+

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.

Attachment #9077853 - Flags: approval-mozilla-esr68?
Attachment #9077853 - Flags: approval-mozilla-esr68+
Attachment #9077853 - Flags: approval-mozilla-beta?
Attachment #9077853 - Flags: approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

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.

Flags: qe-verify+

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

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