Closed Bug 1543599 Opened 5 years ago Closed 5 years ago

Scroll Behaviour on StickyTableHeader at Version 66.0.3

Categories

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

66 Branch
defect
Not set
normal

Tracking

()

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

People

(Reporter: wiriadinata, Assigned: emilio)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression, reproducible)

Attachments

(2 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:66.0) Gecko/20100101 Firefox/66.0

Steps to reproduce:

open website http://lembarsaham.com/

scroll to middle page

Actual results:

The page keep scrolling

Expected results:

The page should not scroll, please compare with version 65

I can reproduce.
And If disabled scroll anchor layout.css.scroll-anchoring.enabled=false, the problem is solved.

Component: Untriaged → Layout: Scrolling and Overflow
Keywords: reproducible
Product: Firefox → Core
Regressions: scroll-anchoring
No longer regressions: scroll-anchoring
Status: UNCONFIRMED → NEW
Ever confirmed: true

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fbe6548db11ded24b5221180719ce66e785dc3c6&tochange=ad851d4345c08f7e0e5d5578652004194a6e667f

Regressed by: Bug 1305957

:rhunt, Your bunch of patch seems to cause the regression. Can you please look into this?

Has Regression Range: --- → yes
Has STR: --- → yes
Flags: needinfo?(rhunt)
Keywords: regression
Regressed by: 1305957

Bug is unassigned and has no priority set, given that we shipped 66 with it and that we are in our last betas for 67, marking as wontfix for our upcoming release.

Hi Jessie, any chance you could help un-stick this scroll anchoring regression? Thanks!

Flags: needinfo?(jbonisteel)

Spoke to rhunt about this and he is going to take a look over the next week or so

Flags: needinfo?(jbonisteel)

I've looked at this under RR and have a better idea what's going on. This appears to be another case of an undesired scroll anchoring adjustment happening based on the timing of layout flushes.

The issue with the page happens as you scroll part way down. At this point it gets into a loop of:

  1. anchor adjustment (suppressed) 1px up ->
  2. anchor selection ->
  3. anchor adjustment 1px down ->
  4. scroll event ->

The (1) suppressed adjustment is from a in-flow/out-flow change. The (3) adjustment is happening in a layout flush from getComputedStyle(..).height. The website JS is probably reconstructing their DOM to emulate position: sticky and we're doing a scroll adjustment in the middle of it (similar to the ebay issue). My best guess is that chrome is not flushing layout or not flushing scroll anchors somehow on (3). I've dug through their code a bit, but wasn't able to find anything obvious.

Bug 1529702 has some notes on avoiding this kind of compat issue by not applying adjustments during layout flushes and moving them to a well specified point like rAF. I think that would really help here.

Flags: needinfo?(rhunt)
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Attached file Not fully reduced, but very trimmed down test-case. (obsolete) (deleted) —
Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
Attached file Almost fully reduced. (obsolete) (deleted) —

The only thing the reduction is missing is isolating what part of the jQuery plugin is causing this.

Attachment #9077874 - Attachment is obsolete: true
Attached file reduced test-case. (deleted) —
Attachment #9077879 - Attachment is obsolete: true

Since that means that we won't suppress them when switching display back (since
we have no frame to pull the old style from).

We may want to match Chrome more exactly and don't do this any time display
changes (which if I'm reading their code correctly is what they do...).

But for now I've done the minimal thing and added a test.

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a8a7dd2693b
Don't suppress scroll anchoring adjustments when switching display to `none`. r=dholbert
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17838 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

Is this something you wanted to nominate for uplift?

Flags: needinfo?(emilio)

Comment on attachment 9078191 [details]
Bug 1543599 - Don't suppress scroll anchoring adjustments when switching display to none. r=rhunt,dholbert

Beta/Release Uplift Approval Request

  • User impact if declined: Bogus scrolling on some common sites when display and position are changed at the same time.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: comment 0 or so.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Incorrectly taking the code path stops disabling some scroll anchoring adjustments, which is effectively what we've shipped until 66.

Incorrectly not taking the code path is the state of Firefox before the patch landed, with this bug being the consequence.

  • String changes made/needed: none
Flags: needinfo?(emilio)
Attachment #9078191 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Hello!

Reproduced the issue using Firefox 68.0a1 (20190411161115) on Windows 10x64.
The issue is verified fixed with Firefox 70.0a1 (20190722214346) on Windows 10x64, macOS 10.14 and Ubuntu 18.04.

Comment on attachment 9078191 [details]
Bug 1543599 - Don't suppress scroll anchoring adjustments when switching display to none. r=rhunt,dholbert

Fixes a scroll anchoring regression which can cause broken scrolling on some sites by effectively reverting us to the previous behavior. Approved for 69.0b8.

Emilio, is this something we should consider fixing on ESR68 also? If so, it'll need a rebased patch.

Flags: needinfo?(emilio)
Attachment #9078191 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

I'd rather uplift both this and bug 1559627 if so, it's probably less risky and bug 1559627 is also kinda nasty.

Flags: needinfo?(emilio)

Comment on attachment 9078191 [details]
Bug 1543599 - Don't suppress scroll anchoring adjustments when switching display to none. r=rhunt,dholbert

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Webpages scroll in weird ways.
  • User impact if declined: See above.
  • Fix Landed on Version: 70
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): This needs bug 1559627 too. I think they're not too risky but that one's definitely not a one-liner.
  • String or UUID changes made by this patch: none
Attachment #9078191 - Flags: approval-mozilla-esr68?

Hello,
This issue is fixed while using 69.0b8 (20190723212625) on Windows 10x64, macOS 10.14 and Ubuntu 18.04.
The problem is that while verifying this on 69.0b8 (20190723212625) I encountered bug 1568762. Should we close this as verified after all the builds are tested and use bug 1568762 for the rest of the work? Thank you.

Flags: needinfo?(emilio)

Well, it really depends on whether this bug really causes bug 1568762 or it was pre-existing. If it's caused it, then we need to think more about it. But I can repro bug 1568762 with scroll anchoring disabled so I suspect this is not at fault.

Flags: needinfo?(emilio)
Blocks: 1568778

It seems that bug is not a regression from this one, so I think it should be closed as verified.

Yes, and sorry for any confusion created .
Then this particular issue is verified on Firefox 69.0b8 (20190723212625) on Windows 10x64, macOS 10.14 and Ubuntu 18.04.
Updating final flags after the ESR Uplift. Thank you!

Comment on attachment 9078191 [details]
Bug 1543599 - Don't suppress scroll anchoring adjustments when switching display to none. r=rhunt,dholbert

Another scroll anchoring regression fix. Approved for 68.1esr.

Attachment #9078191 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

Hello,
The issue is verified fixed with Firefox 68.1.0esr (20190729203625) on Windows 10x64, macOS 10.14 and Ubuntu 18.04. Tested using test case from comment 10 and STR from comment 0.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: