1128102 - Reduce impact of changing CSS scroll-snap-* and scroll-behavior CSS properties.

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[:kip (Kearwood Gilbert)]

The value of some CSS properties are stored in ScrollbarStyles.  When these CSS properties are changed, nsChangeHint_ReconstructFrame is returned by nsStyleDisplay::CalcDifference.

For many of these CSS properties, nsChangeHint_ReconstructFrame is stronger than necessary, and another method of ensuring that nsCSSFrameConstructor::PropagateScrollToViewport is called could be more efficient:

- scroll-behavior
- scroll-snap-type-x
- scroll-snap-type-y
- scroll-snap-points-x
- scroll-snap-points-y
- scroll-snap-destination

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Scroll snapping shouldn't even be propagated to the viewport nowadays.

Comment 2

[Timothy Nikkel (:tnikkel)]

I've got a patch for this on try server already, looks pretty green.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Ah sweet :)

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1128102. Don't reconstruct the frame for scroll-snap-type or scroll-behavior style changes. r?emilio,dholbert

The reason these used to have to reconstruct the frame was so that we called nsPresContext::UpdateViewportScrollStylesOverride to update nsPresContext::mViewportScrollStyles. That field used to store scroll behavior and scroll snap data, it no longer does.

In the current code mScrollBehavior is only checked here https://searchfox.org/mozilla-central/rev/d34f9713ae128a3138c2b70d8041a535f1049d19/layout/generic/nsGfxScrollFrame.cpp#8111 before we perform a new scroll, so no change is needed, any future scroll operation will look in the correct place.

scroll-snap-type is similar except we also store it on the FrameMetrics we send to the compositor so we need to repaint.

Note that even currently where we reconstruct the frame on scroll-snap-type changes we do not perform snapping if we move from none to mandatory or proximity, this is bug 1530253. So by removing the frame reconstruct we are not regressing that since we currently fail to do it.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44edbb96bd2e
Don't reconstruct the frame for scroll-snap-type or scroll-behavior style changes. r=emilio

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9272254 [details]
Bug 1128102. Don't reconstruct the frame for scroll-snap-type or scroll-behavior style changes. r?emilio,dholbert

Beta/Release Uplift Approval Request

• User impact if declined: Prevents flickering and generally improves performance with stuff like bug 1764655.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: bug 1764655 shouldn't reproduce after this for example
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very simple change that avoids expensive computations on some CSS property changes.
• String changes made/needed: none

Comment 7

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/44edbb96bd2e

Comment 8

[Dianna Smith [:diannaS]]

Comment on attachment 9272254 [details]
Bug 1128102. Don't reconstruct the frame for scroll-snap-type or scroll-behavior style changes. r?emilio,dholbert

Approved for 100.0b6

Comment 9

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5285323f4056

Comment 10

[Negritas Sergiu, Desktop QA]

Hello! I have managed to reproduce the issue with Firefox 100.0a1 (2022-04-01) on Ubuntu 20.04 with the test cases provided in bug https://bugzilla.mozilla.org/show_bug.cgi?id=1764655 . I can confirm the issue is fixed with firefox 101.0a1(2022-04-15) and 100.0b6 on Ubuntu 20.04, MacOS 12 and Windows 10.

However I would like to mention that with test case number 3 there is still a flicker on the first iframe and from what I understand from the comments on refresh of the page it is expected to be red at loading the page.