1863620 - Use an accelerated animation for scrollbar fades.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

It seems this ought to simplify a bunch of the code in ScrollbarActivity.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: WIP: Bug 1863620 - WIP - Use an accelerated animation for scrollbar fades.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

So the attached patch seems to work (and ought to work), but it causes some flicker, only on root scrollframes. For example searchfox.org works great, but this page shows some minor flickering when scrolling.

Hiro do you happen to know off-hand what might be going on? Otherwise I'll dig more tomorrow.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

The transitions seem to start / stop at the right time...

Comment 4

[Hiroyuki Ikezoe (:hiro)]

I don't know offhand. And I actually tried the change locally I don't see any flickers at all. And in the searchfox.org case, it's not root, it's a sub scroll container. Maybe what I am seeing is a different version of searchfox?

Also note that flickering during scrolling sounds odd, because the change should only affect things during the scrollbar is fading out, right? Then it's not while scrolling.

My best guess is the searchfox what you are seeing is a different version and it uses position:sticky or some such and the flicker has happened without the change.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

The flicker is not in searchfox, it's in pages where we use the root scrollbar. But actually I found it, we're not doing the post-traversal for style roots other than the document element, fix:

diff --git a/layout/style/ServoStyleSet.cpp b/layout/style/ServoStyleSet.cpp
index e2055b7ce6dfc..91baf008a382c 100644
--- a/layout/style/ServoStyleSet.cpp
+++ b/layout/style/ServoStyleSet.cpp
@@ -811,9 +811,9 @@ bool ServoStyleSet::StyleDocument(ServoTraversalFlags aFlags) {
                   !parent->HasAnyOfFlags(Element::kAllServoDescendantBits));
 
     postTraversalRequired |=
