In order to enable the smooth scrolling behavior described in Bug 1010538 for scripted scroll position updates, the ScrollOptions parameter must be implemented for DOM interfaces: Window interface: void scroll(double x, double y, optional ScrollOptions options); void scrollTo(double x, double y, optional ScrollOptions options); void scrollBy(double x, double y, optional ScrollOptions options); Element interface: void scrollIntoView(boolean top, optional ScrollOptions options); attribute (double or ScrollOptionsVertical) scrollTop; attribute (double or ScrollOptionsHorizontal) scrollLeft; More details are included in Bug 1010538

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

During implementation, I noticed that the Mozilla extensions, window.scrollByLines and window.scrollByPages, share much of the same implementation. Would it be useful to add the ScrollOptions parameters for smooth scrolling to those as well?

(In reply to :kip (Kearwood Gilbert) from comment #2) > During implementation, I noticed that the Mozilla extensions, > window.scrollByLines and window.scrollByPages, share much of the same > implementation. > > Would it be useful to add the ScrollOptions parameters for smooth scrolling > to those as well? If Gecko's internal lines/page scroll (e.g. in response to mouse wheel events or PgDown) use these extensions (or derivatives) internally, then I think it would be useful that either one of these happen: 1. They're converted to internally use scroll/scrollTo/scrollBy with ScrollOptions. or 2. scrollByLines and scrollByPages will use the ScrollOptions as comment 2 suggests. The reason being that the implementation of this feature might end up as based on APZ (async pan zoom) for smoother animation, and this could benefit automatically our page/lines scrolls in response to user inputs.

(In reply to Avi Halachmi (:avih) from comment #3) > (In reply to :kip (Kearwood Gilbert) from comment #2) > > During implementation, I noticed that the Mozilla extensions, > > window.scrollByLines and window.scrollByPages, share much of the same > > implementation. > > > > Would it be useful to add the ScrollOptions parameters for smooth scrolling > > to those as well? > > If Gecko's internal lines/page scroll (e.g. in response to mouse wheel > events or PgDown) use these extensions (or derivatives) internally, then I > think it would be useful that either one of these happen: > > 1. They're converted to internally use scroll/scrollTo/scrollBy with > ScrollOptions. > > or > > 2. scrollByLines and scrollByPages will use the ScrollOptions as comment 2 > suggests. > > The reason being that the implementation of this feature might end up as > based on APZ (async pan zoom) for smoother animation, and this could benefit > automatically our page/lines scrolls in response to user inputs. Internally, these functions are based on scrollTo. It is just a matter of passing ScrollOptions through.

(In reply to :kip (Kearwood Gilbert) from comment #2) > During implementation, I noticed that the Mozilla extensions, > window.scrollByLines and window.scrollByPages, share much of the same > implementation. Are they exposed to web content? They shouldn't, IMHO, and if they are, they should be removed (at least for content) or specced.

> Are they exposed to web content? Yes, they are. And they have been since at least 2002. Trying to remove or standardize those is a good idea, but shouldn't block this bug.

It appears that our current WebIDL implementation does not support unions that contain dictionaries, as requested in the CSSOM spec for the Element interface changes: attribute (double or ScrollOptionsVertical) scrollTop; attribute (double or ScrollOptionsHorizontal) scrollLeft; Perhaps a work-around is possible; otherwise, I will check with #developers and see if it will be necessary to add support for such unions and/or suggest a different interface.

Demonstration using scroll behavior to smoothly scroll to the top and bottom of a page

Bug 1022818 - Experimental implementation of CSSOM-View smooth scorlling DOM Methods (In Progress) - Experimental implementation of CSSOM-View scroll behavior DOM methods and main-thread implementation of smooth scrolling. - Element scrollTop and scrollLeft attribute interface has not yet been updated, due to limitations of WebIDL parser / code generator. - Smooth scrolling parameters are exposed as preferences to enable UX tuning. - Before requesting a review, this patch will be split up into several stages.

(In reply to :kip (Kearwood Gilbert) from comment #7) > It appears that our current WebIDL implementation does not support unions > that contain dictionaries, as requested in the CSSOM spec for the Element > interface changes: > > attribute (double or ScrollOptionsVertical) scrollTop; > attribute (double or ScrollOptionsHorizontal) scrollLeft; > > Perhaps a work-around is possible; otherwise, I will check with #developers > and see if it will be necessary to add support for such unions and/or > suggest a different interface. Did you do that already? If not, you can ping :bz as he probably knows the WebIDL stuff the best. If there's a bug for implementing it, please add it to the dep tree.

Bug 1022818 - Experimental implementation of CSSOM-View smooth scrolling DOM Methods (In Progress) - Experimental implementation of CSSOM-View scroll behavior DOM methods and main-thread implementation of smooth scrolling. - Element scrollTop and scrollLeft attribute interface has not yet been updated, due to limitations of WebIDL parser / code generator. - Smooth scrolling parameters are exposed as preferences to enable UX tuning. - Before requesting a review, this patch will be split up into several stages. - Updated the default scrolling parameters

Bug 1022818 - Experimental implementation of CSSOM-View smooth scorlling DOM Methods (In Progress) - Experimental implementation of CSSOM-View scroll behavior DOM methods and main-thread implementation of smooth scrolling. - Element scrollTop and scrollLeft attribute interface has not yet been updated, due to limitations of WebIDL parser / code generator. - Smooth scrolling parameters are exposed as preferences to enable UX tuning. - Before requesting a review, this patch will be split up into several stages. - Increased default value for layout.css.scroll-behavior.spring-constant

(In reply to Florian Bender from comment #10) > (In reply to :kip (Kearwood Gilbert) from comment #7) > > It appears that our current WebIDL implementation does not support unions > > that contain dictionaries, as requested in the CSSOM spec for the Element > > interface changes: > > > > attribute (double or ScrollOptionsVertical) scrollTop; > > attribute (double or ScrollOptionsHorizontal) scrollLeft; > > > > Perhaps a work-around is possible; otherwise, I will check with #developers > > and see if it will be necessary to add support for such unions and/or > > suggest a different interface. > > Did you do that already? If not, you can ping :bz as he probably knows the > WebIDL stuff the best. If there's a bug for implementing it, please add it > to the dep tree. I have found an existing bug, which is marked as fixed: Bug 918011 - Support dictionaries inside unions A comment on the bug states, "This will work as long as the dictionaries inside the union are in separate webidl files." I will attempt that and see if it will work for the CSSOM-View Element.scrollTop and Element.scrollLeft attributes.

(In reply to :kip (Kearwood Gilbert) from comment #13) > (In reply to Florian Bender from comment #10) > > (In reply to :kip (Kearwood Gilbert) from comment #7) > > > It appears that our current WebIDL implementation does not support unions > > > that contain dictionaries, as requested in the CSSOM spec for the Element > > > interface changes: > > > > > > attribute (double or ScrollOptionsVertical) scrollTop; > > > attribute (double or ScrollOptionsHorizontal) scrollLeft; > > > > > > Perhaps a work-around is possible; otherwise, I will check with #developers > > > and see if it will be necessary to add support for such unions and/or > > > suggest a different interface. > > > > Did you do that already? If not, you can ping :bz as he probably knows the > > WebIDL stuff the best. If there's a bug for implementing it, please add it > > to the dep tree. > > I have found an existing bug, which is marked as fixed: > > Bug 918011 - Support dictionaries inside unions > > A comment on the bug states, "This will work as long as the dictionaries > inside the union are in separate webidl files." > > I will attempt that and see if it will work for the CSSOM-View > Element.scrollTop and Element.scrollLeft attributes. I attempted moving the dictionaries to a separate webidl file, with the same resulting exception: WebIDL.WebIDLError: error: An attribute cannot be of a union type if one of its member types (or one of its member types's member types, and so on) is a dictionary type Bug 918011 added support for unions that contain dictionaries; however, this exception is specifically for such unions used with attributes. I have created Bug 1033554 to track this.

(In reply to :kip (Kearwood Gilbert) from comment #14) > (In reply to :kip (Kearwood Gilbert) from comment #13) > > (In reply to Florian Bender from comment #10) > > > (In reply to :kip (Kearwood Gilbert) from comment #7) > > > > It appears that our current WebIDL implementation does not support unions > > > > that contain dictionaries, as requested in the CSSOM spec for the Element > > > > interface changes: > > > > > > > > attribute (double or ScrollOptionsVertical) scrollTop; > > > > attribute (double or ScrollOptionsHorizontal) scrollLeft; > > > > > > > > Perhaps a work-around is possible; otherwise, I will check with #developers > > > > and see if it will be necessary to add support for such unions and/or > > > > suggest a different interface. > > > > > > Did you do that already? If not, you can ping :bz as he probably knows the > > > WebIDL stuff the best. If there's a bug for implementing it, please add it > > > to the dep tree. > > > > I have found an existing bug, which is marked as fixed: > > > > Bug 918011 - Support dictionaries inside unions > > > > A comment on the bug states, "This will work as long as the dictionaries > > inside the union are in separate webidl files." > > > > I will attempt that and see if it will work for the CSSOM-View > > Element.scrollTop and Element.scrollLeft attributes. > > I attempted moving the dictionaries to a separate webidl file, with the same > resulting exception: > > WebIDL.WebIDLError: error: An attribute cannot be of a union type if one of > its member types (or one of its member types's member types, and so on) is a > dictionary type > > Bug 918011 added support for unions that contain dictionaries; however, this > exception is specifically for such unions used with attributes. > > I have created Bug 1033554 to track this. I have reviewed this with :bz. It is now clear that the CSSOM-View WebIDL for the Element.scrollTop and Element.scrollLeft attributes is invalid. Either the CSSOM-View specification or WebIDL specification will need to be changed. Two issues are involved. The first is that the scrollTop and scrollLeft attributes are expected to return a different type in their getter than in their setter's. Secondly, if they do return the Union with a dictionary, it would result in a new object being created on each access. A (less than ideal) workaround would be to implement these attributes with an "any" type and handle it manually. Perhaps this may be the ideal time to make adjustments to the CSSOM-View spec, as other UA's have also begun implementation but have not yet released: https://code.google.com/p/chromium/issues/detail?id=243871 I will file a bug at w3c.org regarding the scrollTop and scrollLeft attribute changes. Until the WebIDL changes for these attributes are resolved, the rest of the smooth scrolling behavior functionality will not be blocked and can move forward.

- Extended the Element and Window webidl interfaces as described in the CSSOM-View smooth-scrolling specification. - The Element.scrollTop and Element.scrollLeft changes have been omitted until either WebIDL is extended to allow properties to have union datatypes that contain dictionaries or the CSSOM-View smooth-scroll specification is upddated. This will not prevent the other interface changes from being useful. - Implemented wrapper functions for the nsGlobalWindow and Element classes to connect to the new WebIDL bindings. The ScrollOptions parameters are ignored in this patch, and used in Part 3 of this patch series. Note: Please apply patches from bug 1026023 first

- Updated Window XPCOM bindings to match webidl updates. - Does not include updates for Element XPCOM bindings, as the corresponding webidl for the CSSOM-View scroll-behavior specification may change (See part 1 patch)

- Updated scrolling methods in nsGlobalWindow to accept a mozilla::dom::ScrollBehavior parameter to select between the instant and smooth MSD motion. - Updated XPCOM and WebIDL versions of scrolling methods in nsGlobalWindow to pass the correct value of mozilla::dom::ScrollBehavior and activate smooth scrolling. - Moved implementation of Element::ScrollIntoView into a new function that takes a mozilla::dom::ScrollBehavior, which is used by the original function and the new WebIDL binding version. - These functions will need to be updated again to support the scroll-behavior CSS property in Bug 1010538.

This demo allows you to test out the various CSSOM View scrolling DOM methods with vertical scrolling.

- Updated scrolling methods in nsGlobalWindow to accept a mozilla::dom::ScrollBehavior parameter to select between the instant and smooth MSD motion. - Updated XPCOM and WebIDL versions of scrolling methods in nsGlobalWindow to pass the correct value of mozilla::dom::ScrollBehavior and activate smooth scrolling. - Moved implementation of Element::ScrollIntoView into a new function that takes a mozilla::dom::ScrollBehavior, which is used by the original function and the new WebIDL binding version. - These functions will need to be updated again to support the scroll-behavior CSS property in Bug 1010538. V2 Patch: Fixed bug that could cause an assertion to fire when scrolling with a touchpad or mouse wheel while simultaneously calling the DOM scrolling methods.

- Updated scrolling methods in nsGlobalWindow to accept a mozilla::dom::ScrollBehavior parameter to select between the instant and smooth MSD motion. - Updated XPCOM and WebIDL versions of scrolling methods in nsGlobalWindow to pass the correct value of mozilla::dom::ScrollBehavior and activate smooth scrolling. - Moved implementation of Element::ScrollIntoView into a new function that takes a mozilla::dom::ScrollBehavior, which is used by the original function and the new WebIDL binding version. - These functions will need to be updated again to support the scroll-behavior CSS property in Bug 1010538. V3 Patch: Moved the nsGfxScrollFrame crash fix to patchset in Bug 1026023

Comment on attachment 8451960 [details] [diff] [review] Bug 1022818 - Part 1: Update webidl interfaces I have assigned you to review the DOM related changes. Please advise if someone else would be better to assign these to.

Comment on attachment 8451964 [details] [diff] [review] Bug 1022818 - Part 2: Update idl for XPCOM bindings Please advise if keeping these XPCOM interfaces up to date is wanted / needed. I can adjust the dependent patches for this bug if we don't wish to update the XPCOM interfaces.

I don't think we should change the XPCOM interfaces, personally.

(In reply to Boris Zbarsky [:bz] from comment #24) > I don't think we should change the XPCOM interfaces, personally. Thanks Boris, I'll update the patch set to exclude XPCOM interface changes.

- Updated scrolling methods in nsGlobalWindow to accept a mozilla::dom::ScrollBehavior parameter to select between the instant and smooth MSD motion. - Updated WebIDL binding boilerplate scrolling functions in nsGlobalWindow to pass the correct value of mozilla::dom::ScrollBehavior to the implementation and functions, activating smooth scrolling. - Moved implementation of Element::ScrollIntoView into a new function that takes a mozilla::dom::ScrollBehavior, which is used by the original function and the new WebIDL binding version. - These functions will need to be updated again to support the scroll-behavior CSS property in Bug 1010538. V4 Patch: - As per Comment 24 and Comment 25, the XPCOM interface changes have been removed; "part2" patch now replaces the prior "part2" and "part3" patches.

This patchset (including Bug 1026023's patchset) have been pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=a25dc63ce42f Some tests are expected to fail, as dom/tests/mochitest/general/test_interfaces.html has not been updated to match the webidl changes. (This has not yet been reviewed by a DOM peer)

I have submitted a bug at w3.org to request changes to the Element interface or to have the WebIDL specification extended to support different getter and setter datatypes for the Element.scrollTop and Element.scrollLeft attributes: https://www.w3.org/Bugs/Public/show_bug.cgi?id=26294

Comment on attachment 8452652 [details] [diff] [review] Bug 1022818 - Part 2: Implement Smooth Scrolling (V4 Patch) Review of attachment 8452652 [details] [diff] [review]: ----------------------------------------------------------------- I'm not a peer for content or dom, but this looks fine to me.

Comment on attachment 8451960 [details] [diff] [review] Bug 1022818 - Part 1: Update webidl interfaces >+++ b/content/base/public/Element.h > void ScrollIntoView(bool aTop); Why do we need to keep that overload around? Seems like ScrollIntoView with no args could call the two-arg version, right? >+++ b/dom/base/nsGlobalWindow.cpp >+nsGlobalWindow::ScrollBy(int32_t aXScrollDif, int32_t aYScrollDif, Probably better to put this by the other ScrollBy impl, since this is the one that will end up actually having the code while the other one ends up calling it. Same for the other scrollBy* bits.

Comment on attachment 8452652 [details] [diff] [review] Bug 1022818 - Part 2: Implement Smooth Scrolling (V4 Patch) >- Moved implementation of Element::ScrollIntoView into a new function that > takes a mozilla::dom::ScrollBehavior, which is used by the original function > and the new WebIDL binding version. I don't think we'll need that if we change the 0-arg version to construct a dictionary and nix the one-arg version. But either way. >+ void ScrollIntoView(bool aTop, mozilla::dom::ScrollBehavior aBehavior); Why not just ScrollBehavior? This is all inside the mozilla::dom namespace. That applies throughout this patch, in nsGlobalWindow code too. >+++ b/layout/base/nsIPresShell.h >+ * If SCROLL_SMOOTH is set and CSSOM-VIEW scroll-behavior >+ * is enabled, we will scroll smoothly using Where is that "is enabled" check? I don't see one. I think it's fine to just have SCROLL_SMOOTH always mean ScrollMode::SMOOTH_MSD but you should probably send an intent to implement and ship mail if you haven't sent an intent to ship yet. r=me with that.

- Extended the Element and Window webidl interfaces as described in the CSSOM-View smooth-scrolling specification. - The Element.scrollTop and Element.scrollLeft changes have been omitted until either WebIDL is extended to allow properties to have union datatypes that contain dictionaries or the CSSOM-View smooth-scroll specification is upddated. This will not prevent the other interface changes from being useful. - Implemented wrapper functions for the nsGlobalWindow to connect to the new WebIDL bindings. The ScrollOptions parameters are ignored in this patch, and used in Part 3 of this patch series. V2 Patch: Updated based on review in Comment 30

>+ options.mBehavior = ScrollBehavior::Auto; No need: that's the default value. In fact, you could even do: ScrollIntoView(true, ScrollOptions()); and similar at other callsites, I believe, since they take a const ref.

(In reply to Boris Zbarsky [:bz] from comment #33) > >+ options.mBehavior = ScrollBehavior::Auto; > > No need: that's the default value. > > In fact, you could even do: > > ScrollIntoView(true, ScrollOptions()); > > and similar at other callsites, I believe, since they take a const ref. Good point. I'll update the patch.

- Extended the Element and Window webidl interfaces as described in the CSSOM-View smooth-scrolling specification. - The Element.scrollTop and Element.scrollLeft changes have been omitted until either WebIDL is extended to allow properties to have union datatypes that contain dictionaries or the CSSOM-View smooth-scroll specification is upddated. This will not prevent the other interface changes from being useful. - Implemented wrapper functions for the nsGlobalWindow and Element classes to connect to the new WebIDL bindings. The ScrollOptions parameters are ignored in this patch, and used in Part 3 of this patch series. V3 Patch: Reduced unnecessary code, based on Comment 33

- Updated ScrollTo method in nsGlobalWindow to accept a mozilla::dom::ScrollOptions parameter to select between the instant and smooth MSD motion. - Updated WebIDL binding boilerplate scrolling functions in nsGlobalWindow to pass the correct value of mozilla::dom::ScrollBehavior to the implementation and functions, activating smooth scrolling. - These functions will need to be updated again to support the scroll-behavior CSS property in Bug 1010538. V5 Patch: Updated based on review in Comment 31 and Comment 33

Comment on attachment 8459709 [details] [diff] [review] Bug 1022818 - Part 1: Update webidl interfaces (V3 Patch), r=bz r=me

This must not be enabled on Firefox for Android until the Java based scrolling classes are updated. I have created Bug 1041833 to track the changes needed before enabling this on Android.

(In reply to :kip (Kearwood Gilbert) from comment #28) > I have submitted a bug at w3.org to request changes to the Element interface > or to have the WebIDL specification extended to support different getter and > setter datatypes for the Element.scrollTop and Element.scrollLeft attributes: > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=26294 I have also posted on www-style: http://lists.w3.org/Archives/Public/www-style/2014Jul/0385.html

(In reply to :kip (Kearwood Gilbert) from comment #39) > (In reply to :kip (Kearwood Gilbert) from comment #28) > > I have submitted a bug at w3.org to request changes to the Element interface > > or to have the WebIDL specification extended to support different getter and > > setter datatypes for the Element.scrollTop and Element.scrollLeft attributes: > > > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=26294 > > I have also posted on www-style: > > http://lists.w3.org/Archives/Public/www-style/2014Jul/0385.html Bug 1045754 has been added to track implementation of the alternative WebIDL once a decision has been made on the invalid WebIDL specified for the extended Element.scrollTop and Element.scrollLeft attributes. The rest of the CSSOM-View scroll-behavior DOM methods implemented in this patch set can be landed separately.

- Verify that instant scroll-behavior is synchronous. - Verify that smooth scroll-behavior is asynchronous. - Verify that smooth scroll-behavior is triggered by CSSOM-View DOM methods. - Verify that instant scroll-behavior interrupts smooth scroll-behavior animation. - Verify that smooth scroll-behavior is not framerate dependant. - Verify that smooth scroll-behavior physics simulations used by animations converge and allow the animation to reach completion. - CSSOM-View scroll-behavior smooth scroll animations must produce the same results independently of frame-rate: - Reference samples of scroll position for each frame are captured from a smooth scroll at 120fps for variations in X-Distance, Y-Distance. - Test samples are captured from an animation with the same parameters at varying frame rates. - Variance in position at each sampled interval is compared to the 120fps reference. To pass the test, the position of each test sample must match the reference position with a tolerance of one test sample frame's range of motion. This range of motion is calculated by the position delta of the reference samples one test frame duration before and after. - The duration of the reference sample animation and the test sample animation must match within 1 frame to pass the test. - The simulation driving the animation must converge and stop on the destination position for the test to pass.

This has been pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ab1079006527

Comment on attachment 8464182 [details] [diff] [review] Bug 1022818 - Part 3: CSSOM-View Scroll-Behavior and MSD Smooth Scroll Tests >+ "instant scroll-behavior must be synchronous"); ... >+ "instant scroll-behavior must be synchronous"); Worth having a slightly different message on the second one. >+ "instant scroll-behavior must interrupt smooth scroll-behavior animation"); >+ "instant scroll-behavior must interrupt smooth scroll-behavior animation"); And here, especially since the two conditions are identical. r=me, though I worry about this timing out on slower hardware....

- Verify that instant scroll-behavior is synchronous. - Verify that smooth scroll-behavior is asynchronous. - Verify that smooth scroll-behavior is triggered by CSSOM-View DOM methods. - Verify that instant scroll-behavior interrupts smooth scroll-behavior animation. - Verify that smooth scroll-behavior is not framerate dependant. - Verify that smooth scroll-behavior physics simulations used by animations converge and allow the animation to reach completion. - CSSOM-View scroll-behavior smooth scroll animations must produce the same results indendently of frame-rate: - Reference samples of scroll position for each frame are captured from a smooth scroll at 120fps for variations in X-Distance, Y-Distance. - Test samples are captured from an animation with the same parameters at varying framerates. - Variance in position at each sampled interval is compared to the 120fps reference. To pass the test, the position of each test sample must match the reference position with a tolerance of one test sample frame's range of motion. This range of motion is calculated by the position delta of the reference samples one test frame duration before and after. - The duration of the reference sample animation and the test sample animation must match within 1 frame to pass the test. - The simulation driving the animation must converge and stop on the destination position for the test to pass. V2 Patch: Updated test descriptions to be unique as per Comment 43.

(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #43) > Comment on attachment 8464182 [details] [diff] [review] > Bug 1022818 - Part 3: CSSOM-View Scroll-Behavior and MSD Smooth Scroll Tests > > >+ "instant scroll-behavior must be synchronous"); > ... > >+ "instant scroll-behavior must be synchronous"); > > Worth having a slightly different message on the second one. > > >+ "instant scroll-behavior must interrupt smooth scroll-behavior animation"); > >+ "instant scroll-behavior must interrupt smooth scroll-behavior animation"); > > And here, especially since the two conditions are identical. > > r=me, though I worry about this timing out on slower hardware.... The test results are not sensitive to timing, as it takes control of the refresh driver; however, if the test itself timing out becomes a problem, each pass can be broken up into separate tests or we could reduce the number of test passes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c5208ca3274 https://hg.mozilla.org/integration/mozilla-inbound/rev/bfe7d53e220a https://hg.mozilla.org/integration/mozilla-inbound/rev/9b37b8ae8302

Backed out for B2G debug mochitest orange. https://hg.mozilla.org/integration/mozilla-inbound/rev/ac466c05dc31 https://tbpl.mozilla.org/php/getParsedLog.php?id=45058242&tree=Mozilla-Inbound

And mochitest-e10s: https://tbpl.mozilla.org/php/getParsedLog.php?id=45061798&tree=Mozilla-Inbound

- Verify that instant scroll-behavior is synchronous. - Verify that smooth scroll-behavior is asynchronous. - Verify that smooth scroll-behavior is triggered by CSSOM-View DOM methods. - Verify that instant scroll-behavior interrupts smooth scroll-behavior animation. - Verify that smooth scroll-behavior is not framerate dependant. - Verify that smooth scroll-behavior physics simulations used by animations converge and allow the animation to reach completion. - CSSOM-View scroll-behavior smooth scroll animations must produce the same results indendently of frame-rate: - Reference samples of scroll position for each frame are captured from a smooth scroll at 120fps for variations in X-Distance, Y-Distance. - Test samples are captured from an animation with the same parameters at varying framerates. - Variance in position at each sampled interval is compared to the 120fps reference. To pass the test, the position of each test sample must match the reference position with a tolerance of one test sample frame's range of motion. This range of motion is calculated by the position delta of the reference samples one test frame duration before and after. - The duration of the reference sample animation and the test sample animation must match within 1 frame to pass the test. - The simulation driving the animation must converge and stop on the destination position for the test to pass. V3 Patch: - Reduced number of combinations of delta-x and delta-y scrolls in the test to reduce possibility of timing out. - Fixed intermittent test failures on B2G and e10s by waiting for paints to occur before sampling the scroll positions.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #48) > And mochitest-e10s: > https://tbpl.mozilla.org/php/getParsedLog.php?id=45061798&tree=Mozilla- > Inbound I have reproduced the orange mochitest failure and updated the tests to address the issue.

Comment on attachment 8470332 [details] [diff] [review] Bug 1022818 - Part 3: CSSOM-View Scroll-Behavior and MSD Smooth Scroll Tests (V3 Patch) As bz is away, would you have some idle cycles to review this updated mochitest?

Comment on attachment 8470332 [details] [diff] [review] Bug 1022818 - Part 3: CSSOM-View Scroll-Behavior and MSD Smooth Scroll Tests (V3 Patch) Review of attachment 8470332 [details] [diff] [review]: ----------------------------------------------------------------- The actual tests look fine. Just a few nits. ::: layout/generic/test/test_scroll_behavior.html @@ +19,5 @@ > + </style> > + <script type="application/javascript" > + src="/tests/SimpleTest/SimpleTest.js"></script> > + <script type="application/javascript" > + src="/tests/SimpleTest/paint_listener.js"></script> These can fit on one line. The 'type' can be removed if necessary. @@ +22,5 @@ > + <script type="application/javascript" > + src="/tests/SimpleTest/paint_listener.js"></script> > + <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/> > + <script type="application/javascript"> > + There's a bunch of trailing whitespace that would be good to remove. @@ +46,5 @@ > + } > + }, false); > + > + > + function test_scroll_behavior_interruption(next_test) { Would be better to use camelCase in tests.

- Verify that instant scroll-behavior is synchronous. - Verify that smooth scroll-behavior is asynchronous. - Verify that smooth scroll-behavior is triggered by CSSOM-View DOM methods. - Verify that instant scroll-behavior interrupts smooth scroll-behavior animation. - Verify that smooth scroll-behavior is not framerate dependant. - Verify that smooth scroll-behavior physics simulations used by animations converge and allow the animation to reach completion. - CSSOM-View scroll-behavior smooth scroll animations must produce the same results indendently of frame-rate: - Reference samples of scroll position for each frame are captured from a smooth scroll at 120fps for variations in X-Distance, Y-Distance. - Test samples are captured from an animation with the same parameters at varying framerates. - Variance in position at each sampled interval is compared to the 120fps reference. To pass the test, the position of each test sample must match the reference position with a tolerance of one test sample frame's range of motion. This range of motion is calculated by the position delta of the reference samples one test frame duration before and after. - The duration of the reference sample animation and the test sample animation must match within 1 frame to pass the test. - The simulation driving the animation must converge and stop on the destination position for the test to pass. V4 Patch: - Updated formatting based on code review in Comment 52

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a2dcc132344 https://hg.mozilla.org/integration/mozilla-inbound/rev/d24b367f3ac4 https://hg.mozilla.org/integration/mozilla-inbound/rev/e40ffcb782f8

Reverted for B2G failures in test_scroll_behavior.html: https://tbpl.mozilla.org/php/getParsedLog.php?id=45848049&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=45849464&tree=Mozilla-Inbound remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3e75ebeda724 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/effcdf26dbc2 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3aaa5903622a

see comment 55

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #56) > see comment 55 I believe this to be a problem with the test. The test takes control of the refresh driver with SpecialPowers and expects the results to be deterministic. I have already updated the tests to wait for paints to flush. Perhaps this is not be enough for the frame metrics to sync back to the main thread. Is it possible that SpecialPowers.DOMWindowUtils.getOMTAStyle should be used to sample the scrolling animation more reliably for test purposes?

Bug 1022818 - Part 1: Update webidl interfaces - Extended the Element and Window webidl interfaces as described in the CSSOM-View smooth-scrolling specification. - The Element.scrollTop and Element.scrollLeft changes have been omitted until either WebIDL is extended to allow properties to have union datatypes that contain dictionaries or the CSSOM-View smooth-scroll specification is upddated. This will not prevent the other interface changes from being useful. - Implemented wrapper functions for the nsGlobalWindow to connect to the new WebIDL bindings. The ScrollOptions parameters are ignored in this patch, and used in Part 3 of this patch series. V4 Patch: Un-bitrotted

Bug 1022818 - Part 2: Implement Smooth Scrolling - Updated ScrollTo method in nsGlobalWindow to accept a mozilla::dom::ScrollOptions parameter to select between the instant and smooth MSD motion. - Updated WebIDL binding boilerplate scrolling functions in nsGlobalWindow to pass the correct value of mozilla::dom::ScrollBehavior to the implementation and functions, activating smooth scrolling. - These functions will need to be updated again to support the scroll-behavior CSS property in Bug 1010538. V6 Patch: Un-bitrotted

Bug 1022818 - Part 3: Tests for CSSOM-View Smooth-Scroll DOM API Methods and MSD Animation - Verify that instant scroll-behavior is synchronous. - Verify that smooth scroll-behavior is asynchronous. - Verify that smooth scroll-behavior is triggered by CSSOM-View DOM methods. - Verify that instant scroll-behavior interrupts smooth scroll-behavior animation. - Verify that smooth scroll-behavior is not framerate dependant. - Verify that smooth scroll-behavior physics simulations used by animations converge and allow the animation to reach completion. - CSSOM-View scroll-behavior smooth scroll animations must produce the same results indendently of frame-rate: - Reference samples of scroll position for each frame are captured from a smooth scroll at 120fps for variations in X-Distance, Y-Distance. - Test samples are captured from an animation with the same parameters at varying framerates. - Variance in position at each sampled interval is compared to the 120fps reference. To pass the test, the position of each test sample must match the reference position with a tolerance of one test sample frame's range of motion. This range of motion is calculated by the position delta of the reference samples one test frame duration before and after. - The duration of the reference sample animation and the test sample animation must match within 1 frame to pass the test. - The simulation driving the animation must converge and stop on the destination position for the test to pass. V5 Patch: - Fixed bug in nsGfxScrollFrame which caused erratic behavior when mochitests took control of the refresh driver (Was using TimeStamp::Now() rather than getting the time from the refresh driver. - Disabled the scroll-behavior test on e10s and b2g (The test will be enabled for all targets in Bug 1022825 which adds support for APZ)

Pushed updated patch set to try: https://tbpl.mozilla.org/?tree=Try&rev=2303ed576878

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b105c3b51be https://hg.mozilla.org/integration/mozilla-inbound/rev/605438d299a8 https://hg.mozilla.org/integration/mozilla-inbound/rev/c5827bb87f76

https://hg.mozilla.org/mozilla-central/rev/5b105c3b51be https://hg.mozilla.org/mozilla-central/rev/605438d299a8 https://hg.mozilla.org/mozilla-central/rev/c5827bb87f76

I experimented with this a bit, and I see some behavior which could be considered a bug IMO. When scrolling smoothly repeatedly and quickly, if I increase the zoom level while it scrolls (I used ctrl+mouse-wheel-up), then it's likely that it will seem to "hit a wall" while scrolling down - at a position derived from the zoom level change. And this "wall" will stay there for the next scrolls too. E.g. open a page, set its zoom level to 100% (ctrl-0), open the browser console and paste this (warning.. it's kinda frantic): (function loop(){ window.scrollTo(0, Math.random() * window.scrollMaxY, {behavior: "smooth"}); setTimeout(loop, 100 + 400 * Math.random()); })(),0 Now increase the zoom level by a lot. Many times it will stop reaching the bottom of the page on the next scrolls. When I did the same without the {behavior: "smooth"} part, then I didn't see the issue.

Oh, comment 64 was with Firefox Nightly 2014-09-12 and with the pref layout.css.scroll-behavior.enabled = true, on Windows 8.1.

Can you reproduce the symptom on comment 64? is this a bug?

(In reply to Avi Halachmi (:avih) from comment #66) > Can you reproduce the symptom on comment 64? is this a bug? I am attempting to reproduce now. I suspect that this is a bug caused by incorrect unit conversions in the AsyncPanZoomControlller.cpp part of the patches. In particular, it may be necessary to also multiply by the zoom when converting coordinates for the smooth scroll destination. I'll create a new bug to track this and will clear the needinfo when I confirm this is the same issue.

Have not yet reproduced. Setting needsinfo so I will not lose track.