Bug 210634 – REGRESSION (r254790): No longer get smooth scrolling on music.apple.com

• Component: Scrolling
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

1. Go to https://tv.apple.com/us/show/the-morning-show/umc.cmc.25tn3v8ku4b39tr6ccgb8nl6m
2. Click on the ">"button in the episodes shelf

No longer get smooth scrolling after r254790.

[Simon Fraser (smfr)]

Parsing the new properties needs to be off by default until it actually works.

[Simon Fraser (smfr)]

Or behind an experimental feature at least.

[Simon Fraser (smfr)]

rdar://problem/61906093

[Simon Fraser (smfr)]

Need to see if the page is using @supports()

[cathiechen]

Sorry, the url is not accessible to me.

[Simon Fraser (smfr)]

Oops, sorry about that.

It also reproduces on https://music.apple.com/us/browse, in any of the horizontal scrollers.

It's "scroll-behavior: smooth" combined with "scroll-snap: x mandatory"

[cathiechen]

I see... I can reproduce it now. I'll take a look today.

[cathiechen]

The page use `if ("scrollBehavior" in document.documentElement.style)` to indicate the implement of scroll-behavior. It will perform JS animated scroll if not implement.
Currently, "scrollBehavior" is available in style whether smoothscrolling enable or not. Only invalidate the css value.

Need to find a way to disable the property by settings flag in Source/WebCore/css/CSSProperties.json?
I saw the property can disable by "* runtime-flag:"(runtime flag) and "* enable-if:"(compile flag).

[Simon Fraser (smfr)]

I think you should add an Experimental Feature flag for it, and plumb down into CSS parsing.

[cathiechen]

Created attachment 396988 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 396988 [details]
Patch

Should it just be a RuntimeEnabledFeature instead? That would avoid all this extra work.

[Darin Adler]

Comment on attachment 396988 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396988&action=review

Windows build failure seems like a real thing.

> Source/WebCore/css/CSSStyleDeclaration.cpp:271
> +    auto* settingsPtr = parentElement() ? &(parentElement()->document().settings()) : nullptr;

No need for the () around the settings here.

> Source/WebCore/css/CSSStyleDeclaration.cpp:294
> +    auto* settingsPtr = parentElement() ? &(parentElement()->document().settings()) : nullptr;

Ditto.

> Source/WebCore/css/CSSStyleDeclaration.cpp:329
> +            // TODO: Should take account for flags in settings().

WebKit project uses FIXME, not TODO.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:146
> +        // TODO: Should take account for flags in settings().

Ditto.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:607
> +            // TODO: Should take account for flags in settings().

Ditto.

[Darin Adler]

Comment on attachment 396988 [details]
Patch

Windows *test* failure, I mean.

[cathiechen]

(In reply to Simon Fraser (smfr) from comment #11)
> Comment on attachment 396988 [details]
> Patch
> 
> Should it just be a RuntimeEnabledFeature instead? That would avoid all this
> extra work.

Hi Simon,

I don't have a strong opinion about this. I'm fine to use RuntimeEnabledFeature instead.
Honestly, I feel a bit confused about when to use RuntimeEnabledFeature or Settings.
Experimental WebPreference + settings is quite popular now, so scroll-behavior used it.
And in that case, it seems necessary to add an option to CSSProperties.json.
However, if we decide the CSS properties should use RuntimeEnabledFeature instead, that also 
seems reasonable to me.

[cathiechen]

Comment on attachment 396988 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396988&action=review

Hi Darin,
Thanks for the review:)
Regarding the Windows test failure, sorry, I don't have Windows work evn.
It seems this change is not platform related. If the experimental flag works well, it should be fine...

>> Source/WebCore/css/CSSStyleDeclaration.cpp:271
>> +    auto* settingsPtr = parentElement() ? &(parentElement()->document().settings()) : nullptr;
> 
> No need for the () around the settings here.

Done.

>> Source/WebCore/css/CSSStyleDeclaration.cpp:329
>> +            // TODO: Should take account for flags in settings().
> 
> WebKit project uses FIXME, not TODO.

Done.

[cathiechen]

Created attachment 397054 [details]
Patch

[cathiechen]

Created attachment 397063 [details]
Patch

[cathiechen]

Created attachment 397067 [details]
Patch

[cathiechen]

Created attachment 397069 [details]
Patch

[cathiechen]

Created attachment 397071 [details]
Patch

[cathiechen]

> Regarding the Windows test failure, sorry, I don't have Windows work evn.
> It seems this change is not platform related. If the experimental flag works
> well, it should be fine...

CSSOMViewSmoothScrolling seems not supported on win/DumpRenderTree.cpp. It works well now.

[EWS]

Committed r260491: <https://trac.webkit.org/changeset/260491>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397071 [details].

[Truitt Savell]

It looks like the new test fast/scrolling/scroll-behavior-invalidate-if-disabled.html

added in https://trac.webkit.org/changeset/260491/webkit

is a flaky failure.

History:
https://results.webkit.org/?suite=layout-tests&test=fast%2Fscrolling%2Fscroll-behavior-invalidate-if-disabled.html

Diff:
--- /Volumes/Data/slave/catalina-release-tests-wk1/build/layout-test-results/fast/scrolling/scroll-behavior-invalidate-if-disabled-expected.txt
+++ /Volumes/Data/slave/catalina-release-tests-wk1/build/layout-test-results/fast/scrolling/scroll-behavior-invalidate-if-disabled-actual.txt
@@ -1 +1 @@
-Pass test scrollBehavior should be invalidate if CSSOMViewSmoothScrollingEnabled is disabled.
+Fail test scrollBehavior should be invalidate if CSSOMViewSmoothScrollingEnabled is disabled.

[Jonathan Bedard]

(In reply to Truitt Savell from comment #23)
> It looks like the new test
> fast/scrolling/scroll-behavior-invalidate-if-disabled.html
> 
> added in https://trac.webkit.org/changeset/260491/webkit
> 
> is a flaky failure.

But only on WebKit 1, for what that's worth:

https://results.webkit.org/?suite=layout-tests&test=fast%2Fscrolling%2Fscroll-behavior-invalidate-if-disabled.html&flavor=wk1&flavor=wk2

[cathiechen]

Hmmm, the flaky failure seems related to propertyInfoCache. I'll file a new bug to fix it.