CSSOM-View defines a new css property, 'scroll-behavior'. When set to 'smooth', scrolling boxes will be scrolled in a smooth fashion using a user-agent-defined function.

In addition to enabling smooth scrolling effects, more complex behavior such as scroll snapping can be implemented using this feature without having to update the scroll position frame-by-frame.

Initial brainstorm of implementation details and decisions required: Initial Questions ----------------- - What model should be used for the animation / physics model? * Mass-spring-damper model? * Fitted smoothstep? - Need to identify platform differences that effect behavior (touch screen, mobile form factor, DPI differences) - Fixed point (app units) or floating point physics calculation? If using fixed point math, ensure that physics loop can converge. - Should the position be allowed to over-shoot the target? - Should momentum be maintained prior to start of scrolling animation? - When smooth-behavior is set to “instant” during the scrolling animation, should the animation stop in its current position or instantly move to the target position. Desired Behavior ---------------- - Animation duration should be deterministic and consistent given the same initial momentum and distance to the target scroll position. - Animations should be able to be chained together or continue movement resulting from user interaction with a fluid result. - Animation should not be resolution-dependent. (Should be HiDPI aware) - Animation should be not be framerate-dependent. - Is Euler integration close enough? * Should use Euler integration with multiple iterations of a small fixed deltatime + interpolation? * Use an RK4 Integrator? Solution -------- - Animation should not be allowed to over-shoot the target. - Momentum should be retained at the start of the animation * From user manipulation / dragging. * From previous scroll animation. - Implement as a critically-damped mass-spring-damper model. - When scrolling bi-directionally, implement a single degree of freedom mass-spring-damper model independently on the X and Y axis to avoid spiralling motion. - Will not overshoot target or oscillate. - Inherit momentum from prior user interaction or animations. - Use RK4 Integrator for ‘close enough’ frame-rate independent motion. - Implement constraints (maximum velocity / force) to ensure stability and convergence. - When within a single pixel, clamp to target integer pixel value and end animation. - When smooth-behavior is set to “instant” during the scrolling animation, should the animation should instantly move to the target position. - This behavior should be consistent with Bug 964097 and may need to be tuned to conform with the conventions of each platform.

I have created a Javascript implementation of a Critically-Damped mass-spring-damper model to illustrate the motion that could be applied to the smooth scroll effect.

The property of not overshooting is only maintained when starting from a zero-momentum state. If you can direction rapidly, or perform a "flick" gesture, it can momentarily overshoot. This seems to feel good to me, but I look forward to everyone's feedback.

Feels pretty nice to me, although I think the movement slows down a little too much towards the end as it slides into place. Adjusting that last part to be just a tad faster would be nice, I think.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5) > Feels pretty nice to me, although I think the movement slows down a little > too much towards the end as it slides into place. Adjusting that last part > to be just a tad faster would be nice, I think. The deceleration curve parameters can be be exposed through some prefs for UX tuning, much like the APZ fling movement.

After reviewing the APZ code, it seems a natural fit to implement the critically-damped-movement within APZ's Axis class. The Axis class encapsulates the velocity-friction model that is used for fling gestures and already calculates the initial parameter (velocity) needed to animate the smoothed scroll position update.

If we implement the behavior within APZ classes, then this feature will potentially land sooner on B2G than on Desktop. Bug 1013364 tracks the dependencies blocking APZ being enabled on desktop by default:

It sounds like you haven't yet decided where the code for this will live. Can you confirm that? Note that bug 1013364 probably won't be fully fixed for a while (order of many months) and Fennec also doesn't use the APZ classes yet, so if this is implement in APZ code it will be unavailable to everything other than B2G (and Metro) for a while.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9) > It sounds like you haven't yet decided where the code for this will live. > Can you confirm that? Note that bug 1013364 probably won't be fully fixed > for a while (order of many months) and Fennec also doesn't use the APZ > classes yet, so if this is implement in APZ code it will be unavailable to > everything other than B2G (and Metro) for a while. I haven't yet decided specifically where the code will live. An immediate use case of this feature will be for the B2G home screen; however, it would be nice to land it on desktop earlier. Perhaps I could encapsulate the scrolling behavior in such a way that it can be used with and without APZ with minimal code redundancy. The question is whether the additional complexity of an additional class for this is beneficial, or if the time is better spent on other features, such as landing APZ on desktop.

