Bug 189472

Created attachment 349300 [details] Patch

Dean/Tim: we need to communicate new policies for EFs.

(In reply to Simon Fraser (smfr) from comment #2) > Dean/Tim: we need to communicate new policies for EFs. Thanks. So with the new policy this bug should just be to remove CSSOMViewScrollAPI from the experimental category and hence enable the new change by default. On the one hand, I have concern that such a change could be a bit abrupt for web developers and I would prefer a smoother path that still allows to get early feedback. On the other hand, I think WebKit developers should be aware of this asap so they stop using document.body.scroll* API in standard mode when writing tests.

Created attachment 350628 [details] Patch

Simon and I chatted a bit about this at TPAC. He agreed with my assessment that the main compat risk is probably for webview and other embedders (if I recall we relied on a compat quirk in chromium to get the old behavior under Android WebView). Simon's recommendation was to expose this up to Safari as a WebPreference, and he'd get Safari to explicitly enable it. We didn't talk about the testing situation though. I assume if it'll be enabled in Safari, then you'd want it enabled for the webkit tests too?

(In reply to Rick Byers from comment #5) > Simon and I chatted a bit about this at TPAC. He agreed with my assessment > that the main compat risk is probably for webview and other embedders (if I > recall we relied on a compat quirk in chromium to get the old behavior > under Android WebView). Simon's recommendation was to expose this up to > Safari as a WebPreference, and he'd get Safari to explicitly enable it. > > We didn't talk about the testing situation though. I assume if it'll be > enabled in Safari, then you'd want it enabled for the webkit tests too? Thanks for the update, Rick. There is already a WebPreference for this. Currently, it is an "Experimental Feature" disabled by default but enabled for tests (hence not in accordance with the proposed new policy https://lists.webkit.org/pipermail/webkit-dev/2018-September/030123.html). Attachment 350628 [details] makes the feature non-experimental and enabled by default. From your comment, I think I need at least to be sure that the features remain disabled for the various iOS webviews. Not sure about other WebKit ports but I would say it's better to align with Safari's behavior.

Created attachment 353163 [details] Patch

Comment on attachment 353163 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353163&action=review The ChangeLog is messed up (duplicated). > Source/WebKit/Shared/WebPreferencesDefaultValues.h:38 > +#if PLATFORM(GTK) || PLATFORM(WPE) > +#define DEFAULT_CSSOM_VIEW_SCROLLING_API_ENABLED true > +#else > +#define DEFAULT_CSSOM_VIEW_SCROLLING_API_ENABLED false > +#endif We should only have different defaults than Mac if there's a very strong reason for it. I guess if it will be on in Safari, but not Mac platform apps, that could qualify as a strong reason. I was going to suggest just turning it on with a header for any tests that need it: <!-- webkit-test-runner [internal:CSSOMViewScrollingAPIEnabled=true ] --> But you probably want it enabled in all tests, not just specific tests?

(In reply to Michael Catanzaro from comment #8) > Comment on attachment 353163 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=353163&action=review > > The ChangeLog is messed up (duplicated). oops, will fix. > > > Source/WebKit/Shared/WebPreferencesDefaultValues.h:38 > > +#if PLATFORM(GTK) || PLATFORM(WPE) > > +#define DEFAULT_CSSOM_VIEW_SCROLLING_API_ENABLED true > > +#else > > +#define DEFAULT_CSSOM_VIEW_SCROLLING_API_ENABLED false > > +#endif > > We should only have different defaults than Mac if there's a very strong > reason for it. > > I guess if it will be on in Safari, but not Mac platform apps, that could > qualify as a strong reason. Yes per comment 5 that seems to be the plan. But let's see what Apple people say. I just wanted to be sure we are ok having the standard behavior enabled on our ports. > I was going to suggest just turning it on with a header for any tests that > need it: > > <!-- webkit-test-runner [internal:CSSOMViewScrollingAPIEnabled=true ] --> > > But you probably want it enabled in all tests, not just specific tests? The option is already enabled for all tests (I did that before the new policy proposal) and for this kind of behavior change I don't think it's actually a good idea to enable it on a per-test basis. Anyway, if Apple people confirm they want to enable it on Safari by default then this should indeed remain enabled for all tests.

<rdar://problem/45594753>

If this would wait a couple of months, I'd be willing to take this change on macOS/iOS as well.

(In reply to Simon Fraser (smfr) from comment #11) > If this would wait a couple of months, I'd be willing to take this change on > macOS/iOS as well. I think there is no hurry, we can wait that things are ready to be taken on macOS/iOS.

Comment on attachment 353163 [details] Patch Let's turn it on everywhere now.

Comment on attachment 350628 [details] Patch (In reply to Simon Fraser (smfr) from comment #13) > Comment on attachment 353163 [details] > Patch > > Let's turn it on everywhere now. OK, then I think we can take this previous patch, which still applies on ToT.

(In reply to Frédéric Wang (:fredw) from comment #14) > Comment on attachment 350628 [details] > Patch > > (In reply to Simon Fraser (smfr) from comment #13) > > Comment on attachment 353163 [details] > > Patch > > > > Let's turn it on everywhere now. > > OK, then I think we can take this previous patch, which still applies on ToT. @smfr: review ping?

Comment on attachment 350628 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350628&action=review > Source/WebKit/Shared/WebPreferences.yaml:-1199 > - category: experimental Please keep this as "category: internal" so we can switch it off in the UI for testing.

Created attachment 359722 [details] Patch for landing

Comment on attachment 359722 [details] Patch for landing Clearing flags on attachment: 359722 Committed r240250: <https://trac.webkit.org/changeset/240250>

All reviewed patches have been landed. Closing bug.