Bug 210634

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

Or behind an experimental feature at least.

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

Sorry, the url is not accessible to me.

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"

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

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).

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

Created attachment 396988 [details] Patch

Comment on attachment 396988 [details] Patch Should it just be a RuntimeEnabledFeature instead? That would avoid all this extra work.

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 on attachment 396988 [details] Patch Windows *test* failure, I mean.

(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 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.

Created attachment 397054 [details] Patch

Created attachment 397063 [details] Patch

Created attachment 397067 [details] Patch

Created attachment 397069 [details] Patch

Created attachment 397071 [details] Patch

> 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.

Committed r260491: <https://trac.webkit.org/changeset/260491> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397071 [details].

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.

(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

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