219305 – Add support for overscroll-behavior parsing

RESOLVED FIXED 219305

Add support for overscroll-behavior parsing

https://bugs.webkit.org/show_bug.cgi?id=219305

Summary Add support for overscroll-behavior parsing

cathiechen

Reported 2020-11-27 06:24:11 PST

Support parsing overscroll-behavior CSS property, per https://drafts.csswg.org/css-overscroll-1/#overscroll-behavior-properties

Attachments
Patch (96.32 KB, patch) 2020-11-27 07:53 PST, cathiechen	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (96.69 KB, patch) 2020-11-27 08:49 PST, cathiechen	no flags	Details Formatted Diff Diff
Patch (88.37 KB, patch) 2020-11-29 02:41 PST, cathiechen	no flags	Details Formatted Diff Diff
Patch (88.06 KB, patch) 2020-11-30 05:12 PST, cathiechen	no flags	Details Formatted Diff Diff
Patch (93.26 KB, patch) 2020-12-02 03:11 PST, cathiechen	no flags	Details Formatted Diff Diff
Patch (93.52 KB, patch) 2020-12-02 06:09 PST, cathiechen	no flags	Details Formatted Diff Diff
Patch (88.00 KB, patch) 2020-12-03 05:45 PST, cathiechen	no flags	Details Formatted Diff Diff
Patch (87.99 KB, patch) 2020-12-03 06:07 PST, cathiechen	no flags	Details Formatted Diff Diff
Patch (92.96 KB, patch) 2020-12-04 03:32 PST, cathiechen	no flags	Details Formatted Diff Diff
Patch (92.81 KB, patch) 2020-12-09 09:37 PST, cathiechen	no flags	Details Formatted Diff Diff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

cathiechen

Comment 1 2020-11-27 07:53:33 PST

Created attachment 414943 [details] Patch

EWS Watchlist

Comment 2 2020-11-27 07:54:18 PST

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

cathiechen

Comment 3 2020-11-27 08:49:17 PST

Created attachment 414947 [details] Patch

Sam Weinig

Comment 4 2020-11-28 11:29:26 PST

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

cathiechen

Comment 5 2020-11-29 02:19:47 PST

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

cathiechen

Comment 6 2020-11-29 02:41:13 PST

Created attachment 414989 [details] Patch

cathiechen

Comment 7 2020-11-30 05:12:54 PST

Created attachment 415016 [details] Patch

Simon Fraser (smfr)

Comment 8 2020-11-30 09:48:23 PST

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

cathiechen

Comment 9 2020-12-02 03:07:09 PST

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?

cathiechen

Comment 10 2020-12-02 03:11:13 PST

Created attachment 415203 [details] Patch

cathiechen

Comment 11 2020-12-02 06:09:57 PST

Created attachment 415217 [details] Patch

Simon Fraser (smfr)

Comment 12 2020-12-02 09:15:21 PST

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.

cathiechen

Comment 13 2020-12-03 05:37:45 PST

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.

cathiechen

Comment 14 2020-12-03 05:41:43 PST

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

cathiechen

Comment 15 2020-12-03 05:45:03 PST

Created attachment 415298 [details] Patch

cathiechen

Comment 16 2020-12-03 06:07:30 PST

Created attachment 415300 [details] Patch

cathiechen

Comment 17 2020-12-04 03:01:02 PST

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

cathiechen

Comment 18 2020-12-04 03:32:02 PST

Created attachment 415399 [details] Patch

Radar WebKit Bug Importer

Comment 19 2020-12-04 06:25:17 PST

<rdar://problem/71977393>

cathiechen

Comment 20 2020-12-09 09:37:39 PST

Created attachment 415771 [details] Patch

cathiechen

Comment 21 2020-12-09 09:56:32 PST

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

Simon Fraser (smfr)

Comment 22 2020-12-09 10:07:17 PST

Sure.

cathiechen

Comment 23 2020-12-09 22:23:30 PST

Thanks:)

EWS

Comment 24 2020-12-09 22:39:48 PST

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

Ole Strøm

Comment 25 2020-12-10 00:59:17 PST

Nice!!! 🥳 Thank you for this, Cathie!

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component CSS

Assignee

cathiechen

Reported

2020-11-27 06:24 PST

Modified

2020-12-10 00:59 PST History

CC List

17 users Show

URL

Keywords InRadar

Depends on

Blocks

176454

Dependencies

tree graph