1568778 - StickyTableHeaders still doesn't work correctly even after bug 1543599.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

Attachment: t.html

Not a regression from bug 1543599, but sticky table headers still don't play well with scroll anchoring.

STR:

• Scroll down using the wheel or trackpad.
• Scroll up again.
• Scroll down again.

ER:

• Headers stick.

AR:

• They don't. They stick if you scroll with the keyboard, though, and if layout.css.scroll-anchoring.enabled=false.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

So there are two issues here:

• We get into a scroll anchoring loop so that the page ends up wasting a lot of cycles for no good reason. This happens on Chrome as well, and can be seen by doing window.addEventListener("scroll", () => console.log("scroll")) on devtools. I have a fix for that, which also fixes the test-case but...
• The root cause of the issue is that the scroll position reported to script is not updated properly. You can see that doing setInterval(() => console.log(window.pageYOffset), 500).

This is not reproducible if you turn scroll anchoring off, but I think it's a bad interaction with the relative scrolling that scroll anchoring does, or something... Botond, do you know how the APZ scroll offset is communicated back to the main thread so I can get a hint of where to look at?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1568778 - Relative scrolls shouldn't clobber APZs offset. r=botond

This fixes it, but I have no idea whether it's 100% right, or what is the best
way to add a test for this.

It seems that APZ will process relative updates by accumulating them to the APZ
scroll offset, and since relative offsets before this patch clobber the APZ
offset we never end up writing back to the main thread the APZ scroll offset
(because APZCCallbackHelper::IsScrollInProgress will return true).

Thus, I think this is the right fix, but please review carefully since I'm not
very familiar with this code.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I think I found the relevant place :)

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1568778 - Do suppress adjustments when switching an undisplayed element to be abspos. r=dholbert

One of the issues with the test-case in this bug is that the page consumes a ton
of CPU due to scroll anchor adjustments being triggered from scroll events,
which in turn cause other scroll events to fire.

This happens in Chrome as well (just scroll to the bottom of the test-case, and
do addEventListener("scroll", () => console.log("scroll")) on devtools. But I
think it's worth fixing. This patch fixes it and overall I think it's a slightly
better approach to suppress adjustments than what we're doing.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Test-case that doesn't need scroll anchoring.

So my patch above causes test_relative_update.html to fail, so I need to fix it somehow.

This test-case reproduces the issue without the need of scroll anchoring. Scroll anchoring only exposes this issue since it uses relative updates.

Botond, given my APZ patch above doesn't seem to be alright, do you have an idea of what's the right fix here? Hopefully this test-case helps.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Just for the record:

18:23 <botond> emilio: so what is the testcase at https://bug1568778.bmoattachments.org/attachment.cgi?id=9080615 trying to do? what is the expected behaviour?
18:23 <firebot> https://bugzil.la/1568778 — NEW, [e-mail] — StickyTableHeaders still doesn't work correctly even after bug 1543599.
18:24 <@emilio> botond: oh, sorry, should've elaborated
18:24 <@emilio> botond: open the test-case, and open devtools, you'll see a lot of scroll events logging `pageYOffset` (i.e., the main-thread scroll position of the root scroller)
18:25 <@emilio> botond: now scroll using the track pad, or whatever other APZ-scrolling mechanism, and note how the logged offset doesn't change
18:25 <@emilio> botond: i.e., the APZ scroll position is not properly set back in the main thread
18:26 <@emilio> botond: the scroll relatively in one direction, then in another, is basically what scroll anchoring ends up doing in that website (the second patch in the bug avoids the infinite-adjustments)
18:27 <botond> emilio: got it, thanks!
18:28 <@emilio> botond: np, and sorry, should've noted that on the bug

Comment 7

[Botond Ballo [:botond]]

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

Botond, given my APZ patch above doesn't seem to be alright, do you have an idea of what's the right fix here?

We discussed this on IRC, but to summarize briefly:

• The APZ scroll offset does get synced over to the main thread as soon as it stops performing scrollBy() calls, even for just a few frames.
• If the main thread is continuously performing scrollBy() calls as in this testcase, we never sync over the APZ scroll offset, because every time we try the main thread's in a "new relative update is pending" state.
• We can probably make this work (i.e. sync over the APZ scroll offset even in scenarios like this) if necessary, but it is probably not going to be a straightforward fix, so we should probably spin this off into a new bug and land the scroll anchoring related fix here.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c757750b882
Do suppress adjustments when switching an undisplayed element to be abspos. r=dholbert

Comment 9

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

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

Comment 10

[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 11

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/0c757750b882

Comment 12

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

Upstream PR merged

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Hi Emilio, is this something we should consider backporting to fix bug 1568762 on the branches or should this ride the trains?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

I don't think it's too risky, but I'd prefer it to ride the trains or maybe uplift to beta unless there are other sites than the one in bug 1568778 affected.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

OK, let's wontfix for ESR68 and let this bake a bit before deciding on Beta.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Hi Emilio, any thoughts on what to do for 69 here?

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Probably worth letting it ride the trains.

Comment 18

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

Bugbug thinks this bug is a regression, but please revert this change in case of error.