|
Description
Frédéric Wang (:fredw)
2018-07-26 06:41:00 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.
Created attachment 346513 [details]
WIP Patch
Beware "scroll-behavior on body element not propagated to the viewport" - https://github.com/w3c/csswg-drafts/issues/2990 Created attachment 346627 [details]
WIP Patch
Rebasing
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. (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. (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. (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. (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 Created attachment 348010 [details]
WIP Patch
Created attachment 348493 [details]
Testcase
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...).
Created attachment 348500 [details]
WIP Patch
(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). 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. Created attachment 348903 [details]
Testcase (scrollIntoView)
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]). Created attachment 349043 [details]
Patch (applies on top of 189352)
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.
Created attachment 349536 [details]
Patch
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.
Created attachment 349879 [details] Patch for EWS (includes patch from bug 189579) Created attachment 350255 [details]
Patch
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 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 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 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 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 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 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 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 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
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). (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. (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. Created attachment 350409 [details]
Patch
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. Sure we can; see CSSParserContext. Created attachment 350566 [details]
Patch (add missing header in TextCodecReplacement.cpp)
Created attachment 350567 [details]
Patch (add missing header in TextCodecReplacement.cpp)
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 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 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 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 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 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 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 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
Created attachment 350610 [details]
Patch
Removing the compilation flag.
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 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 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 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 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 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
Created attachment 350621 [details]
Patch
Tim, do you think we need to do UI process scrolling here? Created attachment 353958 [details]
Patch
Created attachment 353960 [details]
Patch
Trying to resend to EWS after having fixed the issue with rotating unified sources / missing headers.
Created attachment 354071 [details]
Patch
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 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
Created attachment 354082 [details]
Patch
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 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
(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... Created attachment 354319 [details]
Patch
(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. Created attachment 354320 [details]
Patch
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 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 (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 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. Created attachment 356043 [details]
Patch
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 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 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. > 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 Created attachment 358313 [details] Patch Rebasing patch after the std::optional changes (https://trac.webkit.org/changeset/239427/webkit). Created attachment 363090 [details]
Patch
Just rebasing to fix conflicts due with new test options.
Created attachment 363093 [details]
Patch
Build failures on GTK/WPE were due to unified source rotating, hopefully this is fixed by https://trac.webkit.org/changeset/242466/webkit Created attachment 368453 [details]
Patch
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 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 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 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 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 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 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 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
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. Created attachment 372999 [details]
Patch
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. Created attachment 377582 [details]
Patch
Just trying to rebase... (not tested)
(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. Created attachment 377813 [details]
Patch
Ooops, wrong patch was previously attached.
Created attachment 378193 [details]
Fixed scroll content flush issue on ios
Fixed scroll content flush issue on ios
Created attachment 382485 [details]
Using native scroll for iOS element scroll
Using native scroll for iOS element scroll
Created attachment 382550 [details]
Patch
Created attachment 382561 [details]
Patch
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 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 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 ;-) Created attachment 384462 [details]
Patch
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 Created attachment 384514 [details]
Patch
Created attachment 384517 [details]
Patch
Created attachment 384623 [details]
Patch
Created attachment 384720 [details]
Patch
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 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. (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:) Created attachment 384789 [details]
Patch
Created attachment 384800 [details]
Patch
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 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. 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 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.
(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:) *** Bug 93238 has been marked as a duplicate of this bug. *** (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? With the last comment on this from almost a year ago, I'd like to inquire about the status of this issue. Is it being actively worked on? Hello, I was delighted to see that iOS 14 finally shipped support for behavior: smooth without a polyfill (although behind the flag "CSSOM View Smooth Scrolling". However I would like to give some feedback: it is way too slow to be usable. I'm using smooth scroll for my slider and it is the perfect speed in firefox and acceptable in chrome. The polyfill that I'm using (https://www.npmjs.com/package/smoothscroll-polyfill) has 468ms which I think is ok. Please try going to https://www.officedepot.se on an iPhone and clicking the prev/next arrows on the product carousel on the front page, then go into settings->safari->experimental features and enable smooth scrolling. Go back to safari and reload the page. You will notice that safari's delay is around a second and doesn't provide a great user experience. IMO the smooth scroll API should support duration and curve options like .animate() but until that time comes, PLEASE don't make it twice as slow as all other browsers, forcing developers to polyfill away the thing you needed 3 years to implement just to achieve consistency in different browsers. Created attachment 440977 [details]
Patch
Finally enabled. Thanks everyone for the work. https://trac.webkit.org/changeset/284029/webkit \o/ Awesome! Thanks, Simon! Awesome news! I have tried on Safari TP, and the animation is quite junky though. Maybe timing difference, but Firefox smooth scroll feels butter smooth with a very nice timing effect, while Safari one looks janky and way too fast. As there some plans to improve it? Firefox one is absolutely perfect. The duration and acceleration curve was chosen to match native animations on macOS. What page are you testing where it looks janky? (In reply to Simon Fraser (smfr) from comment #131) > The duration and acceleration curve was chosen to match native animations on > macOS. What page are you testing where it looks janky? I have tried this: https://focal-theme-ivory.myshopify.com/ On this page go to the very last section called "As the years go by" and click on the arrows to switch between items of the timeline. It goes super fast and does not feel very smooth (at least compared to Firefox one). However I understand this may have been done on purpose to match native one. (In reply to mic.gallego from comment #132) > (In reply to Simon Fraser (smfr) from comment #131) > > The duration and acceleration curve was chosen to match native animations on > > macOS. What page are you testing where it looks janky? > > I have tried this: https://focal-theme-ivory.myshopify.com/ > On this page go to the very last section called "As the years go by" and > click on the arrows to switch between items of the timeline. It goes super > fast and does not feel very smooth (at least compared to Firefox one). > However I understand this may have been done on purpose to match native one. Actually on this page you can try as well the "Shop essentials" section to go through the products (by clicking on the arrow). This one is especially janky on Safari TP, while butter smooth on Firefox. The reasons the animation at the bottom of https://focal-theme-ivory.myshopify.com/ is slow is because it's running a scroll animation on an overflow:hidden element, which does not get scrolling optimizations by default. I filed bug 232419 to cover this case. |