Closed Bug 1568778 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 2 open bugs)

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

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

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?

Flags: needinfo?(botond)
Blocks: 1568762

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.

I think I found the relevant place :)

Flags: needinfo?(botond)
Assignee: nobody → emilio

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.

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.

Flags: needinfo?(botond)

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, emilio@crisal.io — 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

(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.
Flags: needinfo?(botond)
Blocks: 1569599
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0c757750b882
Do suppress adjustments when switching an undisplayed element to be abspos. r=dholbert
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/18148 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: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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

Flags: needinfo?(emilio)
Flags: in-testsuite+

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.

Flags: needinfo?(emilio)

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

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

Flags: needinfo?(emilio)

Probably worth letting it ride the trains.

Flags: needinfo?(emilio)

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

Keywords: regression
Attachment #9080585 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.