Implement CSSOM-View smooth scrolling DOM Methods

RESOLVED FIXED in mozilla34

Status

()

defect
P3
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: kip, Assigned: kip)

Tracking

(Depends on 1 bug, Blocks 1 bug, {dev-doc-needed, perf})

Trunk
mozilla34
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [c=handeye p= s= u=], )

Attachments

(4 attachments, 17 obsolete attachments)

12.85 KB, text/html
Details
10.95 KB, patch
Details | Diff | Splinter Review
11.68 KB, patch
Details | Diff | Splinter Review
15.72 KB, patch
roc
: review+
Details | Diff | Splinter Review
Assignee

Description

5 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
Assignee

Updated

5 years ago
See Also: → 1022825
Assignee

Updated

5 years ago
Depends on: 1022825
Assignee

Comment 1

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

Comment 2

5 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?
(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.
Assignee

Comment 4

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

Updated

5 years ago
Keywords: dev-doc-needed

Comment 5

5 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.
Flags: needinfo?(bzbarsky)
> 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.
Flags: needinfo?(bzbarsky)
Assignee

Comment 7

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

Updated

5 years ago
Depends on: 1026023
See Also: → 1026023
Assignee

Comment 8

5 years ago
Demonstration using scroll behavior to smoothly scroll to the top and bottom of a page
Assignee

Comment 9

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

Comment 10

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

Comment 11

5 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
Attachment #8443576 - Attachment is obsolete: true
Assignee

Comment 12

5 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
Attachment #8443583 - Attachment is obsolete: true
Assignee

Comment 13

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

Updated

5 years ago
Depends on: 1033554
Assignee

Comment 14

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

Comment 15

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

Comment 16

5 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
Attachment #8443724 - Attachment is obsolete: true
Assignee

Comment 17

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

Comment 18

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

Comment 19

5 years ago
This demo allows you to test out the various CSSOM View scrolling DOM methods with vertical scrolling.
Attachment #8443573 - Attachment is obsolete: true
Assignee

Comment 20

5 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.
Attachment #8451971 - Attachment is obsolete: true
Assignee

Comment 21

5 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
Attachment #8452046 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8452451 - Flags: review?(matt.woodrow)
Assignee

Comment 22

5 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.
Attachment #8451960 - Flags: review?(bzbarsky)
Assignee

Comment 23

5 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.
Attachment #8451964 - Flags: review?(bzbarsky)
I don't think we should change the XPCOM interfaces, personally.
Assignee

Comment 25

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

Comment 26

5 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.
Attachment #8451964 - Attachment is obsolete: true
Attachment #8452451 - Attachment is obsolete: true
Attachment #8451964 - Flags: review?(bzbarsky)
Attachment #8452451 - Flags: review?(matt.woodrow)
Attachment #8452652 - Flags: review?(matt.woodrow)
Assignee

Comment 27

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

Comment 28

5 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
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.
Attachment #8452652 - Flags: review?(matt.woodrow) → review?(bzbarsky)
Blocks: 1037576
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.
Attachment #8451960 - Flags: review?(bzbarsky) → review+
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.
Attachment #8452652 - Flags: review?(bzbarsky) → review+
Assignee

Comment 32

5 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
Attachment #8451960 - Attachment is obsolete: true
>+  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.
Assignee

Comment 34

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

Comment 35

5 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
Attachment #8459692 - Attachment is obsolete: true
Attachment #8459709 - Flags: review?(bzbarsky)
Assignee

Comment 36

5 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
Attachment #8452652 - Attachment is obsolete: true
Comment on attachment 8459709 [details] [diff] [review]
Bug 1022818 - Part 1: Update webidl interfaces (V3 Patch), r=bz

r=me
Attachment #8459709 - Flags: review?(bzbarsky) → review+
Assignee

Updated

5 years ago
Attachment #8459709 - Attachment description: Bug 1022818 - Part 1: Update webidl interfaces (V3 Patch) → Bug 1022818 - Part 1: Update webidl interfaces (V3 Patch), r=bz
Assignee

Updated

5 years ago
Blocks: 1041833
Assignee

Comment 38

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

Updated

5 years ago
Status: NEW → ASSIGNED
Keywords: perf
Whiteboard: [c=handeye p= s= u=]
Assignee

Comment 39

5 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
Assignee

Updated

5 years ago
Blocks: 1045754
See Also: → 1045754
Assignee

Comment 40

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

Comment 41

5 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.
Attachment #8464182 - Flags: review?(bzbarsky)
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....
Attachment #8464182 - Flags: review?(bzbarsky) → review+
Assignee

Comment 44

5 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.
Attachment #8464182 - Attachment is obsolete: true
Assignee

Comment 45

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

Updated

5 years ago
Keywords: checkin-needed
Assignee

Updated

5 years ago
Blocks: 945584
Priority: -- → P3
Assignee

Comment 49

5 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.
Attachment #8465762 - Attachment is obsolete: true
Assignee

Comment 50

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

Comment 51

5 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?
Attachment #8470332 - Flags: review?(roc)
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.
Attachment #8470332 - Flags: review?(roc) → review+
Assignee

Comment 53

5 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
Attachment #8470332 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Comment 57

5 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?
Flags: needinfo?(kgilbert)
Assignee

Comment 58

5 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
Attachment #8459709 - Attachment is obsolete: true
Assignee

Comment 59

5 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
Attachment #8459715 - Attachment is obsolete: true
Assignee

Comment 60

5 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)
Attachment #8471836 - Attachment is obsolete: true
Attachment #8478547 - Flags: review?(roc)
Assignee

Comment 61

5 years ago
Pushed updated patch set to try:

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

Updated

5 years ago
Keywords: checkin-needed

Updated

5 years ago
Depends on: 1062406
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?
Flags: needinfo?(kgilbert)
Assignee

Comment 67

5 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.
Flags: needinfo?(kgilbert)
Assignee

Comment 68

5 years ago
Have not yet reproduced.  Setting needsinfo so I will not lose track.
Flags: needinfo?(kgilbert)
Assignee

Updated

5 years ago
No longer blocks: 1045754
Assignee

Updated

5 years ago
Flags: needinfo?(kgilbert)

Updated

4 years ago
Depends on: 1131740
You need to log in before you can comment on or make changes to this bug.