|
|
Created:
3 years, 10 months ago by sunyunjia Modified:
3 years, 6 months ago CC:
darktears, blink-layers+watch_chromium.org, blink-reviews, blink-reviews-animation_chromium.org, blink-reviews-bindings_chromium.org, blink-reviews-dom_chromium.org, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dglazkov+blink, dshwang, eae+blinkwatch, Eric Willigers, jchaffraix+rendering, kenneth.christiansen, kinuko+watch, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, rjwright, rwlbuis, shans, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement Element.scrollIntoView for scroll-behavior: smooth.
Currently in Chrome, a call to Element.scrollIntoView will calculate the amount
each scroller needs to scroll to align the Element as specified and instantly
set the scroll position on that scroller (going from the innermost to the
outermost scroller).
If Element.scrollIntoView is called with scroll-behavior: smooth, instead of
scrolling instantly, we should animate to the desired position.
We can’t simply call setScrollPosition in PaintLayerScrollableArea and
FrameView with ScrollBehaviorSmooth because of the following reasons:
- We want to run the animation from the outermost scroller to the
innermost scroller
- We want to run the animation one after another. That is, the scroll animation
on the child scroller should start after the animation on the parent scroller
is finished.
This patch adds a new class, ProgrammaticScrollCoordinator, that manages
programmatic scroll animations.
BUG=648446
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2650343008
Cr-Original-Commit-Position: refs/heads/master@{#475387}
Committed: https://chromium.googlesource.com/chromium/src/+/bbf870d971d95af7ae1ee688a7ed100e3787d02b
Review-Url: https://codereview.chromium.org/2650343008
Cr-Commit-Position: refs/heads/master@{#477953}
Committed: https://chromium.googlesource.com/chromium/src/+/61b73ca665e42dfb7953de8a4daf1d4144805fef
Patch Set 1 #
Total comments: 26
Patch Set 2 : Revised according to the comments. We are still missing tests. #
Total comments: 8
Patch Set 3 : Using isSmooth and adjust alignment. #Patch Set 4 : Added SimTest. #
Total comments: 21
Patch Set 5 : Adds test for nested container case.:wq #Patch Set 6 : Rename to SmoothScrollSequencer and remove the exception state. #Patch Set 7 : Rebase #
Total comments: 30
Patch Set 8 : Abort the current animation when new scrollIntoView comes in. #Patch Set 9 : Use writing mode to decide x and y for inline and block. Test added. #Patch Set 10 : Added web platform test. #
Total comments: 2
Patch Set 11 : Rebase #
Total comments: 53
Patch Set 12 : Fixed the comments #
Total comments: 16
Patch Set 13 : Updated. #Patch Set 14 : Change the default setting. #Patch Set 15 : SmoothScrollSequencer should use SetOffset instead of AnimateToOffset #Patch Set 16 : Add a short comment. #Patch Set 17 : Rebase. #Patch Set 18 : Rebase #
Total comments: 2
Patch Set 19 : Fixed nits. #Patch Set 20 : Abort smooth scrolls in SetScrollOffset #Patch Set 21 : Set default value of AbortSequencedSmoothScrolls to kAbort. #Patch Set 22 : Get the BUILD back. #Patch Set 23 : Rebase #Patch Set 24 : Add ScrollType: kSequencedSmoothScroll #
Total comments: 4
Patch Set 25 : Rebase... #Patch Set 26 : Sequenced-smooth-scrolls only happen on main thread for now. #
Total comments: 6
Patch Set 27 : Rebased. #Patch Set 28 : Fixed nits. #
Total comments: 5
Messages
Total messages: 218 (176 generated)
Description was changed from ========== Implement Element.scrollIntoView for scroll-behavior: smooth. BUG=648446 ========== to ========== Implement Element.scrollIntoView for scroll-behavior: smooth. BUG=648446 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Description was changed from ========== Implement Element.scrollIntoView for scroll-behavior: smooth. BUG=648446 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Implement Element.scrollIntoView for scroll-behavior: smooth. Currently in Chrome, a call to Element.scrollIntoView will calculate the amount each scroller needs to scroll to align the Element as specified and instantly set the scroll position on that scroller (going from the innermost to the outermost scroller). If Element.scrollIntoView is called with scroll-behavior: smooth, instead of scrolling instantly, we should animate to the desired position. We can’t simply call setScrollPosition in PaintLayerScrollableArea and FrameView with ScrollBehaviorSmooth because of the following reasons: - We want to run the animation from the outermost scroller to the innermost scroller - We want to run the animation one after another. That is, the scroll animation on the child scroller should start after the animation on the parent scroller is finished. This patch adds a new class, ProgrammaticScrollCoordinator, that manages programmatic scroll animations. BUG=648446 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
sunyunjia@chromium.org changed reviewers: + bokan@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi David, This is the patch that implements smooth scrolling for scrollIntoView. Please take a look, thanks! I wonder should we add the link to spec and design doc somewhere in the issue description?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ikilpatrick@chromium.org changed reviewers: + ikilpatrick@chromium.org
Is there an intent to implement for this?
On 2017/01/28 00:46:18, ikilpatrick wrote: > Is there an intent to implement for this? Yah, though it's showing it's age: https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/smooth$20...
https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:437: options.setBlock("start"); according to the spec we should also initialize the |inline| member to "nearest" https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:446: options.setBlock("end"); Similarly, we need to set |inline| here too. https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:459: scrollIntoViewWithOptions(options); move this below the if statement, make the "else" block return. https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:484: if (options.block() == "end") use |else if| https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:486: if (options.block() == "nearest") should this be "center"? "nearest" from the spec appears to be closer to "alignToEdgeIfNeeded" https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:491: bounds, ScrollAlignment::alignToEdgeIfNeeded, alignment, How come we don't do anything for the inline direction? https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Element.idl (right): https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.idl:97: [RaisesException] void scrollIntoView(optional (ScrollIntoViewOptions or boolean)? arg); I don't know enough about IDL, I'd like someone who knows to take a look. https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:4396: scrollBehavior == ScrollBehaviorSmooth) { Add a DCHECK that we don't have a UserScroll passing Smooth behavior. https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right): https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:233: if (scrollType == ProgrammaticScroll && Ditto here for DCHECK https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/ScrollIntoViewOptions.idl (right): https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ScrollIntoViewOptions.idl:1: enum ScrollLogicalPosition { "start", "center", "end", "nearest" }; Do these have to be strings? https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:655: if (style()->getScrollBehavior() == ScrollBehaviorSmooth || Does the spec mention what should happen when the style and options disagree on behavior? https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1657: getProgrammaticScrollCoordinator()->queueAnimation(this, newScrollOffset); Hmm, maybe I'm missing something obvious here, but why do we need this new ProgrammaticScrollCoordinator rather than using the existing path for Element::scrollTo that just calls setScrollOffset with smooth behavior? Is it because scrollIntoView can cause coordinated scrolling between many boxes and we need to a way to queue those up and play them back linearly?
Hi David, Here is an updated version revised according to the comments. We are still missing tests. https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:437: options.setBlock("start"); On 2017/02/02 22:51:49, bokan wrote: > according to the spec we should also initialize the |inline| member to "nearest" Done. https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:446: options.setBlock("end"); On 2017/02/02 22:51:49, bokan wrote: > Similarly, we need to set |inline| here too. Done. https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:459: scrollIntoViewWithOptions(options); On 2017/02/02 22:51:49, bokan wrote: > move this below the if statement, make the "else" block return. Done. https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:484: if (options.block() == "end") On 2017/02/02 22:51:49, bokan wrote: > use |else if| Done. https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:486: if (options.block() == "nearest") On 2017/02/02 22:51:49, bokan wrote: > should this be "center"? "nearest" from the spec appears to be closer to > "alignToEdgeIfNeeded" Done. https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:491: bounds, ScrollAlignment::alignToEdgeIfNeeded, alignment, On 2017/02/02 22:51:49, bokan wrote: > How come we don't do anything for the inline direction? Done. I'm simply using x as inline direction and y as block direction. Is there any chance that it's the other way? https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Element.idl (right): https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.idl:97: [RaisesException] void scrollIntoView(optional (ScrollIntoViewOptions or boolean)? arg); On 2017/02/02 22:51:50, bokan wrote: > I don't know enough about IDL, I'd like someone who knows to take a look. Acknowledged. https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:4396: scrollBehavior == ScrollBehaviorSmooth) { On 2017/02/02 22:51:50, bokan wrote: > Add a DCHECK that we don't have a UserScroll passing Smooth behavior. Done. https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right): https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:233: if (scrollType == ProgrammaticScroll && On 2017/02/02 22:51:50, bokan wrote: > Ditto here for DCHECK Done. https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/ScrollIntoViewOptions.idl (right): https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ScrollIntoViewOptions.idl:1: enum ScrollLogicalPosition { "start", "center", "end", "nearest" }; On 2017/02/02 22:51:50, bokan wrote: > Do these have to be strings? Yeah, that's according to the specs: https://www.w3.org/TR/cssom-view-1/#dictdef-scrollintoviewoptions https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:655: if (style()->getScrollBehavior() == ScrollBehaviorSmooth || On 2017/02/02 22:51:50, bokan wrote: > Does the spec mention what should happen when the style and options disagree on > behavior? Thanks and yes. Updated according to the spec https://www.w3.org/TR/cssom-view-1/#scrolling https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1657: getProgrammaticScrollCoordinator()->queueAnimation(this, newScrollOffset); On 2017/02/02 22:51:50, bokan wrote: > Hmm, maybe I'm missing something obvious here, but why do we need this new > ProgrammaticScrollCoordinator rather than using the existing path for > Element::scrollTo that just calls setScrollOffset with smooth behavior? Is it > because scrollIntoView can cause coordinated scrolling between many boxes and we > need to a way to queue those up and play them back linearly? Right! See here: https://docs.google.com/a/chromium.org/document/d/1EeGvVnlHjIVVKwU-Y-A4uYtB-B...
https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:491: bounds, ScrollAlignment::alignToEdgeIfNeeded, alignment, On 2017/02/10 23:25:20, sunyunjia wrote: > On 2017/02/02 22:51:49, bokan wrote: > > How come we don't do anything for the inline direction? > > Done. I'm simply using x as inline direction and y as block direction. Is there > any chance that it's the other way? For western languages that's right, x is inline and y is block. But in a context where we're using "logical" directions like here, it's more precise to use inline and block. https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2650343008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1657: getProgrammaticScrollCoordinator()->queueAnimation(this, newScrollOffset); On 2017/02/10 23:25:20, sunyunjia wrote: > On 2017/02/02 22:51:50, bokan wrote: > > Hmm, maybe I'm missing something obvious here, but why do we need this new > > ProgrammaticScrollCoordinator rather than using the existing path for > > Element::scrollTo that just calls setScrollOffset with smooth behavior? Is it > > because scrollIntoView can cause coordinated scrolling between many boxes and > we > > need to a way to queue those up and play them back linearly? > > Right! See here: > https://docs.google.com/a/chromium.org/document/d/1EeGvVnlHjIVVKwU-Y-A4uYtB-B... Got it. I think the name makes it sound like it's related to ScrollingCoordinator which it isn't. I think SmoothScrollSequencer or something similar might be better. https://codereview.chromium.org/2650343008/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2650343008/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:456: scrollIntoViewWithOptions(options); This scrollIntoViewWithOptions and the one above can still be moved down to the bottom of the function. It's more clear that way that we're always calling it unless we've hit an error condition. https://codereview.chromium.org/2650343008/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:500: inlineAlignment = ScrollAlignment::alignClosestEdgeAlways; Hmm, this used to have "IfNeeded" but now everything is "Always" and I don't really see why that's changing with smoothness. I also see in the spec steps that there are some cases where we don't do anything. I don't really know the details here, could you confirm what we should actually be doing and that the ScrollAlignment enums match the we define in the spec. This will presumably become more clear with tests. https://codereview.chromium.org/2650343008/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2650343008/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:654: ScrollBehavior behavior = ScrollBehaviorAuto; This should be ScrollBehaviorInstant so that we pass either Smooth or Instant to scrollIntoView. scrollIntoView should either DCHECK that it's not Auto or make the param an isSmooth bool. https://codereview.chromium.org/2650343008/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h (right): https://codereview.chromium.org/2650343008/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h:193: Member<ProgrammaticScrollCoordinator> m_programmaticScrollCoordinator; What should happen if two separate scrollable DIVs have an element get scrollIntoView'd? I would expect them both to scroll simultaneously but having this be a field on ScrollingCoordinator would mean they'd happen one after the other, right? Perhaps this should be a member on ScrollableArea?
For the inline and block direction, I'm having a hard time figuring out which ancestor's writing-mode we should use. Its closest ancestor? Its closest scrollable ancestor? How about the situation where we need to scroll two layers to put the element into view? etc. Perhaps we should talk to people who wrote the spec? The only browser that has implemented ScrollIntoViewOptions is Firefox and it's still under experiment. See https://developer.mozilla.org/en/docs/Web/API/Element/scrollIntoView#Browser_... They're simply using vertical as block and horizontal as inline, as implemented in this patch. Maybe we could leave as it is for now? https://codereview.chromium.org/2650343008/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2650343008/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:456: scrollIntoViewWithOptions(options); On 2017/02/21 21:33:00, bokan wrote: > This scrollIntoViewWithOptions and the one above can still be moved down to the > bottom of the function. It's more clear that way that we're always calling it > unless we've hit an error condition. Done. https://codereview.chromium.org/2650343008/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:500: inlineAlignment = ScrollAlignment::alignClosestEdgeAlways; On 2017/02/21 21:33:00, bokan wrote: > Hmm, this used to have "IfNeeded" but now everything is "Always" and I don't > really see why that's changing with smoothness. I also see in the spec steps > that there are some cases where we don't do anything. I don't really know the > details here, could you confirm what we should actually be doing and that the > ScrollAlignment enums match the we define in the spec. > > This will presumably become more clear with tests. Done. My bad, I should've checked the spec closer. https://codereview.chromium.org/2650343008/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2650343008/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:654: ScrollBehavior behavior = ScrollBehaviorAuto; On 2017/02/21 21:33:00, bokan wrote: > This should be ScrollBehaviorInstant so that we pass either Smooth or Instant to > scrollIntoView. scrollIntoView should either DCHECK that it's not Auto or make > the param an isSmooth bool. Done. https://codereview.chromium.org/2650343008/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h (right): https://codereview.chromium.org/2650343008/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h:193: Member<ProgrammaticScrollCoordinator> m_programmaticScrollCoordinator; On 2017/02/21 21:33:01, bokan wrote: > What should happen if two separate scrollable DIVs have an element get > scrollIntoView'd? I would expect them both to scroll simultaneously but having > this be a field on ScrollingCoordinator would mean they'd happen one after the > other, right? Perhaps this should be a member on ScrollableArea? Well, on a second thought, there might be conflict if we try to scroll the two separate DIVs simultaneously. So I think they should happen one after the other and the current design is good for now.
Hi David, Could you review this updated smooth-scroll when you have time, so we could land this first patch and have it working. Thanks!
https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:456: "Smooth ScrollIntoView is an Experimental Web" I don't think we usually throw for a feature being off. I think we should just ignore the behavior option silently if the REF isn't on (but still scrollIntoView). https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.idl (right): https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.idl:97: [RaisesException] void scrollIntoView(optional (ScrollIntoViewOptions or boolean)? arg); Rather than the "or" for the argument, could we add an overload that takes ScrollIntoViewOptions and is RuntimeEnabled? That way we don't change our public interface without turning on the flag. https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:4478: ScrollOffset(exposeRect.x().toFloat(), exposeRect.y().toFloat())); Store the ScrollOffset in a variable above, outside the if(isSmooth)/else https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.h (right): https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:464: ProgrammaticScrollCoordinator* getProgrammaticScrollCoordinator() Please change the name to avoid confusion with the regular ScrollingCoordinator. They're very unrelated. As I mentioned a while ago, SmoothScrollSequencer or Scheduler, something along those lines would be more descriptive of what it does. https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h (right): https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h:192: Member<ProgrammaticScrollCoordinator> m_programmaticScrollCoordinator; We want ScrollingCoordinator to eventually go away. Perhaps Page would be a better place for this. https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ProgrammaticScrollCoordinator.cpp (right): https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ProgrammaticScrollCoordinator.cpp:36: if (!m_queue.empty()) { This whole block can be replaced with runQueuedAnimations(). https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right): https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollableArea.h:84: virtual ProgrammaticScrollCoordinator* getProgrammaticScrollCoordinator() I don't think this should be part of this class' interface. As mentioned above, Page is probably a better place and I think everything has access to Page. https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp (right): https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:55: for (int i = 0; i < 1500; ++i) { You can modify beginFrame to take a deltaTime so that we can just instantly jump by x milliseconds, rather than doing this loop. https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:59: ASSERT_LT(window().scrollY(), 1000); Is there a reason this range is so wide? Presumably there should be no in-determinism in this test, right? https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:65: } We should add a test for the nested case to test the queuing mechanism.
https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right): https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollableArea.h:84: virtual ProgrammaticScrollCoordinator* getProgrammaticScrollCoordinator() On 2017/03/28 16:29:53, bokan wrote: > I don't think this should be part of this class' interface. As mentioned above, > Page is probably a better place and I think everything has access to Page. When each scrollableArea finishes its scroll, ProgrammaticScrollAnimator(PSA) will notify SmoothScrollSequencer so that the next scrollableArea can start scroll (see PSA in this patch). Unfortunately, neither of PSA and ScrollableArea has access to Page. In fact, implementations of ScrollableArea(PaintLayerScrollableArea, FrameView, RootFrameViewport) have different path to access page. So I guess the best way to access SmoothScrollSequencer from PSA is through ScrollableArea's interface?
https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right): https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollableArea.h:84: virtual ProgrammaticScrollCoordinator* getProgrammaticScrollCoordinator() On 2017/03/31 15:42:45, sunyunjia wrote: > On 2017/03/28 16:29:53, bokan wrote: > > I don't think this should be part of this class' interface. As mentioned > above, > > Page is probably a better place and I think everything has access to Page. > > When each scrollableArea finishes its scroll, ProgrammaticScrollAnimator(PSA) > will notify SmoothScrollSequencer so that the next scrollableArea can start > scroll (see PSA in this patch). Unfortunately, neither of PSA and ScrollableArea > has access to Page. In fact, implementations of > ScrollableArea(PaintLayerScrollableArea, FrameView, RootFrameViewport) have > different path to access page. So I guess the best way to access > SmoothScrollSequencer from PSA is through ScrollableArea's interface? Ah, yes...ScrollableArea is in platform. It's a bit verbose but ScrollableArea->layoutBox()->document().page() should do the trick. You'll have to nullcheck layoutBox but if it doesn't have a layoutbox it wont be scrollable so you can just abort. I'd add a private helper where you need it to get the Page this way.
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #10 (id:180001) has been deleted
Patchset #9 (id:160001) has been deleted
Patchset #7 (id:120001) has been deleted
Patchset #5 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi David, We're preparing to ship smooth scroll and I've sent out an Intent-to-ship. I've fixed the comments in the patch and please let me know if anything needs to be updated. Thanks! https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:456: "Smooth ScrollIntoView is an Experimental Web" On 2017/03/28 16:29:53, bokan wrote: > I don't think we usually throw for a feature being off. I think we should just > ignore the behavior option silently if the REF isn't on (but still > scrollIntoView). Done. https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.idl (right): https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.idl:97: [RaisesException] void scrollIntoView(optional (ScrollIntoViewOptions or boolean)? arg); On 2017/03/28 16:29:53, bokan wrote: > Rather than the "or" for the argument, could we add an overload that takes > ScrollIntoViewOptions and is RuntimeEnabled? That way we don't change our public > interface without turning on the flag. Acknowledged. https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:4478: ScrollOffset(exposeRect.x().toFloat(), exposeRect.y().toFloat())); On 2017/03/28 16:29:53, bokan wrote: > Store the ScrollOffset in a variable above, outside the if(isSmooth)/else Done. https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.h (right): https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:464: ProgrammaticScrollCoordinator* getProgrammaticScrollCoordinator() On 2017/03/28 16:29:53, bokan wrote: > Please change the name to avoid confusion with the regular ScrollingCoordinator. > They're very unrelated. As I mentioned a while ago, SmoothScrollSequencer or > Scheduler, something along those lines would be more descriptive of what it > does. Done. https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h (right): https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h:192: Member<ProgrammaticScrollCoordinator> m_programmaticScrollCoordinator; On 2017/03/28 16:29:53, bokan wrote: > We want ScrollingCoordinator to eventually go away. Perhaps Page would be a > better place for this. Done. https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ProgrammaticScrollCoordinator.cpp (right): https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ProgrammaticScrollCoordinator.cpp:36: if (!m_queue.empty()) { On 2017/03/28 16:29:53, bokan wrote: > This whole block can be replaced with runQueuedAnimations(). Done. https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp (right): https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:55: for (int i = 0; i < 1500; ++i) { On 2017/03/28 16:29:53, bokan wrote: > You can modify beginFrame to take a deltaTime so that we can just instantly jump > by x milliseconds, rather than doing this loop. Done. https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:59: ASSERT_LT(window().scrollY(), 1000); On 2017/03/28 16:29:53, bokan wrote: > Is there a reason this range is so wide? Presumably there should be no > in-determinism in this test, right? Done. https://codereview.chromium.org/2650343008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:65: } On 2017/03/28 16:29:53, bokan wrote: > We should add a test for the nested case to test the queuing mechanism. Done. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp (right): https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:56: compositor().beginFrame(); Seems I have to add this extra beginFrame() to start the animation.
So is the plan (and resolution to the exceptions) to just ship it? I'm fine with that plan, but if that's the case we should ensure we have more robust tests. I've added some comments. In addition, you've also added some new options for block/inline behavior so we should add tests for those too. They don't have to be smooth though, I'd just add some instant scroll layout tests to make sure the alignment is correct. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:135: #include "core/page/scrolling/ScrollingCoordinator.h" Unused, remove. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:484: // Inline Alignment to the top / bottom and to the closest edge. Should this be left / right, rather than top / bottom? https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:485: ScrollAlignment inlineAlignment = ScrollAlignment::alignToEdgeIfNeeded; For consistency with the above block, default to alignLeftAlways here or default the blockAlignment to alignToEdgeIfNeeded. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:498: if (document().page()) { nit: remove braces https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (left): https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4508: Nit: the blank line was fine https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp (right): https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp:80: getScrollableArea()->getSmoothScrollSequencer()->notifyAnimationFinished(); What other programmatic animations is this class handling? We should make sure we can distinguish between the ones that the sequencer needs to know about. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right): https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableArea.h:84: virtual SmoothScrollSequencer* getSmoothScrollSequencer() const { return 0; } nit: return nullptr https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.cpp (right): https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.cpp:30: scrollable->programmaticScrollAnimator().animateToOffset(offset); Unless I'm missing something, I think it'd be better to pop the animation when you start it on the animator here, rather than in notifyFinished. Otherwise, calling runQueuedAnimations repeatedly will run the same animation. If we ever have a bug where nofityAnimationFinished isn't called, we wont just miss playing the next animation - we'll stall the whole queue. (I just dealt with such a bug on the CC side) https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h (right): https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h:16: class PLATFORM_EXPORT SmoothScrollSequencer Please add a class-level comment about what this class' purpose is (why we need it) and what it does https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h:37: explicit SmoothScrollSequencer(); There's no argument here so there's no implicit conversion possible. No need for explicit. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp (right): https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:56: compositor().beginFrame(); On 2017/04/07 13:53:21, sunyunjia wrote: > Seems I have to add this extra beginFrame() to start the animation. Does this mean the ASSERT above should be scrollY == 0? I can understand why the first frame might not move the animation (the next frame after the animation is set is used to set the start time) but I'm a little confused why it needs two. Could you do some debugging to determine why? https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:63: ASSERT_GE(window().scrollY(), 1000); This should be testing scrollY == 1000 right? if we scroll further we've overshot. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:112: If we start another programmatic smooth scroll (or even instant), should that cancel the original one? Or just queue after it? We should have a test for that case.
Also, in that case, I think we should wait until after the branch next week. It'd be beneficial to give this some bake time before shipping.
Patchset #10 (id:250001) has been deleted
Patchset #9 (id:230001) has been deleted
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #9 (id:270001) has been deleted
Patchset #8 (id:210001) has been deleted
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi David, I updated the patch and included an automated web platform test. PTAL, thanks! https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:135: #include "core/page/scrolling/ScrollingCoordinator.h" On 2017/04/07 15:56:52, bokan (OOO until May 9) wrote: > Unused, remove. Done. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:484: // Inline Alignment to the top / bottom and to the closest edge. On 2017/04/07 15:56:52, bokan (OOO until May 9) wrote: > Should this be left / right, rather than top / bottom? Done. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:485: ScrollAlignment inlineAlignment = ScrollAlignment::alignToEdgeIfNeeded; On 2017/04/07 15:56:52, bokan (OOO until May 9) wrote: > For consistency with the above block, default to alignLeftAlways here or default > the blockAlignment to alignToEdgeIfNeeded. I set the default value according to the spec: https://www.w3.org/TR/cssom-view-1/#dom-element-scrollintoview It says, If arg is not specified or is true, let the block dictionary member of options have the value "start", and let the inline dictionary member of options have the value "nearest" https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:498: if (document().page()) { On 2017/04/07 15:56:52, bokan (OOO until May 9) wrote: > nit: remove braces Done. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (left): https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4508: On 2017/04/07 15:56:52, bokan (OOO until May 9) wrote: > Nit: the blank line was fine Done. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp (right): https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp:80: getScrollableArea()->getSmoothScrollSequencer()->notifyAnimationFinished(); On 2017/04/07 15:56:52, bokan (OOO until May 9) wrote: > What other programmatic animations is this class handling? We should make sure > we can distinguish between the ones that the sequencer needs to know about. Done. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right): https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableArea.h:84: virtual SmoothScrollSequencer* getSmoothScrollSequencer() const { return 0; } On 2017/04/07 15:56:52, bokan (OOO until May 9) wrote: > nit: return nullptr Done. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.cpp (right): https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.cpp:30: scrollable->programmaticScrollAnimator().animateToOffset(offset); On 2017/04/07 15:56:52, bokan (OOO until May 9) wrote: > Unless I'm missing something, I think it'd be better to pop the animation when > you start it on the animator here, rather than in notifyFinished. Otherwise, > calling runQueuedAnimations repeatedly will run the same animation. If we ever > have a bug where nofityAnimationFinished isn't called, we wont just miss playing > the next animation - we'll stall the whole queue. (I just dealt with such a bug > on the CC side) Done. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h (right): https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h:16: class PLATFORM_EXPORT SmoothScrollSequencer On 2017/04/07 15:56:52, bokan (OOO until May 9) wrote: > Please add a class-level comment about what this class' purpose is (why we need > it) and what it does Done. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h:37: explicit SmoothScrollSequencer(); On 2017/04/07 15:56:52, bokan (OOO until May 9) wrote: > There's no argument here so there's no implicit conversion possible. No need for > explicit. Done. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp (right): https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:56: compositor().beginFrame(); On 2017/04/07 15:56:52, bokan wrote: > On 2017/04/07 13:53:21, sunyunjia wrote: > > Seems I have to add this extra beginFrame() to start the animation. > > Does this mean the ASSERT above should be scrollY == 0? I can understand why the > first frame might not move the animation (the next frame after the animation is > set is used to set the start time) but I'm a little confused why it needs two. > Could you do some debugging to determine why? I did some investigation. In the 1st BeginFrame(), ProgrammaticScrollAnimator::run_state_ is set to kRunningOnMainThread from kWaitingToSendToCompositor. In the 2nd BeginFrame(), TickAnimaion() got run and the start_time_ is set to then, which means no elapsed_time yet thus no scroll. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:63: ASSERT_GE(window().scrollY(), 1000); On 2017/04/07 15:56:52, bokan wrote: > This should be testing scrollY == 1000 right? if we scroll further we've > overshot. Done. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:112: On 2017/04/07 15:56:52, bokan wrote: > If we start another programmatic smooth scroll (or even instant), should that > cancel the original one? Or just queue after it? We should have a test for that > case. Thanks! I would cancel the original one. https://codereview.chromium.org/2650343008/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/RootFrameViewport.h (right): https://codereview.chromium.org/2650343008/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/RootFrameViewport.h:83: bool ScheduleAnimation() override; Not sure why this wasn't implemented. It was returning false and preventing the viewport from scrolling
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Sorry for the large amount of feedback but it's mostly about local code - I'm happy with the overall approach and tests now. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:485: ScrollAlignment inlineAlignment = ScrollAlignment::alignToEdgeIfNeeded; On 2017/05/12 18:40:24, sunyunjia wrote: > On 2017/04/07 15:56:52, bokan (OOO until May 9) wrote: > > For consistency with the above block, default to alignLeftAlways here or > default > > the blockAlignment to alignToEdgeIfNeeded. > > I set the default value according to the spec: > https://www.w3.org/TR/cssom-view-1/#dom-element-scrollintoview > > It says, If arg is not specified or is true, let the block dictionary member of > options have the value "start", and let the inline dictionary member of options > have the value "nearest" Ah, thanks and sorry, I missed that that was specified. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp (right): https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:56: compositor().beginFrame(); On 2017/05/12 18:40:25, sunyunjia wrote: > On 2017/04/07 15:56:52, bokan wrote: > > On 2017/04/07 13:53:21, sunyunjia wrote: > > > Seems I have to add this extra beginFrame() to start the animation. > > > > Does this mean the ASSERT above should be scrollY == 0? I can understand why > the > > first frame might not move the animation (the next frame after the animation > is > > set is used to set the start time) but I'm a little confused why it needs two. > > Could you do some debugging to determine why? > > I did some investigation. In the 1st BeginFrame(), > ProgrammaticScrollAnimator::run_state_ is set to kRunningOnMainThread from > kWaitingToSendToCompositor. In the 2nd BeginFrame(), TickAnimaion() got run and > the start_time_ is set to then, which means no elapsed_time yet thus no scroll. Ahh, that makes sense - thanks for digging into it. https://codereview.chromium.org/2650343008/diff/160002/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:112: On 2017/05/12 18:40:25, sunyunjia wrote: > On 2017/04/07 15:56:52, bokan wrote: > > If we start another programmatic smooth scroll (or even instant), should that > > cancel the original one? Or just queue after it? We should have a test for > that > > case. > > Thanks! I would cancel the original one. Correct me if I'm wrong but I think your current patch does this for another scroll into view, but what about a programatic scroll that's not scroll into view? (e.g. a smooth ScrollTo). In that case, I think sequenced_for_smooth_scroll_ in the ProgrammticScrollAnimator will reset to false but you'll still have all your other nested scrollers queued in the sequencer. You should reset the queue from ProgrammaticScrollAnimator when an animation is cancelled and sequenced_for_smooth_scroll_ is true. https://codereview.chromium.org/2650343008/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/RootFrameViewport.h (right): https://codereview.chromium.org/2650343008/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/RootFrameViewport.h:83: bool ScheduleAnimation() override; On 2017/05/12 18:40:25, sunyunjia wrote: > Not sure why this wasn't implemented. It was returning false and preventing the > viewport from scrolling Acknowledged. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/scroll-into-view/check-scroll-position.html (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/scroll-into-view/check-scroll-position.html:7: <script src='/resources/testharness.js'></script> Super-nit: it's customary for testharness includes to be closer to the top, below the DOCTYPE https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/scroll-into-view/check-scroll-position.html:23: assert_approx_equals(window.scrollX, x, 1); I'm kind of new to testharness.js but my read is that all asserts need to be inside step() context. If this happens after a rAF we'll be outside of one. I think you should wrap this in funct.step(function() { assert... }); https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/scroll-into-view/check-scroll-position.html:74: function end() {} Pass in null for the last case and check `next` before calling it in animate() https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:457: GetDocument().EnsurePaintLocationDataValidForNode(this); The call to EnsurePaintLocationDataValidForNode should be moved from this and the above method into the method that takes a ScrollIntoViewOptions. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:465: scrollIntoViewWithOptions(options); Can we just call the ScrollIntoViewOptionsOrBoolean method by casting the parameter? That way we're not duplicating this block. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:468: static ScrollAlignment InputAlignmentToXYAlignment( IMHO, ToPhysicalAlignment would be a better name. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:470: bool is_x, Add an enum in ScrollTypes called ScrollOrientation { kHorizontal, kVertical }; and use that instead of a bool, more readable. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:474: alignment = options.inlinePosition(); Nit: newline after this `if` https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:490: if (is_x) Nit: use ? : operator like above. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:494: // if (is_vertical) Remove https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:501: GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheetsForNode(this); This should probably just be EnsurePaintLocationDataValidForNode, as mentioned in my comment above. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:510: if (options.behavior() == "smooth") { Nit: no braces https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:520: if (GetDocument().GetPage()) We should probably abort at the top if we don't have a page. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.h (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.h:489: Nit: remove newline https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:235: DCHECK(scroll_type != kUserScroll); If we're going to DCHECK this, lets DCHECK that it's a kProgrammaticScroll. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBox.cpp:674: is_smooth = true; This is another way of saying: bool is_smooth = scroll_behavior == kScrollBehaviorSmooth || (scroll_behavior == kScrollBehaviorAuto && Style()->GetScrollBehavior() == kScrollBehaviorSmooth); https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp:85: GetScrollableArea()->GetSmoothScrollSequencer()->RunQueuedAnimations(); You need to handle the case where GetSmoothScrollSequencer returns nullptr (here and anywhere else you call it). https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp:180: if (sequenced_for_smooth_scroll_) { Put these into an "AnimationFinished" helper and call that from both places. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableArea.h:108: bool is_smooth = false); This has few enough callers, and you always pass it in anyway, that I'd avoid making it a default parameter. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.cpp (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.cpp:35: queue_.pop_back(); nit: this should happen right after reading queue_.back() as its related. As a rule of thumb, try to keep related operations as close together as you can. (Assuming you don't *need* it to be popped after calling AnimateToOffset) https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h:16: // A sequencer that stacks the nested scrollers from inside to outside, s/stacks/queues https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h:20: : public GarbageCollectedFinalized<SmoothScrollSequencer> { This class doesn't require finalization so it should inherit from GarbageCollected https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h:32: // Abort the currently running anmation and all the animations in the queue. nit: typo: anmation https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:58: ASSERT_GT(Window().scrollY(), 250); These tests should be deterministic since we control time now - make all these an ASSERT_EQ (or ASSERT_NEAR if floats are getting in the way) https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:85: MainFrame().ExecuteScriptAndReturnValue( Elliott's suggestion was to avoid calling things via JS in SimTests. Prefer to make these calls via C++ unless there's a reason not to. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:115: TEST_F(SmoothScrollTest, NewScrollIntoViewAbortsCurrentAnimation) { The test I'm most interested in is one that has some nested scrollers and mid scroll aborts and scrollintoview's a new nested scroller. I'd like to make sure the ScrollSequencer's queue is correctly cleared in those cases. Also, a test that's like this but uses window.scrollTo or something that's not scrollIntoView.
Hi David, Thanks for the careful review. I learned a lot from it. I've fixed all of them except whether SmoothScrollSequencer should be marked as GarbageCollectedFinalized. I'm not familiar with the oilpan stuff, but the compiler says the vector queue_ needs finalization. As I added a complicated nested-smooth-scroll aborts nested-smooth-scroll test, should I remove the two tests(SmoothScroll and NestedContainer) before it? In order to better coordinate all the scroll animations, I also use SmoothScrollSequencer to control window.scrollTo/scrollBy operations. Thanks! https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/scroll-into-view/check-scroll-position.html (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/scroll-into-view/check-scroll-position.html:7: <script src='/resources/testharness.js'></script> On 2017/05/15 17:15:27, bokan wrote: > Super-nit: it's customary for testharness includes to be closer to the top, > below the DOCTYPE Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/scroll-into-view/check-scroll-position.html:23: assert_approx_equals(window.scrollX, x, 1); On 2017/05/15 17:15:27, bokan wrote: > I'm kind of new to testharness.js but my read is that all asserts need to be > inside step() context. If this happens after a rAF we'll be outside of one. I > think you should wrap this in funct.step(function() { assert... }); Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/scroll-into-view/check-scroll-position.html:74: function end() {} On 2017/05/15 17:15:27, bokan wrote: > Pass in null for the last case and check `next` before calling it in animate() Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:457: GetDocument().EnsurePaintLocationDataValidForNode(this); On 2017/05/15 17:15:27, bokan wrote: > The call to EnsurePaintLocationDataValidForNode should be moved from this and > the above method into the method that takes a ScrollIntoViewOptions. Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:465: scrollIntoViewWithOptions(options); On 2017/05/15 17:15:27, bokan wrote: > Can we just call the ScrollIntoViewOptionsOrBoolean method by casting the > parameter? That way we're not duplicating this block. Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:468: static ScrollAlignment InputAlignmentToXYAlignment( On 2017/05/15 17:15:27, bokan wrote: > IMHO, ToPhysicalAlignment would be a better name. Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:470: bool is_x, On 2017/05/15 17:15:28, bokan wrote: > Add an enum in ScrollTypes called ScrollOrientation { kHorizontal, kVertical }; > and use that instead of a bool, more readable. Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:474: alignment = options.inlinePosition(); On 2017/05/15 17:15:28, bokan wrote: > Nit: newline after this `if` Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:490: if (is_x) On 2017/05/15 17:15:27, bokan wrote: > Nit: use ? : operator like above. Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:494: // if (is_vertical) On 2017/05/15 17:15:27, bokan wrote: > Remove Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:501: GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheetsForNode(this); On 2017/05/15 17:15:28, bokan wrote: > This should probably just be EnsurePaintLocationDataValidForNode, as mentioned > in my comment above. Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:510: if (options.behavior() == "smooth") { On 2017/05/15 17:15:27, bokan wrote: > Nit: no braces Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:520: if (GetDocument().GetPage()) On 2017/05/15 17:15:27, bokan wrote: > We should probably abort at the top if we don't have a page. Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.h (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.h:489: On 2017/05/15 17:15:28, bokan wrote: > Nit: remove newline Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:235: DCHECK(scroll_type != kUserScroll); On 2017/05/15 17:15:28, bokan wrote: > If we're going to DCHECK this, lets DCHECK that it's a kProgrammaticScroll. Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBox.cpp:674: is_smooth = true; On 2017/05/15 17:15:28, bokan wrote: > This is another way of saying: > > bool is_smooth = > scroll_behavior == kScrollBehaviorSmooth || > (scroll_behavior == kScrollBehaviorAuto && > Style()->GetScrollBehavior() == kScrollBehaviorSmooth); Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp:85: GetScrollableArea()->GetSmoothScrollSequencer()->RunQueuedAnimations(); On 2017/05/15 17:15:28, bokan wrote: > You need to handle the case where GetSmoothScrollSequencer returns nullptr (here > and anywhere else you call it). Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp:180: if (sequenced_for_smooth_scroll_) { On 2017/05/15 17:15:28, bokan wrote: > Put these into an "AnimationFinished" helper and call that from both places. Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableArea.h:108: bool is_smooth = false); On 2017/05/15 17:15:28, bokan wrote: > This has few enough callers, and you always pass it in anyway, that I'd avoid > making it a default parameter. Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.cpp (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.cpp:35: queue_.pop_back(); On 2017/05/15 17:15:28, bokan wrote: > nit: this should happen right after reading queue_.back() as its related. As a > rule of thumb, try to keep related operations as close together as you can. > (Assuming you don't *need* it to be popped after calling AnimateToOffset) Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h:16: // A sequencer that stacks the nested scrollers from inside to outside, On 2017/05/15 17:15:28, bokan wrote: > s/stacks/queues Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h:20: : public GarbageCollectedFinalized<SmoothScrollSequencer> { On 2017/05/15 17:15:28, bokan wrote: > This class doesn't require finalization so it should inherit from > GarbageCollected I'm not very familiar with Oilpan management, but the compiler tells me that "Field std::vector<ScrollerAndOffsetPair> queue_ requires finalization." https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h:32: // Abort the currently running anmation and all the animations in the queue. On 2017/05/15 17:15:28, bokan wrote: > nit: typo: anmation Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:58: ASSERT_GT(Window().scrollY(), 250); On 2017/05/15 17:15:28, bokan wrote: > These tests should be deterministic since we control time now - make all these > an ASSERT_EQ (or ASSERT_NEAR if floats are getting in the way) Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:85: MainFrame().ExecuteScriptAndReturnValue( On 2017/05/15 17:15:28, bokan wrote: > Elliott's suggestion was to avoid calling things via JS in SimTests. Prefer to > make these calls via C++ unless there's a reason not to. Done. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:115: TEST_F(SmoothScrollTest, NewScrollIntoViewAbortsCurrentAnimation) { On 2017/05/15 17:15:28, bokan wrote: > The test I'm most interested in is one that has some nested scrollers and mid > scroll aborts and scrollintoview's a new nested scroller. I'd like to make sure > the ScrollSequencer's queue is correctly cleared in those cases. > > Also, a test that's like this but uses window.scrollTo or something that's not > scrollIntoView. Done.
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Patchset #12 (id:370001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> As I added a complicated nested-smooth-scroll aborts nested-smooth-scroll test, should I remove the two tests(SmoothScroll and NestedContainer) before it? No, it's fine to leave them as more targeted, simple tests. https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h (right): https://codereview.chromium.org/2650343008/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h:20: : public GarbageCollectedFinalized<SmoothScrollSequencer> { On 2017/05/19 16:24:29, sunyunjia wrote: > On 2017/05/15 17:15:28, bokan wrote: > > This class doesn't require finalization so it should inherit from > > GarbageCollected > > I'm not very familiar with Oilpan management, but the compiler tells me that > "Field std::vector<ScrollerAndOffsetPair> queue_ requires finalization." Ah, we should be using HeapVector instead since we're storing handles into the Oilpan heap. In that case, to my comment in TRACE, you only need to trace the HeapVector and it'll do the iteration for you I think. See how this is done for Fullscreen::fullscreen_element_stack_. https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/scroll-into-view/check-scroll-position.html (right): https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/scroll-into-view/check-scroll-position.html:27: next(); You'll need to check if next is null now. https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:470: ScrollOrientation writing_mode) { While it kind of makes sense here, writing mode isn't a *scroll* orientation so I think leaving this as `bool is_vertical_writing_mode` would be better. https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1213: void LocalDOMWindow::scrollHelper(ScrollableArea* viewport, This name is too generic. Maybe "scrollViewportTo"? https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:177: smooth_scroll_sequencer_ = SmoothScrollSequencer::Create(); Since we're using this for more than just smooth scrollIntoViews now, lets just create it in the constructor rather than lazily. https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.cpp (right): https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.cpp:44: DEFINE_TRACE(SmoothScrollSequencer) { I think you'll also need to visit each ScrollableArea in the queue https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h (right): https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h:19: class PLATFORM_EXPORT SmoothScrollSequencer lets mark this class as final https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp (right): https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:123: "<div id='container2' style='height: 1000px; overflow: scroll'>" This <div> is missing a </div> https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:171: Compositor().BeginFrame(1); Add one or two more begin frames here to make sure there isn't still another animation queued on container1
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! I've updated the patch. The compiler says, static_assert failed "HeapVectorBacking doesn't support objects that cannot be cleared as unused with memset." So I use std::pair<double, double> to store ScrollOffset in the HeapVector. https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/scroll-into-view/check-scroll-position.html (right): https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/scroll-into-view/check-scroll-position.html:27: next(); On 2017/05/19 18:37:14, bokan wrote: > You'll need to check if next is null now. Done. https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:470: ScrollOrientation writing_mode) { On 2017/05/19 18:37:14, bokan wrote: > While it kind of makes sense here, writing mode isn't a *scroll* orientation so > I think leaving this as `bool is_vertical_writing_mode` would be better. Done. https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1213: void LocalDOMWindow::scrollHelper(ScrollableArea* viewport, On 2017/05/19 18:37:14, bokan wrote: > This name is too generic. Maybe "scrollViewportTo"? Done. https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:177: smooth_scroll_sequencer_ = SmoothScrollSequencer::Create(); On 2017/05/19 18:37:14, bokan wrote: > Since we're using this for more than just smooth scrollIntoViews now, lets just > create it in the constructor rather than lazily. Done. https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.cpp (right): https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.cpp:44: DEFINE_TRACE(SmoothScrollSequencer) { On 2017/05/19 18:37:14, bokan wrote: > I think you'll also need to visit each ScrollableArea in the queue Acknowledged. https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h (right): https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.h:19: class PLATFORM_EXPORT SmoothScrollSequencer On 2017/05/19 18:37:14, bokan wrote: > lets mark this class as final Done. https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp (right): https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:123: "<div id='container2' style='height: 1000px; overflow: scroll'>" On 2017/05/19 18:37:14, bokan wrote: > This <div> is missing a </div> Done. https://codereview.chromium.org/2650343008/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/SmoothScrollTest.cpp:171: Compositor().BeginFrame(1); On 2017/05/19 18:37:14, bokan wrote: > Add one or two more begin frames here to make sure there isn't still another > animation queued on container1 Done.
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Patchset #13 (id:410001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/05/19 22:30:41, sunyunjia wrote: > Thanks! I've updated the patch. The compiler says, static_assert failed > "HeapVectorBacking doesn't support objects that cannot be cleared as unused with > memset." So I use std::pair<double, double> to store ScrollOffset in the > HeapVector. Ah, that's because ScrollOffset (aka FloatSize) has a non-default constructor that clears the floats to 0. I believe you can make it compile by adding WTF_ALLOW_CLEAR_UNUSED_SLOTS_WITH_MEM_FUNCTIONS(blink::FloatSize); to FloatSize.h. +sof@ to confirm that this is the right thing to do. See SmoothScrollSequencer.h - we're trying to create a HeapVector< std::pair<Member<ScrollableArea>, FloatSize >.
bokan@chromium.org changed reviewers: + sigbjornf@opera.com
Oops, actually +sof@ (please see my comment above).
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/23 15:28:12, bokan wrote: > On 2017/05/19 22:30:41, sunyunjia wrote: > > Thanks! I've updated the patch. The compiler says, static_assert failed > > "HeapVectorBacking doesn't support objects that cannot be cleared as unused > with > > memset." So I use std::pair<double, double> to store ScrollOffset in the > > HeapVector. > > Ah, that's because ScrollOffset (aka FloatSize) has a non-default constructor > that clears the floats to 0. I believe you can make it compile by adding > WTF_ALLOW_CLEAR_UNUSED_SLOTS_WITH_MEM_FUNCTIONS(blink::FloatSize); > to FloatSize.h. > > +sof@ to confirm that this is the right thing to do. See SmoothScrollSequencer.h > - we're trying to create a HeapVector< std::pair<Member<ScrollableArea>, > FloatSize >. Thanks, that's done. Should I send the patch to other reviewers?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/25 13:27:58, sunyunjia wrote: > On 2017/05/23 15:28:12, bokan wrote: > > On 2017/05/19 22:30:41, sunyunjia wrote: > > > Thanks! I've updated the patch. The compiler says, static_assert failed > > > "HeapVectorBacking doesn't support objects that cannot be cleared as unused > > with > > > memset." So I use std::pair<double, double> to store ScrollOffset in the > > > HeapVector. > > > > Ah, that's because ScrollOffset (aka FloatSize) has a non-default constructor > > that clears the floats to 0. I believe you can make it compile by adding > > WTF_ALLOW_CLEAR_UNUSED_SLOTS_WITH_MEM_FUNCTIONS(blink::FloatSize); > > to FloatSize.h. > > > > +sof@ to confirm that this is the right thing to do. See > SmoothScrollSequencer.h > > - we're trying to create a HeapVector< std::pair<Member<ScrollableArea>, > > FloatSize >. > > Thanks, that's done. Should I send the patch to other reviewers? annotation is fine (and the right one), perhaps add a short comment next to it to help the reader understand purpose?
Ok, lgtm from me then :)
sunyunjia@chromium.org changed reviewers: + jbroman@chromium.org
Thanks bokan@ and sof@ ! jbroman@, could you please review this patch for "third_party/Webkit/Source/platform/"? Thanks!
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rs lgtm https://codereview.chromium.org/2650343008/diff/530001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/geometry/FloatSize.h (right): https://codereview.chromium.org/2650343008/diff/530001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/geometry/FloatSize.h:202: // Allows Oilpan to create objects with this class. See SmoothScrollSequencer.h nit: It's a vector trait thing, specifically. Also the reference to SmoothScrollSequencer.h doesn't seem like it explains much more. Suggest something like "Allows this class to be stored in a HeapVector." https://codereview.chromium.org/2650343008/diff/530001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.cpp (right): https://codereview.chromium.org/2650343008/diff/530001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/SmoothScrollSequencer.cpp:4: #include "platform/scroll/SmoothScrollSequencer.h" nit: blank line after copyright header
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sunyunjia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, jbroman@chromium.org Link to the patchset: https://codereview.chromium.org/2650343008/#ps550001 (title: "Fixed nits.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 550001, "attempt_start_ts": 1496094778194080, "parent_rev": "33588a10e58a5a53909fbcc2886f71c6cb4aec07", "commit_rev": "bbf870d971d95af7ae1ee688a7ed100e3787d02b"}
Message was sent while issue was closed.
Description was changed from ========== Implement Element.scrollIntoView for scroll-behavior: smooth. Currently in Chrome, a call to Element.scrollIntoView will calculate the amount each scroller needs to scroll to align the Element as specified and instantly set the scroll position on that scroller (going from the innermost to the outermost scroller). If Element.scrollIntoView is called with scroll-behavior: smooth, instead of scrolling instantly, we should animate to the desired position. We can’t simply call setScrollPosition in PaintLayerScrollableArea and FrameView with ScrollBehaviorSmooth because of the following reasons: - We want to run the animation from the outermost scroller to the innermost scroller - We want to run the animation one after another. That is, the scroll animation on the child scroller should start after the animation on the parent scroller is finished. This patch adds a new class, ProgrammaticScrollCoordinator, that manages programmatic scroll animations. BUG=648446 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Implement Element.scrollIntoView for scroll-behavior: smooth. Currently in Chrome, a call to Element.scrollIntoView will calculate the amount each scroller needs to scroll to align the Element as specified and instantly set the scroll position on that scroller (going from the innermost to the outermost scroller). If Element.scrollIntoView is called with scroll-behavior: smooth, instead of scrolling instantly, we should animate to the desired position. We can’t simply call setScrollPosition in PaintLayerScrollableArea and FrameView with ScrollBehaviorSmooth because of the following reasons: - We want to run the animation from the outermost scroller to the innermost scroller - We want to run the animation one after another. That is, the scroll animation on the child scroller should start after the animation on the parent scroller is finished. This patch adds a new class, ProgrammaticScrollCoordinator, that manages programmatic scroll animations. BUG=648446 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2650343008 Cr-Commit-Position: refs/heads/master@{#475387} Committed: https://chromium.googlesource.com/chromium/src/+/bbf870d971d95af7ae1ee688a7ed... ==========
Message was sent while issue was closed.
Committed patchset #19 (id:550001) as https://chromium.googlesource.com/chromium/src/+/bbf870d971d95af7ae1ee688a7ed...
Message was sent while issue was closed.
Looks this CL made the "WebKit Linux Trusty Leak" bot red. https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty... Unexpected Failures: * fast/scroll-behavior/subframe-interrupted-scroll.html * virtual/scroll_customization/fast/scroll-behavior/subframe-interrupted-scroll.html * virtual/threaded/fast/scroll-behavior/subframe-interrupted-scroll.html https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty... https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty... https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty... Please take a look.
Message was sent while issue was closed.
A revert of this CL (patchset #19 id:550001) has been created in https://codereview.chromium.org/2911103002/ by tyoshino@chromium.org. The reason for reverting is: Looks this CL made the "WebKit Linux Trusty Leak" bot red. See https://codereview.chromium.org/2650343008/#msg131.
Message was sent while issue was closed.
Description was changed from ========== Implement Element.scrollIntoView for scroll-behavior: smooth. Currently in Chrome, a call to Element.scrollIntoView will calculate the amount each scroller needs to scroll to align the Element as specified and instantly set the scroll position on that scroller (going from the innermost to the outermost scroller). If Element.scrollIntoView is called with scroll-behavior: smooth, instead of scrolling instantly, we should animate to the desired position. We can’t simply call setScrollPosition in PaintLayerScrollableArea and FrameView with ScrollBehaviorSmooth because of the following reasons: - We want to run the animation from the outermost scroller to the innermost scroller - We want to run the animation one after another. That is, the scroll animation on the child scroller should start after the animation on the parent scroller is finished. This patch adds a new class, ProgrammaticScrollCoordinator, that manages programmatic scroll animations. BUG=648446 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2650343008 Cr-Commit-Position: refs/heads/master@{#475387} Committed: https://chromium.googlesource.com/chromium/src/+/bbf870d971d95af7ae1ee688a7ed... ========== to ========== Implement Element.scrollIntoView for scroll-behavior: smooth. Currently in Chrome, a call to Element.scrollIntoView will calculate the amount each scroller needs to scroll to align the Element as specified and instantly set the scroll position on that scroller (going from the innermost to the outermost scroller). If Element.scrollIntoView is called with scroll-behavior: smooth, instead of scrolling instantly, we should animate to the desired position. We can’t simply call setScrollPosition in PaintLayerScrollableArea and FrameView with ScrollBehaviorSmooth because of the following reasons: - We want to run the animation from the outermost scroller to the innermost scroller - We want to run the animation one after another. That is, the scroll animation on the child scroller should start after the animation on the parent scroller is finished. This patch adds a new class, ProgrammaticScrollCoordinator, that manages programmatic scroll animations. BUG=648446 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2650343008 Cr-Commit-Position: refs/heads/master@{#475387} Committed: https://chromium.googlesource.com/chromium/src/+/bbf870d971d95af7ae1ee688a7ed... ==========
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #23 (id:630001) has been deleted
Patchset #20 (id:570001) has been deleted
Patchset #20 (id:590001) has been deleted
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
@bokan, I've updated the patch by aborting SequencedSmoothScroll in more places to make sure that there is no memory leak. These include: 1. Abort them when we do UserScroll. 2. Abort them in most SetScrollOffset, except for those called inside a smooth scroll process. Please take a look at the changes. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/01 19:21:29, sunyunjia wrote: > @bokan, > > I've updated the patch by aborting SequencedSmoothScroll in more places to make > sure that there is no memory leak. These include: > 1. Abort them when we do UserScroll. > 2. Abort them in most SetScrollOffset, except for those called inside a smooth > scroll process. > > Please take a look at the changes. Thanks! We discussed the other day adding a new ScrollType for SequencedScroll but you chose to add a new param instead. Looking through the patch it seems that's because animations on RootFrameViewport call through the Distribute method which calls SetScrollOffset and so we'd abort the animation there (I hadn't thought of that, drats). Is this right? Before we go down this path, I'd like to see if there's some changes we could make to get away with using a new ScrollType instead. Assuming the reason is the distribute problem above, I have two ideas that might help here (though this code is complex so they may have their own drawbacks I'm missing right now): 1. ProgrammaticScrollAnimator knows when it's ticking an animation in a sequence. You should be able to change the scroll type in PSA::NotifyOffsetChanged and it should get propagated all the way through DistributeScrollBetweenViewports 2. DistributeScrollBetweenViewports shouldn't be calling primary|secondary.SetScrollOffset unless it's called from RFV::SetScrollOffset. To me, the meaning of SetScrollOffset is "initiate a new scroll". If we're ticking an animation, we shouldn't be calling it - we should be using UpdateScrollOffset. Could you try changing to use the scroll type with the above (or let me know why we can't)? The reason I'd like to avoid adding a parameter vs a new scroll type is that adding a new param is a combinatoric increase in complexity - we now have to consider how it interacts with each other parameter and each kind of scroll type so it's more difficult to reason about. Adding a new scroll type isn't free, but at least we don't have to worry about how it interacts with the other scroll types. (PS, when you rebase a patch, please upload the unchanged but rebased patch first - then upload your changes. If you upload both the rebase and your changes, especially on a patch this large, it makes it impossible for me to see the changes you made without looking at the entire patch)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #24 (id:710001) has been deleted
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bokan@, I added kSequencedSmoothScroll as a ScrollType to indicate whether the scroll should cancel the animation in sequencer. PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, I like this much better. https://codereview.chromium.org/2650343008/diff/730001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp (right): https://codereview.chromium.org/2650343008/diff/730001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp:34: ScrollOffsetChanged(offset, kSequencedSmoothScroll); It doesn't matter today since ScrollOffsetChanged doesn't do anything based on Programmatic vs SequencedSmoothScroll, but this should be based on sequenced_for_smooth_scroll_. https://codereview.chromium.org/2650343008/diff/730001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): https://codereview.chromium.org/2650343008/diff/730001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:177: scroll_type != kCompositorScroll) { I think we want to cancel if we get a kCompositorScroll. kCompositorScroll means the scroll changed in CC (i.e. the user scrolled the layer on the compositor) so it's a user scroll, just outside of Blink. I do think that we probably don't want to abort a sequenced scroll on Clamping and Anchoring scrolls though.
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #27 (id:790001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #27 (id:810001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bokan@, PTAL, thanks! https://codereview.chromium.org/2650343008/diff/730001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp (right): https://codereview.chromium.org/2650343008/diff/730001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp:34: ScrollOffsetChanged(offset, kSequencedSmoothScroll); On 2017/06/06 21:34:18, bokan wrote: > It doesn't matter today since ScrollOffsetChanged doesn't do anything based on > Programmatic vs SequencedSmoothScroll, but this should be based on > sequenced_for_smooth_scroll_. Done. https://codereview.chromium.org/2650343008/diff/730001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): https://codereview.chromium.org/2650343008/diff/730001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:177: scroll_type != kCompositorScroll) { On 2017/06/06 21:34:18, bokan wrote: > I think we want to cancel if we get a kCompositorScroll. kCompositorScroll means > the scroll changed in CC (i.e. the user scrolled the layer on the compositor) so > it's a user scroll, just outside of Blink. I do think that we probably don't > want to abort a sequenced scroll on Clamping and Anchoring scrolls though. Done.
Just a suggested rearrangement of one line but otherwise lgtm to reland https://codereview.chromium.org/2650343008/diff/770001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp (right): https://codereview.chromium.org/2650343008/diff/770001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp:34: if (sequenced_for_smooth_scroll_) nit: scroll_type = sequenced_for_smooth_scroll ? kSequenced : kProgrammatic https://codereview.chromium.org/2650343008/diff/770001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): https://codereview.chromium.org/2650343008/diff/770001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:177: scroll_type != kAnchoringScroll) { just FYI: I'm not sure how anchoring scrolls should interact with this since a scroll anchor could change the location of the thing we're trying to scroll into view. But stopping the scroll into view doesn't sound good to me either and trying to recalculate on the fly sounds incredibly complex for what should be an edge case so I think this is fine unless we find it doesn't work well in practice.
https://codereview.chromium.org/2650343008/diff/770001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp (right): https://codereview.chromium.org/2650343008/diff/770001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp:129: // information to the compositor thread. Oh, and please file a bug to make this work with compositor. Add the bug # here.
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2650343008/diff/770001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp (right): https://codereview.chromium.org/2650343008/diff/770001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp:34: if (sequenced_for_smooth_scroll_) On 2017/06/07 17:56:00, bokan wrote: > nit: scroll_type = sequenced_for_smooth_scroll ? kSequenced : kProgrammatic Done. https://codereview.chromium.org/2650343008/diff/770001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp:129: // information to the compositor thread. On 2017/06/07 17:56:53, bokan wrote: > Oh, and please file a bug to make this work with compositor. Add the bug # here. Done. https://codereview.chromium.org/2650343008/diff/770001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): https://codereview.chromium.org/2650343008/diff/770001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:177: scroll_type != kAnchoringScroll) { On 2017/06/07 17:56:00, bokan wrote: > just FYI: I'm not sure how anchoring scrolls should interact with this since a > scroll anchor could change the location of the thing we're trying to scroll into > view. But stopping the scroll into view doesn't sound good to me either and > trying to recalculate on the fly sounds incredibly complex for what should be an > edge case so I think this is fine unless we find it doesn't work well in > practice. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sunyunjia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org, bokan@chromium.org Link to the patchset: https://codereview.chromium.org/2650343008/#ps850001 (title: "Fixed nits.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 850001, "attempt_start_ts": 1496930062882270, "parent_rev": "59f0f198fe28e6c3881f54416552552ad5434cb0", "commit_rev": "61b73ca665e42dfb7953de8a4daf1d4144805fef"}
Message was sent while issue was closed.
Description was changed from ========== Implement Element.scrollIntoView for scroll-behavior: smooth. Currently in Chrome, a call to Element.scrollIntoView will calculate the amount each scroller needs to scroll to align the Element as specified and instantly set the scroll position on that scroller (going from the innermost to the outermost scroller). If Element.scrollIntoView is called with scroll-behavior: smooth, instead of scrolling instantly, we should animate to the desired position. We can’t simply call setScrollPosition in PaintLayerScrollableArea and FrameView with ScrollBehaviorSmooth because of the following reasons: - We want to run the animation from the outermost scroller to the innermost scroller - We want to run the animation one after another. That is, the scroll animation on the child scroller should start after the animation on the parent scroller is finished. This patch adds a new class, ProgrammaticScrollCoordinator, that manages programmatic scroll animations. BUG=648446 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2650343008 Cr-Commit-Position: refs/heads/master@{#475387} Committed: https://chromium.googlesource.com/chromium/src/+/bbf870d971d95af7ae1ee688a7ed... ========== to ========== Implement Element.scrollIntoView for scroll-behavior: smooth. Currently in Chrome, a call to Element.scrollIntoView will calculate the amount each scroller needs to scroll to align the Element as specified and instantly set the scroll position on that scroller (going from the innermost to the outermost scroller). If Element.scrollIntoView is called with scroll-behavior: smooth, instead of scrolling instantly, we should animate to the desired position. We can’t simply call setScrollPosition in PaintLayerScrollableArea and FrameView with ScrollBehaviorSmooth because of the following reasons: - We want to run the animation from the outermost scroller to the innermost scroller - We want to run the animation one after another. That is, the scroll animation on the child scroller should start after the animation on the parent scroller is finished. This patch adds a new class, ProgrammaticScrollCoordinator, that manages programmatic scroll animations. BUG=648446 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2650343008 Cr-Original-Commit-Position: refs/heads/master@{#475387} Committed: https://chromium.googlesource.com/chromium/src/+/bbf870d971d95af7ae1ee688a7ed... Review-Url: https://codereview.chromium.org/2650343008 Cr-Commit-Position: refs/heads/master@{#477953} Committed: https://chromium.googlesource.com/chromium/src/+/61b73ca665e42dfb7953de8a4daf... ==========
Message was sent while issue was closed.
Committed patchset #28 (id:850001) as https://chromium.googlesource.com/chromium/src/+/61b73ca665e42dfb7953de8a4daf...
Message was sent while issue was closed.
foolip@chromium.org changed reviewers: + foolip@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2650343008/diff/850001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/scroll-into-view/check-scroll-position.html (right): https://codereview.chromium.org/2650343008/diff/850001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/scroll-into-view/check-scroll-position.html:4: <title> Check End Position of ScrollIntoView</title> Some feedback on this test. Because the spec is at https://drafts.csswg.org/cssom-view/#dom-element-scrollintoview, this test should be under cssom-view/. That directory is a bit messy, but since this is testing element.scrollIntoView, perhaps calling it cssom-view/scrollIntoView.html or maybe scrollIntoView-smooth.html would be OK. On this line, there's some leading whitespace, and can you s/ScrollIntoView/scrollIntoView/ everywhere to match the name of the API? https://codereview.chromium.org/2650343008/diff/850001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/scroll-into-view/check-scroll-position.html:5: <div id='container' style='height: 2500px; width: 2500px;'> Running this test manually the results aren't in view, and scrolling to find it might interfere with the test if you don't know if it's done yet. This will fix that: add_completion_callback(() => document.getElementById('container').remove()); https://codereview.chromium.org/2650343008/diff/850001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/scroll-into-view/check-scroll-position.html:18: function animate (funct, x, y, next) { s/funct/test/, as funct here is bound to a Test instance, it was not clear at first read. https://codereview.chromium.org/2650343008/diff/850001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/scroll-into-view/check-scroll-position.html:19: if (frames < 500) { Looks like this test will wait for 2000 frames in total, which is >30 seconds at 60 fps. Surprisingly, the runtimes seen on https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType... are much more reasonable, maybe when running tests we tick frames as fast as possible? In any case, this test at http://web-platform.test:8000/scroll-into-view/check-scroll-position.html is very slow, can anything be done to make it faster? Perhaps, for example, the scroll position can be monitored, and once it's stopped changing for a second, then we assume it's done? Or perhaps a new web-exposed API is needed for web developers and test writers alike to tell if a scroll is ongoing?
Message was sent while issue was closed.
https://codereview.chromium.org/2650343008/diff/850001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.idl (right): https://codereview.chromium.org/2650343008/diff/850001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.idl:97: void scrollIntoView(optional (ScrollIntoViewOptions or boolean)? arg); This does not match the IDL in https://drafts.csswg.org/cssom-view/#extension-to-the-element-interface and unfortunately there can be differences in cases like this, see https://github.com/heycam/webidl/issues/351 Can you either have the spec changed or match the spec, and make sure that both scrollIntoView(undefined) and scrollIntoView(null) are tested? |