miranda.j.emery Reporter
	Description • 11 years ago

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

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

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

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

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

	miranda.j.emery Reporter
	Comment 4 • 11 years ago

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

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

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

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

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?

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

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.

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

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.

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

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

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

v2 Patch:
- Un-bitrotted

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

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.

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

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

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

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.

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

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!

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

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.

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

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

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

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

Updated patch description for consistency.

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

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!

	Carsten Book [:Tomcat]
	Comment 23 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 24 • 9 years ago

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

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

v6 Patch:
- Un-bitrotted

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

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

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

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

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

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

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

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

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 30 • 9 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 31 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 9 years ago

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