1543599 - Scroll Behaviour on StickyTableHeader at Version 66.0.3

Status: VERIFIED FIXED

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

Description

[wiriadinata]

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

Comment 1

[Alice0775 White]

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

Comment 2

[Alice0775 White]

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?

Comment 3

[Pascal Chevrel:pascalc]

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.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

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

Comment 5

[Jessie [:jbonisteel] pls NI]

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

Comment 6

[Ryan Hunt [:rhunt]]

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.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Reduced-ish test-case from https://github.com/jmosbech/StickyTableHeaders/issues/159 here: https://jsfiddle.net/vLx4z3n5/1/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Not fully reduced, but very trimmed down test-case.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Almost fully reduced.

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

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: reduced test-case.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1543599 - Don't suppress scroll anchoring adjustments when switching display to `none`. r=rhunt,dholbert

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.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a8a7dd2693b
Don't suppress scroll anchoring adjustments when switching display to `none`. r=dholbert

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17838 for changes under testing/web-platform/tests

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 15

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/9a8a7dd2693b

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Is this something you wanted to nominate for uplift?

Comment 18

[Emilio Cobos Álvarez (: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

Comment 19

[Alexandru Trif, Desktop Test Engineering [:atrif]]

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 20

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

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

Comment 22

[Emilio Cobos Álvarez (: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

Comment 23

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/34ac17699d07

Comment 24

[Alexandru Trif, Desktop Test Engineering [:atrif]]

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.

Comment 25

[Emilio Cobos Álvarez (: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.

Comment 26

[Emilio Cobos Álvarez (:emilio)]

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

Comment 27

[Alexandru Trif, Desktop Test Engineering [:atrif]]

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 28

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Alexandru Trif, Desktop Test Engineering [:atrif]]

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.