969250 - Implement CSS scroll snapping for scrollbars

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[miranda.j.emery]

Attachment: Implements scroll snapping for scrollbars

Extends https://bugzilla.mozilla.org/show_bug.cgi?id=945584 to cover scroll bars

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8372061 [details] [diff] [review]
Implements scroll snapping for scrollbars

Review of attachment 8372061 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1059,5 @@
>  {
> +  if (!mStartPointSet) {
> +    mStartPoint = GetScrollPosition();
> +    mStartPointSet = true;
> +  }

It's not clear to me why we need to track the start point for these commands.

@@ +1087,5 @@
>  
>  void
> +ScrollFrameHelper::CompleteButtonScroll(nsScrollbarFrame* aScrollbar)
> +{
> +  mStartPointSet = false;

I think we should check if this is already false and bail out if it is.

@@ +1093,5 @@
> +  nsPoint dest = GetScrollPosition();
> +  nsRect allowedRange = GetAllowedRange(mStartPoint, isHorizontal);
> +  dest = GetScrollSnapPointScrollbar(dest, allowedRange);
> +  allowedRange = GetAllowedRange(dest, isHorizontal);
> +  ScrollTo(dest, nsIScrollableFrame::SMOOTH, &allowedRange);

It's not obvious what this is for. Please add a comment explaining what this does and why.

@@ +1155,5 @@
> +  nsPoint dest = GetScrollPosition();
> +  nsRect allowedRange = GetAllowedRange(mStartPoint, isHorizontal);
> +  dest = GetScrollSnapPointSlider(dest, allowedRange);
> +  allowedRange = GetAllowedRange(dest, isHorizontal);
> +  ScrollTo(dest, nsIScrollableFrame::SMOOTH, &allowedRange);

This needs documentation too

@@ +2802,5 @@
> +               nscoord* aBestEdge);
> +  nsPoint GetBestEdge() { return mBestEdge; }
> +protected:
> +  nsPoint mDestination;         // gives the position after scrolling but before snapping
> +  nsPoint mCurPos;              // gives the position before scrolling

call this mSartPos

@@ +2803,5 @@
> +  nsPoint GetBestEdge() { return mBestEdge; }
> +protected:
> +  nsPoint mDestination;         // gives the position after scrolling but before snapping
> +  nsPoint mCurPos;              // gives the position before scrolling
> +  nsPoint mScrollingDirection;  // always either -1, 0, or 1

nsIntPoint I think

@@ +2812,5 @@
> +
> +CalcScrollbarSnapPoints::CalcScrollbarSnapPoints(nsPoint aDestination,
> +                                                 nsPoint aCurPos,
> +                                                 nsPoint aDirection,
> +                                                 nsRect aAllowedRange)

const T& for all these

@@ +2820,5 @@
> +  mScrollingDirection = aDirection;
> +  if (mScrollingDirection.x < 0) mScrollingDirection.x = -1;
> +  if (mScrollingDirection.x > 0) mScrollingDirection.x = 1;
> +  if (mScrollingDirection.y < 0) mScrollingDirection.y = -1;
> +  if (mScrollingDirection.y > 0) mScrollingDirection.y = 1;

Separate lines, and don't forget ()?

@@ +3067,5 @@
> +  if (direction.x < 0)
> +    flags |= SCROLL_LEFT;
> +  else if (direction.x > 0)
> +    flags |= SCROLL_RIGHT;
> +  if (direction.y < 0) 

Trailing space. Also {}

::: layout/generic/nsGfxScrollFrame.h
@@ +211,5 @@
>     */
>    void ScrollToRestoredPosition();
>  
> +  nsPoint GetScrollSnapPointSlider(nsPoint aDestination, nsRect aAllowedRange);
> +  nsPoint GetScrollSnapPointScrollbar(nsPoint aDestination, nsRect aAllowedRange);

Document these methods.

@@ +425,5 @@
>    // True if this frame has been scrolled at least once
>    bool mHasBeenScrolled:1;
>  
> +  nsPoint mStartPoint;
> +  bool mStartPointSet;

These names need to be more descriptive. Maybe "mThumbDragStartPoint" and "mIsDraggingThumb"?

@@ +683,5 @@
> +    return mHelper.GetScrollSnapPointSlider(aDestination, aAllowedRange);
> +  }
> +  virtual nsPoint GetScrollSnapPointScrollbar(nsPoint aDestination, nsRect aAllowedRange) {
> +    return mHelper.GetScrollSnapPointScrollbar(aDestination, aAllowedRange);
> +  }

