Bug 189472 - Adjust default value of CSSOMViewScrollingAPI
Summary: Adjust default value of CSSOMViewScrollingAPI
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords: InRadar
Depends on:
Blocks: 5991
  Show dependency treegraph
 
Reported: 2018-09-10 01:29 PDT by Frédéric Wang (:fredw)
Modified: 2018-10-27 03:17 PDT (History)
8 users (show)

See Also:


Attachments
Patch (1.52 KB, patch)
2018-09-10 01:32 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (2.03 KB, patch)
2018-09-24 02:45 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (4.48 KB, patch)
2018-10-26 01:24 PDT, Frédéric Wang (:fredw)
fred.wang: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2018-09-10 01:29:56 PDT
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.
Comment 1 Frédéric Wang (:fredw) 2018-09-10 01:32:44 PDT
Created attachment 349300 [details]
Patch
Comment 2 Simon Fraser (smfr) 2018-09-12 10:01:32 PDT
Dean/Tim: we need to communicate new policies for EFs.
Comment 3 Frédéric Wang (:fredw) 2018-09-12 23:09:19 PDT
(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.
Comment 4 Frédéric Wang (:fredw) 2018-09-24 02:45:14 PDT
Created attachment 350628 [details]
Patch
Comment 5 Rick Byers 2018-10-25 10:55:36 PDT
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?
Comment 6 Frédéric Wang (:fredw) 2018-10-25 11:09:54 PDT
(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.
Comment 7 Frédéric Wang (:fredw) 2018-10-26 01:24:13 PDT
Created attachment 353163 [details]
Patch
Comment 8 Michael Catanzaro 2018-10-26 07:30:52 PDT
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?
Comment 9 Frédéric Wang (:fredw) 2018-10-26 08:19:35 PDT
(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.
Comment 10 Radar WebKit Bug Importer 2018-10-26 11:47:58 PDT
<rdar://problem/45594753>
Comment 11 Simon Fraser (smfr) 2018-10-26 22:26:29 PDT
If this would wait a couple of months, I'd be willing to take this change on macOS/iOS as well.
Comment 12 Frédéric Wang (:fredw) 2018-10-27 03:17:28 PDT
(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.