(In reply to :kip (Kearwood Gilbert) from comment #2) Initial conversations on implementation of scroll-behavior: smooth considered that the property would effectively be "read-only", much like how position: sticky has been implemented. When smooth scrolling is enabled for an element, CSSOM methods would return the target location immediately as though the scrolling has already completed. Effectively, Javascript would not be aware of any effect from enabling smooth scrolling. This has the advantage that applications will not depend on the smooth scrolling behavior, which is defined as UA and platform specific behavior which may change over time. (User interface and input device paradigms may change in the future for example). The disadvantages of synthesizing the position in the compositor include that the UA must handle position updates of elements that need to move in a coordinated fashion with the scroll position. This has already been completed for "position: sticky". To enable some popular effects, smooth scrolling could be augmented by enabling the scroll position to be applied as a time source to animations.

I have posted an Intent to Implement on dev-platform: https://groups.google.com/forum/#!topic/mozilla.dev.platform/mrsNyaLj3Ig

(In reply to :kip (Kearwood Gilbert) from comment #11) > (In reply to :kip (Kearwood Gilbert) from comment #2) > > Initial conversations on implementation of scroll-behavior: smooth > considered that the property would effectively be "read-only", much like how Correction: "write-only"

Discussions related to implementation of this are here: http://lists.w3.org/Archives/Public/www-style/2014May/0255.html And here: http://lists.w3.org/Archives/Public/www-style/2013May/0361.html

(In reply to :kip (Kearwood Gilbert) from comment #11) > (In reply to :kip (Kearwood Gilbert) from comment #2) > > Initial conversations on implementation of scroll-behavior: smooth > considered that the property would effectively be "write-only", much like how > position: sticky has been implemented. When smooth scrolling is enabled for > an element, CSSOM methods would return the target location immediately as > though the scrolling has already completed. Effectively, Javascript would > not be aware of any effect from enabling smooth scrolling. > In the event that a scroll operation is cancelled, either by user interaction (scroll wheel, touch gestures, etc) or through script, the "write-only" nature proposed above will have "lied" to scripts about the ending position at the start of the animation. Once the animation is cancelled, the DOM methods would need to return the true scroll position, which may appear to scripts as scrolling in the reverse direction.

Looping in Vivien for his feedback. Please /msg me on irc, email, or skype when convenient.

Based on the feedback on dev-platform, here are some changes to the implementation proposed in Comment 2: > Initial conversations on implementation of scroll-behavior: smooth > considered that the property would effectively be "write-only", much like how > position: sticky has been implemented. When smooth scrolling is enabled for > an element, CSSOM methods would return the target location immediately as > though the scrolling has already completed. Effectively, Javascript would > not be aware of any effect from enabling smooth scrolling. The CSSOM methods will return the actual target location rather than the target location. Making the property "write-only" introduced unnecessary complexity in cases such as cancelling scrolling (See comment 15) and was inconsistent with the behavior when performing a fling gesture.

To address the feedback on dev-platform regarding the uncertainty of use cases for the scroll-behavior css property, the CSSOM scrolling API's can be implemented first as they appear to provide the most immediate value. If the updated scrolling API lands without the scroll-behavior css property, then a separate bug will be filed to track the scroll-behavior css property independently. Also, it is expected that the smooth scrolling will land in two stages. First will be an implementation based on APZ to solve our immediate use case (B2G home page) with a follow-up bug tracking an implementation independent of APZ.

(In reply to :kip (Kearwood Gilbert) from comment #18) > ... > Also, it is expected that the smooth scrolling will land in two stages. > First will be an implementation based on APZ to solve our immediate use case > (B2G home page) with a follow-up bug tracking an implementation independent > of APZ. Do we have to do the non-APZ version?

:kip, if you intent to implement the DOM API parts first, please file a new bug blocking bug 964097 so this bug can focus on the CSS property. For the plumbing parts (both APZ and non-APZ) you may want to add yet another (or two) bug(s) (blocking bug 964097) to keep the bugs small and focused. Thanks!

As per Comment 20, Bug 1022818 has been added to track the DOM API parts. This one (Bug 1010538) will track the scroll-behavior CSS property.

Bug 1022825 has been created to separately track the implementation of the smooth scrolling movement model for APZ.

Thanks a lot! Great to see some progress here :)

(In reply to :kip (Kearwood Gilbert) from comment #16) > Looping in Vivien for his feedback. Please /msg me on irc, email, or skype > when convenient. Sorry I'm really not sure which kind of informations you expect from me.

This may look familiar, as it includes much of the refactoring that will also be needed for Bug 945584 (Implement CSS scroll snapping).

This may look familiar, as it includes much of the refactoring that will also be needed for Bug 945584 (Implement CSS scroll snapping).

Tests will be included in later patch

V2 Patch: - Changed type-o in patch comment (Bug number was missing a digit)

scroll-behavior-1.html - Viewport Scrolling, expected to be smooth: - DOM Method not specifying scroll-behavior, for <body> tag with 'scroll-behavior: smooth’ scroll-behavior-2.html - Div Scrolling, expected to be smooth: - DOM Method not specifying scroll-behavior, for <div> tag with 'scroll-behavior: smooth’ - DOM Method specifying ‘auto’ scroll-behavior, for <div> tag with ‘scroll-behavior: smooth’ - DOM Method specifying 'smooth’ scroll-behavior, for <div> tag with ‘scroll-behavior: smooth’ - DOM Method specifying ‘smooth’ scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method specifying ‘smooth’ scroll-behavior, for <div> tag with no scroll-behavior attribute. scroll-behavior-3.html - Div Scrolling, expected to be instant: - DOM Method not specifying scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method not specifying scroll-behavior, for <div> tag with no scroll-behavior attribute. - DOM Method specifying ‘instant’ scroll-behavior, for <div> tag with ‘scroll-behavior: smooth’ - DOM Method specifying ‘instant’ scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method specifying ‘instant’ scroll-behavior, for <div> tag with no scroll-behavior attribute. - DOM Method specifying ‘auto’ scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method specifying ‘auto’ scroll-behavior, for <div> tag with no scroll-behavior attribute.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=45486b067548

Can you remind me what the use-cases are for this property?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32) > Can you remind me what the use-cases are for this property? This property, part of the CSSOM-View scroll-behavior specification, essentially allows you change the default "behavior" parameter for CSSOM-View scroll DOM calls. For example, you would normally write this every time you want to smoothly scroll the viewport: window.scrollTo(1234,5678, {behavior: "smooth"}); Instead, you could specify that smooth is the default with CSS: <body style="scroll-behavior: smooth"> Then the smooth scrolling happens transparently with: window.scrollTo(1234,5678); The uses cases for this are the same as smooth scrolling in general: - Go to top buttons, go to bottom buttons, scroll section into view buttons - Carousel style scrolling animations - Adjusting / overriding the destination of a fling gesture (scroll nudging) - Log file / chat interface automatic scrolling to new messages

(In reply to :kip (Kearwood Gilbert) from comment #33) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32) > > Can you remind me what the use-cases are for this property? > > This property, part of the CSSOM-View scroll-behavior specification, > essentially allows you change the default "behavior" parameter for > CSSOM-View scroll DOM calls. > > For example, you would normally write this every time you want to smoothly > scroll the viewport: > > window.scrollTo(1234,5678, {behavior: "smooth"}); > > Instead, you could specify that smooth is the default with CSS: > > <body style="scroll-behavior: smooth"> > > Then the smooth scrolling happens transparently with: > > window.scrollTo(1234,5678); > > The uses cases for this are the same as smooth scrolling in general: > - Go to top buttons, go to bottom buttons, scroll section into view buttons > - Carousel style scrolling animations > - Adjusting / overriding the destination of a fling gesture (scroll nudging) > - Log file / chat interface automatic scrolling to new messages Another way to explain... CSSOM-View smooth scroll-behavior enables scripts to perform scrolling that does not abruptly shift the view, block the user's input, or feel janky when interacting with touch screen gestures, momentum based scrolling, and over-scrolling. By setting the scroll-behavior CSS property to "smooth", this effect is enabled for existing scripts without any modification.

V2 Patch: - Corrected mochitest failures reported by try push

(In reply to :kip (Kearwood Gilbert) from comment #33) > This property, part of the CSSOM-View scroll-behavior specification, > essentially allows you change the default "behavior" parameter for > CSSOM-View scroll DOM calls. > > For example, you would normally write this every time you want to smoothly > scroll the viewport: > > window.scrollTo(1234,5678, {behavior: "smooth"}); > > Instead, you could specify that smooth is the default with CSS: > > <body style="scroll-behavior: smooth"> > > Then the smooth scrolling happens transparently with: > > window.scrollTo(1234,5678); Right, I already know that :-). The question is why a CSS property would be preferred to the DOM API in some cases. > The uses cases for this are the same as smooth scrolling in general: > - Go to top buttons, go to bottom buttons, scroll section into view buttons These buttons presumably run scripts to do those actions via scrollTo or some other DOM scrolling API. Given they're already calling a DOM scrolling API, it makes sense for them to pass {behavior:"smooth"} as a parameter to that API. > - Carousel style scrolling animations > - Adjusting / overriding the destination of a fling gesture (scroll nudging) > - Log file / chat interface automatic scrolling to new messages These also all sound like scripts calling DOM APIs, to me. It seems to me a CSS property would be preferred to DOM API parameter when smooth scrolling is a property of the content being scrolled rather than the scroll action itself, OR if we want scrolling due to built-in UA scroll gestures to be smooth even when the UA/platform would default to instant scrolling (because in the latter case no DOM API is called to trigger scrolling, so there's nowhere to pass in a ScrollOptions). I can't think of an example of the former. In the latter case, almost all of our platforms make almost all built-in scroll gestures smooth in some way already, and I can't think of a good reason to override the platform defaults when they're not.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36) > (In reply to :kip (Kearwood Gilbert) from comment #33) > > This property, part of the CSSOM-View scroll-behavior specification, > > essentially allows you change the default "behavior" parameter for > > CSSOM-View scroll DOM calls. > > > > For example, you would normally write this every time you want to smoothly > > scroll the viewport: > > > > window.scrollTo(1234,5678, {behavior: "smooth"}); > > > > Instead, you could specify that smooth is the default with CSS: > > > > <body style="scroll-behavior: smooth"> > > > > Then the smooth scrolling happens transparently with: > > > > window.scrollTo(1234,5678); > > Right, I already know that :-). The question is why a CSS property would be > preferred to the DOM API in some cases. > > > The uses cases for this are the same as smooth scrolling in general: > > - Go to top buttons, go to bottom buttons, scroll section into view buttons > > These buttons presumably run scripts to do those actions via scrollTo or > some other DOM scrolling API. Given they're already calling a DOM scrolling > API, it makes sense for them to pass {behavior:"smooth"} as a parameter to > that API. > > > - Carousel style scrolling animations > > - Adjusting / overriding the destination of a fling gesture (scroll nudging) > > - Log file / chat interface automatic scrolling to new messages > > These also all sound like scripts calling DOM APIs, to me. > > It seems to me a CSS property would be preferred to DOM API parameter when > smooth scrolling is a property of the content being scrolled rather than the > scroll action itself, OR if we want scrolling due to built-in UA scroll > gestures to be smooth even when the UA/platform would default to instant > scrolling (because in the latter case no DOM API is called to trigger > scrolling, so there's nowhere to pass in a ScrollOptions). > > I can't think of an example of the former. In the latter case, almost all of > our platforms make almost all built-in scroll gestures smooth in some way > already, and I can't think of a good reason to override the platform > defaults when they're not. I can only think of one non-script-driven event that results in non-smooth scrolling -- named anchor navigation. The CSSOM-View specification, section 14.1 states: http://www.w3.org/TR/cssom-view/#smooth-scrolling:-the-%27scroll-behavior%27-property "The 'scroll-behavior' property specifies the scrolling behavior for a scrolling box, when scrolling happens due to navigation or CSSOM scrolling APIs. Scrolls that are performed by the user are not affected by this property. When this property is specified on the root element, it applies to the viewport instead." By "due to navigation", perhaps this could include named anchor links? Would you see value added by enabling smooth scrolling for these anchors in a declarative manner?

(In reply to :kip (Kearwood Gilbert) from comment #37) > Would you see value added by enabling smooth scrolling for these anchors in > a declarative manner? Yes, I think there's some value there. I considered just turning on smooth scrolling for anchor navigations across the board, but that would probably break sites. It's not a ton of value though. In most cases it would be trivial to add an onclick handler to the link which calls Element.scrollIntoView instead. How about we put this on ice until someone asks for it?

Can we land the code, and leave the pref (off) for now? A bunch of the code is actually shared with Scroll Snapping (bug 945584) We'll need to split off a new pref so we don't turn off off the DOM APIs.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38) > How about we put this on ice until someone asks for it? I'd quite like this, if my request counts. Having to add an onclick to get smooth scrolling for anchor links is a lot more awkward than just specifying some CSS.

Comment on attachment 8483864 [details] [diff] [review] Bug 1010538: Part 1 - Style support for scroll-behavior Review of attachment 8483864 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following comments addressed. Now that bug 931668 has landed, you will need to handle mScrollBehavior in nsStyleDisplay::CalcDifference by returning nsChangeHint_NeutralChange if there is a different value. You can add a check to the massive if statement at the end of that function. ::: layout/style/nsComputedDOMStyle.cpp @@ +2701,5 @@ > nsCSSProps::kOrientKTable)); > return val; > } > > + No need for this additional blank line. ::: layout/style/test/property_database.js @@ +5400,5 @@ > + inherited: false, > + type: CSS_TYPE_LONGHAND, > + initial_values: [ "instant" ], > + other_values: [ "smooth" ], > + invalid_values: [] Let's stick a couple of invalid values in there, even though we're unlikely to run into trouble with parsing. For example box-decoration-break lists [ "auto", "none", "1px" ], which seems reasonable.

Comment on attachment 8484370 [details] [diff] [review] Bug 1010538: Part 2 - Add scroll-behavior to ScrollbarStyles (V2 Patch) Review of attachment 8484370 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/ScrollbarStyles.h @@ +16,5 @@ > // Always one of NS_STYLE_OVERFLOW_SCROLL, NS_STYLE_OVERFLOW_HIDDEN, > // or NS_STYLE_OVERFLOW_AUTO. > uint8_t mHorizontal; > uint8_t mVertical; > + // Always one of NS_STYLE_SCROLL_BEHAVIOR_INSTANT or Can we call this NS_STYLE_SCROLL_BEHAVIOR_AUTO? Because AFAIK "INSTANT" actually means "platform default", which usually isn't instant! @@ +21,5 @@ > + // NS_STYLE_SCROLL_BEHAVIOR_SMOOTH > + uint8_t mScrollBehavior; > + ScrollbarStyles(uint8_t h, uint8_t v, uint8_t b) : mHorizontal(h), > + mVertical(v), > + mScrollBehavior(b) {} While you're here, fix the parameter names to be aH, aV, aB.

Comment on attachment 8483869 [details] [diff] [review] Bug 1010538: Part 3 - Implement scroll-behavior CSS property Review of attachment 8483869 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsGlobalWindow.cpp @@ +7261,5 @@ > > + ScrollbarStyles styles = sf->GetScrollbarStyles(); > + bool smoothScroll = aOptions.mBehavior == ScrollBehavior::Smooth || > + (aOptions.mBehavior == ScrollBehavior::Auto && > + styles.mScrollBehavior == NS_STYLE_SCROLL_BEHAVIOR_SMOOTH); Less indent here @@ +7315,5 @@ > // Perhaps Web content does too. > + ScrollbarStyles styles = sf->GetScrollbarStyles(); > + bool smoothScroll = aOptions.mBehavior == ScrollBehavior::Smooth || > + (aOptions.mBehavior == ScrollBehavior::Auto && > + styles.mScrollBehavior == NS_STYLE_SCROLL_BEHAVIOR_SMOOTH); And here

Comment on attachment 8486106 [details] [diff] [review] Bug 1010538: Part 4 - Tests (V2 Patch) Review of attachment 8486106 [details] [diff] [review]: ----------------------------------------------------------------- Great checking comment, thanks!

(In reply to Jet Villegas (:jet) from comment #39) > Can we land the code, and leave the pref (off) for now? A bunch of the code > is actually shared with Scroll Snapping (bug 945584) > > We'll need to split off a new pref so we don't turn off off the DOM APIs. I will update the patches such that the CSS property will utilize a separate preference. Also, please note that this CSS property has now landed in Chrome and is enabled by default. Current Chrome Canary builds are scrolling smoothly with the "scroll to top", "scroll to bottom", "scroll down 100px", and "scroll up 100px" for the example attached to this bug. Perhaps compatibility will be a driving factor.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #42) > Comment on attachment 8484370 [details] [diff] [review] > Bug 1010538: Part 2 - Add scroll-behavior to ScrollbarStyles (V2 Patch) > > Review of attachment 8484370 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/ScrollbarStyles.h > @@ +16,5 @@ > > // Always one of NS_STYLE_OVERFLOW_SCROLL, NS_STYLE_OVERFLOW_HIDDEN, > > // or NS_STYLE_OVERFLOW_AUTO. > > uint8_t mHorizontal; > > uint8_t mVertical; > > + // Always one of NS_STYLE_SCROLL_BEHAVIOR_INSTANT or > > Can we call this NS_STYLE_SCROLL_BEHAVIOR_AUTO? Because AFAIK "INSTANT" > actually means "platform default", which usually isn't instant! > Thanks for taking time for this review. I agree that this naming is a bit confusing. Are you okay with having the enumeration say NS_STYLE_SCROLL_BEHAVIOR_AUTO while the corresponding CSS property and DOM parameter say "instant", or are you suggesting that the W3 spec be changed? Please note that this has already landed in Chrome Canary with "instant" and "smooth" as the CSS values. Perhaps we should try not to introduce some confusion with the "auto" value passed to the DOM calls. When {scroll-behavior: "auto"} is passed to the CSSOM-View scrolling functions, "auto" indicates that the value of the CSS property be used rather than overridden. In this case, if the css scroll-behavior value was set to "instant", passing "auto" to the DOM call evaluates to "instant". From the DOM perspective, "instant" is consistent with the default behavior in that a scroll offset set with a DOM function is applied immediately without animation. This does not apply to scrolling performed as a direct result of user input / gestures. (Overscroll snapback, fling gestures, dragging, and keyboard scrolling all scroll smoothly by default) Perhaps there is a naming convention that more clearly differentiates these in a UA independent manner?

- Implemented boilerplate to add the scroll-behavior CSS property. V2 Patch: - Updated with review in Comment 41 - Added layout.css.scroll-behavior.property-enabled preference to allow the scroll-behavior CSS property to be enabled independently of the CSSOM-View DOM scrolling api extensions for smooth scrolling.

- Added SCROLL_SMOOTH_AUTO flag to nsIPresShell to enable selection of scroll behavior through CSS. - Updated Element and Window scrolling DOM methods to enable smooth scrolling set through the scroll-behavior CSS property. V2 Patch: - Reduced indentation as per review in Comment 43

(In reply to Cameron McCormack (:heycam) from comment #41) > Now that bug 931668 has landed, you will need to handle mScrollBehavior in > nsStyleDisplay::CalcDifference by returning nsChangeHint_NeutralChange if > there is a different value. You can add a check to the massive if statement > at the end of that function. Did you miss this bit (or maybe you just haven't rebased on top of bug 931668 yet)?

scroll-behavior-1.html - Viewport Scrolling, expected to be smooth: - DOM Method not specifying scroll-behavior, for <body> tag with 'scroll-behavior: smooth’ scroll-behavior-2.html - Div Scrolling, expected to be smooth: - DOM Method not specifying scroll-behavior, for <div> tag with 'scroll-behavior: smooth’ - DOM Method specifying ‘auto’ scroll-behavior, for <div> tag with ‘scroll-behavior: smooth’ - DOM Method specifying 'smooth’ scroll-behavior, for <div> tag with ‘scroll-behavior: smooth’ - DOM Method specifying ‘smooth’ scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method specifying ‘smooth’ scroll-behavior, for <div> tag with no scroll-behavior attribute. scroll-behavior-3.html - Div Scrolling, expected to be instant: - DOM Method not specifying scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method not specifying scroll-behavior, for <div> tag with no scroll-behavior attribute. - DOM Method specifying ‘instant’ scroll-behavior, for <div> tag with ‘scroll-behavior: smooth’ - DOM Method specifying ‘instant’ scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method specifying ‘instant’ scroll-behavior, for <div> tag with no scroll-behavior attribute. - DOM Method specifying ‘auto’ scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method specifying ‘auto’ scroll-behavior, for <div> tag with no scroll-behavior attribute. V3 Patch: - Enabling additional preference in reftest.list, layout.css.scroll-behavior.property-enabled, required for CSS scroll-behavior property to be enabled.

(In reply to :kip (Kearwood Gilbert) from comment #45) > Also, please note that this CSS property has now landed in Chrome and is > enabled by default. Current Chrome Canary builds are scrolling smoothly > with the "scroll to top", "scroll to bottom", "scroll down 100px", and > "scroll up 100px" for the example attached to this bug. This shouldn't be the case. The property is still behind a flag in Chrome. Are you sure you don't have "Experimental web platform features" enabled in Chrome? If you're seeing this enabled by default, it's a bug so please let me know. (I tested in the latest Canary and I don't see smooth scrolling with your example.)

(In reply to Cameron McCormack (:heycam) from comment #49) > (In reply to Cameron McCormack (:heycam) from comment #41) > > Now that bug 931668 has landed, you will need to handle mScrollBehavior in > > nsStyleDisplay::CalcDifference by returning nsChangeHint_NeutralChange if > > there is a different value. You can add a check to the massive if statement > > at the end of that function. > > Did you miss this bit (or maybe you just haven't rebased on top of bug > 931668 yet)? Thanks for noticing this Cameron. I missed this. I'll rebase and update the patches.

(In reply to Ali Juma [:ajuma] from comment #51) > (In reply to :kip (Kearwood Gilbert) from comment #45) > > Also, please note that this CSS property has now landed in Chrome and is > > enabled by default. Current Chrome Canary builds are scrolling smoothly > > with the "scroll to top", "scroll to bottom", "scroll down 100px", and > > "scroll up 100px" for the example attached to this bug. > > This shouldn't be the case. The property is still behind a flag in Chrome. > Are you sure you don't have "Experimental web platform features" enabled in > Chrome? If you're seeing this enabled by default, it's a bug so please let > me know. (I tested in the latest Canary and I don't see smooth scrolling > with your example.) Indeed, the "Experimental web platform features" flag was enabled. Disabling this also disabled the smooth scrolling effects in the demo file attached to this bug. Perhaps there is still opportunity to adjust the naming of attributes if it helps to clarify issues such as the one raised in Comment 42 and Comment 46.

- Implemented boilerplate to add the scroll-behavior CSS property. - Added layout.css.scroll-behavior.property-enabled preference to allow the scroll-behavior CSS property to be enabled independently of the CSSOM-View DOM scrolling api extensions for smooth scrolling. V3 Patch: - Updated nsStyleDisplay::CalcDifference to include a check for mScrollBehavior as per Comment 41.

- When an anchor link is clicked, the SCROLL_SMOOTH_AUTO flag is now set by PresShell::GoToAnchor when calling PresShell::ScrollContentIntoView. - Added an arguement, aAnimateScroll, to PresShell:GoToAnchor to indicate that the scroll may be animated. This will only be set to true when an anchor link is clicked. Opening a page with an anchor link will not trigger such animations.

(In reply to :kip (Kearwood Gilbert) from comment #46) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #42) > > Comment on attachment 8484370 [details] [diff] [review] > > Bug 1010538: Part 2 - Add scroll-behavior to ScrollbarStyles (V2 Patch) > > > > Review of attachment 8484370 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: layout/base/ScrollbarStyles.h > > @@ +16,5 @@ > > > // Always one of NS_STYLE_OVERFLOW_SCROLL, NS_STYLE_OVERFLOW_HIDDEN, > > > // or NS_STYLE_OVERFLOW_AUTO. > > > uint8_t mHorizontal; > > > uint8_t mVertical; > > > + // Always one of NS_STYLE_SCROLL_BEHAVIOR_INSTANT or > > > > Can we call this NS_STYLE_SCROLL_BEHAVIOR_AUTO? Because AFAIK "INSTANT" > > actually means "platform default", which usually isn't instant! > > > Thanks for taking time for this review. I agree that this naming is a bit > confusing. Are you okay with having the enumeration say > NS_STYLE_SCROLL_BEHAVIOR_AUTO while the corresponding CSS property and DOM > parameter say "instant", or are you suggesting that the W3 spec be changed? > Please note that this has already landed in Chrome Canary with "instant" and > "smooth" as the CSS values. > > Perhaps we should try not to introduce some confusion with the "auto" value > passed to the DOM calls. When {scroll-behavior: "auto"} is passed to the > CSSOM-View scrolling functions, "auto" indicates that the value of the CSS > property be used rather than overridden. In this case, if the css > scroll-behavior value was set to "instant", passing "auto" to the DOM call > evaluates to "instant". > > From the DOM perspective, "instant" is consistent with the default behavior > in that a scroll offset set with a DOM function is applied immediately > without animation. This does not apply to scrolling performed as a direct > result of user input / gestures. (Overscroll snapback, fling gestures, > dragging, and keyboard scrolling all scroll smoothly by default) > > Perhaps there is a naming convention that more clearly differentiates these > in a UA independent manner? Setting needinfo, looking for your feedback on naming of "auto", "instant", and "smooth".

I think the CSS property should support "instant", "smooth" and "auto". I think that gives us the following logical and consistent algorithm: First check the DOM parameter (if any): if it's "instant" or "smooth", use that. Otherwise check the CSS scroll-behavior value for the element: if it's "instant" or "smooth", use that. Otherwise use the platform/UA default. If that sounds good to you, please implement it here and propose it on www-style.

Bug 1010538: Part 1 - Style support for scroll-behavior - Implemented boilerplate to add the scroll-behavior CSS property. - Added layout.css.scroll-behavior.property-enabled preference to allow the scroll-behavior CSS property to be enabled independently of the CSSOM-View DOM scrolling api extensions for smooth scrolling. V4 Patch: - CSS scroll-behavior property can now be set to "auto", "instant", or "smooth".

- ScrollbarStyles extended to support the scroll-behavior CSS property - Corrected naming of parameters to ScrollbarStyles CTOR V3 Patch: - Unbitrotted; scroll-behavior CSS property can now be set to "auto", "instant", or "smooth"

Bug 1010538: Part 3 - Implement scroll-behavior CSS property - Added SCROLL_SMOOTH_AUTO flag to nsIPresShell to enable selection of scroll behavior through CSS. - Updated Element and Window scrolling DOM methods to enable smooth scrolling set through the scroll-behavior CSS property. V3 Patch: - scroll-behavior css property can now be set to "auto", "instant", or "smooth"

scroll-behavior-1.html - Viewport Scrolling, expected to be smooth: - DOM Method not specifying scroll-behavior, for <body> tag with 'scroll-behavior: smooth’ scroll-behavior-2.html - Div Scrolling, expected to be smooth: - DOM Method not specifying scroll-behavior, for <div> tag with 'scroll-behavior: smooth’ - DOM Method specifying ‘auto’ scroll-behavior, for <div> tag with ‘scroll-behavior: smooth’ - DOM Method specifying 'smooth’ scroll-behavior, for <div> tag with ‘scroll-behavior: smooth’ - DOM Method specifying ‘smooth’ scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method specifying ‘smooth’ scroll-behavior, for <div> tag with ‘scroll-behavior: auto’ - DOM Method specifying ‘smooth’ scroll-behavior, for <div> tag with no scroll-behavior attribute. scroll-behavior-3.html - Div Scrolling, expected to be instant: - DOM Method not specifying scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method not specifying scroll-behavior, for <div> tag with ‘scroll-behavior: auto’ - DOM Method not specifying scroll-behavior, for <div> tag with no scroll-behavior attribute. - DOM Method specifying ‘instant’ scroll-behavior, for <div> tag with ‘scroll-behavior: smooth’ - DOM Method specifying ‘instant’ scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method specifying ‘instant’ scroll-behavior, for <div> tag with ‘scroll-behavior: auto’ - DOM Method specifying ‘instant’ scroll-behavior, for <div> tag with no scroll-behavior attribute. - DOM Method specifying ‘auto’ scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method specifying ‘auto’ scroll-behavior, for <div> tag with ‘scroll-behavior: auto’ - DOM Method specifying ‘auto’ scroll-behavior, for <div> tag with no scroll-behavior attribute. V4 Patch: - Added tests for scroll-behavior CSS set to "auto"

- When an anchor link is clicked, the SCROLL_SMOOTH_AUTO flag is now set by PresShell::GoToAnchor when calling PresShell::ScrollContentIntoView. - Added an arguement, aAnimateScroll, to PresShell:GoToAnchor to indicate that the scroll may be animated. This will only be set to true when an anchor link is clicked. Opening a page with an anchor link will not trigger such animations. V2 Patch: - Unbitrotted

scroll-behavior-1.html - Viewport Scrolling, expected to be smooth: - DOM Method not specifying scroll-behavior, for <body> tag with 'scroll-behavior: smooth’ scroll-behavior-2.html - Div Scrolling, expected to be smooth: - DOM Method not specifying scroll-behavior, for <div> tag with 'scroll-behavior: smooth’ - DOM Method specifying ‘auto’ scroll-behavior, for <div> tag with ‘scroll-behavior: smooth’ - DOM Method specifying 'smooth’ scroll-behavior, for <div> tag with ‘scroll-behavior: smooth’ - DOM Method specifying ‘smooth’ scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method specifying ‘smooth’ scroll-behavior, for <div> tag with ‘scroll-behavior: auto’ - DOM Method specifying ‘smooth’ scroll-behavior, for <div> tag with no scroll-behavior attribute. scroll-behavior-3.html - Div Scrolling, expected to be instant: - DOM Method not specifying scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method not specifying scroll-behavior, for <div> tag with ‘scroll-behavior: auto’ - DOM Method not specifying scroll-behavior, for <div> tag with no scroll-behavior attribute. - DOM Method specifying ‘instant’ scroll-behavior, for <div> tag with ‘scroll-behavior: smooth’ - DOM Method specifying ‘instant’ scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method specifying ‘instant’ scroll-behavior, for <div> tag with ‘scroll-behavior: auto’ - DOM Method specifying ‘instant’ scroll-behavior, for <div> tag with no scroll-behavior attribute. - DOM Method specifying ‘auto’ scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method specifying ‘auto’ scroll-behavior, for <div> tag with ‘scroll-behavior: auto’ - DOM Method specifying ‘auto’ scroll-behavior, for <div> tag with no scroll-behavior attribute. V4 Patch: - #scroll_4 scroll-behavior css attribute corrected to match test description

scroll-behavior-4.html - Anchor Link Scrolling - Navigate to Anchor, for <div> tag with no ‘scroll-behavior’ attribute. Expected to be instantaneous. - Navigate to Anchor, for <div> tag with ‘scroll-behavior: auto’. Expected to be instantaneous. - Navigate to Anchor, for <div> tag with ‘scroll-behavior: instant’. Expected to be instantaneous. - Navigate to Anchor, for <div> tag with ‘scroll-behavior: smooth’. Expected to be smooth.

- Setting Element.ScrollLeft or Element.ScrollRight will now scroll smoothly if the scroll-behavior CSS property is set to "smooth".

scroll-behavior-4.html - Anchor Link Scrolling - Navigate to Anchor, for <div> tag with no ‘scroll-behavior’ attribute. Expected to be instantaneous. - Navigate to Anchor, for <div> tag with ‘scroll-behavior: auto’. Expected to be instantaneous. - Navigate to Anchor, for <div> tag with ‘scroll-behavior: instant’. Expected to be instantaneous. - Navigate to Anchor, for <div> tag with ‘scroll-behavior: smooth’. Expected to be smooth. V3 Patch: - Corrected broken tests. They were always returning success.

scroll-behavior-5.html - Element.ScrollLeft and Element.ScrollTop scroll-behavior tests - Set Element.ScrollLeft and Element.ScrollTop, for <div> tag with no 'scroll-behavior' attribute. Expected to be instantaneous. - Set Element.ScrollLeft and Element.ScrollTop, for <div> tag with 'scroll-behavior: auto'. Expected to be instantaneous. - Set Element.ScrollLeft and Element.ScrollTop, for <div> tag with 'scroll-behavior: instant'. Expected to be instantaneous. - Set Element.ScrollLeft and Element.ScrollTop, for <div> tag with 'scroll-behavior: smooth'. Expected to be smooth.

All of the issues and feedback from reviews that I am aware of has been captured in the updated patches. I have pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=e08958937ad2 Once (if) this passes, then I will request reviews for the modified patches.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #57) > I think the CSS property should support "instant", "smooth" and "auto". I > think that gives us the following logical and consistent algorithm: > > First check the DOM parameter (if any): if it's "instant" or "smooth", use > that. > Otherwise check the CSS scroll-behavior value for the element: if it's > "instant" or "smooth", use that. > Otherwise use the platform/UA default. > > If that sounds good to you, please implement it here and propose it on > www-style. I have updated the patches to include "auto" for the CSS scroll-behavior value. I will follow up on www-style as well. Currently the "auto" and "instant" CSS scroll-behavior values have the same effect. Is there anything that we currently do smoothly by default that should be done without animation when CSS scroll-behavior is set to "instant"? Perhaps the "instant" CSS scroll-behavior value could be used to disable the smooth scrolling motion that nudges into the snap points for scroll snapping (Bug 945584)? (Might be of dubious value as IMHO, as the primary value added by declarative scroll snapping is to provide smoother animations)

(In reply to :kip (Kearwood Gilbert) from comment #69) > Currently the "auto" and "instant" CSS scroll-behavior values have the same > effect. > > Is there anything that we currently do smoothly by default that should be > done without animation when CSS scroll-behavior is set to "instant"? Sure. Keyboard page-up/page-down, arrow-up/arrow-down for example. Plus similar scrollbar actions. > Perhaps the "instant" CSS scroll-behavior value could be used to disable the > smooth scrolling motion that nudges into the snap points for scroll snapping > (Bug 945584)? (Might be of dubious value as IMHO, as the primary value > added by declarative scroll snapping is to provide smoother animations) I don't have a strong feeling but I lean toward making snapping instant as well.

- Added SCROLL_SMOOTH_AUTO flag to nsIPresShell to enable selection of scroll behavior through CSS. - Updated Element and Window scrolling DOM methods to enable smooth scrolling set through the scroll-behavior CSS property. V4 Patch: - Keyboard, scroll bar, mousewheel, and any other events that scroll smoothly with the general.smoothScroll preference enabled will now scroll instantly if the scroll frame has "scroll-behavior: instant" applied through CSS.

Comment on attachment 8495450 [details] [diff] [review] Bug 1010538: Part 1 - Style support for scroll-behavior (v4 Patch) Cam reviewed the earlier revision of this patch. It has been updated to support scroll-behavior CSS value of "auto".

Comment on attachment 8497127 [details] [diff] [review] Bug 1010538: Part 3 - Implement scroll-behavior CSS property (v4 Patch) You have previously reviewed this patch -- It has been updated to support "auto" values for the scroll-behavior CSS property.

Comment on attachment 8495524 [details] [diff] [review] Bug 1010538: Part 4 - Tests (v5 Patch) You have previously reviewed this patch -- It has been updated to include tests for scroll-behavior CSS set to "auto"

Comment on attachment 8495458 [details] [diff] [review] Bug 1010538: Part 5 - Enable automatic smooth scrolling for anchor links (v2 patch) Cam is away.. Please advise if someone else should help with these reviews.

I have filed a bug on w3.org requesting addition of the "auto" value for the scroll-behavior CSS property in the CSSOM-View specification: https://www.w3.org/Bugs/Public/show_bug.cgi?id=26941

The conversation within the w3.org bug has expanded into a new thread on www-style: http://lists.w3.org/Archives/Public/www-style/2014Oct/0014.html I have summarized the w3.org bug commentary: The CSSOM-View specification, section 14.1 (14.1 Smooth Scrolling: The 'scroll-behavior' Property), describes the 'scroll-behavior' CSS property as allowing two values, 'instant' and 'smooth': http://dev.w3.org/csswg/cssom-view/#smooth-scrolling:-the-%27scroll-behavior%27-property The default value, 'instant', may be misleading as many scrolling actions are animated smoothly by default in our implementation (and others). I propose that 'auto' be an allowed value, and be the default. The value of 'smooth' would remain as defined in the CSSOM-View specification; however, there would be some distinction between 'auto' and 'instant'. 'auto' may result in a mixture of smooth and instant scrolling animations, defined by existing UA behavior. 'instant' will result in instant scrolling for all events. If a UA does not scroll smoothly by default for any events, 'auto' and 'instant' can be synonymous in their implementation. In our implementation, smooth scrolling occurs by default with keyboard scrolling, with scroll-bar interaction, and to align the scroll offset to snap points for CSS scroll snapping. (http://dev.w3.org/csswg/css-snappoints/) Section 14.1 (Smooth Scrolling: The 'scroll-behavior' Property) states: "The 'scroll-behavior' property specifies the scrolling behavior for a scrolling box, when scrolling happens due to navigation or CSSOM scrolling APIs. Scrolls that are performed by the user are not affected by this property." This seems to exclude smooth scrolling due to keyboard arrow/page/home/end buttons, mouse wheels, and scroll bar events which would continue to scroll smoothly even with 'scroll-behavior: instant' set. When combined with CSS scroll snapping, the destination of these smooth scroll animations is altered to ensure that the destination is a valid snap point. This is grey area in terms of being "performed by the user"; if interpreted differently by each UA implementation, would result in inconsistent results. It is desirable in some circumstances for content to disable all smooth scrolling, including scrolling that is not due to navigation or CSSOM scrolling APIs. One use case would be a spreadsheet application. In this case, it might be desirable to snap to a cell boundary without the animation. To enable content authors to explicitly disable all smooth scrolling animation (including the scroll bar smooth scrolling and scroll snapping animations), would the scroll-behavior CSS property be the ideal interface for this?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (on vacation Sep 27 to Oct 12) from comment #70) > (In reply to :kip (Kearwood Gilbert) from comment #69) > > Currently the "auto" and "instant" CSS scroll-behavior values have the same > > effect. > > > > Is there anything that we currently do smoothly by default that should be > > done without animation when CSS scroll-behavior is set to "instant"? > > Sure. Keyboard page-up/page-down, arrow-up/arrow-down for example. Plus > similar scrollbar actions. > > > Perhaps the "instant" CSS scroll-behavior value could be used to disable the > > smooth scrolling motion that nudges into the snap points for scroll snapping > > (Bug 945584)? (Might be of dubious value as IMHO, as the primary value > > added by declarative scroll snapping is to provide smoother animations) > > I don't have a strong feeling but I lean toward making snapping instant as > well. Would you suggest that flings be disabled as well? Also, can you think of some use cases? For the instant scroll snapping, spreadsheets seem to benefit; however, I am having difficulty finding examples beyond that case.

(In reply to :kip (Kearwood Gilbert) from comment #78) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (on vacation Sep > 27 to Oct 12) from comment #70) > > (In reply to :kip (Kearwood Gilbert) from comment #69) > > > Currently the "auto" and "instant" CSS scroll-behavior values have the same > > > effect. > > > > > > Is there anything that we currently do smoothly by default that should be > > > done without animation when CSS scroll-behavior is set to "instant"? > > > > Sure. Keyboard page-up/page-down, arrow-up/arrow-down for example. Plus > > similar scrollbar actions. > > > > > Perhaps the "instant" CSS scroll-behavior value could be used to disable the > > > smooth scrolling motion that nudges into the snap points for scroll snapping > > > (Bug 945584)? (Might be of dubious value as IMHO, as the primary value > > > added by declarative scroll snapping is to provide smoother animations) > > > > I don't have a strong feeling but I lean toward making snapping instant as > > well. > Would you suggest that flings be disabled as well? Also, can you think of > some use cases? For the instant scroll snapping, spreadsheets seem to > benefit; however, I am having difficulty finding examples beyond that case. On the www-style thread, I have posed some use cases for disabling smooth scroll when scroll snapping is not in effect; however, I tend to agree with the response I am receiving there -- in most cases this decision should be decided by the UA (or via a preference), with the exception of scroll snapping animation. Allowing content to disable fling gesture and keyboard smooth scrolling will result in a negative experience for the user. I suggest this behavior: scroll-behavior set to 'auto': - Default behavior. - CSSOM-View DOM methods scroll instantly by default - Anchor navigation occurs instantly - Scroll Snapping is performed smoothly - Scrollbar, fling gestures, keyboard events animate smoothly. scroll-behavior set to 'instant': - Same as 'auto', above except that scroll snapping is performed instantly. scroll-behavior set to 'smooth': - CSSOM-View DOM methods scroll smoothly by default - Anchor navigation animates smoothly - Scroll Snapping is performed smoothly - Scrollbar, fling gestures, keyboard events animate smoothly. Does this seem reasonable?

Would it be possible to review and land this while the decision is being made on what behavior becomes instant with the "instant" value? (With the layout.css.scroll-behavior.property-enabled preference turned off) These patches are blocking Bug 1045754 which was expected to land for FX34.

Please see Comment 80 - I would like to know if you are too busy to review these or if we are delaying due to decisions on the scroll-behavior "instant" effects on keyboard and fling gestures. Please advise if this can be reviewed and landed in time for FX34. Bug 1045754 has been r+'ed but is blocking on this as well.

Kip: roc's status undicates his review availability (on vacation Sep 27 to Oct 12.) Let's find another reviewer. tn?

Comment on attachment 8497127 [details] [diff] [review] Bug 1010538: Part 3 - Implement scroll-behavior CSS property (v4 Patch) Review of attachment 8497127 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/Element.cpp @@ +609,5 @@ > > uint32_t flags = nsIPresShell::SCROLL_OVERFLOW_HIDDEN; > if (aOptions.mBehavior == ScrollBehavior::Smooth) { > flags |= nsIPresShell::SCROLL_SMOOTH; > + } else if(aOptions.mBehavior == ScrollBehavior::Auto) { nit: space before '(' ::: dom/base/nsGlobalWindow.cpp @@ +7197,5 @@ > > + ScrollbarStyles styles = sf->GetScrollbarStyles(); > + bool smoothScroll = aOptions.mBehavior == ScrollBehavior::Smooth || > + (aOptions.mBehavior == ScrollBehavior::Auto && > + styles.mScrollBehavior == NS_STYLE_SCROLL_BEHAVIOR_SMOOTH); I feel like this condition should be refactored into a helper method, perhaps on the options object or on the ScrollbarStyles object. ::: layout/base/nsPresShell.cpp @@ +3535,5 @@ > + if (gfxPrefs::ScrollBehaviorEnabled() > + && (aFlags & nsIPresShell::SCROLL_SMOOTH > + || (aFlags & nsIPresShell::SCROLL_SMOOTH_AUTO > + && aFrameAsScrollable->GetScrollbarStyles().mScrollBehavior > + == NS_STYLE_SCROLL_BEHAVIOR_SMOOTH))) { This would be more readable with a few local variables: bool autoBehaviourIsSmooth = (aFrameAsScrollable->GetScrollableStyles().mScrollBehavior == NS_STYLE_SCROLL_BEHAVIOR_SMOOTH); bool smoothScroll = (aFlags & nsIPresShell::SCROLL_SMOOTH) || ((aFlags & nsIPresShell::SMOOTH_SCROLL_AUTO) && autoBehaviorIsSmooth); if (gfxPrefs::ScrollBehaviorEnabled() && smoothScroll) { ... }

Comment on attachment 8495524 [details] [diff] [review] Bug 1010538: Part 4 - Tests (v5 Patch) Review of attachment 8495524 [details] [diff] [review]: ----------------------------------------------------------------- I didn't really look at scroll-behavior-3 ::: layout/reftests/scrolling/scroll-behavior-1.html @@ +40,5 @@ > +<body> > + <div id="a_box"></div> > + <div id="another_box"></div> > +<script> > + function doTest() { A brief comment explaining the test would be nice. From my reading of it, without ?ref it just scrolls to (0,0) instantly (which seems redundant because that's where the page will start off). With ?ref it will do a smooth-scroll to (500,500) and then immediately do another smooth scroll back to (0,0) before the first smooth scroll has a chance to do anything. So both cases should end up at 0,0. Might also be good rename "a_box" to "coloured_block" and "another_box" to "make_page_scrollable" so their purposes are clearer. ::: layout/reftests/scrolling/scroll-behavior-2.html @@ +89,5 @@ > + // Interrupt any smooth scrolling > + for (var i=1; i <= 6; i++) { > + document.getElementById("scroll_" + i).scrollLeft > + = document.getElementById("scroll_" + i).scrollLeft; > + document.getElementById("scroll_" + i).scrollTop Not clear to me how this test works just by looking at the code. I would expect some of the scrollboxes to do smooth scrolling and therefore get interrupted before they get to their destination. However the non-'?ref' codepath should end up with all boxes visible. Maybe I'm missing something.

> ::: layout/reftests/scrolling/scroll-behavior-1.html > @@ +40,5 @@ > > +<body> > > + <div id="a_box"></div> > > + <div id="another_box"></div> > > +<script> > > + function doTest() { > > A brief comment explaining the test would be nice. From my reading of it, > without ?ref it just scrolls to (0,0) instantly (which seems redundant > because that's where the page will start off). With ?ref it will do a > smooth-scroll to (500,500) and then immediately do another smooth scroll > back to (0,0) before the first smooth scroll has a chance to do anything. So > both cases should end up at 0,0. > > Might also be good rename "a_box" to "coloured_block" and "another_box" to > "make_page_scrollable" so their purposes are clearer. > I'll update this with some comments... The initial scroll to 0,0 is a sanity check, which is not expected to have an effect if everything is working as expected. (Perhaps a previous test could have a side-effect resulting in the initial scroll position not being 0,0) The test expects there to be a smooth scroll from (0,0) to (500,500). The test does not wait for this scroll to complete. We can infer that the scroll was performed smoothly at the end of the test if the position was not immediately updated (smooth scrolls are asynchronous). If the scroll was not performed smoothly, the scroll position would have been instantly updated to (500, 500).

> ::: layout/reftests/scrolling/scroll-behavior-2.html > @@ +89,5 @@ > > + // Interrupt any smooth scrolling > > + for (var i=1; i <= 6; i++) { > > + document.getElementById("scroll_" + i).scrollLeft > > + = document.getElementById("scroll_" + i).scrollLeft; > > + document.getElementById("scroll_" + i).scrollTop > > Not clear to me how this test works just by looking at the code. I would > expect some of the scrollboxes to do smooth scrolling and therefore get > interrupted before they get to their destination. However the non-'?ref' > codepath should end up with all boxes visible. Maybe I'm missing something. Thanks again for the review! For the scroll-behavior-2 test, all of the "a" boxes should be visible and none of the the "b" boxes should be visible, indicating that all of the scrolls were performed smoothly (inferred by the asynchronous and remaining at their starting positions when interrupted). This test may look a little odd due to its use of Element.ScrollIntoView. The smooth scrolling aware version of Element.ScrollTo, Element.ScrollBy, Element.ScrollLeft, and Element.ScrollTop are added by Bug 1045754 which is dependent on this bug landing first. Bug 1045754 updates the tests in this bug to include tests for these other methods.

(In reply to Jet Villegas (:jet) from comment #82) > Kip: roc's status undicates his review availability (on vacation Sep 27 to > Oct 12.) Let's find another reviewer. tn? I don't think I'm going to be able to bring myself up to speed on the relevant issues here in order to grant a meaningful review in a day. Sorry.

During our UI+GFX standup meeting today, it was suggested that you might be able to help with this review. Would you have a few extra cycled, mcav?

I think you may have the wrong guy. :) I don't have any expertise in gecko reviews.

(In reply to Marcus Cavanaugh [:mcav] <mcav@mozilla.com> from comment #89) > I think you may have the wrong guy. :) I don't have any expertise in gecko > reviews. Indeed, Sorry. This message was intended for mstange... During our UI+GFX standup meeting today, it was suggested that you might be able to help with this review. Would you have a few extra cycles, mstange?

During our UI+GFX standup meeting today, it was suggested that you might be able to help with this review. Would you have a few extra cycles, mstange?

- Added SCROLL_SMOOTH_AUTO flag to nsIPresShell to enable selection of scroll behavior through CSS. - Updated Element and Window scrolling DOM methods to enable smooth scrolling set through the scroll-behavior CSS property. - Keyboard, scroll bar, mousewheel, and any other events that scroll smoothly with the general.smoothScroll preference enabled will now scroll instantly if the scroll frame has "scroll-behavior: instant" applied through CSS. V5 Patch: - Updated with feedback from Kats in Comment 83

I can take a look on Monday, but by then roc will already be back. How urgent is this?

We'd like this in 35, which is going to Aurora on Monday/Tuesday. If we get it within a day (Kip, is this green on try?), and are ready to check in on Tuesday, I think the uplift approval won't be a problem. Markus, if you can take a look, that'd be great - roc is back on Monday, but I'm pretty sure his queue is going to be rather large.

I have pushed to try again, with the latest version of these patches: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=149376e2100f

Comment on attachment 8495450 [details] [diff] [review] Bug 1010538: Part 1 - Style support for scroll-behavior (v4 Patch) Review of attachment 8495450 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me. The changes relative to attachment 8483864 [details] [diff] [review] (which was r+ed by heycam) are really minor.

Comment on attachment 8495451 [details] [diff] [review] Bug 1010538: Part 2 - Add scroll-behavior to ScrollbarStyles (V3 Patch), r=mstange Review of attachment 8495451 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. The html/body interaction is kinda yucky, but it's up to the spec to decide how to handle it. ::: layout/base/nsCSSFrameConstructor.cpp @@ +2311,1 @@ > return false; So if I have a page with "body { overflow: hidden; }", this means that if some other stylesheet on this page sets "html { scroll-behavior: smooth; }", the body overflow rule will be ignored, right? Has this been brought up in the standards discussion? Is there a test for this?

Comment on attachment 8495458 [details] [diff] [review] Bug 1010538: Part 5 - Enable automatic smooth scrolling for anchor links (v2 patch) Review of attachment 8495458 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsIPresShell.h @@ +595,3 @@ > */ > + virtual nsresult GoToAnchor(const nsAString& aAnchorName, bool aScroll, > + bool aAnimateScroll = false) = 0; Can you make the new argument a uint32_t aAdditionalScrollFlags = 0 and have the relevant callers pass nsIPresShell::SCROLL_SMOOTH_AUTO instead of true? Boolean arguments are hard to read when you only see the caller and don't want to look up the method definition.

Comment on attachment 8495573 [details] [diff] [review] Bug 1010538: Part 7 - Add support to Element.ScrollLeft and Element.ScrollTop, r=mstange Review of attachment 8495573 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, and this code gets replaced by an r+ed patch in bug 1010538 anyway.

Comment on attachment 8503508 [details] [diff] [review] Bug 1010538: Part 3 - Implement scroll-behavior CSS property (v5 Patch) Review of attachment 8503508 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsPresShell.cpp @@ +3531,5 @@ > nsIScrollableFrame::ScrollMode scrollMode = nsIScrollableFrame::INSTANT; > + bool autoBehaviorIsSmooth = (aFrameAsScrollable->GetScrollbarStyles().mScrollBehavior > + == NS_STYLE_SCROLL_BEHAVIOR_SMOOTH); > + bool smoothScroll = (aFlags & nsIPresShell::SCROLL_SMOOTH) > + || ((aFlags & nsIPresShell::SCROLL_SMOOTH_AUTO) && autoBehaviorIsSmooth); || goes on the end of the preceding line ::: layout/generic/nsGfxScrollFrame.cpp @@ +1786,4 @@ > { > + if (!Preferences::GetBool(SMOOTH_SCROLL_PREF_NAME, false)) { > + return false; > + } else if (gfxPrefs::ScrollBehaviorEnabled()) { No else after return please.

Comment on attachment 8495524 [details] [diff] [review] Bug 1010538: Part 4 - Tests (v5 Patch) Review of attachment 8495524 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/reftests/scrolling/scroll-behavior-3.html @@ +109,5 @@ > + document.getElementById("box8b").scrollIntoView(false, {behavior: "auto"}); > + document.getElementById("box9b").scrollIntoView(false, {behavior: "auto"}); > + document.getElementById("box10b").scrollIntoView(false, {behavior: "auto"}); > + > + // Scroll_8 is a control, expected to scroll smoothly Scroll_11 @@ +119,5 @@ > + = document.getElementById("scroll_" + i).scrollLeft; > + document.getElementById("scroll_" + i).scrollTop > + = document.getElementById("scroll_" + i).scrollTop; > + } > + } else { Please add a comment here, like "Scroll all boxes except box 11"

While testing the tryserver build I noticed that dynamic changes to the scroll-behavior property on html/body after pageload don't have an effect on whether anchor scrolling is smooth. Since the property isn't enabled by default this doesn't need to be fixed before these patches are landed, but we'll at least need to file a bug about it.

(In reply to Markus Stange [:mstange] from comment #102) > While testing the tryserver build I noticed that dynamic changes to the > scroll-behavior property on html/body after pageload don't have an effect on > whether anchor scrolling is smooth. Since the property isn't enabled by > default this doesn't need to be fixed before these patches are landed, but > we'll at least need to file a bug about it. Added as Bug 1082098

There is an intermittent test failure in the scroll-behavior-1.html and scroll-behavior-3.html reftests on B2G ICS OPT. I am reviewing them now.

- Implemented boilerplate to add the scroll-behavior CSS property. - Added layout.css.scroll-behavior.property-enabled preference to allow the scroll-behavior CSS property to be enabled independently of the CSSOM-View DOM scrolling api extensions for smooth scrolling. V5 Patch: un-bitrotted

(In reply to Markus Stange [:mstange] from comment #97) > Comment on attachment 8495451 [details] [diff] [review] > Bug 1010538: Part 2 - Add scroll-behavior to ScrollbarStyles (V3 Patch), > r=mstange > > Review of attachment 8495451 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good to me. The html/body interaction is kinda yucky, but it's up to > the spec to decide how to handle it. > > ::: layout/base/nsCSSFrameConstructor.cpp > @@ +2311,1 @@ > > return false; > > So if I have a page with "body { overflow: hidden; }", this means that if > some other stylesheet on this page sets "html { scroll-behavior: smooth; }", > the body overflow rule will be ignored, right? Has this been brought up in > the standards discussion? Is there a test for this? I'll follow up with a bug to track this. Perhaps it just needs to be documented and appropriately tested; otherwise, it will need to be changed before the pref is enabled.

- Implemented boilerplate to add the scroll-behavior CSS property. - Added layout.css.scroll-behavior.property-enabled preference to allow the scroll-behavior CSS property to be enabled independently of the CSSOM-View DOM scrolling api extensions for smooth scrolling. v6 Patch: - Updated formatting with review feedback in Comment 100

(In reply to :kip (Kearwood Gilbert) from comment #104) > There is an intermittent test failure in the scroll-behavior-1.html and > scroll-behavior-3.html reftests on B2G ICS OPT. I am reviewing them now. I was able to reproduce these failures locally and tracked down the issue to the excessive scroll sizes. I have updated the tests to use 2000x2000 rather than 10000x10000 scrolling divs, with more consistent results. I have pushed the updated patchset to try to re-run the reftests on b2g: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=43650d01d4cb

scroll-behavior-1.html - Viewport Scrolling, expected to be smooth: - DOM Method not specifying scroll-behavior, for <body> tag with 'scroll-behavior: smooth’ scroll-behavior-2.html - Div Scrolling, expected to be smooth: - DOM Method not specifying scroll-behavior, for <div> tag with 'scroll-behavior: smooth’ - DOM Method specifying ‘auto’ scroll-behavior, for <div> tag with ‘scroll-behavior: smooth’ - DOM Method specifying 'smooth’ scroll-behavior, for <div> tag with ‘scroll-behavior: smooth’ - DOM Method specifying ‘smooth’ scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method specifying ‘smooth’ scroll-behavior, for <div> tag with ‘scroll-behavior: auto’ - DOM Method specifying ‘smooth’ scroll-behavior, for <div> tag with no scroll-behavior attribute. scroll-behavior-3.html - Div Scrolling, expected to be instant: - DOM Method not specifying scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method not specifying scroll-behavior, for <div> tag with ‘scroll-behavior: auto’ - DOM Method not specifying scroll-behavior, for <div> tag with no scroll-behavior attribute. - DOM Method specifying ‘instant’ scroll-behavior, for <div> tag with ‘scroll-behavior: smooth’ - DOM Method specifying ‘instant’ scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method specifying ‘instant’ scroll-behavior, for <div> tag with ‘scroll-behavior: auto’ - DOM Method specifying ‘instant’ scroll-behavior, for <div> tag with no scroll-behavior attribute. - DOM Method specifying ‘auto’ scroll-behavior, for <div> tag with ‘scroll-behavior: instant’ - DOM Method specifying ‘auto’ scroll-behavior, for <div> tag with ‘scroll-behavior: auto’ - DOM Method specifying ‘auto’ scroll-behavior, for <div> tag with no scroll-behavior attribute. V6 Patch: - Updated with review in Comment 101 - Reduced scrolling size from 10000x10000 to 2000x2000 as per Comment 108

- When an anchor link is clicked, the SCROLL_SMOOTH_AUTO flag is now set by PresShell::GoToAnchor when calling PresShell::ScrollContentIntoView. - Added an arguement, aAnimateScroll, to PresShell:GoToAnchor to indicate that the scroll may be animated. This will only be set to true when an anchor link is clicked. Opening a page with an anchor link will not trigger such animations. V3 Patch: - Updated with review in Comment 98

scroll-behavior-4.html - Anchor Link Scrolling - Navigate to Anchor, for <div> tag with no ‘scroll-behavior’ attribute. Expected to be instantaneous. - Navigate to Anchor, for <div> tag with ‘scroll-behavior: auto’. Expected to be instantaneous. - Navigate to Anchor, for <div> tag with ‘scroll-behavior: instant’. Expected to be instantaneous. - Navigate to Anchor, for <div> tag with ‘scroll-behavior: smooth’. Expected to be smooth. V4 Patch: - un-bitrotted - Removed extraneous reftest file - Reduced scrolling size from 10000x10000 to 2000x2000 as per Comment 108

scroll-behavior-5.html - Element.ScrollLeft and Element.ScrollTop scroll-behavior tests - Set Element.ScrollLeft and Element.ScrollTop, for <div> tag with no 'scroll-behavior' attribute. Expected to be instantaneous. - Set Element.ScrollLeft and Element.ScrollTop, for <div> tag with 'scroll-behavior: auto'. Expected to be instantaneous. - Set Element.ScrollLeft and Element.ScrollTop, for <div> tag with 'scroll-behavior: instant'. Expected to be instantaneous. - Set Element.ScrollLeft and Element.ScrollTop, for <div> tag with 'scroll-behavior: smooth'. Expected to be smooth. V2 Patch: - Un-bitrotted - Reduced scrolling size from 10000x10000 to 2000x2000 as per Comment 108

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e287db609ed https://hg.mozilla.org/integration/mozilla-inbound/rev/968aa79b1200 https://hg.mozilla.org/integration/mozilla-inbound/rev/5e6ced1dbcce https://hg.mozilla.org/integration/mozilla-inbound/rev/92cd29a4e132 https://hg.mozilla.org/integration/mozilla-inbound/rev/1c3fcc7507fc https://hg.mozilla.org/integration/mozilla-inbound/rev/d3d806ad2b26 https://hg.mozilla.org/integration/mozilla-inbound/rev/61ef66e30c40 https://hg.mozilla.org/integration/mozilla-inbound/rev/4fef557e79e0

https://hg.mozilla.org/mozilla-central/rev/6e287db609ed https://hg.mozilla.org/mozilla-central/rev/968aa79b1200 https://hg.mozilla.org/mozilla-central/rev/5e6ced1dbcce https://hg.mozilla.org/mozilla-central/rev/92cd29a4e132 https://hg.mozilla.org/mozilla-central/rev/1c3fcc7507fc https://hg.mozilla.org/mozilla-central/rev/d3d806ad2b26 https://hg.mozilla.org/mozilla-central/rev/61ef66e30c40 https://hg.mozilla.org/mozilla-central/rev/4fef557e79e0

Almost...need to pref it on.

Now that bug 1087562 and bug 1087559 are fixed. We're done here.

We have triaged this bug wrt doc. It has been added to our backlog. Page to create: https://developer.mozilla.org/en-US/docs/Web/CSS/scroll-behavior Doc about how to doc a new CSS property: https://developer.mozilla.org/en-US/docs/MDN/Contribute/Howto/Document_a_CSS_property Feel free to help us writing it :-)

(In reply to Jean-Yves Perrier [:teoli] from comment #117) > We have triaged this bug wrt doc. It has been added to our backlog. > > Page to create: > https://developer.mozilla.org/en-US/docs/Web/CSS/scroll-behavior Created the documentation. Happy New Year! Sebastian

We also added a note here: https://developer.mozilla.org/en-US/Firefox/Releases/36#CSS