Why are these methods here?

::: modules/libpref/src/init/all.js
@@ +1748,5 @@
>  // Is support for scroll-snap enabled?
>  pref("layout.css.scroll-snap.enabled", false);
>  
> +// When scrolling using the thumb, snapping only occurs when the distance
> +// scrolled in pixels is above this threshold. A value <= 0 means snapping

"using the scrollbar thumb", "in CSS pixels"

@@ +1750,5 @@
>  
> +// When scrolling using the thumb, snapping only occurs when the distance
> +// scrolled in pixels is above this threshold. A value <= 0 means snapping
> +// always occurs.
> +pref("layout.css.scroll-snap-threshold", 20);

Call this layout.css.scroll-snap.thumb-threshold

Comment 2

[miranda.j.emery]

Attachment: Implements scroll snapping for scrollbars

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8373090 [details] [diff] [review]
Implements scroll snapping for scrollbars

Review of attachment 8373090 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1096,5 @@
> +  nsPoint currentPos = GetScrollPosition();
> +  nsRect allowedRange = GetAllowedRange(mScrollingStartPoint, isHorizontal);
> +  // Given the current position, the closest snap point in the direction of
> +  // scrolling is returned.
> +  nsPoint finalPos = GetScrollSnapPointScrollbar(currentPos, allowedRange);

As discussed, let's just pass the start point in here and make GetScrollSnapPointScrollbar be responsible for rejecting snap points that are too close to the starting point.

@@ +2825,5 @@
> +                                                 const nsPoint& aDirection,
> +                                                 const nsRect& aAllowedRange)
> +{
> +  mDestination = aDestination;
> +  mStartPos = aStartPos;

Use C++ syntax for initializers:
     ...)
 : mDestination(aDestination)
 , mStartPos(aStartPos)
etc

@@ +2826,5 @@
> +                                                 const nsRect& aAllowedRange)
> +{
> +  mDestination = aDestination;
> +  mStartPos = aStartPos;
> +  mScrollingDirection = nsIntPoint(0,0);

Don't need to initialize this since the default initializer does it.

@@ +2839,5 @@
> +  }
> +  if (aDirection.y > 0) {
> +    mScrollingDirection.y = 1;
> +  }
> +  mAllowedRange = aAllowedRange;

We shouldn't need an allowed range in this class. Once we've computed the best snappoint, the creator of this class can just reject it if it's too close to the starting point of the scroll gesture.

@@ +3101,5 @@
> +  threshold = nsPresContext::CSSPixelsToAppUnits(threshold);
> +  nsPoint distance = aDestination - mScrollingStartPoint;
> +  if (std::abs(distance.x) < threshold && std::abs(distance.y) < threshold) {
> +    return aDestination;
> +  }

As discussed, we should search in both directions (but only in a limited range) for a snappoint to scroll to in this case.

::: layout/generic/nsGfxScrollFrame.h
@@ +217,5 @@
> +   */
> +  nsPoint GetScrollSnapPointScrollbar(nsPoint aDestination, nsRect aAllowedRange);
> +  /**
> +   * GetScrollSnapPointSlider compares the total distance scrolled to a threshold. If the distance is
> +   * below the threshold, no snapping occurs, otherwise GetScrollSnapPointScrollbar is called.

Which distance do you mean by "the distance"?

@@ +219,5 @@
> +  /**
> +   * GetScrollSnapPointSlider compares the total distance scrolled to a threshold. If the distance is
> +   * below the threshold, no snapping occurs, otherwise GetScrollSnapPointScrollbar is called.
> +   */
> +  nsPoint GetScrollSnapPointSlider(nsPoint aDestination, nsRect aAllowedRange);

