Bug 188043 - Add support for ScrollOptions' ScrollBehavior and CSS scroll-behavior properties
Summary: Add support for ScrollOptions' ScrollBehavior and CSS scroll-behavior properties
Status: NEW
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: https://drafts.csswg.org/cssom-view/#...
Keywords:
Depends on: 188300 188343 189258 189352 189541 189579 189787 189894
Blocks: 189907 191357
  Show dependency treegraph
 
Reported: 2018-07-26 06:41 PDT by Frédéric Wang (:fredw)
Modified: 2018-12-07 10:52 PST (History)
14 users (show)

See Also:


Attachments
WIP Patch (32.23 KB, patch)
2018-08-02 08:05 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
WIP Patch (85.52 KB, patch)
2018-08-03 12:11 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
WIP Patch (88.48 KB, patch)
2018-08-06 08:09 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
WIP Patch (96.00 KB, patch)
2018-08-24 08:20 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Testcase (5.72 KB, text/html)
2018-08-30 03:28 PDT, Frédéric Wang (:fredw)
no flags Details
WIP Patch (107.77 KB, patch)
2018-08-30 03:34 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
WIP Patch (102.21 KB, patch)
2018-08-30 09:02 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Testcase (scrollIntoView) (2.68 KB, text/html)
2018-09-05 03:02 PDT, Frédéric Wang (:fredw)
no flags Details
Patch (116.39 KB, patch)
2018-09-05 03:04 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (applies on top of 189352) (112.66 KB, patch)
2018-09-06 10:36 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (113.68 KB, patch)
2018-09-11 06:01 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (113.70 KB, patch)
2018-09-12 02:35 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (113.81 KB, patch)
2018-09-13 07:55 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch for EWS (includes patch from bug 189579) (114.75 KB, patch)
2018-09-17 01:53 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (169.33 KB, patch)
2018-09-20 13:20 PDT, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.33 MB, application/zip)
2018-09-20 15:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.33 MB, application/zip)
2018-09-20 17:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews201 for win-future (12.78 MB, application/zip)
2018-09-20 17:49 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews205 for win-future (12.80 MB, application/zip)
2018-09-20 20:10 PDT, Build Bot
no flags Details
Patch (157.87 KB, patch)
2018-09-21 13:03 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (add missing header in TextCodecReplacement.cpp) (153.93 KB, patch)
2018-09-23 07:45 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (add missing header in TextCodecReplacement.cpp) (153.92 KB, patch)
2018-09-23 07:46 PDT, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.39 MB, application/zip)
2018-09-23 09:07 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.03 MB, application/zip)
2018-09-23 09:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (3.08 MB, application/zip)
2018-09-23 09:56 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (3.08 MB, application/zip)
2018-09-23 11:59 PDT, Build Bot
no flags Details
Patch (105.36 KB, patch)
2018-09-23 22:51 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.53 MB, application/zip)
2018-09-24 00:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.99 MB, application/zip)
2018-09-24 00:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (3.07 MB, application/zip)
2018-09-24 00:58 PDT, Build Bot
no flags Details
Patch (119.55 KB, patch)
2018-09-24 01:19 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (123.68 KB, patch)
2018-11-06 02:57 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (123.68 KB, patch)
2018-11-06 03:45 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (132.47 KB, patch)
2018-11-07 00:58 PST, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.44 MB, application/zip)
2018-11-07 02:57 PST, Build Bot
no flags Details
Patch (129.64 KB, patch)
2018-11-07 04:35 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.55 MB, application/zip)
2018-11-07 06:27 PST, Build Bot
no flags Details
Patch (134.71 KB, patch)
2018-11-09 01:15 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (134.62 KB, patch)
2018-11-09 01:25 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (132.07 KB, patch)
2018-11-29 13:27 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch with workaround for bug 192134 comment 9 (127.98 KB, patch)
2018-11-29 23:34 PST, 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-07-26 06:41:00 PDT
ScrollToOptions is implemented without scroll behavior support (bug 161610) and ScrollIntoViewOptions is not implemented yet (bug 161611). Both of them derive from ScrollOptions, which accepts a ScrollBehavior parameter:

