:kip (Kearwood Gilbert) Assignee
	Description • 10 years ago

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

	:kip (Kearwood Gilbert) Assignee
	Comment 1 • 10 years ago

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

	:kip (Kearwood Gilbert) Assignee
	Comment 2 • 10 years ago

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?

	Avi Halachmi (:avih)
	Comment 3 • 10 years ago

(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.

	:kip (Kearwood Gilbert) Assignee
	Comment 4 • 10 years ago

(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.

	Florian Bender
	Comment 5 • 10 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 10 years ago

> 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.

	:kip (Kearwood Gilbert) Assignee
	Comment 7 • 10 years ago

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.

	:kip (Kearwood Gilbert) Assignee
	Comment 8 • 10 years ago

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

	:kip (Kearwood Gilbert) Assignee
	Comment 9 • 10 years ago

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.

	Florian Bender
	Comment 10 • 10 years ago

(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.

	:kip (Kearwood Gilbert) Assignee
	Comment 11 • 10 years ago

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

	:kip (Kearwood Gilbert) Assignee
	Comment 12 • 10 years ago

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

	:kip (Kearwood Gilbert) Assignee
	Comment 13 • 10 years ago

(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.

	:kip (Kearwood Gilbert) Assignee
	Comment 14 • 10 years ago

(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.

	:kip (Kearwood Gilbert) Assignee
	Comment 15 • 10 years ago

(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.

	:kip (Kearwood Gilbert) Assignee
	Comment 16 • 10 years ago

- 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

	:kip (Kearwood Gilbert) Assignee
	Comment 17 • 10 years ago

- 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)

	:kip (Kearwood Gilbert) Assignee
	Comment 18 • 10 years ago

- 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.

	:kip (Kearwood Gilbert) Assignee
	Comment 19 • 10 years ago

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

	:kip (Kearwood Gilbert) Assignee
	Comment 20 • 10 years ago

- 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.

	:kip (Kearwood Gilbert) Assignee
	Comment 21 • 10 years ago

- 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

	:kip (Kearwood Gilbert) Assignee
	Comment 22 • 10 years ago

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.

	:kip (Kearwood Gilbert) Assignee
	Comment 23 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 10 years ago

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

	:kip (Kearwood Gilbert) Assignee
	Comment 25 • 10 years ago

(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.

	:kip (Kearwood Gilbert) Assignee
	Comment 26 • 10 years ago

- 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.

	:kip (Kearwood Gilbert) Assignee
	Comment 27 • 10 years ago

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)

	:kip (Kearwood Gilbert) Assignee
	Comment 28 • 10 years ago

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

	Matt Woodrow (:mattwoodrow)
	Comment 29 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 31 • 10 years ago

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.

	:kip (Kearwood Gilbert) Assignee
	Comment 32 • 10 years ago

- 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

	Boris Zbarsky [:bzbarsky]
	Comment 33 • 10 years ago

>+  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.

	:kip (Kearwood Gilbert) Assignee
	Comment 34 • 10 years ago

(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.

	:kip (Kearwood Gilbert) Assignee
	Comment 35 • 10 years ago

- 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

	:kip (Kearwood Gilbert) Assignee
	Comment 36 • 10 years ago

- 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

	Boris Zbarsky [:bzbarsky]
	Comment 37 • 10 years ago

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

r=me

	:kip (Kearwood Gilbert) Assignee
	Comment 38 • 10 years ago

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.

	:kip (Kearwood Gilbert) Assignee
	Comment 39 • 10 years ago

(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

	:kip (Kearwood Gilbert) Assignee
	Comment 40 • 10 years ago

(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.

	:kip (Kearwood Gilbert) Assignee
	Comment 41 • 10 years ago

- 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.

	:kip (Kearwood Gilbert) Assignee
	Comment 42 • 10 years ago

This has been pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=ab1079006527

	Boris Zbarsky [:bzbarsky]
	Comment 43 • 10 years ago

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....

	:kip (Kearwood Gilbert) Assignee
	Comment 44 • 10 years ago

- 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.

	:kip (Kearwood Gilbert) Assignee
	Comment 45 • 10 years ago

(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.

	Ryan VanderMeulen [:RyanVM]
	Comment 46 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 47 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 48 • 10 years ago

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

	:kip (Kearwood Gilbert) Assignee
	Comment 49 • 10 years ago

- 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.

	:kip (Kearwood Gilbert) Assignee
	Comment 50 • 10 years ago

(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.

	:kip (Kearwood Gilbert) Assignee
	Comment 51 • 10 years ago

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?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 52 • 10 years ago

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.

	:kip (Kearwood Gilbert) Assignee
	Comment 53 • 10 years ago

- 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

	Ryan VanderMeulen [:RyanVM]
	Comment 54 • 10 years ago

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

	Ed Morley [:emorley]
	Comment 55 • 10 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 56 • 10 years ago

see comment 55

	:kip (Kearwood Gilbert) Assignee
	Comment 57 • 10 years ago

(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?

	:kip (Kearwood Gilbert) Assignee
	Comment 58 • 10 years ago

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

	:kip (Kearwood Gilbert) Assignee
	Comment 59 • 10 years ago

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

	:kip (Kearwood Gilbert) Assignee
	Comment 60 • 10 years ago

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)

	:kip (Kearwood Gilbert) Assignee
	Comment 61 • 10 years ago

Pushed updated patch set to try:

https://tbpl.mozilla.org/?tree=Try&rev=2303ed576878

	Ryan VanderMeulen [:RyanVM]
	Comment 62 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 63 • 10 years ago

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

	Avi Halachmi (:avih)
	Comment 64 • 10 years ago

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.

	Avi Halachmi (:avih)
	Comment 65 • 10 years ago

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

	Avi Halachmi (:avih)
	Comment 66 • 10 years ago

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

	:kip (Kearwood Gilbert) Assignee
	Comment 67 • 10 years ago

(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.

	:kip (Kearwood Gilbert) Assignee
	Comment 68 • 10 years ago

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