1999954 - anchored content that compensates for scroll jumps around when scrolling

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Timothy Nikkel (:tnikkel)]

Attachment: anchor-pos-1.html

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1999954. Schedule a paint to make sure anchored content that compensates for scroll gets repainted when it scrolls. r?#layout-reviewers,hiro,botond

We have a path

https://searchfox.org/firefox-main/rev/4fd0fa7e5814c0b51f1dd075821988377bc56cc1/layout/generic/ScrollContainerFrame.cpp#3088

that skips the content paint if a scroll can be completed purely by apz updating compositor side. When we hit that, nothing else will schedule a paint.

This call to update the position of the anchored content

https://searchfox.org/firefox-main/rev/4fd0fa7e5814c0b51f1dd075821988377bc56cc1/layout/base/PresShell.cpp#11869

will call MarkNeedsDisplayItemRebuild(), so that positioned will be repainted if we paint, but we don't schedule a paint. I guess this pattern worked before because everybody that changed the position of a frame would also called Invalidate on the frame.

It's okay to skip the fast path mentioned above because that fast path isn't valid when we have anchored content that compensates for scroll because that anchored content wasn't painted with a display port.

Comment 2

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

Set release status flags based on info from the regressing bug 1987963

Comment 3

[Timothy Nikkel (:tnikkel)]

Feature not prefed on to ride the trains in 146.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: anchor-pos-2.html

Comment 5

[Timothy Nikkel (:tnikkel)]

The patch is apparantly not enough to fix this second testcase with a chained anchor. Even if I invalidate every member of mAnchorPosPositioned in UpdateAnchorPosForScroll, and invalidate in AbsoluteContainingBlock.cpp the two places where it calls aKidFrame->SetPosition. The frame does get put in the right spot (clicking, changing tabs will redraw it in the right spot). Which is odd.

If I call FrameNeedsReflow every member of mAnchorPosPositioned in UpdateAnchorPosForScroll then the bug does go away.

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: sticky-scroll.html

This testcase with sticky looks even worse.

Comment 7

[Timothy Nikkel (:tnikkel)]

I'm going to un-assign myself for now. The attached patch only fixes the simplest case and I'm not sure yet what the full proper fix is. I intend to come back to this but I'm not actively working on this right now and somone else can pick this up in the meantime. I'll tag dshin as author of the regressor.

Comment 8

[Timothy Nikkel (:tnikkel)]

Possible new theory: the positioned elements are being placed at the wrong location and then moved to the correct location, but no schedule paint happens for this second move.

Comment 9

[Timothy Nikkel (:tnikkel)]

Now that bug 1988032 has landed I recommend setting the pref apz.async_scroll_css_anchor_pos to false while investigating this so that async scrolling does not complicate the investigation (it adds another layer to unpack).

Comment 10

[Timothy Nikkel (:tnikkel)]

Had a bit of free time so I was able to debug this a bit (but I don't think I'll have further time this week).

In the second testcase we are not even considering the yellow box might be affected by the scrolling in UpdateAnchorPosForScroll. So updating UpdateAnchorPosForScroll to understand chained anchors should hopefully fix that.

In the third testcase UpdateAnchorPosForScroll is updating the scroll offset of the anchored content as if the box it is anchored to is not sticky pos (as if it scrolls normally). So teaching UpdateAnchorPosForScroll to understand sticky pos might be the right fix for that. Important note for this: in ScrollContainerFrame::ScrollToImpl we currently call UpdateAnchorPosForScroll before mStickyContainer->UpdatePositions (which updates the position of sticky pos frames), reversing that order might be required for this fix.

Comment 11

[David Shin[:dshin]]

1. We need to not skip painting for any scroller affecting anchor-scrolled elements, or just schedule a repaint for scroll-compensated positioned frames. This is regardless of what :tnikkel identified in Comment 10, which contain different root causes.

2. We can't just fix UpdateAnchorPosForScroll to take sticky/chain anchored elements into account, because anchor position we use to position during reflow accidentally take scrolling of those anchors into account, since GetOffsetToIgnoringScrolling uses GetPosition, which contains scroll-accounted, offseted positions for sticky/chain anchored elements. Because we can't easily separate scroll offsets from GetPosition values of these frames, we really need to fix both GetOffsetToIgnoringScrolling and AnchorPositioningUtils::GetScrollOffsetFor. As in Comment 10, we may need additional work for sticky frames - Bug 2002789 filed for this.

3. Without fixing Bug 2002789, the only workaround is to trigger reflow on scroll-compensated positioned frames that anchor to sticky/chain anchored frames.

I'll be using this bug for refining the repainting approach + triggering reflow for positioned frames anchoring to sticky/chain anchored frames.

Comment 12

[David Shin[:dshin]]

Attachment: Bug 1999954: Explicitly schedule a paint for scroll-compensated anchor-positioned frames. r=#anchor-pos

Comment 13

[David Shin[:dshin]]

Attachment: Bug 1999954: Force reflow for anchor positioned frames whose anchors are fixed/chain anchor positioned. r=#anchor-pos

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/f2dc3d6423d3
https://hg.mozilla.org/integration/autoland/rev/af330d17ec6d
Explicitly schedule a paint for scroll-compensated anchor-positioned frames. r=jwatt
https://github.com/mozilla-firefox/firefox/commit/d4a743237461
https://hg.mozilla.org/integration/autoland/rev/e43104cddd59
Force reflow for anchor positioned frames whose anchors are fixed/chain anchor positioned. r=layout-anchor-positioning-reviewers,layout-reviewers,jwatt,emilio

Comment 15

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

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

Comment 16

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/af330d17ec6d
https://hg.mozilla.org/mozilla-central/rev/e43104cddd59

Comment 17

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

Upstream PR merged by moz-wptsync-bot