Support parsing overscroll-behavior CSS property, per https://drafts.csswg.org/css-overscroll-1/#overscroll-behavior-properties
Created attachment 414943 [details] Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Created attachment 414947 [details] Patch
Comment on attachment 414947 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414947&action=review > Source/WebKitLegacy/mac/WebView/WebPreferenceKeysPrivate.h:174 > +#define WebKitOverscrollBehaviorEnabledPreferenceKey @"WebKitOverscrollBehaviorEnabled" There is no need to add this. Tests can access preference state without it. > Source/WebKitLegacy/mac/WebView/WebPreferences.mm:3119 > +- (BOOL)overscrollBehaviorEnabled > +{ > + return [self _boolValueForKey:WebKitOverscrollBehaviorEnabledPreferenceKey]; > +} > + > +- (void)setOverscrollBehaviorEnabled:(BOOL)flag > +{ > + [self _setBoolValue:flag forKey:WebKitOverscrollBehaviorEnabledPreferenceKey]; > +} There is no need to add this. Tests can access preference state without it. > Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:325 > +@property (nonatomic) BOOL overscrollBehaviorEnabled; There is no need to add this. Tests can access preference state without it. > Tools/DumpRenderTree/TestOptions.cpp:109 > + { "OverscrollBehaviorEnabled", false }, We have mostly been enabling experimental features by defaults in tests (which happens automatically). What is your intention with this change? > LayoutTests/imported/w3c/web-platform-tests/css/css-overscroll-behavior/inheritance.html:1 > +<!DOCTYPE html><!-- webkit-test-runner [ OverscrollBehaviorEnabled=true ] --> In general, we try not to edit the imported WPT tests unless we intend to upstream those changes at the same time. If you instead follow the pattern of having experimental features on by default (in the tests) this change should not be necessary.
Comment on attachment 414947 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414947&action=review Hi Sam, Thanks for the review! >> Source/WebKitLegacy/mac/WebView/WebPreferenceKeysPrivate.h:174 >> +#define WebKitOverscrollBehaviorEnabledPreferenceKey @"WebKitOverscrollBehaviorEnabled" > > There is no need to add this. Tests can access preference state without it. Done, thanks! >> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:3119 >> +} > > There is no need to add this. Tests can access preference state without it. Done >> Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:325 >> +@property (nonatomic) BOOL overscrollBehaviorEnabled; > > There is no need to add this. Tests can access preference state without it. Done >> Tools/DumpRenderTree/TestOptions.cpp:109 >> + { "OverscrollBehaviorEnabled", false }, > > We have mostly been enabling experimental features by defaults in tests (which happens automatically). What is your intention with this change? I see, I think I can remove this. >> LayoutTests/imported/w3c/web-platform-tests/css/css-overscroll-behavior/inheritance.html:1 >> +<!DOCTYPE html><!-- webkit-test-runner [ OverscrollBehaviorEnabled=true ] --> > > In general, we try not to edit the imported WPT tests unless we intend to upstream those changes at the same time. If you instead follow the pattern of having experimental features on by default (in the tests) this change should not be necessary. Yes, make sense. I'll remove this.
Created attachment 414989 [details] Patch
Created attachment 415016 [details] Patch
Comment on attachment 415016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415016&action=review > Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:597 > + humanReadableDescription: "CSS Overscroll Behavior prototype" Don't say "prototype". This should say "Enable CSS overscroll-behavior" > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3146 > + case CSSPropertyOverscrollBehavior: > + return cssValuePool.createValue(std::max(style.overscrollBehaviorX(), style.overscrollBehaviorY())); > + case CSSPropertyOverscrollBehaviorX: > + return cssValuePool.createValue(style.overscrollBehaviorX()); > + case CSSPropertyOverscrollBehaviorY: > + return cssValuePool.createValue(style.overscrollBehaviorY()); You need to check if the feature is enabled here. > Source/WebCore/platform/ScrollTypes.h:50 > + Auto = 0, No need for = 0 > LayoutTests/fast/scrolling/overscroll-behavior-invalidate-if-disable.html:13 > +<!DOCTYPE html><!-- webkit-test-runner [ OverscrollBehaviorEnabled=false ] --> > +<html> > + <head> > + <script> > + if (window.testRunner) > + testRunner.dumpAsText(); > + > + var result = "overscrollBehavior" in document.documentElement.style ? "FAIL" : "PASS"; > + document.write(`${result} test overscrollBehavior should be invalidated if overscrollBehaviorEnabled is disabled.`); > + > + </script> > + </head> > +</html> This kind of test should be a js-test-pre/js-test-post test; avoid document.write(). You also need to test getComputedStyle(). Also, I think this test should live in fast/css.
Comment on attachment 415016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415016&action=review Hi Simon, Thanks for the review! >> Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:597 >> + humanReadableDescription: "CSS Overscroll Behavior prototype" > > Don't say "prototype". This should say "Enable CSS overscroll-behavior" Done >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3146 >> + return cssValuePool.createValue(style.overscrollBehaviorY()); > > You need to check if the feature is enabled here. Done! >> Source/WebCore/platform/ScrollTypes.h:50 >> + Auto = 0, > > No need for = 0 Done. >> LayoutTests/fast/scrolling/overscroll-behavior-invalidate-if-disable.html:13 >> +</html> > > This kind of test should be a js-test-pre/js-test-post test; avoid document.write(). > > You also need to test getComputedStyle(). > Also, I think this test should live in fast/css. Done, thanks! It seems for CSSComputedStyleDeclaration we can't get Settings in CSSStyleDeclaration::namedItem by current interfaces. I added CSSComputedStyleDeclaration::getSettings() to make sure we get settings and check flags while parsing propertyID. WDYT?
Created attachment 415203 [details] Patch
Created attachment 415217 [details] Patch
Comment on attachment 415217 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415217&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2264 > +const Settings* CSSComputedStyleDeclaration::getSettings() const > +{ > + return &m_element->document().settings(); > +} You don't appear to be using this. If you do use it, call it settings() (no "get"), and have it return a reference. > Source/WebCore/css/CSSStyleDeclaration.cpp:257 > +const Settings* CSSStyleDeclaration::getSettings() const Just settings(). > Source/WebKitLegacy/win/Interfaces/IWebPreferencesPrivate.idl:279 > + HRESULT overscrollBehaviorEnabled([ out, retval ] BOOL*); > + HRESULT setOverscrollBehaviorEnabled([in] BOOL enabled); I don't think any of these Windows changes are necessary.
Comment on attachment 415217 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415217&action=review Hi Simon, Thanks for the swift review! >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2264 >> +} > > You don't appear to be using this. If you do use it, call it settings() (no "get"), and have it return a reference. Setttings is needed by propertyInfoFromJavaScriptCSSPropertyName() of CSSStyleDeclaration.cpp, to make sure the CSS property id is invalid if the settings flag is off. Hmmm, the problem of returning a reference is that CSSStyleDeclaration::settings can not ensure that it has access to Settings. It might be null.
Comment on attachment 415217 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415217&action=review >> Source/WebCore/css/CSSStyleDeclaration.cpp:257 >> +const Settings* CSSStyleDeclaration::getSettings() const > > Just settings(). Done >> Source/WebKitLegacy/win/Interfaces/IWebPreferencesPrivate.idl:279 >> + HRESULT setOverscrollBehaviorEnabled([in] BOOL enabled); > > I don't think any of these Windows changes are necessary. Done
Created attachment 415298 [details] Patch
Created attachment 415300 [details] Patch
(In reply to cathiechen from comment #14) > >> Source/WebKitLegacy/win/Interfaces/IWebPreferencesPrivate.idl:279 > >> + HRESULT setOverscrollBehaviorEnabled([in] BOOL enabled); > > > > I don't think any of these Windows changes are necessary. > > Done Hmm, the test is failed on Windows. It seems WebViewPreferencesChangedGenerated.cpp.erb has not finished yet. Looks like we need to keep this.
Created attachment 415399 [details] Patch
<rdar://problem/71977393>
Created attachment 415771 [details] Patch
Rebased the code. Hi Simon, Are you OK with the comments in #13 and #17? If yes, I guess we can land it after the EWS checking:)
Sure.
Thanks:)
Committed r270613: <https://trac.webkit.org/changeset/270613> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415771 [details].
Nice!!! 🥳 Thank you for this, Cathie!