Bug 210634

Summary: REGRESSION (r254790): No longer get smooth scrolling on music.apple.com
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: ScrollingAssignee: cathiechen <cathiechen>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, esprehn+autocc, ews-watchlist, fred.wang, glenn, gyuyoung.kim, hi, jbedard, joepeck, macpherson, menard, simon.fraser, tsavell, webkit-bug-importer
Priority: P1 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=205009
Bug Depends on:    
Bug Blocks: 210917    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Simon Fraser (smfr) 2020-04-16 20:58:20 PDT
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.
Comment 1 Simon Fraser (smfr) 2020-04-16 20:59:27 PDT
Parsing the new properties needs to be off by default until it actually works.
Comment 2 Simon Fraser (smfr) 2020-04-16 20:59:43 PDT
Or behind an experimental feature at least.
Comment 3 Simon Fraser (smfr) 2020-04-16 21:00:08 PDT
rdar://problem/61906093
Comment 4 Simon Fraser (smfr) 2020-04-16 21:00:44 PDT
Need to see if the page is using @supports()
Comment 5 cathiechen 2020-04-16 21:19:53 PDT
Sorry, the url is not accessible to me.
Comment 6 Simon Fraser (smfr) 2020-04-16 22:02:36 PDT
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"
Comment 7 cathiechen 2020-04-16 22:20:52 PDT
I see... I can reproduce it now. I'll take a look today.
Comment 8 cathiechen 2020-04-17 19:40:28 PDT
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).
Comment 9 Simon Fraser (smfr) 2020-04-18 09:37:09 PDT
I think you should add an Experimental Feature flag for it, and plumb down into CSS parsing.
Comment 10 cathiechen 2020-04-20 10:29:17 PDT
Created attachment 396988 [details]
Patch
Comment 11 Simon Fraser (smfr) 2020-04-20 11:07:08 PDT
Comment on attachment 396988 [details]
Patch

Should it just be a RuntimeEnabledFeature instead? That would avoid all this extra work.
Comment 12 Darin Adler 2020-04-20 15:00:56 PDT
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.
Comment 13 Darin Adler 2020-04-20 15:01:46 PDT
Comment on attachment 396988 [details]
Patch

Windows *test* failure, I mean.
Comment 14 cathiechen 2020-04-20 21:42:03 PDT
(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.
Comment 15 cathiechen 2020-04-20 21:57:42 PDT
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.
Comment 16 cathiechen 2020-04-20 22:01:41 PDT
Created attachment 397054 [details]
Patch
Comment 17 cathiechen 2020-04-21 01:52:31 PDT
Created attachment 397063 [details]
Patch
Comment 18 cathiechen 2020-04-21 02:56:22 PDT
Created attachment 397067 [details]
Patch
Comment 19 cathiechen 2020-04-21 03:26:34 PDT
Created attachment 397069 [details]
Patch
Comment 20 cathiechen 2020-04-21 03:55:44 PDT
Created attachment 397071 [details]
Patch
Comment 21 cathiechen 2020-04-21 07:28:17 PDT
> 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.
Comment 22 EWS 2020-04-21 20:03:47 PDT
Committed r260491: <https://trac.webkit.org/changeset/260491>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397071 [details].
Comment 23 Truitt Savell 2020-04-22 10:01:02 PDT
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.
Comment 24 Jonathan Bedard 2020-04-22 10:13:46 PDT
(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
Comment 25 cathiechen 2020-04-23 09:22:27 PDT
Hmmm, the flaky failure seems related to propertyInfoCache. I'll file a new bug to fix it.