-        Servo_TraverseSubtree(root, mRawData.get(), &snapshots, aFlags);
-    postTraversalRequired |= root->HasAnyOfFlags(
-        Element::kAllServoDescendantBits | NODE_NEEDS_FRAME);
+        Servo_TraverseSubtree(root, mRawData.get(), &snapshots, aFlags) ||
+        root->HasAnyOfFlags(Element::kAllServoDescendantBits |
+                            NODE_NEEDS_FRAME);
 
     {
       uint32_t existingBits = mDocument->GetServoRestyleRootDirtyBits();
@@ -851,14 +851,13 @@ bool ServoStyleSet::StyleDocument(ServoTraversalFlags aFlags) {
   // traversal caused, for example, the font-size to change, the SMIL style
   // won't be updated until the next tick anyway.
   if (GetPresContext()->EffectCompositor()->PreTraverse(aFlags)) {
-    nsINode* styleRoot = mDocument->GetServoRestyleRoot();
-    Element* root =
-        styleRoot->IsElement() ? styleRoot->AsElement() : rootElement;
-
-    postTraversalRequired |=
-        Servo_TraverseSubtree(root, mRawData.get(), &snapshots, aFlags);
-    postTraversalRequired |= root->HasAnyOfFlags(
-        Element::kAllServoDescendantBits | NODE_NEEDS_FRAME);
+    DocumentStyleRootIterator iter(mDocument->GetServoRestyleRoot());
+    while (Element* root = iter.GetNextStyleRoot()) {
+      postTraversalRequired |=
+          Servo_TraverseSubtree(root, mRawData.get(), &snapshots, aFlags) ||
+          root->HasAnyOfFlags(Element::kAllServoDescendantBits |
+                              NODE_NEEDS_FRAME);
+    }
   }
 
   return postTraversalRequired;

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Great to hear that. I was totally mis-reading the original comment. Now I see the flickers (on bugzilla for example), the flickers you meant is flickering on the fading out scrollbar thumb, rather than the scrolled content, that totally makes sense. :)

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1863620 - Make animation traversal work for style roots other than the document element. r=hiro

This fixes some flickering when transitioning doc-level anonymous
content.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1863620 - Set referrer info before calling SetComplete(). r=#style,#layout

This makes sure that the URLExtraData contains the right referrer info,
and thus that mChromeRulesEnabled is correct.

Remove some useless includes while at it.

Depends on D193046

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1863620 - Make variable values track the url data they came from. r=zrhoffman,#style,#layout

This is needed to be able to use chrome environment variables in
chrome stylesheets and have them work in non-chrome documents.

This will be used to communicate the right transition duration in
scrollbars.css, but should also be useful to have the right base URI for
<url>-typed custom properties.

Depends on D193047

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1863620 - Use an accelerated animation for overlay scrollbar. r=hiro

This also simplifies ScrollbarActivity, and removes some attributes that
aren't looked up while at it.

Depends on D193048

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e48259fe357
Make variable values track the url data they came from. r=zrhoffman

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76f2bafacbc1
Remove a function that should land in one of the other patches in this bug.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c84b8379060
Make animation traversal work for style roots other than the document element. r=boris

Comment 14

[Norisz Fay [:noriszfay]]

Backed out changeset 2c84b8379060 for causing failures on test_group_mouseevents.html | helper_drag_root_scrollbar_mouse_not_over_thumb.html

Backout link

Push with failures

Failure log

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/792dc1806b0f
Make animation traversal work for style roots other than the document element. r=boris

Comment 16

[Norisz Fay [:noriszfay]]

Backout was a false positive. Relanded changeset.

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/4e48259fe357
https://hg.mozilla.org/mozilla-central/rev/76f2bafacbc1
https://hg.mozilla.org/mozilla-central/rev/792dc1806b0f

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e697f4f502e3
Set referrer info before calling SetComplete(). r=hiro

Comment 19

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/e697f4f502e3

Comment 21

[Emilio Cobos Álvarez (:emilio)]

browser/base/content/test/pageinfo/browser_pageinfo_rtl.js and a couple others fail with a "never disabled vsync". I need to look into that.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e34670e74c31
Use an accelerated animation for overlay scrollbar. r=hiro

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/298c668b4347
Disable overlay scrollbar animations in more tests.

Comment 24

[Timothy Nikkel (:tnikkel)]

(In reply to Pulsebot from comment #23)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/298c668b4347
Disable overlay scrollbar animations in more tests.

I'm curious, how is this causing tests to fail?

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Partially faded scrollbars in WPT. That's because we know trigger the transition also when we show the scrollbars, not only when we hide them. On android (only) we used to not have the fade animation because the scrollbars remained visible.

Comment 26

[Sandor Molnar[:smolnar]]

Backed out for causing multiple failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/f21ddbefe76eef1f2801d2157a2d1c3ee0e21556

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | docshell/test/navigation/test_bug1379762.html | new window should only be loaded once

Failure log wpt #1

Failure log wpt #2

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1863620 - Don't evict a page from legacy bfcache for DOM mutations on scrollbars. r=smaug

Before the animation was mutating the style attribute, and WPT on
android actually triggered the mutation before unload, and never mutated
again, because of a testing-only pref:

https://searchfox.org/mozilla-central/rev/42acdc9cd5ae89222bdceeeaed7bacac755be48f/testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/firefox_android.py#142

It's possible that this improves bfcache on android given that.

Depends on D193049

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3815a2b0d112
Use an accelerated animation for overlay scrollbar. r=hiro
https://hg.mozilla.org/integration/autoland/rev/79c2895d1a95
Don't evict a page from legacy bfcache for DOM mutations on scrollbars. r=smaug

Comment 29

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/3815a2b0d112
https://hg.mozilla.org/mozilla-central/rev/79c2895d1a95

Comment 30

[Andra Esanu (needinfo me)]

(In reply to Pulsebot from comment #23)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/298c668b4347
Disable overlay scrollbar animations in more tests.

== Change summary for alert #40367 (as of Thu, 23 Nov 2023 11:01:58 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
2%	twitter LastVisualChange	linux1804-64-shippable-qr	bytecode-cached fission warm webrender	1,105.85 -> 1,078.88	Before/After
2%	twitter LastVisualChange	linux1804-64-shippable-qr	bytecode-cached fission warm webrender	1,100.32 -> 1,080.57	Before/After

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=40367

Comment 31

[Andra Esanu (needinfo me)]

(In reply to Pulsebot from comment #28)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3815a2b0d112
Use an accelerated animation for overlay scrollbar. r=hiro
https://hg.mozilla.org/integration/autoland/rev/79c2895d1a95
Don't evict a page from legacy bfcache for DOM mutations on scrollbars.
r=smaug

== Change summary for alert #40376 (as of Thu, 23 Nov 2023 14:06:20 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
5%	bing FirstVisualChange	android-hw-a51-11-0-aarch64-shippable-qr	warm webrender	194.25 -> 183.91

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=40376

Comment 32

[Donal Meehan [:dmeehan]]

Backed out of beta
https://hg.mozilla.org/releases/mozilla-beta/rev/48d2052b377a35b0f40e52b8efad5e0ea10e0ebe