https://drafts.csswg.org/cssom-view/#enumdef-scrollbehavior

When the specified ScrollBehavior is "auto" (default), the choice between instant or smooth scrolling is decided from the CSS property:

https://drafts.csswg.org/cssom-view/#propdef-scroll-behavior

For details, see https://drafts.csswg.org/cssom-view/#scrolling
Comment 1 Frédéric Wang (:fredw) 2018-08-02 08:05:54 PDT
Created attachment 346381 [details]
WIP Patch

WIP. This is just some CSS parsing + DOM API changes + refactoring... Still need to do the actual implementation & testing, use compilation / runtime flag and split the patch.
Comment 2 Frédéric Wang (:fredw) 2018-08-03 12:11:52 PDT
Created attachment 346513 [details]
WIP Patch
Comment 3 jonjohnjohnson 2018-08-06 07:54:14 PDT
Beware "scroll-behavior on body element not propagated to the viewport" - https://github.com/w3c/csswg-drafts/issues/2990
Comment 4 Frédéric Wang (:fredw) 2018-08-06 08:09:48 PDT
Created attachment 346627 [details]
WIP Patch

Rebasing
Comment 5 jonjohnjohnson 2018-08-06 14:27:44 PDT
fred.wang@free.fr, can you confirm how similarly webkits implementation of scroll-snap feels to the coming scroll-behavior: smooth? Wondering, because of plans to use programmatic and user scrolling with scroll-snap and hoping they feel the same as far as the timing function. An example would be, http://output.jsbin.com/xirowix, scroll horizontally to see the menu open.
Comment 6 Frédéric Wang (:fredw) 2018-08-07 00:50:30 PDT
(In reply to jonjohnjohnson from comment #5)
> fred.wang@free.fr, can you confirm how similarly webkits implementation of
> scroll-snap feels to the coming scroll-behavior: smooth? Wondering, because
> of plans to use programmatic and user scrolling with scroll-snap and hoping
> they feel the same as far as the timing function. An example would be,
> http://output.jsbin.com/xirowix, scroll horizontally to see the menu open.

Thanks for your interest and comment. To be honest, the current patch is far from being ready for now I've only landed some preliminary refactoring and prepared the DOM/CSS parsing and developer flags. For the actual smooth scrolling it probably makes sense to be compatible with CSS scroll snap. Note however that scroll snap spec was not always clear about programmatic scrolling last time I checked (e.g. bug 160622) and the CSSOM View is very vague about smooth scrolling (  https://drafts.csswg.org/cssom-view/#concept-smooth-scroll ). I'll keep people updated here once I make more progress.
Comment 7 jonjohnjohnson 2018-08-07 07:21:58 PDT
(In reply to Frédéric Wang (:fredw) from comment #6)
> Thanks for your interest and comment.

Mostly interested in how when is used, the user perceives that they provide the same experience. Less on compatibility between the features and more that the duration and timing functions (easing) of scroll effects between the two features "fit" with each other, when devs may want to use both features on the same scrollport/snapport.

In that example I linked, http://output.jsbin.com/xirowix, it shows just that. And if you view it in chrome, or in this video http://cl.ly/1S0P472I203S, you can see how user horizontal scrolling feels different than when you would click the menu text link to programmatically open the menu with smooth scrolling. The smooth seems to have a longer duration and closer to linear timing function. When brought up to the csswg it's been difficult to explain my quandary, I hope I'm being clear here.
Comment 8 Frédéric Wang (:fredw) 2018-08-07 09:33:06 PDT
(In reply to jonjohnjohnson from comment #7)
> Mostly interested in how when is used, the user perceives that they provide
> the same experience. Less on compatibility between the features and more
> that the duration and timing functions (easing) of scroll effects between
> the two features "fit" with each other, when devs may want to use both
> features on the same scrollport/snapport.
> 

As I said, I have not looked into the actual scrolling yet (and I'll only be back two this in two weeks) so for now it's just speculation.... I want to check what is currently available in WebKit and see whether we can rely on it (and hence share the same implementation/behavior). But the thing here is AFAIK for iOS user's smooth scrolling & timing is handled in the UI process while my guess for programmatic scrolling is that we would just do things in the Web process, so that would be two separate code paths. The scroll position in the two processes are regularly synchronized but their might be some timing delay. Not to mention that other ports implement scrolling differently. So at that point I'm concern about possible inconsistencies, but let's see.

> In that example I linked, http://output.jsbin.com/xirowix, it shows just
> that. And if you view it in chrome, or in this video
> http://cl.ly/1S0P472I203S, you can see how user horizontal scrolling feels
> different than when you would click the menu text link to programmatically
> open the menu with smooth scrolling. The smooth seems to have a longer
> duration and closer to linear timing function. When brought up to the csswg
> it's been difficult to explain my quandary, I hope I'm being clear here.

Regarding the CSSWG, I understand they don't want to overspecify things for now. I guess that's not bad as that gives us some freedom, let's see what happens with implementations and if we can use that experience to clarify the spec later, if needed.
Comment 9 jonjohnjohnson 2018-08-07 10:21:49 PDT
(In reply to Frédéric Wang (:fredw) from comment #8)

> ...So at that point I'm concern about possible inconsistencies,
> but let's see.

> Regarding the CSSWG, I understand they don't want to overspecify things for
> now. I guess that's not bad as that gives us some freedom, let's see what
> happens with implementations and if we can use that experience to clarify
> the spec later, if needed.

Completely understandable, just highlighting that ideally the two types of scrolling will offer the same affect, for the users sake, and that's as much as I'd hope the spec would like to suggest. Consistency within individual browsers.

Good luck when you dig into the codebase in two weeks or so. Thanks for your time. And aside from the intricacies of different UI and web processes, I know that, even if @tabatkins thinks the timing functions shouldn't ideally be treated the same between snap and smooth, I think the landscape of native inertial UIs and specifically this js implementation of what would be both snap and smooth http://output.jsbin.com/micaz/ it shows that a consistent timing function/shape feels ideal. Grab or scroll the block block around. Tap/click the red block. Each way, it uses the same bezier shape and duration to animate it with a css transition.

Looking forward to seeing how your implementation ended up. :D
Comment 10 Frédéric Wang (:fredw) 2018-08-24 08:20:53 PDT
Created attachment 348010 [details]
WIP Patch
Comment 11 Frédéric Wang (:fredw) 2018-08-30 03:28:43 PDT
Created attachment 348493 [details]
Testcase
Comment 12 Frédéric Wang (:fredw) 2018-08-30 03:34:34 PDT
Created attachment 348494 [details]
WIP Patch

This patch relies on existing UI smooth scrolling on Linux to implement programmatic smooth scrolling.

Status:
- CSS & DOM parsing of scroll behaviors implemented.
- Window or Element scrolling is implemented.
- Works for horizontal and vertical scrolling but not both directions (due to current implementation of ScrollAnimationSmooth)
- Behavior is not as smooth as Firefox/Chromium (again this is the implementation of ScrollAnimationSmooth)
- Only tested on WebKitGTK, probably some bugs and missing features to handle (clamping, snapping...).
Comment 13 Frédéric Wang (:fredw) 2018-08-30 09:02:08 PDT
Created attachment 348500 [details]
WIP Patch
Comment 14 Frédéric Wang (:fredw) 2018-08-31 07:51:20 PDT
(In reply to Frédéric Wang (:fredw) from comment #8)
> (In reply to jonjohnjohnson from comment #7)
> > Mostly interested in how when is used, the user perceives that they provide
> > the same experience. Less on compatibility between the features and more
> > that the duration and timing functions (easing) of scroll effects between
> > the two features "fit" with each other, when devs may want to use both
> > features on the same scrollport/snapport.
> > 
> 
> As I said, I have not looked into the actual scrolling yet (and I'll only be
> back two this in two weeks) so for now it's just speculation.... I want to
> check what is currently available in WebKit and see whether we can rely on
> it (and hence share the same implementation/behavior). But the thing here is
> AFAIK for iOS user's smooth scrolling & timing is handled in the UI process
> while my guess for programmatic scrolling is that we would just do things in
> the Web process, so that would be two separate code paths. The scroll
> position in the two processes are regularly synchronized but their might be
> some timing delay. Not to mention that other ports implement scrolling
> differently. So at that point I'm concern about possible inconsistencies,
> but let's see.

So after fixing some regressions with the Safari external SDK build, I've been able to confirm that my proof-of-concept patch works on iOS and macOS too.

I've relied on the existing ScrollAnimationSmooth class to emulate the programmatic scrolling and as I said, this is done in the Web process. It seems Chrome devs did similar thing but implemented a completely separate animator class: 

https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/scroll/programmatic_scroll_animator.h

So after this first experiment, I see there could be consistenties between UI vs programmatic scrolls or between browsers. In particular, I guess we need to decide parameters to define the exact scroll animation (e.g. acceleration or deceleration of the smooth scrolling, Bézier curve etc) and these are not specified by the CSSOM-view spec. At the moment, the default values in ScrollAnimationSmooth give a less smooth behavior than what I see in Firefox and Chrome. It's also likely the scrolling is different from UI scrolling (which IIUC is based on the initial strength of the gesture rather than the distance between initial/final position).
Comment 15 jonjohnjohnson 2018-08-31 09:04:43 PDT
Great to hear that things are progressing! 

(In reply to Frédéric Wang (:fredw) from comment #14)
> It's also likely the scrolling is different from UI
> scrolling (which IIUC is based on the initial strength of the gesture rather
> than the distance between initial/final position).

In this link http://output.jsbin.com/micaz/ when you tap the red block or when you scroll on mobile (or drag on desktop), the same timing/feel is applied. Does it feel "incorrect" to you when doing one interaction or the other?

I think both types of interactions feel "correct" regardless of strength of fling or distance. I have the inkling that users are used to this uniformity from common gestures or movements of native interfaces, like drawers or menus moving around the screen.

> In particular, I guess we need to decide parameters to define the exact scroll animation (e.g. acceleration or deceleration of the smooth scrolling, Bézier curve etc) and these are not specified by the CSSOM-view spec.

I think whatever parameters are decided would benefit from being consistent between "ScrollAnimationSmooth" and "UI scrolling" within a browser. Again, not necessarily between browsers. In my original case (http://output.jsbin.com/xirowix) when users are moving things around the screen with both types of scrolling, the inconsistencies are just too noticeable in most browsers implementations.
Comment 16 Frédéric Wang (:fredw) 2018-09-05 03:02:09 PDT
Created attachment 348903 [details]
Testcase (scrollIntoView)
Comment 17 Frédéric Wang (:fredw) 2018-09-05 03:04:57 PDT
Created attachment 348904 [details]
Patch

Rebasing after https://trac.webkit.org/changeset/235659...

This new version supports smooth scrolling for scrollIntoView (see attachment 348903 [details]).
Comment 18 Frédéric Wang (:fredw) 2018-09-06 10:36:02 PDT
Created attachment 349043 [details]
Patch (applies on top of 189352)
Comment 19 Frédéric Wang (:fredw) 2018-09-11 06:01:20 PDT
Created attachment 349393 [details]
Patch

This is essentially rebasing and minor header issue. It also addresses some scroll offset synchronization bug between the animation instances and the scrollarea.
Comment 20 Frédéric Wang (:fredw) 2018-09-12 02:35:52 PDT
Created attachment 349536 [details]
Patch
Comment 21 Frédéric Wang (:fredw) 2018-09-13 07:55:37 PDT
Created attachment 349668 [details]
Patch

This version now works with scrolling in two directions at the same time. So I think the patch is now good enough for a first implementation. I need to write tests and fix the build failure with unified builds before asking a review.
Comment 22 Frédéric Wang (:fredw) 2018-09-17 01:53:23 PDT
Created attachment 349879 [details]
Patch for EWS (includes patch from bug 189579)
Comment 23 Frédéric Wang (:fredw) 2018-09-20 13:20:41 PDT
Created attachment 350255 [details]
Patch
Comment 24 Build Bot 2018-09-20 13:23:19 PDT
Attachment 350255 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 77 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Build Bot 2018-09-20 15:27:35 PDT
Comment on attachment 350255 [details]
Patch

Attachment 350255 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9286806

New failing tests:
fast/frames/flattening/scrolling-in-object.html
Comment 26 Build Bot 2018-09-20 15:27:37 PDT
Created attachment 350271 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 27 Build Bot 2018-09-20 17:27:01 PDT
Comment on attachment 350255 [details]
Patch

Attachment 350255 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9288030

New failing tests:
fast/frames/flattening/scrolling-in-object.html
Comment 28 Build Bot 2018-09-20 17:27:03 PDT
Created attachment 350286 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 29 Build Bot 2018-09-20 17:49:21 PDT
Comment on attachment 350255 [details]
Patch

Attachment 350255 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9288403

New failing tests:
fast/events/mouse-moved-remove-frame-crash.html
fast/repaint/object-as-iframe-navigate-to-same-document-anchor-repaint.html
fast/repaint/object-as-iframe-hide-and-show-document-at-anchor.html
Comment 30 Build Bot 2018-09-20 17:49:33 PDT
Created attachment 350288 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 31 Build Bot 2018-09-20 20:10:15 PDT
Comment on attachment 350255 [details]
Patch

Attachment 350255 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9290168

New failing tests:
fast/repaint/object-as-iframe-hide-and-show-document-at-anchor.html
fast/repaint/object-as-iframe-navigate-to-same-document-anchor-repaint.html
fast/events/mouse-moved-remove-frame-crash.html
Comment 32 Build Bot 2018-09-20 20:10:27 PDT
Created attachment 350309 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 33 Michael Catanzaro 2018-09-20 20:27:03 PDT
Why do you need the compile flag?

Nowadays we prefer just the runtime flag unless there is some particular reason that a compile flag is additionally required (e.g. new dependency, or platform-specific code).
Comment 34 Frédéric Wang (:fredw) 2018-09-21 10:23:27 PDT
(In reply to Michael Catanzaro from comment #33)
> Why do you need the compile flag?
> 
> Nowadays we prefer just the runtime flag unless there is some particular
> reason that a compile flag is additionally required (e.g. new dependency, or
> platform-specific code).

If this feature is not considered stable enough and port maintainers want to completely disable this feature then the compile-time flag is still necessary (we cannot disable properties at runtime in the CSS parser). I personally don't need it but let's see what reviewers say.
Comment 35 Michael Catanzaro 2018-09-21 13:02:10 PDT
(In reply to Frédéric Wang (:fredw) from comment #34)
> (we cannot disable properties at runtime in the CSS parser)

That seems like a good answer to me.
Comment 36 Frédéric Wang (:fredw) 2018-09-21 13:03:59 PDT
Created attachment 350409 [details]
Patch
Comment 37 Frédéric Wang (:fredw) 2018-09-21 13:06:29 PDT
I'm not sure what are these build errors on MacOS (it does not happen for me locally) ; let's try again EWS. I suspect it is again an instance of UnifiedBuild sources rotating. Error message seems similar to bug 189541.
Comment 38 Simon Fraser (smfr) 2018-09-21 13:10:24 PDT
Sure we can; see CSSParserContext.
Comment 39 Frédéric Wang (:fredw) 2018-09-23 07:45:24 PDT
Created attachment 350566 [details]
Patch (add missing header in TextCodecReplacement.cpp)
Comment 40 Frédéric Wang (:fredw) 2018-09-23 07:46:55 PDT
Created attachment 350567 [details]
Patch (add missing header in TextCodecReplacement.cpp)
Comment 41 Build Bot 2018-09-23 09:06:59 PDT
Comment on attachment 350567 [details]
Patch (add missing header in TextCodecReplacement.cpp)

Attachment 350567 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9321274

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-subframe-root.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-default-css.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-element.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-scrollintoview-nested.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-subframe-window.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-root.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-window.html
Comment 42 Build Bot 2018-09-23 09:07:02 PDT
Created attachment 350568 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 43 Build Bot 2018-09-23 09:30:21 PDT
Comment on attachment 350567 [details]
Patch (add missing header in TextCodecReplacement.cpp)

Attachment 350567 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9321311

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-subframe-root.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-element.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-smooth-positions.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-subframe-window.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-root.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-window.html
Comment 44 Build Bot 2018-09-23 09:30:23 PDT
Created attachment 350571 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 45 Build Bot 2018-09-23 09:56:41 PDT
Comment on attachment 350567 [details]
Patch (add missing header in TextCodecReplacement.cpp)

Attachment 350567 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9321332

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-subframe-root.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-default-css.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-element.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-scrollintoview-nested.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-subframe-window.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-root.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-window.html
Comment 46 Build Bot 2018-09-23 09:56:44 PDT
Created attachment 350572 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 47 Build Bot 2018-09-23 11:59:54 PDT
Comment on attachment 350567 [details]
Patch (add missing header in TextCodecReplacement.cpp)

Attachment 350567 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9322059

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-subframe-root.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-default-css.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-element.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-scrollintoview-nested.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-subframe-window.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-root.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-window.html
Comment 48 Build Bot 2018-09-23 11:59:56 PDT
Created attachment 350579 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 49 Frédéric Wang (:fredw) 2018-09-23 22:51:54 PDT
Created attachment 350610 [details]
Patch

Removing the compilation flag.
Comment 50 Build Bot 2018-09-24 00:15:10 PDT
Comment on attachment 350610 [details]
Patch

Attachment 350610 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9327212

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-subframe-root.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-default-css.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-element.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-scrollintoview-nested.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-subframe-window.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-root.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-window.html
Comment 51 Build Bot 2018-09-24 00:15:12 PDT
Created attachment 350615 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 52 Build Bot 2018-09-24 00:27:25 PDT
Comment on attachment 350610 [details]
Patch

Attachment 350610 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9327210

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-subframe-root.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-element.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-smooth-positions.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-subframe-window.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-root.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-window.html
Comment 53 Build Bot 2018-09-24 00:27:28 PDT
Created attachment 350617 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 54 Build Bot 2018-09-24 00:58:07 PDT
Comment on attachment 350610 [details]
Patch

Attachment 350610 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9327240

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-subframe-root.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-default-css.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-element.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-scrollintoview-nested.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-subframe-window.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-root.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-window.html
Comment 55 Build Bot 2018-09-24 00:58:09 PDT
Created attachment 350620 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 56 Frédéric Wang (:fredw) 2018-09-24 01:19:32 PDT
Created attachment 350621 [details]
Patch
Comment 57 Simon Fraser (smfr) 2018-10-12 14:11:06 PDT
Tim, do you think we need to do UI process scrolling here?
Comment 58 Frédéric Wang (:fredw) 2018-11-06 02:57:02 PST
Created attachment 353958 [details]
Patch
Comment 59 Frédéric Wang (:fredw) 2018-11-06 03:45:20 PST
Created attachment 353960 [details]
Patch

Trying to resend to EWS after having fixed the issue with rotating unified sources / missing headers.
Comment 60 Frédéric Wang (:fredw) 2018-11-07 00:58:51 PST
Created attachment 354071 [details]
Patch
Comment 61 Build Bot 2018-11-07 02:57:06 PST
Comment on attachment 354071 [details]
Patch

Attachment 354071 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9891489

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-subframe-root.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-element.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-smooth-positions.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-subframe-window.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-root.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-window.html
Comment 62 Build Bot 2018-11-07 02:57:08 PST
Created attachment 354079 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 63 Frédéric Wang (:fredw) 2018-11-07 04:35:06 PST
Created attachment 354082 [details]
Patch
Comment 64 Build Bot 2018-11-07 06:27:33 PST
Comment on attachment 354082 [details]
Patch

Attachment 354082 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9892834

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-subframe-root.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-element.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-smooth-positions.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-subframe-window.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-root.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-window.html
Comment 65 Build Bot 2018-11-07 06:27:35 PST
Created attachment 354086 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 66 Frédéric Wang (:fredw) 2018-11-07 10:19:33 PST
(In reply to Build Bot from comment #65)
> Created attachment 354086 [details]
> Archive of layout-test-results from ews107 for mac-sierra-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-wk2-ews.
> Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

OK, it looks like the case "Aborting an ongoing smooth scrolling on an element with an instant scrolling" no longer pass since I initially wrote this test. I need to figure out what's going on here...
Comment 67 Frédéric Wang (:fredw) 2018-11-09 01:15:07 PST
Created attachment 354319 [details]
Patch
Comment 68 Frédéric Wang (:fredw) 2018-11-09 01:16:51 PST
(In reply to Frédéric Wang (:fredw) from comment #66)
> OK, it looks like the case "Aborting an ongoing smooth scrolling on an
> element with an instant scrolling" no longer pass since I initially wrote
> this test. I need to figure out what's going on here...

OK, I just had forgotten to modify ScrollAnimatorMac::cancelAnimations() to call the parent implementation... Hopefully this is ready for review now.
Comment 69 Frédéric Wang (:fredw) 2018-11-09 01:25:26 PST
Created attachment 354320 [details]
Patch
Comment 70 Frédéric Wang (:fredw) 2018-11-22 07:13:17 PST
For those who are interested I made a quick video here: https://vimeo.com/301932304

(In reply to Simon Fraser (smfr) from comment #57)
> Tim, do you think we need to do UI process scrolling here?

Indeed I would greatly appreciate some feedback on this thanks.

That said, I believe a large portion of this patch makes sense independently of actual the smooth scrolling implementation (CSS parsing, DOM APIs, tests...) and the UI process question seems very port-specific (iOS is the only port doing user scrolling in the UI process IIUC). How about taking the current proposal (it's currently under a preference flag) and do any later improvements in follow-up bugs?
Comment 71 Don Olmstead 2018-11-29 10:08:20 PST
Comment on attachment 354320 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=354320&action=review

Informal review.

Couple nits on returns.

Also remove code for ScrollAnimationSmooth since that's going away when https://bugs.webkit.org/show_bug.cgi?id=192128 lands.

> Source/WebCore/page/DOMWindow.cpp:1545
> +    return scrollBy(fromCoordinates(x, y));

No reason to have a return in a void

> Source/WebCore/page/DOMWindow.cpp:1567
> +    return scrollTo(fromCoordinates(x, y), clamping);

Same
Comment 72 Don Olmstead 2018-11-29 10:09:34 PST
(In reply to Don Olmstead from comment #71)
> Comment on attachment 354320 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354320&action=review
> 
> Informal review.
> 
> Couple nits on returns.
> 
> Also remove code for ScrollAnimationSmooth since that's going away when
> https://bugs.webkit.org/show_bug.cgi?id=192128 lands.
> 
> > Source/WebCore/page/DOMWindow.cpp:1545
> > +    return scrollBy(fromCoordinates(x, y));
> 
> No reason to have a return in a void
> 
> > Source/WebCore/page/DOMWindow.cpp:1567
> > +    return scrollTo(fromCoordinates(x, y), clamping);
> 
> Same

Sorry ScrollAnimaTORSmooth. Those names are way too much alike...
Comment 73 Frédéric Wang (:fredw) 2018-11-29 13:24:19 PST
Comment on attachment 354320 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=354320&action=review

Thanks Don! Yes, I'll remove the changes to ScrollAnimatorSmooth (<-- hopefully I got it right this time).

>>> Source/WebCore/page/DOMWindow.cpp:1545
>>> +    return scrollBy(fromCoordinates(x, y));
>> 
>> No reason to have a return in a void
> 
> Sorry ScrollAnimaTORSmooth. Those names are way too much alike...

Good catch, it seems the return was originally present when I uploaded attachment 346381 [details] and I wrongly keep it in follow-up rebase.
Comment 74 Frédéric Wang (:fredw) 2018-11-29 13:27:00 PST
Created attachment 356043 [details]
Patch
Comment 75 Frédéric Wang (:fredw) 2018-11-29 23:34:44 PST
Created attachment 356151 [details]
Patch with workaround for bug 192134 comment 9

The new mac build failures are due to UnifiedBuild sources rotating. See https://bugs.webkit.org/show_bug.cgi?id=192134#c9
Comment 76 Simon Fraser (smfr) 2018-11-30 13:30:48 PST
Comment on attachment 356151 [details]
Patch with workaround for bug 192134 comment 9

View in context: https://bugs.webkit.org/attachment.cgi?id=356151&action=review

This looks OK but I'm concerned about whether we'll find the animation curve acceptable on macOS and iOS. Ideally we'd drive the animated scroll on the scrolling thread, and for iOS run it in the UI process.

> Source/WebCore/page/DOMWindow.cpp:1590
> +    // FIXME: Should we use document()->scrollingElement()?
> +    // See https://github.com/w3c/csswg-drafts/issues/2977

Seems odd to ask this question here.

> Source/WebCore/page/ScrollToOptions.h:47
> +    ScrollToOptions options;
> +    options.left = x;
> +    options.top = y;
> +    return options;

Can this be return { x, y } ?

> Source/WebCore/rendering/RenderLayer.cpp:2561
> +                // FIXME: Should we use contentDocument()->scrollingElement()?
> +                // See https://github.com/w3c/csswg-drafts/issues/2977
> +                if (ownerElement->contentDocument() && ownerElement->contentDocument()->documentElement() && useSmoothScrolling(options.behavior, *ownerElement->contentDocument()->documentElement()))

This doesn't seem to be the right place to be asking this question. Surely all FrameView scrolls should do whatever scrolling via scrollingElement does.

> Source/WebCore/rendering/RenderLayer.cpp:2592
> +            // See https://github.com/w3c/csswg-drafts/issues/2977
> +            if (renderer().document().documentElement() && useSmoothScrolling(options.behavior, *renderer().document().documentElement()))

Ditto.
Comment 77 Frédéric Wang (:fredw) 2018-11-30 13:53:37 PST
Comment on attachment 356151 [details]
Patch with workaround for bug 192134 comment 9

View in context: https://bugs.webkit.org/attachment.cgi?id=356151&action=review

Thanks for the review. As I said in comment 70, I think a large portion of this patch is valid anyway. Maybe we can take it (keeping the feature disabled by default) and refine the smooth scrolling implementation in follow-up patches?

>> Source/WebCore/page/DOMWindow.cpp:1590
>> +    // See https://github.com/w3c/csswg-drafts/issues/2977
> 
> Seems odd to ask this question here.

The spec says that the scroll behavior is taken from the associated element, which is always the root element.
However, the spec also says that window.scroll() should be invoked when scrollTo is called on the scrollingElement (which can be either the root or the body).
So it seems to me that for consistency the associated element should be the scrollingElement.

However, it's true it's a spec question. How do you suggest to make the comment in WebKit?

>> Source/WebCore/page/ScrollToOptions.h:47
>> +    return options;
> 
> Can this be return { x, y } ?

IIRC, I got build failures when trying to cast brace-enclosed initializer to ScrollToOptions now that we have the parent ScrollOptions class with a "behavior" member. That's why fromCoordinates was introduced.

>> Source/WebCore/rendering/RenderLayer.cpp:2561
>> +                if (ownerElement->contentDocument() && ownerElement->contentDocument()->documentElement() && useSmoothScrolling(options.behavior, *ownerElement->contentDocument()->documentElement()))
> 
> This doesn't seem to be the right place to be asking this question. Surely all FrameView scrolls should do whatever scrolling via scrollingElement does.

Same comment as for scrollTo.
Comment 78 jonjohnjohnson 2018-11-30 15:12:22 PST
> I see there could be consistenties between UI vs programmatic scrolls or between browsers. In particular, I guess we need to decide parameters to define the exact scroll animation...

> This looks OK but I'm concerned about whether we'll find the animation curve acceptable on macOS and iOS.

Really hoping animation curves (eventually) feel internally consistent, so we can mix scroll-snap and scroll-behavior seamlessly. :D