Bug 188043

Summary: Add support for ScrollOptions' ScrollBehavior and CSS scroll-behavior properties
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: DOMAssignee: cathiechen <cathiechen>
Status: NEW ---    
Severity: Normal CC: ajuma, alex, annulen, bugzilla, cathiechen, cdumez, chris, cmarcelo, contact, dbates, don.olmstead, dvoytenko, dvpdiner2, dyskette, esprehn+autocc, ews-watchlist, fred.wang, giorgio.liscio, glenn, gyuyoung.kim, hi, jamesr, joepeck, kangil.han, koivisto, kondapallykalyan, kyle.bavender, luiz, macpherson, menard, mifenton, miriam, mmaxfield, pdr, rmondello, rniwa, robin, rwlbuis, ryuan.choi, sergio, simon.fraser, thorton, timdream, tonikitoo, tsmd57, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://drafts.csswg.org/cssom-view/#enumdef-scrollbehavior
See Also: https://github.com/w3c/csswg-drafts/issues/2977
https://github.com/w3c/csswg-drafts/issues/157
https://bugs.chromium.org/p/chromium/issues/detail?id=884123
https://github.com/web-platform-tests/wpt/pull/13049
https://github.com/w3c/csswg-drafts/issues/3127
Bug Depends on: 204936, 205059, 188300, 188343, 189258, 189352, 189541, 189579, 189787, 189894, 204882, 205009    
Bug Blocks: 189907, 191357    
Attachments:
Description Flags
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
Testcase
none
WIP Patch
none
WIP Patch
none
Testcase (scrollIntoView)
none
Patch
none
Patch (applies on top of 189352)
none
Patch
none
Patch
none
Patch
none
Patch for EWS (includes patch from bug 189579)
none
Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Archive of layout-test-results from ews201 for win-future
none
Archive of layout-test-results from ews205 for win-future
none
Patch
none
Patch (add missing header in TextCodecReplacement.cpp)
none
Patch (add missing header in TextCodecReplacement.cpp)
ews-watchlist: commit-queue-
Archive of layout-test-results from ews102 for mac-sierra
none
Archive of layout-test-results from ews104 for mac-sierra-wk2
none
Archive of layout-test-results from ews113 for mac-sierra
none
Archive of layout-test-results from ews112 for mac-sierra
none
Patch
none
Archive of layout-test-results from ews100 for mac-sierra
none
Archive of layout-test-results from ews107 for mac-sierra-wk2
none
Archive of layout-test-results from ews117 for mac-sierra
none
Patch
none
Patch
none
Patch
none
Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews100 for mac-sierra
none
Patch
none
Archive of layout-test-results from ews107 for mac-sierra-wk2
none
Patch
none
Patch
none
Patch
none
Patch with workaround for bug 192134 comment 9
none
Patch
none
Patch
none
Patch
none
Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews100 for mac-highsierra
none
Archive of layout-test-results from ews106 for mac-highsierra-wk2
none
Archive of layout-test-results from ews115 for mac-highsierra
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
none
Fixed scroll content flush issue on ios
none
Using native scroll for iOS element scroll
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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
Comment 79 Frédéric Wang (:fredw) 2019-01-04 05:31:45 PST
Created attachment 358313 [details]
Patch

