Implement CSS scroll snapping for scrollbars

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: miranda.j.emery, Assigned: kip)

Tracking

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

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments, 15 obsolete attachments)

41.33 KB, patch
Details | Diff | Splinter Review
14.25 KB, patch
Details | Diff | Splinter Review
Extends https://bugzilla.mozilla.org/show_bug.cgi?id=945584 to cover scroll bars
Attachment #8372061 - Flags: review?(roc)
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
Attachment #8372061 - Flags: review?(roc) → review-
Attachment #8372061 - Attachment is obsolete: true
Attachment #8373090 - Flags: review?(roc)
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*.
Attachment #8373090 - Flags: review?(roc) → review-
I didn't change GetScrollbar because currently it can return either an nsScrollbarFrame* or an nsSliderFrame*
Attachment #8373090 - Attachment is obsolete: true
Attachment #8373772 - Flags: review?(roc)
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
Attachment #8373772 - Flags: review?(roc) → review-
Attachment #8373772 - Attachment is obsolete: true
Attachment #8374541 - Flags: review?(roc)
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?
Attachment #8374541 - Flags: review?(roc) → review+
Posted patch Tests (obsolete) — Splinter Review
Attachment #8375086 - Flags: review?(roc)
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.
Attachment #8375086 - Flags: review?(roc) → review-
Attachment #8374541 - Attachment is obsolete: true
Posted patch Tests (obsolete) — Splinter Review
Attachment #8375086 - Attachment is obsolete: true
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.
Assignee: miranda.j.emery → kgilbert
- Work in progress, updated to match implementation of scroll snapping in Bug 945584.
Attachment #8383468 - Attachment is obsolete: true
Blocks: 1138658
v2 Patch:
- Un-bitrotted
Attachment #8558782 - Attachment is obsolete: true
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.
Attachment #8571611 - Attachment is obsolete: true
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
Attachment #8571670 - Attachment is obsolete: true
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.
Attachment #8572237 - Attachment is obsolete: true
Attachment #8572308 - Flags: review?(roc)
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!
Attachment #8572308 - Flags: review?(roc) → review+
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.
Attachment #8383469 - Attachment is obsolete: true
Attachment #8574935 - Flags: review?(roc)
I have pushed to try, running mochitests on all platforms:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d76e74d2153
Updated patch description for consistency.
Attachment #8574935 - Attachment is obsolete: true
Attachment #8574935 - Flags: review?(roc)
Attachment #8574937 - Flags: review?(roc)
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!
Attachment #8574937 - Flags: review?(roc) → review+
Keywords: checkin-needed
v6 Patch:
- Un-bitrotted
Attachment #8572308 - Attachment is obsolete: true
V7 Patch:
- Unbitrotted
- Replaced MOZ_OVERRIDE with "override" (See Bug 1145631)
Attachment #8578855 - Attachment is obsolete: true
v2 Patch:
- Now detecting OSX 10.6 and adjusting the position of synthesized mouse events to match the off-center scrollbar thumb controls.
Attachment #8574937 - Attachment is obsolete: true
Pushed to try to verify that the test works properly on all platforms now:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58abf516c941
Try push shows that the test should pass on 10.6 now.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9711c8b664ea
https://hg.mozilla.org/mozilla-central/rev/cdbafbf3f35a
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Blocks: 1148219
You need to log in before you can comment on or make changes to this bug.