Adjust default value of CSSOMViewScrollingAPI

• Component: DOM
• Status: RESOLVED FIXED

[Frédéric Wang (:fredw)]

After bug 182230, the option is enabled when running tests. This bug is to enable it for developers/testers in order to allow more experiments.

[Frédéric Wang (:fredw)]

Created attachment 349300 [details]
Patch

[Simon Fraser (smfr)]

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

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

Created attachment 350628 [details]
Patch

[Rick Byers]

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?

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

Created attachment 353163 [details]
Patch

[Michael Catanzaro]

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?

[Frédéric Wang (:fredw)]

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

[Radar WebKit Bug Importer]

<rdar://problem/45594753>

[Simon Fraser (smfr)]

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

[Frédéric Wang (:fredw)]

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

[Simon Fraser (smfr)]

Comment on attachment 353163 [details]
Patch

Let's turn it on everywhere now.

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

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

[Simon Fraser (smfr)]

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.

[Frédéric Wang (:fredw)]

Created attachment 359722 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 359722 [details]
Patch for landing

Clearing flags on attachment: 359722

Committed r240250: <https://trac.webkit.org/changeset/240250>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.