Rebasing patch after the std::optional changes (https://trac.webkit.org/changeset/239427/webkit).
Comment 80 Frédéric Wang (:fredw) 2019-02-27 07:32:59 PST
Created attachment 363090 [details]
Patch

Just rebasing to fix conflicts due with new test options.
Comment 81 Radar WebKit Bug Importer 2019-02-27 07:40:54 PST
<rdar://problem/48436802>
Comment 82 Frédéric Wang (:fredw) 2019-02-27 09:03:53 PST
Created attachment 363093 [details]
Patch
Comment 83 Frédéric Wang (:fredw) 2019-03-05 08:19:39 PST
Build failures on GTK/WPE were due to unified source rotating, hopefully this is fixed by https://trac.webkit.org/changeset/242466/webkit
Comment 84 Frédéric Wang (:fredw) 2019-04-29 07:15:02 PDT
Created attachment 368453 [details]
Patch
Comment 85 EWS Watchlist 2019-04-29 08:31:37 PDT
Comment on attachment 368453 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-element.html
Comment 86 EWS Watchlist 2019-04-29 08:31:40 PDT
Created attachment 368454 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 87 EWS Watchlist 2019-04-29 08:45:16 PDT
Comment on attachment 368453 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-element.html
Comment 88 EWS Watchlist 2019-04-29 08:45:19 PDT
Created attachment 368455 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 89 EWS Watchlist 2019-04-29 09:13:34 PDT
Comment on attachment 368453 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-element.html
Comment 90 EWS Watchlist 2019-04-29 09:13:37 PDT
Created attachment 368459 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 91 EWS Watchlist 2019-04-29 09:16:10 PDT
Comment on attachment 368453 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-element.html
Comment 92 EWS Watchlist 2019-04-29 09:16:13 PDT
Created attachment 368460 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.4
Comment 93 Frédéric Wang (:fredw) 2019-05-29 07:23:26 PDT
Two ideas for future patches:
* Rewrite the tests by introducing new internal testing API (as that was done for frame scrolling) in order to make them fast.
* On iOS, consider animating the scrolling in the UI process.
Comment 94 Frédéric Wang (:fredw) 2019-06-26 20:58:08 PDT
Created attachment 372999 [details]
Patch
Comment 95 Ben Frain 2019-08-15 07:18:02 PDT
Is this waiting to go into WebKit/Safari proper? I'm pretty excited to see iOS/Safari get this. All too often need to use a 3rd party JS lib to get some sort of parity with Chrome.
Comment 96 Frédéric Wang (:fredw) 2019-08-29 07:34:28 PDT
Created attachment 377582 [details]
Patch

Just trying to rebase... (not tested)
Comment 97 Frédéric Wang (:fredw) 2019-08-29 08:25:28 PDT
(In reply to Frédéric Wang (:fredw) from comment #96)
> Created attachment 377582 [details]
> Patch
> 
> Just trying to rebase... (not tested)

Attached tests seem to work in macOS but no longer works in iOS (or at least scrolling is too fast for me).

In any case, Cathie is taking over this work, so I'm re-assigning.
Comment 98 Frédéric Wang (:fredw) 2019-09-01 02:39:05 PDT
Created attachment 377813 [details]
Patch

Ooops, wrong patch was previously attached.
Comment 99 cathiechen 2019-09-06 08:59:32 PDT
Created attachment 378193 [details]
Fixed scroll content flush issue on ios

Fixed scroll content flush issue on ios
Comment 100 cathiechen 2019-10-31 12:06:07 PDT
Created attachment 382485 [details]
Using native scroll for iOS element scroll

Using native scroll for iOS element scroll
Comment 101 cathiechen 2019-10-31 21:32:52 PDT
Created attachment 382550 [details]
Patch
Comment 102 cathiechen 2019-10-31 22:42:32 PDT
Created attachment 382561 [details]
Patch
Comment 103 Simon Fraser (smfr) 2019-11-01 11:28:59 PDT
Comment on attachment 382561 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:960
> +    renderer->setScrollLeft(scrollPosition.x(), ScrollType::Programmatic, clamping);
> +    renderer->setScrollTop(scrollPosition.y(), ScrollType::Programmatic, clamping);

I wonder if this could be just setScrollPosition().

> Source/WebCore/dom/Element.cpp:963
> +    if (renderer->layer())
> +        renderer->layer()->setIsScrollInProgress(false);

Is it OK to not run this code for a smooth scroll?

> Source/WebCore/dom/Element.cpp:1305
> +            // See https://github.com/w3c/csswg-drafts/issues/2977

I would expect the scroll options of the body or document to be consulted, following how overflow is propagated from the body to the root.

> Source/WebCore/dom/Element.cpp:1311
> +            if (document().documentElement() && useSmoothScrolling(ScrollBehavior::Auto, *document().documentElement())) {
> +                auto window = makeRefPtr(document().domWindow());
> +                if (!window)
> +                    return;
> +
> +                window->scrollTo(newLeft * frame->pageZoomFactor() * frame->frameScaleFactor(), frame->view()->scrollY());

Why not just make FrameView::setScrollPosition support animation?

> Source/WebCore/dom/Element.cpp:1328
> +        if (useSmoothScrolling(ScrollBehavior::Auto, *this)) {
> +            IntPoint scrollPosition(
> +                clampToInteger(newLeft * renderer->style().effectiveZoom()),
> +                scrollTop()
> +            );
> +            renderer->scrollToPositionWithAnimation(scrollPosition, ScrollClamping::Clamped);
> +            return;
> +        }
> +
>          renderer->setScrollLeft(static_cast<int>(newLeft * renderer->style().effectiveZoom()), ScrollType::Programmatic);

Rather than special-case smooth scrolling everywhere, I wonder if we should add an OptionSet<> to setScrollLeft() and friends and do the smooth scrolling branch lower down?

> Source/WebCore/dom/Element.cpp:1347
> +            if (document().documentElement() && useSmoothScrolling(ScrollBehavior::Auto, *document().documentElement())) {
> +                auto window = makeRefPtr(document().domWindow());
> +                if (!window)
> +                    return;
> +
> +                window->scrollTo(frame->view()->scrollX(), newTop * frame->pageZoomFactor() * frame->frameScaleFactor());
> +            } else
> +                frame->view()->setScrollPosition(IntPoint(frame->view()->scrollX(), static_cast<int>(newTop * frame->pageZoomFactor() * frame->frameScaleFactor())));

Share code with above.

> Source/WebCore/page/DOMWindow.cpp:1645
> +    // FIXME: Should we use document()->scrollingElement()?
> +    // See https://github.com/w3c/csswg-drafts/issues/2977
> +    if (document()->documentElement() && useSmoothScrolling(scrollToOptions.behavior.valueOr(ScrollBehavior::Auto), *document()->documentElement())) {
> +        view->scrollToOffsetWithAnimation(layoutPos, clamping);
> +        return;
> +    }

Same question as before.

> Source/WebCore/page/FrameView.h:231
> +    bool requestScrollPositionUpdate(const ScrollPosition&, bool /*withAnimation*/ = false) final;

We should probably use an enum for withAnimation.

> Source/WebCore/page/ScrollBehavior.cpp:2
> + * Copyright (C) 2018 Igalia S.L.

2019 now

> Source/WebCore/page/ScrollBehavior.cpp:6
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either

Is it Igalia policy to add new code under LGPL? We prefer the BSD license if possible.

> Source/WebCore/page/ScrollToOptions.h:48
> +inline ScrollToOptions fromCoordinates(double x, double y)
> +{
> +    ScrollToOptions options;
> +    options.left = x;
> +    options.top = y;
> +    return options;
> +};

Can't this just be a constructor on ScrollToOptions ?

> Source/WebCore/page/ScrollToOptions.h:62
>      return options;

------------- I stopped reviewing here --------------
Comment 104 Frédéric Wang (:fredw) 2019-11-25 09:52:50 PST
Comment on attachment 382561 [details]
Patch

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

>> Source/WebCore/page/ScrollBehavior.cpp:6
>> + * License as published by the Free Software Foundation; either
> 
> Is it Igalia policy to add new code under LGPL? We prefer the BSD license if possible.

AFAIK we don't have such a policy. Probably this header was copied from another file instead of using the official boilerplate.
Comment 105 Frédéric Wang (:fredw) 2019-11-26 03:18:06 PST
Comment on attachment 382561 [details]
Patch

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

>> Source/WebCore/dom/Element.cpp:1305
>> +            // See https://github.com/w3c/csswg-drafts/issues/2977
> 
> I would expect the scroll options of the body or document to be consulted, following how overflow is propagated from the body to the root.

IIRC, that's not what the spec currently says... Please comment on the GitHub CSSWG issue ;-)
Comment 106 cathiechen 2019-11-28 10:12:46 PST
Created attachment 384462 [details]
Patch
Comment 107 cathiechen 2019-11-28 10:13:48 PST
Comment on attachment 382561 [details]
Patch

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

>> Source/WebCore/dom/Element.cpp:960
>> +    renderer->setScrollTop(scrollPosition.y(), ScrollType::Programmatic, clamping);
> 
> I wonder if this could be just setScrollPosition().

Done

>> Source/WebCore/dom/Element.cpp:963
>> +        renderer->layer()->setIsScrollInProgress(false);
> 
> Is it OK to not run this code for a smooth scroll?

Hmm, the new patch uses ScrollAnimationStatus to avoid confusion. 
NotInAnimation, InUINativeAnimation and WebAnimationTimerStarted.
We can know IsScrollInProgress through these statuses.

>>> Source/WebCore/dom/Element.cpp:1305
>>> +            // See https://github.com/w3c/csswg-drafts/issues/2977
>> 
>> I would expect the scroll options of the body or document to be consulted, following how overflow is propagated from the body to the root.
> 
> IIRC, that's not what the spec currently says... Please comment on the GitHub CSSWG issue ;-)

Yeah, we can make a change after the spec is determined.

>> Source/WebCore/dom/Element.cpp:1311
>> +                window->scrollTo(newLeft * frame->pageZoomFactor() * frame->frameScaleFactor(), frame->view()->scrollY());
> 
> Why not just make FrameView::setScrollPosition support animation?

This code's changed to let DOMWindow::scrollTo to handle smooth scroll and instant scroll.

>> Source/WebCore/dom/Element.cpp:1328
>>          renderer->setScrollLeft(static_cast<int>(newLeft * renderer->style().effectiveZoom()), ScrollType::Programmatic);
> 
> Rather than special-case smooth scrolling everywhere, I wonder if we should add an OptionSet<> to setScrollLeft() and friends and do the smooth scrolling branch lower down?

Done, deal with smooth scroll inside RenderLayer.

>> Source/WebCore/dom/Element.cpp:1347
>> +                frame->view()->setScrollPosition(IntPoint(frame->view()->scrollX(), static_cast<int>(newTop * frame->pageZoomFactor() * frame->frameScaleFactor())));
> 
> Share code with above.

Done

>> Source/WebCore/page/FrameView.h:231
>> +    bool requestScrollPositionUpdate(const ScrollPosition&, bool /*withAnimation*/ = false) final;
> 
> We should probably use an enum for withAnimation.

Done

>>> Source/WebCore/page/ScrollBehavior.cpp:6
>>> + * License as published by the Free Software Foundation; either
>> 
>> Is it Igalia policy to add new code under LGPL? We prefer the BSD license if possible.
> 
> AFAIK we don't have such a policy. Probably this header was copied from another file instead of using the official boilerplate.

Done, updated to the newest version.

>> Source/WebCore/page/ScrollToOptions.h:48
>> +};
> 
> Can't this just be a constructor on ScrollToOptions ?

Done
Comment 108 cathiechen 2019-11-29 09:29:14 PST
Created attachment 384514 [details]
Patch
Comment 109 cathiechen 2019-11-29 09:56:13 PST
Created attachment 384517 [details]
Patch
Comment 110 cathiechen 2019-12-02 08:35:57 PST
Created attachment 384623 [details]
Patch
Comment 111 cathiechen 2019-12-03 09:59:57 PST
Created attachment 384720 [details]
Patch
Comment 112 Frédéric Wang (:fredw) 2019-12-03 12:30:30 PST
Comment on attachment 384720 [details]
Patch

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

I think this patch is starting to become huge and hard to review. Do you think it would be possible to split it in smaller bits?

For example, maybe something like ¿?
- Add CSS/IDL property under a flag (maybe you can already test these).
- Copy WPT tests to LayoutTests/fast/scrolling/ios/ (failing for now)
- Support iOS smooth scrolling and enable previous tests.
- Support non-iOS plaforms and enable WPT tests.

@smfr: What do you think?

> Source/WebCore/ChangeLog:1
> +2018-11-09  Frederic Wang  <fwang@igalia.com>

I guess this should be you (Cathie) now if you did more changes. You can put me as a "Patch By" author in the changelog if you want.

> Source/WebCore/ChangeLog:15
> +        animation of non-iOS platform relies on the existing ScrollAnimationSmooth.

non-iOS platforms*, executed in the scrolling thread.

> Source/WebCore/ChangeLog:16
> +        On iOS platform, it is using UIScrollView scroll animation. The scroll position is synchronous

do you mean synchronized? Maybe reword:

"On iOS, it relies on UIScrollView's native scroll animation support and is executed in the UI process."

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:163
> +        // Send the scroll position to web side.

to the Web process

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:171
> +    // If during an scroll animation, need to stop it before starting a new one. 

a* scroll animation

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:63
> +    void scrollTo(const FloatPoint&, ScrollType = ScrollType::User, ScrollPositionClamp = ScrollPositionClamp::ToContentEdges, bool needSnycScrollPosition = false);

Sync*

> Source/WebCore/rendering/RenderLayer.cpp:2587
> +        // If UIScrollView is during scroll animation, the scroll offset of web side might be overridden.

is performing a scroll animation

the Web process

> Source/WebCore/rendering/RenderLayer.cpp:2588
> +        // So we need to requestScrollPositionUpdate here and let it be checked on UI side. If the scroll offset on UI side

call requestScrollPositionUpdate
UI process
Web process

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2374
> +        // If _scrollView won't change, we still need to send animation stop info to web side.

to tell the Web process that the animation stopped

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:310
> +        // If the scroll positions are not the same, there's a possibility that the scroll offset from web side has been overridden.

Web process

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:311
> +        // In this case, it need to send the scroll position to Web side.

what is "it"?

> LayoutTests/imported/w3c/ChangeLog:4337
> +        (#�):

what are these changes?
Comment 113 cathiechen 2019-12-03 20:41:33 PST
Comment on attachment 384720 [details]
Patch

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

>> Source/WebCore/ChangeLog:1
>> +2018-11-09  Frederic Wang  <fwang@igalia.com>
> 
> I guess this should be you (Cathie) now if you did more changes. You can put me as a "Patch By" author in the changelog if you want.

Got it, thanks:)

>> Source/WebCore/ChangeLog:15
>> +        animation of non-iOS platform relies on the existing ScrollAnimationSmooth.
> 
> non-iOS platforms*, executed in the scrolling thread.

Done

>> Source/WebCore/ChangeLog:16
>> +        On iOS platform, it is using UIScrollView scroll animation. The scroll position is synchronous
> 
> do you mean synchronized? Maybe reword:
> 
> "On iOS, it relies on UIScrollView's native scroll animation support and is executed in the UI process."

Done

>> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:163
>> +        // Send the scroll position to web side.
> 
> to the Web process

Done

>> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:171
>> +    // If during an scroll animation, need to stop it before starting a new one. 
> 
> a* scroll animation

Done

>> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:63
>> +    void scrollTo(const FloatPoint&, ScrollType = ScrollType::User, ScrollPositionClamp = ScrollPositionClamp::ToContentEdges, bool needSnycScrollPosition = false);
> 
> Sync*

Done

>> Source/WebCore/rendering/RenderLayer.cpp:2587
>> +        // If UIScrollView is during scroll animation, the scroll offset of web side might be overridden.
> 
> is performing a scroll animation
> 
> the Web process

done

>> Source/WebCore/rendering/RenderLayer.cpp:2588
>> +        // So we need to requestScrollPositionUpdate here and let it be checked on UI side. If the scroll offset on UI side
> 
> call requestScrollPositionUpdate
> UI process
> Web process

done

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2374
>> +        // If _scrollView won't change, we still need to send animation stop info to web side.
> 
> to tell the Web process that the animation stopped

done

>> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:310
>> +        // If the scroll positions are not the same, there's a possibility that the scroll offset from web side has been overridden.
> 
> Web process

done

>> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:311
>> +        // In this case, it need to send the scroll position to Web side.
> 
> what is "it"?

Changed to "the scroll position need to be sent to the Web process."

>> LayoutTests/imported/w3c/ChangeLog:4337
>> +        (#�):
> 
> what are these changes?

Done, something wrong with format.
Comment 114 cathiechen 2019-12-03 20:43:32 PST
(In reply to Frédéric Wang (:fredw) from comment #112)
> Comment on attachment 384720 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384720&action=review
> 
> I think this patch is starting to become huge and hard to review. Do you
> think it would be possible to split it in smaller bits?
> 
> For example, maybe something like ¿?
> - Add CSS/IDL property under a flag (maybe you can already test these).
> - Copy WPT tests to LayoutTests/fast/scrolling/ios/ (failing for now)
> - Support iOS smooth scrolling and enable previous tests.
> - Support non-iOS plaforms and enable WPT tests.
> 
> @smfr: What do you think?
> 

Hi Fred,

Thanks for the advice. I think it sounds good to split it for me:)
Comment 115 cathiechen 2019-12-03 22:39:58 PST
Created attachment 384789 [details]
Patch
Comment 116 cathiechen 2019-12-04 03:05:03 PST
Created attachment 384800 [details]
Patch
Comment 117 cathiechen 2019-12-04 03:14:46 PST
Hi,

As Fred mentioned this patch is too big to review.
I'm going to try to split it into:
- Add CSS/IDL property under a flag.
- Support scroll animation by timer in the Web process.
- Support iOS smooth scrolling by using native interfaces.

The current patch contains all the implement and opened to review:)

Thanks and sorry for the inconvenient:)
Comment 118 Frédéric Wang (:fredw) 2019-12-09 06:33:23 PST
Comment on attachment 384800 [details]
Patch

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

I started to review thinking it would be only the non-iOS part, but I realized this is still the big patch. I'm leaving the current comments but will review the other bugs instead..

> Source/WebCore/ChangeLog:18
> +        ScrollAnimationSmooth. On iOS, it relies on UIScrollView's native scroll animation support

I guess this should say that this will be done in a follow-up bug.

> Source/WebKit/ChangeLog:11
> +        interfaces to performance smooth scroll and send the scroll position to the Web process.

Do you need to update comment about iOS?

> Source/WebCore/dom/Element.cpp:1297
> +            // See https://github.com/w3c/csswg-drafts/issues/2977

Maybe we should open a new bug in webkit's tracker referring to that css-wg issue, so that we can fix it if the spec changes.

> Source/WebCore/dom/Element.cpp:1307
> +        renderer->setScrollLeft(effectiveLeft, ScrollType::Programmatic, animated, ScrollClamping::Clamped);

Are you adding this ScrollClamping::Clamped flag on purpose? It seems it is the default value for RenderBox::setScrollLeft

> Source/WebCore/dom/Element.cpp:1320
> +            // See https://github.com/w3c/csswg-drafts/issues/2977

Ditto.

> Source/WebCore/dom/Element.cpp:1330
> +        renderer->setScrollTop(effectiveTop, ScrollType::Programmatic, animated, ScrollClamping::Clamped);

Ditto.

> Source/WebCore/page/DOMWindow.cpp:1636
> +    if (!view->isScrollInProgress() && !scrollToOptions.left.value() && !scrollToOptions.top.value() && view->contentsScrollPosition() == IntPoint(0, 0))

It's already explained in the changelog, but maybe we should add a C++ comment here too about the optimization and why it is skipped when scroll is in progress.

> Source/WebCore/page/DOMWindow.cpp:1644
> +    // See https://github.com/w3c/csswg-drafts/issues/2977

Ditto.
Comment 119 cathiechen 2019-12-13 01:14:10 PST
Hi,

We are done with splitting the patch.
There are three sub-patches:
- parsing (bug: 205009)
- non-native scroll animation (bug: 204882)
- native scroll animation (bug: 204936)
Comment 120 Ryosuke Niwa 2020-03-03 21:09:59 PST
Comment on attachment 384800 [details]
Patch

Since you're not a reviewer, please refrain from setting r- on your own patch (for that matter, even if you're a reviewer, you shouldn't be setting r- on your own patch). Instead, clear r? flag.
Comment 121 cathiechen 2020-03-03 23:32:29 PST
(In reply to Ryosuke Niwa from comment #120)
> Comment on attachment 384800 [details]
> Patch
> 
> Since you're not a reviewer, please refrain from setting r- on your own
> patch (for that matter, even if you're a reviewer, you shouldn't be setting
> r- on your own patch). Instead, clear r? flag.

Ah, I see, sorry for the mistake. This patch has been split into 3 sub patches. So yes, what I actually wanted is to clear the r? flag. Thanks:)
Comment 122 Simon Fraser (smfr) 2020-03-04 13:29:11 PST
*** Bug 93238 has been marked as a duplicate of this bug. ***
Comment 123 jonjohnjohnson 2020-03-07 08:30:40 PST
(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).

It seems the blink team is for creating as much consistency as possible for these scrolling interactions...

"I agree that we should ensure that curves give similar duration for the same distance & velocity. This should not be too difficult."

https://bugs.chromium.org/p/chromium/issues/detail?id=915670#c14

What do you think Frédéric? Do you agree with majidvp@chromium.org?