Use const references here and in GetScrollSnapPointScrollbar.

@@ +432,5 @@
>    // True if this frame has been scrolled at least once
>    bool mHasBeenScrolled:1;
>  
> +  nsPoint mScrollingStartPoint;
> +  bool mIsScrolling;

Document these --- what they mean, when they are set.

Probably also better to call these mInScrollGesture, mScrollGestureStartPoint.

::: layout/xul/nsSliderFrame.cpp
@@ +1058,5 @@
>                               nsEventStatus* aEventStatus)
>  {
>    StopRepeat();
> +  nsIFrame* scrollbar = GetScrollbar();
> +  nsScrollbarFrame* sb = do_QueryFrame(scrollbar);

Let's make GetScrollbar do the do_QueryFrame itself and return an nsScrollbarFrame*.

Comment 4

[miranda.j.emery]

Attachment: Implements scroll snapping for scrollbars

I didn't change GetScrollbar because currently it can return either an nsScrollbarFrame* or an nsSliderFrame*

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8373772 [details] [diff] [review]
Implements scroll snapping for scrollbars

Review of attachment 8373772 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1038,5 @@
> +  if (!mInScrollGesture) {
> +    // mScrollGestureStartPoint is needed to determine which snap points are valid
> +    mScrollGestureStartPoint = GetScrollPosition();
> +    mInScrollGesture = true;
> +  }

Move this common code to a separate shared function StartScrollGestureIfNeeded().

@@ +1098,5 @@
> +  bool isHorizontal = aScrollbar->IsHorizontal();
> +  nsPoint currentPos = GetScrollPosition();
> +  // Given the current position, the closest snap point in the direction of
> +  // scrolling is returned.
> +  nsPoint finalPos = GetScrollSnapPointButtons(currentPos);

GetSnapPointForButtonScroll

@@ +1164,5 @@
> +  bool isHorizontal = aScrollbar->IsHorizontal();
> +  nsPoint currentPos = GetScrollPosition();
> +  // Given the current position, the closest snap point in the direction of
> +  // scrolling is returned.
> +  nsPoint finalPos = GetScrollSnapPointSlider(currentPos);

GetSnapPointForThumbScroll

@@ +2817,5 @@
> +  nsPoint mStartPos;               // gives the position before scrolling
> +  nsIntPoint mScrollingDirection;  // always either -1, 0, or 1
> +  nsPoint mBestEdge;               // keeps track of the position of the current best edge
> +  bool mEdgeFound;                 // true if mBestEdge is storing a valid edge
> +  bool mDirectionUnknown;          // if true edges in any direction are considered

comma after "if true".

@@ +2848,5 @@
> +void
> +CalcScrollbarSnapPoints::AddHorizontalEdge(nscoord aEdge)
> +{
> +  nscoord halfPixel = nsPresContext::CSSPixelsToAppUnits(0.5f);
> +  if (!mDirectionUnknown && std::abs(mStartPos.y - aEdge) <= halfPixel) {

Instead of using 'abs', I think it's clearer to multiply by mScrollingDirection.y to get the distance in the scrolling direction and then compare that to halfPixel.

@@ +2910,5 @@
> +  }
> +  // if there are no edges beyond the scrolling destination, the closest edge between the
> +  // current position and the scrolling destination is used
> +  if (overshoot < 0 && overshoot > curOvershoot) {
> +    *aBestEdge = aEdge;

The code is right but the comment a bit confusing. Just say "the closest edge to the scrolling destination is used".

@@ +3085,5 @@
>  nsPoint
> +ScrollFrameHelper::GetScrollSnapPointButtons(const nsPoint& aDestination)
> +{
> +  int32_t scrollSnapY = GetScrollbarStylesFromFrame().mScrollSnapY;
> +  int32_t scrollSnapX = GetScrollbarStylesFromFrame().mScrollSnapX;

Just call GetScrollbarStylesFromFrame() and store the result in a ScrollbarStyles object instead of separate scrollSnapX/Y.

@@ +3163,5 @@
> +  }
> +  if (scrollSnapX == NS_STYLE_SCROLL_SNAP_TYPE_PROXIMITY &&
> +      std::abs(aDestination.x - finalPos.x) > proximityThreshold) {
> +    finalPos.x = aDestination.x;
> +  }

It seems like we can share a lot of code between this function and the previous function. Maybe we can have a single function doing most of the work which takes a parameter which is the value of thumb-threshold, and pass 0 for that parameter for scrollbar button scrolling?

Also, is there more code that can be shared between scrollbar snapping and the other snapping patches?

@@ +3167,5 @@
> +  }
> +  // If the distance covered by the original scrolling gesture is below the
> +  // threshold, then snapping only occurs if the distance between the current
> +  // position and the snapping edge  is less than double the threshold.
> +  if (std::abs(distance.x) < threshold && 

trailing space

::: layout/generic/nsGfxScrollFrame.h
@@ +219,5 @@
> +  nsPoint GetScrollSnapPointButtons(const nsPoint& aDestination);
> +  /**
> +   * GetScrollSnapPointSlider compares the total distance scrolled to a threshold. If the distance
> +   * scrolled is below the threshold, scrolling direction is ignored and snapping will only occur if
> +   * the distance between the current position and the snapping edge is less than double the threshold. 

trailing whitespace

Comment 6

[miranda.j.emery]

Attachment: Implements scroll snapping for scrollbars

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8374541 [details] [diff] [review]
Implements scroll snapping for scrollbars

Review of attachment 8374541 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2817,5 @@
> +  nsPoint mStartPos;               // gives the position before scrolling
> +  nsIntPoint mScrollingDirection;  // always either -1, 0, or 1
> +  nsPoint mBestEdge;               // keeps track of the position of the current best edge
> +  bool mEdgeFound;                 // true if mBestEdge is storing a valid edge
> +  bool mDirectionUnknown;          // if true, edges in any direction are considered

I think we should call this mDirectionKnown to avoid double negatives.

I also think we should document this more specifically to say that if true, then the direction of the scroll gesture is definitely known (e.g. scrollbar buttons), but if false the UI gesture doesn't have a built-in direction (short thumb drags).

@@ +3084,5 @@
>    }
>  }
>  
>  nsPoint
> +ScrollFrameHelper::GetSnapPointForScrollbarScroll(const nsPoint& aDestination, const nscoord& aThreshold)

aThreshold needs a better name. aDirectionKnownThreshold?

Comment 8

[miranda.j.emery]

Attachment: Tests

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8375086 [details] [diff] [review]
Tests

Review of attachment 8375086 [details] [diff] [review]:
-----------------------------------------------------------------

Basically looks good.

::: layout/base/tests/test_scroll_snapping_scrollbar.html
@@ +40,5 @@
> +  c1.focus();
> +  synthesizeMouse(c1, x, y, { type: "mousedown" });
> +  y += 25;
> +  synthesizeMouse(c1, x, y, { type: "mousemove" });
> +  synthesizeMouse(c1, x, y, { type: "mouseup" });

Add a comment explaining what you're trying to synthesize.

One option is to only enable this test for Windows for now. So you can do a navigator.platform check and if it's not Windows, bail out with todo(false, "support other platforms").

Set x, y and expected in here so they're clearly associated with the synthesized gesture.

Comment 10

[miranda.j.emery]

Attachment: Implements scroll snapping for scrollbars

Comment 11

[miranda.j.emery]

Attachment: Tests

Comment 12

[:kip (Kearwood Gilbert)]

I am implementing scroll snapping (Bug 945584) and will update the attached patches to support the new scroll snapping implementation.  I'll take this bug.

Comment 13

[:kip (Kearwood Gilbert)]

Attachment: Bug 969250 - Part 1: Implement scroll snapping for scrollbars

- Work in progress, updated to match implementation of scroll snapping in Bug 945584.

Comment 14

[:kip (Kearwood Gilbert)]

Attachment: Bug 969250 - Part 1: Implement scroll snapping for scrollbars (v2 Patch)

v2 Patch:
- Un-bitrotted

Comment 15

[:kip (Kearwood Gilbert)]

Attachment: Bug 969250 - Part 1: Implement scroll snapping for scrollbars (v3 Patch)

v3 Patch:
- No longer stomping over page/line/whole scrolling animations with scroll snapping smooth scroll animations.
- Working for OSX scrollbars, including accessibility option to scroll to point clicked rather than paged scrolling.

Comment 16

[:kip (Kearwood Gilbert)]

Attachment: Bug 969250 - Part 1: Implement scroll snapping for scrollbars (v3 Patch)

v3 Patch:
- Implemented scroll snapping support for whole page scrolling and line scrolling through scroll bars.
- Scroll snapping is now triggered at the end of a scroll bar repeated scroll.
- Tested in Ubuntu Linux

Comment 17

[:kip (Kearwood Gilbert)]

Attachment: Bug 969250 - Part 1: Implement scroll snapping for scrollbars (v4 Patch)

v4 Patch:
- Cleaned up patch for review
- Fixed issues with repeat scrolling behaving erratically on Windows 8.1

This patch appears to be working well on all platforms I tested (Windows 8.1, OSX 10.10, and Ubuntu 14.04).  I will also update the tests patch to match.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8572308 [details] [diff] [review]
Bug 969250 - Part 1: Implement scroll snapping for scrollbars (v4 Patch)

Review of attachment 8572308 [details] [diff] [review]:
-----------------------------------------------------------------

excellent!

Comment 19

[:kip (Kearwood Gilbert)]

Attachment: Bug 969250: Part 2 - Tests for scroll snapping for scrollbars

Scrollbar tests verified working on Windows 8.1, OSX 10.10 and Ubuntu 14.04 LTS.

Tests will not be run on mobile due to lack of clickable scrollbars.

Comment 20

[:kip (Kearwood Gilbert)]

I have pushed to try, running mochitests on all platforms:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d76e74d2153

Comment 21

[:kip (Kearwood Gilbert)]

Attachment: Bug 969250 - Part 2: Tests for scroll snapping for scrollbars

Updated patch description for consistency.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8574937 [details] [diff] [review]
Bug 969250 - Part 2: Tests for scroll snapping for scrollbars

Review of attachment 8574937 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdd33ef9606e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2550379cff3c

Comment 24

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7461025&repo=mozilla-inbound

Comment 25

[:kip (Kearwood Gilbert)]

Attachment: Bug 969250 - Part 1: Implement scroll snapping for scrollbars (v6 Patch), r=roc

v6 Patch:
- Un-bitrotted

Comment 26

[:kip (Kearwood Gilbert)]

Attachment: Bug 969250 - Part 1: Implement scroll snapping for scrollbars (v7 Patch),r=roc

V7 Patch:
- Unbitrotted
- Replaced MOZ_OVERRIDE with "override" (See Bug 1145631)

Comment 27

[:kip (Kearwood Gilbert)]

Attachment: Bug 969250 - Part 2: Tests for scroll snapping for scrollbars (v2 Patch), r=roc

v2 Patch:
- Now detecting OSX 10.6 and adjusting the position of synthesized mouse events to match the off-center scrollbar thumb controls.

Comment 28

[:kip (Kearwood Gilbert)]

Pushed to try to verify that the test works properly on all platforms now:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58abf516c941

Comment 29

[:kip (Kearwood Gilbert)]

Try push shows that the test should pass on 10.6 now.

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ed9d5170d13
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c1005c81e52

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Both were backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a88e14943648 under suspicion of causing mass bustage, but it turned out to be some other patch, so relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9711c8b664ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdbafbf3f35a

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9711c8b664ea
https://hg.mozilla.org/mozilla-central/rev/cdbafbf3f35a