Implement CSS scroll snapping

RESOLVED FIXED in Firefox 39

Status

()

Core
Layout
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: roc, Assigned: kip)

Tracking

(Depends on: 1 bug, Blocks: 6 bugs, {dev-doc-complete})

Trunk
mozilla39
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:backlog, firefox39 fixed, relnote-firefox 39+)

Details

(URL)

Attachments

(10 attachments, 76 obsolete attachments)

1.15 KB, text/html
Details
45.27 KB, patch
Details | Diff | Splinter Review
3.58 KB, patch
Details | Diff | Splinter Review
8.65 KB, patch
Details | Diff | Splinter Review
14.14 KB, patch
Details | Diff | Splinter Review
34.93 KB, patch
Details | Diff | Splinter Review
13.42 KB, patch
Details | Diff | Splinter Review
7.00 KB, patch
Details | Diff | Splinter Review
1.51 KB, patch
Details | Diff | Splinter Review
64.50 KB, patch
Details | Diff | Splinter Review
https://wiki.mozilla.org/Gecko:CSSScrollSnapping
http://dev.w3.org/csswg/css-snappoints
We basically want to take the union of the two.

Comment 1

3 years ago
Created attachment 8342159 [details] [diff] [review]
Part 1 style support for scroll snap and scroll snap edge, work in progress
Keywords: dev-doc-needed

Comment 2

3 years ago
Created attachment 8355974 [details] [diff] [review]
Part 2 style support for scroll snap and scroll snap edge
Attachment #8342159 - Attachment is obsolete: true

Comment 3

3 years ago
Created attachment 8355975 [details] [diff] [review]
Part 2 implementation of scroll snapping
Comment on attachment 8355974 [details] [diff] [review]
Part 2 style support for scroll snap and scroll snap edge

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

::: modules/libpref/src/init/all.js
@@ +1906,5 @@
> +pref("layout.css.scroll-snap.enabled", false);
> +
> +// Set the threshold distance below which scrolling will snap to an edge, when
> +// scroll snapping is set to "proximity".
> +pref("layout.css.scroll-snap-proximity-threshold", 10000);

You should say what units this preference is measured in.

Also, this preference should go in part 2, where you actually use it.
Comment on attachment 8355975 [details] [diff] [review]
Part 2 implementation of scroll snapping

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

Let's split this patch into a patch that adds scroll-snap to ScrollbarStyles and does all the propagation etc, and a separate patch that uses those ScrollbarStyles values in nsGfxScrollFrame.

I haven't looked at all the code yet, let's fix the tabs and indenting first :-)

::: layout/base/nsCSSFrameConstructor.cpp
@@ +2306,5 @@
> +  rootStyle = styleSet->ResolveStyleFor(docElement, nullptr);
> +  if (CheckScrollSnap(presContext, rootStyle->StyleDisplay())) {
> +    // tell caller we stole the overflow style from the root element
> +    return docElement;
> +  }

Rather than doing all this, I think we should integrate this with PropagateScrollToViewport so we propagate scroll-snap from the same element that we propagate overflow from. That will simplify the code too.

::: layout/base/tests/mochitest.ini
@@ +162,5 @@
>  support-files = bug921928_event_target_iframe_apps_oop.html
>  [test_mozPaintCount.html]
>  [test_scroll_event_ordering.html]
>  [test_scroll_selection_into_view.html]
> +[test_scroll_snapping.html]

You need to "hg add" this file and then qref to get it into your patch.

::: layout/generic/nsFrame.cpp
@@ +7293,5 @@
>        // will be enough to make them keyboard scrollable.
>        nsIScrollableFrame *scrollFrame = do_QueryFrame(this);
>        if (scrollFrame &&
> +          (scrollFrame->GetScrollbarStyles().mHorizontal != NS_STYLE_OVERFLOW_HIDDEN ||
> +		   scrollFrame->GetScrollbarStyles().mVertical != NS_STYLE_OVERFLOW_HIDDEN) &&

It would be nice to add an IsHiddenInBothDirections() method to ScrollbarStyles and call it here (avoids repeating a call to GetScrollbarStyles() or having to introduce a local variable here to avoid it)

::: layout/generic/nsGfxScrollFrame.cpp
@@ +882,5 @@
>    // changed during frame life cycle without any notifications to accessibility.
>    if (mContent->IsRootOfNativeAnonymousSubtree() ||
>        GetScrollbarStyles() ==
> +        ScrollbarStyles(NS_STYLE_OVERFLOW_HIDDEN, NS_STYLE_OVERFLOW_HIDDEN,
> +		                NS_STYLE_SCROLL_SNAP_NONE, NS_STYLE_SCROLL_SNAP_NONE) ) {

Use that IsHiddenInBothDimensions here. This shouldn't depend on scroll-snapping in any way.

::: layout/generic/nsGfxScrollFrame.h
@@ +209,5 @@
>     */
>    void ScrollToRestoredPosition();
>  
> +  nsPoint GetScrollSnapPoint(nsIScrollableFrame::ScrollUnit aUnit, nsPoint aCurrPos, nsPoint aDestination,
> +	                         float* aNegTolerance, float* aPosTolerance);

Better document this in a comment here, especially the meaning of the parameters.

Comment 6

3 years ago
Created attachment 8356919 [details] [diff] [review]
Part 1 style support for scroll snap and scroll snap edge
Attachment #8355974 - Attachment is obsolete: true

Comment 7

3 years ago
Created attachment 8356921 [details] [diff] [review]
Part 2 adds scroll-snap to ScrollbarStyles

Comment 8

3 years ago
Created attachment 8356922 [details] [diff] [review]
Part 3 implementation of scroll snapping
Attachment #8355975 - Attachment is obsolete: true

Comment 9

3 years ago
Created attachment 8356923 [details] [diff] [review]
Part 4 tests
Attachment #8356919 - Flags: review?(cam)
Comment on attachment 8356921 [details] [diff] [review]
Part 2 adds scroll-snap to ScrollbarStyles

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

Looks good, just a few changes required. Request review from me for the next iteration.

::: layout/base/nsPresContext.h
@@ +629,5 @@
>    nscoord RoundAppUnitsToNearestDevPixels(nscoord aAppUnits) const
>    { return DevPixelsToAppUnits(AppUnitsToDevPixels(aAppUnits)); }
>  
> +  void SetViewportScrollbarStylesOverride(uint8_t aHorizontal, uint8_t aVertical,
> +	                                      uint8_t aScrollSnapX, uint8_t aScrollSnapY)

Let's just change this function to take a ScrollbarStyles parameter so we don't have to keep adding parameters.

@@ +1219,5 @@
>    nscolor               mFocusTextColor;
>  
>    nscolor               mBodyTextColor;
>  
> +  mozilla::ScrollbarStyles mViewportStyleScrollbar;

Add "typedef mozilla::ScrollbarStyles ScrollbarStyles;" to the top of nsPresContext's class definition so that we don't have to use mozilla:: prefixes for it.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +880,5 @@
>  {
>    // Create an accessible regardless of focusable state because the state can be
>    // changed during frame life cycle without any notifications to accessibility.
>    if (mContent->IsRootOfNativeAnonymousSubtree() ||
> +      GetScrollbarStyles().IsHiddenInBothDirections() ) {

Drop space between ) ) while you're here
Comment on attachment 8356922 [details] [diff] [review]
Part 3 implementation of scroll snapping

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2544,5 @@
>    *aLower = aPos - NSToCoordRound(aMultiplier * (aDelta > 0 ? aNegTolerance : aPosTolerance));
>    *aUpper = aPos + NSToCoordRound(aMultiplier * (aDelta > 0 ? aPosTolerance : aNegTolerance));
>  }
>  
> +class SnappingEdgeCallback {

Document this class

@@ +2545,5 @@
>    *aUpper = aPos + NSToCoordRound(aMultiplier * (aDelta > 0 ? aPosTolerance : aNegTolerance));
>  }
>  
> +class SnappingEdgeCallback {
> + 

Remove blank line

@@ +2549,5 @@
> + 
> +public:
> +  virtual void AddHorizontalEdge(nscoord aEdge) = 0;
> +  virtual void AddVerticalEdge(nscoord aEdge) = 0;
> +

Remove blank line

@@ +2552,5 @@
> +  virtual void AddVerticalEdge(nscoord aEdge) = 0;
> +
> +};
> +
> +class CalcSnapPoints : public SnappingEdgeCallback {

And document this class :-)

@@ +2555,5 @@
> +
> +class CalcSnapPoints : public SnappingEdgeCallback {
> +  nsIScrollableFrame::ScrollUnit mUnit;
> +  nsPoint mDestination;
> +  nsPoint mCurrPos;

mCurPos

@@ +2558,5 @@
> +  nsPoint mDestination;
> +  nsPoint mCurrPos;
> +  nsPoint mDistToDest;
> +  nsPoint mFinalPos;
> +  nsPoint mMinToDest;

We put fields at the bottom of the class, after a "protected". And document the fields.

@@ +2560,5 @@
> +  nsPoint mDistToDest;
> +  nsPoint mFinalPos;
> +  nsPoint mMinToDest;
> +public:
> +  CalcSnapPoints (nsIScrollableFrame::ScrollUnit,

Remove space before ( and name these parameters

@@ +2569,5 @@
> +  nsPoint GetFinalPos() { return mFinalPos; }
> +
> +};
> +
> +CalcSnapPoints::CalcSnapPoints (nsIScrollableFrame::ScrollUnit aUnit,

remove space before (

@@ +2585,5 @@
> +void
> +CalcSnapPoints::AddHorizontalEdge(nscoord aEdge)
> +{
> +  // distance to the edge from the current position (before scrolling)
> +  nscoord distToEdge = aEdge - mCurrPos.y;

Can we share code between AddHorizontalEdge and AddVerticalEdge by having a shared function that takes as parameters mCurrPos.x/y, mDestination.x/y, mDistToDest.x/y, etc? Using C++ reference parameters for the values we want to modify? I think we should!

@@ +2596,5 @@
> +      if (std::abs(diff) < mMinToDest.y || !mMinToDest.y) {
> +        mMinToDest.y = std::abs(diff);
> +        mFinalPos.y = aEdge;
> +	    }
> +	  }

Fix indent

@@ +2689,5 @@
> +  } else {
> +    NS_ERROR("Invalid scroll mode");
> +    return;
> +  }
> +

Remove blank line

@@ +2692,5 @@
> +  }
> +
> +}
> +
> +void

static void

@@ +2694,5 @@
> +}
> +
> +void
> +ScrollSnapHelper(SnappingEdgeCallback& aCallback, nsIFrame* aFrame, nsIFrame* aScrolledFrame,
> +                 nsPoint distanceToDest, nsSize scrollPortSize) {

aDistanceToDest, aScrollPortSize

Actually aDistanceToDest should just be called aDirection since it's only the direction we care about. Also, we probably want to collect both edges in some cases, so instead of this I suggest we pass in a uint32_t aFlags, and let it take bitmask flags SCROLL_SNAP_LEFT/RIGHT/TOP/BOTTOM_EDGES. Put the flags parameter last.

@@ +2782,5 @@
> +  if (scrollSnapY != NS_STYLE_SCROLL_SNAP_NONE || scrollSnapX != NS_STYLE_SCROLL_SNAP_NONE) {
> +    CalcSnapPoints calcSnapPoints (aUnit, aDestination, aCurrPos);
> +    ScrollSnapHelper(calcSnapPoints, mScrolledFrame, mScrolledFrame, distToDest, mScrollPort.Size());
> +    nsPoint finalPos = calcSnapPoints.GetFinalPos();
> +    uint32_t proximityThreshold = Preferences::GetInt("layout.css.scroll-snap-proximity-threshold",0);

Space before 0

@@ +2856,5 @@
>    }
>  
>    nsPoint newPos = mDestination +
>      nsPoint(aDelta.x*deltaMultiplier.width, aDelta.y*deltaMultiplier.height);
> +  newPos = GetScrollSnapPoint(aUnit, mDestination, newPos, &negativeTolerance, &positiveTolerance);

I think we need to update deltaMultiplier when scroll snapping is enabled, since that gets multiplied by the tolerance, and we want the tolerance to be basically 1 device pixel, not proportional to the line height or whatever. So you'd set deltaMultiplier to presContext->AppUnitsPerDevPixel() and positiveTolerance to 1.

::: modules/libpref/src/init/all.js
@@ +1902,5 @@
>  // Is support for mix-blend-mode enabled? 
>  pref("layout.css.mix-blend-mode.enabled", false);
>  
> +// Is support for scroll-snap enabled?
> +pref("layout.css.scroll-snap.enabled", false);

This really belongs in the style system patch.
Comment on attachment 8356923 [details] [diff] [review]
Part 4 tests

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

::: layout/base/tests/test_scroll_snapping.html
@@ +10,5 @@
> +
> + <script type="text/javascript">
> +   SpecialPowers.setBoolPref("general.smoothScroll", false);
> +   SpecialPowers.setBoolPref("layout.css.scroll-snap.enabled", true);
> +   SpecialPowers.setIntPref("layout.css.scroll-snap-proximity-threshold", 1000000);

use SpecialPowers.pushPrefEnv

@@ +30,5 @@
> +
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +
> +var count = 1;

call this 'step'

@@ +33,5 @@
> +
> +var count = 1;
> +var startAt = 0;
> +var expected = 200;
> +var c;

call this 'c1'

@@ +42,5 @@
> +  c.addEventListener('scroll', testScrolling);
> +  c.scrollTop = startAt;
> +  c.focus();
> +  synthesizeKey("VK_DOWN", {});
> +

Drop blank lines

@@ +122,5 @@
> +
> +function doTest() {
> +
> +  testScrollSnapping();
> +

Drop blank lines
Comment on attachment 8356919 [details] [diff] [review]
Part 1 style support for scroll snap and scroll snap edge

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

r=me with comments addressed.

::: layout/style/nsCSSKeywordList.h
@@ +360,4 @@
>  CSS_KEY(manual, manual)
>  CSS_KEY(margin-box, margin_box)
> +CSS_KEY(margin-left, margin_left)
> +CSS_KEY(margin-top, margin_top)

Looks like these two aren't used?

::: layout/style/nsCSSPropList.h
@@ +2352,5 @@
>      kBlendModeKTable,
>      CSS_PROP_NO_OFFSET,
>      eStyleAnimType_None)
>  CSS_PROP_DISPLAY(
> +    scroll-snap-y,

This should be named scroll-snap-type-x, according to the wiki page.  (Same for scroll-snap-y below.)  Then a bunch of other related things should be renamed (the nsComputedDOMStyle methods, the name of nsCSSProps::kScrollSnapKTable, etc.).

Also please put the three properties in alphabetical order.

@@ +2372,5 @@
> +    kScrollSnapKTable,
> +    CSS_PROP_NO_OFFSET,
> +    eStyleAnimType_None)
> +CSS_PROP_DISPLAY(
> +    scroll-snap-edge,

This should be scroll-snap-edges.

::: layout/style/nsCSSValue.cpp
@@ +885,5 @@
>        }
>        break;
>  
>      case eCSSProperty_paint_order:
> +	  static_assert

Revert this tab character.

::: layout/style/nsComputedDOMStyle.cpp
@@ +2457,5 @@
> +nsComputedDOMStyle::DoGetScrollSnapY()
> +{
> +  nsROCSSPrimitiveValue* val = new nsROCSSPrimitiveValue;
> +  val->SetIdent(nsCSSProps::ValueToKeywordEnum(StyleDisplay()->mScrollSnapY,
> +                nsCSSProps::kScrollSnapKTable));

This indenting is a bit odd, and it makes it look like kScrollSnapKTable is an argument to SetIdent.  Follow the indenting style used in nsComputedDOMStyle::DoGetEmptyCells for consistency.  Same for the other two DoGetScroll* methods.

@@ +2462,5 @@
> +  return val;
> +}
> +
> +CSSValue*
> +nsComputedDOMStyle::DoGetScrollSnapX()

Please put these three methods in alphabetical order.

::: layout/style/nsComputedDOMStyle.h
@@ +404,5 @@
>    mozilla::dom::CSSValue* DoGetTransformStyle();
>    mozilla::dom::CSSValue* DoGetOrient();
> +  mozilla::dom::CSSValue* DoGetScrollSnapY();
> +  mozilla::dom::CSSValue* DoGetScrollSnapX();
> +  mozilla::dom::CSSValue* DoGetScrollSnapEdge();

Please put these three methods in alphabetical order.

::: layout/style/nsComputedDOMStylePropertyList.h
@@ +178,5 @@
>  COMPUTED_STYLE_PROP(resize,                        Resize)
>  COMPUTED_STYLE_PROP(right,                         Right)
> +COMPUTED_STYLE_PROP(scroll_snap_x,                 ScrollSnapX)
> +COMPUTED_STYLE_PROP(scroll_snap_y,                 ScrollSnapY)
> +COMPUTED_STYLE_PROP(scroll_snap_edge,              ScrollSnapEdge)

Please put these three entries in alphabetical order.

::: layout/style/nsRuleNode.cpp
@@ +5052,5 @@
> +              canStoreInRuleTree,
> +              SETDSC_ENUMERATED | SETDSC_UNSET_INITIAL,
> +              parentDisplay->mScrollSnapEdge, NS_STYLE_SCROLL_SNAP_NONE,
> +              0, 0, 0, 0);
> +

Please put these three blocks in alphabetical order.

::: layout/style/nsStyleConsts.h
@@ +965,5 @@
>  #define NS_STYLE_BLEND_SATURATION                   13
>  #define NS_STYLE_BLEND_COLOR                        14
>  #define NS_STYLE_BLEND_LUMINOSITY                   15
>  
> +// scroll snap

Consistency of the comments above blocks of constants in this file isn't great, but I think it would be better to say something like:

  // See nsStyleDisplay::mScrollSnap{X,Y}

and similarly for the snap edge constants.

::: layout/style/nsStyleStruct.cpp
@@ +2379,5 @@
>    , mAnimationPlayStateCount(aSource.mAnimationPlayStateCount)
>    , mAnimationIterationCountCount(aSource.mAnimationIterationCountCount)
> +  , mScrollSnapY(aSource.mScrollSnapY)
> +  , mScrollSnapX(aSource.mScrollSnapX)
> +  , mScrollSnapEdge(aSource.mScrollSnapEdge)

Put initializers in the order they appear in the class declaration.

::: layout/style/nsStyleStruct.h
@@ +1715,5 @@
>    uint8_t mResize;              // [reset] see nsStyleConsts.h
>    uint8_t mClipFlags;           // [reset] see nsStyleConsts.h
>    uint8_t mOrient;              // [reset] see nsStyleConsts.h
>    uint8_t mMixBlendMode;        // [reset] see nsStyleConsts.h
> +  uint8_t mScrollSnapY;

Put a comment after these like

  // [reset] see nsStyleConsts.h NS_STYLE_SCROLL_SNAP_TYPE_*

::: layout/style/nsStyleUtil.h
@@ +48,5 @@
>                                      nsAString& aResult);
>  
>    static void AppendAngleValue(const nsStyleCoord& aValue, nsAString& aResult);
>  
> +  static void AppendScrollSnapEdgesValue(uint8_t aValue, nsAString& aResult);

Unnecessary?

::: layout/style/test/property_database.js
@@ +4775,5 @@
> +    invalid_values: []
> +  };
> +}
> +
> +if (SpecialPowers.getBoolPref("layout.css.scroll-snap.enabled")) {

You can put all three of these in the same getBoolPref if statement.  Also sort them.
Attachment #8356919 - Flags: review?(cam) → review+
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All

Comment 14

3 years ago
Created attachment 8358037 [details] [diff] [review]
Part 1 style support for scroll snap and scroll snap edge
Attachment #8356919 - Attachment is obsolete: true

Comment 15

3 years ago
Created attachment 8358038 [details] [diff] [review]
Part 2 adds scroll-snap to ScrollbarStyles
Attachment #8356921 - Attachment is obsolete: true
Attachment #8358038 - Flags: review?(roc)

Comment 16

3 years ago
Created attachment 8358040 [details] [diff] [review]
Part 3 implementation of scroll snapping
Attachment #8356922 - Attachment is obsolete: true

Comment 17

3 years ago
Created attachment 8358041 [details] [diff] [review]
Part 4 tests
Attachment #8356923 - Attachment is obsolete: true
Comment on attachment 8358038 [details] [diff] [review]
Part 2 adds scroll-snap to ScrollbarStyles

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

::: layout/base/nsPresContext.h
@@ +629,5 @@
>  
>    nscoord RoundAppUnitsToNearestDevPixels(nscoord aAppUnits) const
>    { return DevPixelsToAppUnits(AppUnitsToDevPixels(aAppUnits)); }
>  
> +  void SetViewportScrollbarStylesOverride(ScrollbarStyles aScrollbarStyle)

Make this "const ScrollbarStyles&" so we don't have to make a copy when the caller's struct is assigned to the parameter.
Attachment #8358038 - Flags: review?(roc) → review+

Updated

3 years ago
Attachment #8358037 - Flags: review?(cam)
Comment on attachment 8358037 [details] [diff] [review]
Part 1 style support for scroll snap and scroll snap edge

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

Didn't need to request review again from me, since I gave r+ with comments, but a couple of remaining nits below:

::: layout/style/nsRuleNode.cpp
@@ +5032,5 @@
>                SETDSC_ENUMERATED | SETDSC_UNSET_INITIAL,
>                parentDisplay->mMixBlendMode, NS_STYLE_BLEND_NORMAL,
>                0, 0, 0, 0);
>  
> +  // scroll-snap-edge: enum, inherit, initial

scroll-snap-edges

::: layout/style/nsStyleStruct.cpp
@@ +2379,5 @@
>    , mAnimationPlayStateCount(aSource.mAnimationPlayStateCount)
>    , mAnimationIterationCountCount(aSource.mAnimationIterationCountCount)
> +  , mScrollSnapEdges(aSource.mScrollSnapEdges)
> +  , mScrollSnapX(aSource.mScrollSnapX)
> +  , mScrollSnapY(aSource.mScrollSnapY)

You missed moving these up so the initializer order matches the order of the fields in the class declaration.

::: layout/style/nsStyleStruct.h
@@ +1717,5 @@
>    uint8_t mOrient;              // [reset] see nsStyleConsts.h
>    uint8_t mMixBlendMode;        // [reset] see nsStyleConsts.h
> +  uint8_t mScrollSnapEdges;     // [reset] see nsStyleConsts.h NS_STYLE_SCROLL_SNAP_EDGES_NONE
> +  uint8_t mScrollSnapX;         // [reset] see nsStyleConsts.h NS_STYLE_SCROLL_SNAP_TYPE_NONE
> +  uint8_t mScrollSnapY;         // [reset] see nsStyleConsts.h NS_STYLE_SCROLL_SNAP_TYPE_NONE

s/NONE/*/
Attachment #8358037 - Flags: review?(cam) → review+

Comment 20

3 years ago
Created attachment 8358096 [details] [diff] [review]
Part 1 style support for scroll snap and scroll snap edge
Attachment #8358037 - Attachment is obsolete: true

Comment 21

3 years ago
Created attachment 8358097 [details] [diff] [review]
Part 2 adds scroll-snap to ScrollbarStyles
Attachment #8358038 - Attachment is obsolete: true
Comment on attachment 8358040 [details] [diff] [review]
Part 3 implementation of scroll snapping

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2610,5 @@
> +{
> +  // distance to the edge from the current position (before scrolling)
> +  nscoord direction = aEdge - aCurPos;
> +  // distance to the edge from the scrolling destination
> +  nscoord diff = aEdge - aDestination;

Try making this always be positive in the direction of scrolling, by multiplying by aScrollingDirection. That may simplify the code. Same for "direction".

Then we can call this 'overshoot' or something more descriptive like that.

If aScrollingDirection == 0 we should just exit immediately.

@@ +2614,5 @@
> +  nscoord diff = aEdge - aDestination;
> +  if (mUnit == nsIScrollableFrame::DEVICE_PIXELS ||
> +      mUnit == nsIScrollableFrame::LINES) {
> +    // checks the edge is in the same direction as scrolling
> +    if (aScrollingDirection > 0 && direction > 0 || aScrollingDirection < 0 && direction < 0) {

I think this is correct but put parens around the && subexpressions to make it clear. Put a line break after the || too.

@@ +2619,5 @@
> +      if (std::abs(diff) < *aMinToDest || !*aMinToDest) {
> +        *aMinToDest = std::abs(diff);
> +        *aFinalPos = aEdge;
> +	    }
> +	  }

Fix indent

@@ +2663,5 @@
> +enum {
> +  SCROLL_LEFT,
> +  SCROLL_RIGHT,
> +  SCROLL_TOP,
> +  SCROLL_BOTTOM

Easier to just make these flags to start with. SCROLL_LEFT = 0x01 etc. Then you can lose all the shifty code.

@@ +2687,5 @@
> +              case SCROLL_LEFT: {
> +                nscoord leftEdge = f->GetOffsetTo(aScrolledFrame).x - margin.left;
> +                aCallback.AddVerticalEdge(leftEdge);
> +                break;
> +              }

These can just be a series of statements:
  if (aFlags & SCROLL_LEFT) {
  }
  if (aFlags & SCROLL_RIGHT) {
  }
etc

Factor out the call to f->GetOffsetTo(aScrolledFrame).

Another way of simplifying this code might be to have the various scrollSnapEdges cases all compute a single rectangle. Then you can have a separate chunk of code shared by all the box types that selects the right edge(s) of the rectangle.

@@ +2782,5 @@
> +                                      float* aPosTolerance)
> +{
> +  int32_t scrollSnapY = GetScrollbarStylesFromFrame().mScrollSnapY;
> +  int32_t scrollSnapX = GetScrollbarStylesFromFrame().mScrollSnapX;
> +  nsPoint direction = aDestination - aCurrPos;

We could normalize this it's either -1, 0 or 1.
Comment on attachment 8358040 [details] [diff] [review]
Part 3 implementation of scroll snapping

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2687,5 @@
> +              case SCROLL_LEFT: {
> +                nscoord leftEdge = f->GetOffsetTo(aScrolledFrame).x - margin.left;
> +                aCallback.AddVerticalEdge(leftEdge);
> +                break;
> +              }

That last trick won't work for CENTER, but you can just special-case that.
Comment on attachment 8358041 [details] [diff] [review]
Part 4 tests

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

::: layout/base/tests/test_scroll_snapping.html
@@ +32,5 @@
> +var c2;
> +
> +function testScrollSnapping() {
> +
> +  c1 = document.getElementById("c1");

remove blank line

@@ +51,5 @@
> +}
> +
> +function testScrolling() {
> +
> +  switch (step) {

remobe blank line
Attachment #8358041 - Flags: review+

Comment 25

3 years ago
Created attachment 8358158 [details] [diff] [review]
Part 3 implementation of scroll snapping
Attachment #8358040 - Attachment is obsolete: true
Attachment #8358158 - Flags: review?(roc)

Comment 26

3 years ago
Created attachment 8358160 [details] [diff] [review]
Part 4 tests

Updated

3 years ago
Attachment #8358041 - Attachment is obsolete: true
Comment on attachment 8358158 [details] [diff] [review]
Part 3 implementation of scroll snapping

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2578,5 @@
> +  nsPoint mDestination;         // gives the position after scrolling but before snapping
> +  nsPoint mCurPos;              // gives the position before scrolling
> +  nsPoint mScrollingDirection;  // always -1, 0, or 1
> +  nsPoint mFinalPos;            // keeps track of the position of the current best edge
> +  nsPoint mMinToDest;           // keeps track of the distance between mFinalPos and mDestination

How about removing this and adding a bool field that tracks whether we've found an edge yet (i.e. whether mFinalPos is valid)?

@@ +2591,5 @@
> +  mDestination = aDestination;
> +  mCurPos = aCurPos;
> +  mScrollingDirection = aScrollingDirection;
> +  mFinalPos = aDestination;
> +  mMinToDest = nsPoint(0,0);

Let's initialize this to nscoord_MAX.

@@ +2624,5 @@
> +      if (std::abs(overshoot) < *aMinToDest || !*aMinToDest) {
> +        *aMinToDest = std::abs(overshoot);
> +        *aFinalPos = aEdge;
> +	    }
> +	  }

TABS!

@@ +2669,5 @@
> +  for (; !childLists.IsDone(); childLists.Next()) {
> +    nsFrameList::Enumerator childFrames(childLists.CurrentList());
> +    for (; !childFrames.AtEnd(); childFrames.Next()) {
> +      nsIFrame* f = childFrames.get();
> +      nsMargin margin = f->GetUsedMargin();

Move this down into the EDGES_MARGIN_BOX case

@@ +2680,5 @@
> +        case NS_STYLE_SCROLL_SNAP_EDGES_MARGIN_BOX: {
> +          edgesRect = nsRect(offset.x - margin.left,
> +                             offset.y - margin.top,
> +                             frameRect.width + margin.left + margin.right - aScrollPortSize.width,
> +                             frameRect.height + margin.top + margin.bottom - aScrollPortSize.height);

I would initialize edgesRect to nsRect(offset, f->GetRect().Size()). Then here you can just write edgesRect.Inflate(margin).

Also I don't think you should subtract the scrollport size here. It gives you a rect whose size doesn't make sense. Do that later in "if (aFlags & SCROLL_RIGHT)" and "if (aFlags & SCROLL_BOTTOM)"

@@ +2693,5 @@
> +          break;
> +        }
> +        case NS_STYLE_SCROLL_SNAP_EDGES_CENTER: {
> +          edgesRect = nsRect(offset.x + frameRect.width/2 - aScrollPortSize.width/2,
> +                             offset.y + frameRect.height/2 - aScrollPortSize.height/2,

Don't subtract aScrollPortSize.width/height here. In fact, let's just call aCallback.AddVertical/HorizontalEdge right here, and wrap the calls below in "if (scrollSnapEdges == NS_STYLE_SCROLL_SNAP_EDGES_MARGIN_BOX || NS_STYLE_SCROLL_SNAP_EDGES_BORDER_BOX)"

@@ +2745,5 @@
> +  } else if (direction.y > 0) {
> +    direction.y = 1;
> +    flags |= SCROLL_BOTTOM;
> +  }
> +  if (scrollSnapY != NS_STYLE_SCROLL_SNAP_TYPE_NONE || scrollSnapX != NS_STYLE_SCROLL_SNAP_TYPE_NONE) {

Move this conditional up to just after we've set scrollSnapX/Y.

Also, invert it and return early, to avoiding indenting the rest of the function.

@@ +2750,5 @@
> +    CalcSnapPoints calcSnapPoints (aUnit, aDestination, aCurrPos, direction);
> +    ScrollSnapHelper(calcSnapPoints, mScrolledFrame, mScrolledFrame, mScrollPort.Size(), flags);
> +    nsPoint finalPos = calcSnapPoints.GetFinalPos();
> +    uint32_t proximityThreshold = Preferences::GetInt("layout.css.scroll-snap-proximity-threshold", 0);
> +    if (scrollSnapY == NS_STYLE_SCROLL_SNAP_TYPE_PROXIMITY && std::abs(aDestination.y - finalPos.y) > proximityThreshold) {

Break lines over 80 characters

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

Here too

Comment 28

3 years ago
Created attachment 8358195 [details] [diff] [review]
Part 3 implementation of scroll snapping
Attachment #8358158 - Attachment is obsolete: true
Attachment #8358158 - Flags: review?(roc)
Attachment #8358195 - Flags: review?(roc)
Comment on attachment 8358195 [details] [diff] [review]
Part 3 implementation of scroll snapping

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

Looking good. Just a few cosmetic things to fix now

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2577,5 @@
> +  nsIScrollableFrame::ScrollUnit mUnit;
> +  nsPoint mDestination;         // gives the position after scrolling but before snapping
> +  nsPoint mCurPos;              // gives the position before scrolling
> +  nsPoint mScrollingDirection;  // always -1, 0, or 1
> +  nsPoint mFinalPos;            // keeps track of the position of the current best edge

Call this mBestEdge

@@ +2603,5 @@
> +
> +void
> +CalcSnapPoints::AddVerticalEdge(nscoord aEdge)
> +{
> +  AddEdge(aEdge, mDestination.x, mCurPos.x, mScrollingDirection.x, &mFinalPos.x, &mEdgeFound);

You don't need to pass &mEdgeFound since it's the same value in both cases, you can refer to it directly in the callee

@@ +2608,5 @@
> +}
> +
> +void
> +CalcSnapPoints::AddEdge(nscoord aEdge, nscoord aDestination, nscoord aCurPos,
> +                        nscoord aScrollingDirection, nscoord* aFinalPos, bool* aEdgeFound)

aBestEdge

@@ +2675,5 @@
> +    nsFrameList::Enumerator childFrames(childLists.CurrentList());
> +    for (; !childFrames.AtEnd(); childFrames.Next()) {
> +      nsIFrame* f = childFrames.get();
> +      uint8_t scrollSnapEdges = f->StyleDisplay()->mScrollSnapEdges;
> +      if (scrollSnapEdges) {

if (scrollSnapEdges != NS_STYLE_SCROLL_SNAP_EDGES_NONE)

@@ +2697,5 @@
> +          }
> +          if (aFlags & SCROLL_BOTTOM) {
> +            aCallback.AddHorizontalEdge(edgesRect.y + edgesRect.height - aScrollPortSize.height);
> +          }
> +        } else if (scrollSnapEdges == NS_STYLE_SCROLL_SNAP_EDGES_CENTER) {

Just write "} else {" and MOZ_ASSERT(scrollSnapEdges == NS_STYLE_SCROLL_SNAP_EDGES_CENTER)

@@ +2698,5 @@
> +          if (aFlags & SCROLL_BOTTOM) {
> +            aCallback.AddHorizontalEdge(edgesRect.y + edgesRect.height - aScrollPortSize.height);
> +          }
> +        } else if (scrollSnapEdges == NS_STYLE_SCROLL_SNAP_EDGES_CENTER) {
> +          if ((aFlags & SCROLL_LEFT) || (aFlags & SCROLL_RIGHT)) {

You can write this as
  if (aFlags & (SCROLL_LEFT | SCROLL_RIGHT))

@@ +2721,5 @@
> +                                      float* aPosTolerance)
> +{
> +  int32_t scrollSnapY = GetScrollbarStylesFromFrame().mScrollSnapY;
> +  int32_t scrollSnapX = GetScrollbarStylesFromFrame().mScrollSnapX;
> +  if (scrollSnapY == NS_STYLE_SCROLL_SNAP_TYPE_NONE && scrollSnapX == NS_STYLE_SCROLL_SNAP_TYPE_NONE) {

Line probably too long
Attachment #8358195 - Flags: review?(roc) → review-

Comment 30

3 years ago
Created attachment 8358207 [details] [diff] [review]
Part 3 implementation of scroll snapping
Attachment #8358195 - Attachment is obsolete: true
Attachment #8358207 - Flags: review?(roc)
Comment on attachment 8358207 [details] [diff] [review]
Part 3 implementation of scroll snapping

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2617,5 @@
> +  nscoord direction = (aEdge - aCurPos) * aScrollingDirection;
> +  // distance to the edge from the scrolling destination in the direction of scrolling
> +  nscoord overshoot = (aEdge - aDestination) * aScrollingDirection;
> +  // distance to the current best edge from the scrolling destination in the direction of scrolling
> +  nscoord curOvershoot = (*aBestEdge - aDestination) * aScrollingDirection;

Add a check up here for direction <= 0 and bail out if so.

In fact we don't even need a variable for direction, you can just test its expression once and then we're done.

@@ +2618,5 @@
> +  // distance to the edge from the scrolling destination in the direction of scrolling
> +  nscoord overshoot = (aEdge - aDestination) * aScrollingDirection;
> +  // distance to the current best edge from the scrolling destination in the direction of scrolling
> +  nscoord curOvershoot = (*aBestEdge - aDestination) * aScrollingDirection;
> +  if (direction > 0 && !mEdgeFound) {

Remove all direction > 0 checks.
Attachment #8358207 - Flags: review?(roc) → review-

Comment 32

3 years ago
Created attachment 8358221 [details] [diff] [review]
Part 3 implementation of scroll snapping
Attachment #8358207 - Attachment is obsolete: true
Attachment #8358221 - Flags: review?(roc)
Comment on attachment 8358221 [details] [diff] [review]
Part 3 implementation of scroll snapping

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2621,5 @@
> +  nscoord curOvershoot = (*aBestEdge - aDestination) * aScrollingDirection;
> +  if (direction <= 0) {
> +    return;
> +  }
> +  if (direction > 0 && !mEdgeFound) {

Remove direction > 0 check
Attachment #8358221 - Flags: review?(roc) → review+
Blocks: 965341

Updated

3 years ago
Blocks: 969250

Updated

3 years ago
Attachment #8358096 - Flags: checkin?

Updated

3 years ago
Attachment #8358097 - Flags: checkin?

Updated

3 years ago
Attachment #8358160 - Flags: checkin?

Updated

3 years ago
Attachment #8358221 - Flags: checkin?
Comment on attachment 8358096 [details] [diff] [review]
Part 1 style support for scroll snap and scroll snap edge

Please just use the checkin-needed keyword in the future. Also, double check your commit messages as you have 2 attached here that say "Part 2" and neither is part 2 according to the attachment titles in the bug. :\
Attachment #8358096 - Flags: checkin?
Attachment #8358097 - Flags: checkin?
Attachment #8358160 - Flags: checkin?
Attachment #8358221 - Flags: checkin?
These patches are also bitrotted and in need of rebasing.

Comment 36

3 years ago
Created attachment 8382533 [details] [diff] [review]
Part 1 style support for scroll snap and scroll snap edge
Attachment #8358096 - Attachment is obsolete: true

Comment 37

3 years ago
Created attachment 8382534 [details] [diff] [review]
Part 2 adds scroll-snap to ScrollbarStyles
Attachment #8358097 - Attachment is obsolete: true

Comment 38

3 years ago
Created attachment 8382537 [details] [diff] [review]
Part 3 implementation of scroll snapping
Attachment #8358221 - Attachment is obsolete: true

Comment 39

3 years ago
Created attachment 8382538 [details] [diff] [review]
Part 4 tests
Attachment #8358160 - Attachment is obsolete: true

Updated

3 years ago
Keywords: checkin-needed

Comment 40

3 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #35)
> These patches are also bitrotted and in need of rebasing.

Sorry about that, I have updated everything
They still don't apply :(
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #41)
> They still don't apply :(

I tried to apply them. It seems like the order to apply them is: part2, then part1 (and it seems like there is some leftover from an other patch here that just need to be removed), then part3.
Can you update the patches and make sure they apply?
Flags: needinfo?(miranda.j.emery)

Comment 44

3 years ago
Hi, sorry for the delay, I'm not working at mozilla any more. I'm quite busy right now but in the weekend I will update the patches.
Flags: needinfo?(miranda.j.emery)
FWIW, even though this has not been standardized yet, I think it would be very valuable to get this landed and enabled for certified (i.e. built-in) apps for FirefoxOS. This way we can test it out and see what problems and shortcomings we run into. That would be very valuable feedback into the spec process.

And since we'd only enable it for certified apps, we wouldn't have to worry about back compat since it's easy for us to update these certified apps along with any changes to the implementation.

Comment 46

3 years ago
(In reply to miranda.j.emery from comment #44)
> … but in the weekend I will update the patches.

Hey miranda, 
are you still planning to do this? Otherwise we should reset the assignee. If you already have some updated patches that weren't uploaded yet (they don't have to work necessarily), it would be great if you can upload them nonetheless! Thanks.
Flags: needinfo?(miranda.j.emery)

Comment 47

3 years ago
Hi, really sorry that I never got around to doing this, I've been extremely busy and it completely slipped my mind. Unfortunately I'm not really going to have any free time for the foreseeable future so I'd recommend resetting the assignee. Sorry again!

(In reply to Florian Bender from comment #46)
> (In reply to miranda.j.emery from comment #44)
> > … but in the weekend I will update the patches.
> 
> Hey miranda, 
> are you still planning to do this? Otherwise we should reset the assignee.
> If you already have some updated patches that weren't uploaded yet (they
> don't have to work necessarily), it would be great if you can upload them
> nonetheless! Thanks.

Updated

3 years ago
Assignee: miranda.j.emery → nobody
This definitely deserves an intent to implement thread when someone picks it up again.  (And so excited to see this happen!)

Comment 49

3 years ago
This needs to be landed ASAP.

Updated

3 years ago
blocking-b2g: --- → 2.0+

Updated

3 years ago
blocking-b2g: 2.0+ → -
feature-b2g: --- → 2.0
I'm following up to make sure we don't have a misunderstanding about this being a 2.0 stop ship feature.
Flags: needinfo?(milan)
Because of possible iterations on this, we want to have time to do it right.  Tagging as 2.1 (Gecko 34), with the aim to do the underlying work in this bug in Gecko 33, so that we can then use it in practice/Gaia before 34 ends.  I'll follow up with Jet.
feature-b2g: 2.0 → 2.1
Flags: needinfo?(milan)

Comment 52

3 years ago
Our current plan is to implement CSSOM-View scroll-behavior (bug 1010538) to cover the Gaia use cases (eg. Home Screen.) Please review that bug to ensure that it covers the use cases you care about, as that public spec is more solid than CSS scroll snapping.
Flags: needinfo?(miranda.j.emery)
Kevin, should we duplicate this to bug 1010538, or close this one because we're doing 1010538?
Flags: needinfo?(kgrandon)
I need to look into it a bit more, but I am not convinced that we will have success in gaia if we only have the solution in bug 1010538. I could be mistaken, but I currently feel like we need both.

For return to top and dragdrop operations we need to programmatically set the scroll position, I think bug 1010538 will give us that.

For scrolling and usage of the homescreen, I think we need something more declarative - and I think that's what this bug would give us? I don't think we will have success racing APZ to calculate which section to scroll to if we only have bug 1010538.
Flags: needinfo?(kgrandon)

Updated

3 years ago
See Also: → bug 964097

Comment 55

3 years ago
This bug and bug 1010538 intend to do two separate things which are not mutually exclusive so they shouldn't be duped to each other. This does not impact product decisions on which functionality is needed more, however. So just sort out the proper flags, please.
Blocks: 1017954
(Assignee)

Updated

3 years ago
Assignee: nobody → kgilbert
(Assignee)

Comment 56

3 years ago
I am picking up where this left off.  The work in Bug 1026023 and Bug 1022825 will be integrated with this patchset to provide better interaction with mouse wheel, touchscreen, fling gesture, and keyboard scrolling events.  Additionally, this will be functioning on the compositor thread to enable the smoothest animation.
(Assignee)

Comment 57

3 years ago
Created attachment 8465111 [details] [diff] [review]
Bug 945584: Part 1 - Style support for scroll snap and scroll snap edge

Un-bitrotted patches
Attachment #8382533 - Attachment is obsolete: true
(Assignee)

Comment 58

3 years ago
Created attachment 8465112 [details] [diff] [review]
Bug 945584: Part 2 - Add scroll-snap to ScrollbarStyles

Un-bitrotted Patches
Attachment #8382534 - Attachment is obsolete: true
(Assignee)

Comment 59

3 years ago
Created attachment 8465113 [details] [diff] [review]
Bug 945584: Part 3 - Implementation of scroll snapping

Un-bitrotted Patches
Attachment #8382537 - Attachment is obsolete: true
(Assignee)

Comment 60

3 years ago
Created attachment 8465114 [details] [diff] [review]
Bug 945584: Part 4 - Tests for scroll snapping

Un-bitrotted Patches
Attachment #8382538 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8465113 - Attachment description: ug 945584: Part 3 - Implementation of scroll snapping → Bug 945584: Part 3 - Implementation of scroll snapping
(Assignee)

Comment 61

3 years ago
Created attachment 8465675 [details]
Scroll Snapping Gallery Demo

I have added a simple horizontally-scrolling scroll snapping demo.  The current patchset is working on desktop through keyboard left and right arrows.
(Assignee)

Comment 62

3 years ago
I have reviewed the existing patches; here are some initial thoughts on completing this...

The implementation affects multiple forms of user scroll gestures and input; only some have been
considered by the existing patches.

It is possible to implement snapping interaction with all of the gestures using the MSD smooth
scrolling code added in Bug 1022825, Bug 1022818, and Bug 1026023.  Using the MSD smooth scroll
function has the added benefit that the scrolls will be animated on the compositor thread for
platforms that support APZ and that the animations will be interrupted appropriately by mouse
wheel, trackpad, keyboard, and touchscreen scroll events inherently.


Drag / Fling Gestures
---------------------

Drag/Fling Option 1: Hard scroll snapping

  - When scroll snapping enabled for a scrollable rect and dragging gesture is completed (touches up),
    transition to an MSD smooth scroll as opposed to a fling animation.
  - If the velocity at the end of the drag gesture is greater than a configured threshold, the
    destination of the MSD smooth scroll will be the next allowed snapping edge in the direction of
    the drag.
  - If the velocity at the end of the drag gesture is less than the threshold, the destination of the
    MSD smooth scroll will be selected such that the minimal panning movement will be required to reach
    an allowed scroll position.

Drag/Fling Option 2: Soft scroll snapping

  - When scroll snapping enabled for a scrollable rect and dragging gesture is completed (touches up),
    transition to an MSD smooth scroll as opposed to a fling animation.
  - If the velocity at the end of the drag gesture is greater than a configured threshold, destination
    of the MSD smooth scroll with be the first snapping edge past the landing position that the fling
    gesture would have reached when its momentum animation completed.
  - If the velocity at the end of the drag gesture is less than the threshold, the destination of the
    MSD smooth scroll will be selected such that the minimal panning movement will be required to reach
    an allowed scroll position.
  - If the drag gesture ends with sufficient momentum, multiple snapping edges may be passed with one
    gesture.

Drag/Fling Option 3: Scroll Nudging

 - At the end of a dragging gesture, transition to a fling gesture as normal.
 - When the momentum of the fling gesture drops below a threshold, transition to an MSD smooth scroll.
 - The destination of the MSD smooth scroll will be the snapping edge closest to the landing position
   that the fling gesture would have reached if it would have been allowed to reach zero momentum.
 - If the drag gesture ends with sufficient momentum, multiple snapping edges may be passed with one
   gesture.

Drag/Fling Option 1 is the simplest to document and implement.
Drag/Fling Option 2 provides the most fluid feel and would work well in most general cases. (IMHO)
Drag/Fling Option 3 is closest to what was proposed for implementation in B2G / GAIA’s home screen.

Perhaps multiple options are desirable in differing cases, and should be exposed through a CSS
attribute (extend scroll-snap-type?)

Zoom Gestures
-------------

 - At the end of a zoom gesture, determine if the scroll position is no longer aligned with an
   allowed snapping edge.
 - Perform an MSD smooth scroll to align the scroll position with an allowed set of horizontal
   and vertical snapping edges.
 - The snapping edges requiring the shortest panning distance are selected for the smooth scroll.

Keyboard Events (arrow keys and page up/down keys)
--------------------------------------------------

 - Already implemented in current patch set.
 - Adjusts destination of existing main thread keyboard scroll animation to be only at allowed
   edge positions.
 - Duration and acceleration / deceleration curves of animation match normal arrow and page
   navigation animation.

Mouse Wheel and Trackpad Pan Gestures
-------------------------------------

 - Already implemented using same keyboard animations, but needs to be re-visited.
 - Momentum scrolling on OSX results in extremely rapid and uncontrollable paging between
   elements with current patchset implementation.
 - Should be implemented to behave as close as possible to the drag / fling gestures on the touch
   screen.
 - As fling gestures are processed by OSX and synthesized as incremental mouse wheel events,
   behavior similar to "Drag/Fling Option 3" may be the best solution for OSX mouse wheel events.
 - Need UX feedback to determine if other desktop platforms should also behave in this manner or
   have platform-specific behavior.  (i.e. momentum scrolling is not present or a clicky mouse
   wheel is used)
(Assignee)

Updated

3 years ago
Depends on: 1026023, 1022825, 1022818
I'm excited to see progress on this, I think we have a few places in Gaia where we are manually implementing scrolling behavior to accomplish scroll snapping - and forgoing the benefits of native scrolling in the process. I'm watching this for bug 1045784 - we're not blocked by it, but looking forward to removing code and improving user experience when it lands.

Updated

3 years ago
Blocks: 1047158
(Assignee)

Comment 64

3 years ago
(In reply to :kip (Kearwood Gilbert) from comment #62)
> I have reviewed the existing patches; here are some initial thoughts on
> completing this...
> 
> The implementation affects multiple forms of user scroll gestures and input;
> only some have been
> considered by the existing patches.
> 
> It is possible to implement snapping interaction with all of the gestures
> using the MSD smooth
> scrolling code added in Bug 1022825, Bug 1022818, and Bug 1026023.  Using
> the MSD smooth scroll
> function has the added benefit that the scrolls will be animated on the
> compositor thread for
> platforms that support APZ and that the animations will be interrupted
> appropriately by mouse
> wheel, trackpad, keyboard, and touchscreen scroll events inherently.
> 
> 
> Drag / Fling Gestures
> ---------------------
> 
> Drag/Fling Option 1: Hard scroll snapping
> 
>   - When scroll snapping enabled for a scrollable rect and dragging gesture
> is completed (touches up),
>     transition to an MSD smooth scroll as opposed to a fling animation.
>   - If the velocity at the end of the drag gesture is greater than a
> configured threshold, the
>     destination of the MSD smooth scroll will be the next allowed snapping
> edge in the direction of
>     the drag.
>   - If the velocity at the end of the drag gesture is less than the
> threshold, the destination of the
>     MSD smooth scroll will be selected such that the minimal panning
> movement will be required to reach
>     an allowed scroll position.
> 
> Drag/Fling Option 2: Soft scroll snapping
> 
>   - When scroll snapping enabled for a scrollable rect and dragging gesture
> is completed (touches up),
>     transition to an MSD smooth scroll as opposed to a fling animation.
>   - If the velocity at the end of the drag gesture is greater than a
> configured threshold, destination
>     of the MSD smooth scroll with be the first snapping edge past the
> landing position that the fling
>     gesture would have reached when its momentum animation completed.
>   - If the velocity at the end of the drag gesture is less than the
> threshold, the destination of the
>     MSD smooth scroll will be selected such that the minimal panning
> movement will be required to reach
>     an allowed scroll position.
>   - If the drag gesture ends with sufficient momentum, multiple snapping
> edges may be passed with one
>     gesture.
> 
> Drag/Fling Option 3: Scroll Nudging
> 
>  - At the end of a dragging gesture, transition to a fling gesture as normal.
>  - When the momentum of the fling gesture drops below a threshold,
> transition to an MSD smooth scroll.
>  - The destination of the MSD smooth scroll will be the snapping edge
> closest to the landing position
>    that the fling gesture would have reached if it would have been allowed
> to reach zero momentum.
>  - If the drag gesture ends with sufficient momentum, multiple snapping
> edges may be passed with one
>    gesture.
> 
> Drag/Fling Option 1 is the simplest to document and implement.
> Drag/Fling Option 2 provides the most fluid feel and would work well in most
> general cases. (IMHO)
> Drag/Fling Option 3 is closest to what was proposed for implementation in
> B2G / GAIA’s home screen.
> 
> Perhaps multiple options are desirable in differing cases, and should be
> exposed through a CSS
> attribute (extend scroll-snap-type?)
> 
> Zoom Gestures
> -------------
> 
>  - At the end of a zoom gesture, determine if the scroll position is no
> longer aligned with an
>    allowed snapping edge.
>  - Perform an MSD smooth scroll to align the scroll position with an allowed
> set of horizontal
>    and vertical snapping edges.
>  - The snapping edges requiring the shortest panning distance are selected
> for the smooth scroll.
> 
> Keyboard Events (arrow keys and page up/down keys)
> --------------------------------------------------
> 
>  - Already implemented in current patch set.
>  - Adjusts destination of existing main thread keyboard scroll animation to
> be only at allowed
>    edge positions.
>  - Duration and acceleration / deceleration curves of animation match normal
> arrow and page
>    navigation animation.
> 
> Mouse Wheel and Trackpad Pan Gestures
> -------------------------------------
> 
>  - Already implemented using same keyboard animations, but needs to be
> re-visited.
>  - Momentum scrolling on OSX results in extremely rapid and uncontrollable
> paging between
>    elements with current patchset implementation.
>  - Should be implemented to behave as close as possible to the drag / fling
> gestures on the touch
>    screen.
>  - As fling gestures are processed by OSX and synthesized as incremental
> mouse wheel events,
>    behavior similar to "Drag/Fling Option 3" may be the best solution for
> OSX mouse wheel events.
>  - Need UX feedback to determine if other desktop platforms should also
> behave in this manner or
>    have platform-specific behavior.  (i.e. momentum scrolling is not present
> or a clicky mouse
>    wheel is used)

Implementation has started, based on "Drag/Fling Option 2: Soft scroll snapping".  This option seems to be the best experience of the 3 and can be used in the most use cases.
(Assignee)

Comment 65

3 years ago
During implementation, I have identified some use cases and details on behavior in
various configurations that may be useful.  I will follow up with more details if the
final implementation differs from this behavior.

Test Use Cases:
---------------

- Must activate scroll snapping:
  - Mouse wheel up, down, left, and right [*1]
  - Trackpad scroll up, down, left, and right
  - Touchscreen drag up, down, left, and right
  - Keyboard arrow up, down, left and right
  - Keyboard home, end
  - Keyboard Page Up and Page Down
  - Scrollbar up, down, left, and right buttons clicked
  - Scrollbar up, down, left, and right buttons held down with repeat
  - Scrollbar up, down, left, and right page clicked (Between button and scroll position
    bar)
  - Scrollbar up, down, left, and right page held down with repeat
  - Scrollbar position bar dragged left, right, up, and down
  - Fling gesture with momentum sufficient to cause over scroll.  Scroll snapping should
    activate after over scroll spring back animation has completed.
  - Scroll frames with necessary CSS attributes
  
- Must not activate scroll snapping:
  - Touchscreen zoom gesture
  - Document reflow
  - Device orientation change (B2G and Fennec)
  - CSSOM View scrolling DOM methods - ScrollTo, ScrollBy, etc..
  - CSSOM View scrolling DOM methods resulting in overscroll spring back animation
  - Scrolling out of range of snapping proximity distance
  - Scroll frames without necessary CSS attributes

Test Configurations:
-------------------

In order to exercise all code paths, multiple configurations must be tested:

- On B2G with APZ enabled
- On Fennec (Fling momentum integration may not yet work on Fennec until APZ Java
  implementation moved to B2G’s native implementation)
- On Desktop with APZ (Mac) / On Desktop without APZ
- With e10s enabled / with e10s disabled
- With smooth scroll enabled and disabled
- With APZ and smooth scroll enabled, fling velocity should be integrated into target
  selection
- Without smooth scroll enabled, target selection must not be sensitive to fling
  velocity (snaps to position immediately after touch screen release)


Smooth vs Instant Scrolling:
----------------------------

- With layout.css.scroll-snap.smooth-scroll preference enabled:
  - Keyboard scrolling and clicky mouse wheel animations should perform a smooth scroll
    using their normal spline driven animations (nsGfxScrollFrame::AsyncSmoothScroll)
    with the destination adjusted to the nearest allowed snap position.
  - All other user input instantiated scrolling should trigger an MSD smooth scroll if
    the target position is not an allowed snapping point and is within the snapping
    proximity distance.
  - All MSD smooth scrolls should integrate momentum of prior MSD smooth scrolls that
    they interrupt or drag gestures that they result from.  The target position used to
    determine the nearest allowed snapping position is calculated as though a fling
    gesture with the inherited velocity was allowed to consume its momentum fully and
    come to rest.  Rather than nudging the scroll position at the end of a flin
    animation, the adjusted target position is integrated into an MSD smooth scroll that
    is triggered immediately after the user input gesture (such as a touch screen drag)
    is completed.
    
- With layout.css.scroll-snap.smooth-scroll preference disabled:
  - Keyboard scrolling should result in an instant scroll to the nearest allowed snap
    position if the “general.smoothScroll.*” preference for the scroll unit is set to
    false; otherwise, keyboard scrolling will behave as if the
    layout.css.scroll-snap.smooth-scroll preference is enabled.
  - All other user input instantiated scrolling should instantly scroll to an allowed
    scroll position.


Scroll Snapping Trigger Timing Special Cases:
---------------------------------------------

- Keyboard arrow, home, end, page up, and page down keys trigger scroll snapping
  immediately on the key down event. (May trigger multiple times due to keyboard repeat.
  Subsequent smooth scrolls will integrate velocity from interrupted events and should
  not appear janky)
  
- Touchscreen drag gesture triggers the scroll-snapping animation when all touch points
  are released.  Velocity normally passed to a fling animation is integrated into the 
  scroll snapping target position calculation and the MSD smooth scroll animation.
  
- Scroll bar left, right, up, down, page up, page down, page left, and page right handle
  clicks trigger scroll snapping immediately on the mouse down event.  (May trigger
  multiple times due to auto-repeat.  Subsequent smooth scrolls will integrate velocity
  from interrupted events and should not appear janky)
  
- Scroll bar drag handles trigger scroll snapping on mouse up events.  Unlike touch-screen
  drag gestures, the momentum of the scroll bar dragging gesture is not integrated into
  the scroll snapping target position calculation or the MSD smooth scroll animation.
  
- Over scroll spring back animation completion, only if over scroll was caused by drag
  gesture.  Must not be triggered after overscroll springback animation resulting from
  CSSOM View DOMscroll* methods.
  
- Trackpad drag gesture end (OSX synthesized velocity mouse wheel / trackpad events
  will be ignored if scroll position is already at an allowed scroll position or is
  already animating with an MSD smooth scroll towards an allowed scroll position.
  
- Effectively, OSX velocity mouse wheel / trackpad events should be completely ignored
  for scroll snapping without proximity / magnet.

[*1]  As it is not possible to differentiate between traditional mouse wheels with detents
      (clicky feeling) and smooth mouse wheel scrolling (smooth feeling wheel, trackpad
      scrolls, or capacitive touch sensor scrolling), it is assumed that mouse wheel
      events represent smooth scrolls.

Comment 66

3 years ago
Hi, Milan, it doesn't look like something we can land in 2.1. Do you think we could move this to 2.2? Thanks!
Flags: needinfo?(milan)
Right, this was a stretch for 2.1 and not tracked as committed feature, so it can go in the backlog.  As for the 2.2 - we need to actually have a Gaia feature depend on it, so something needs to get scheduled for 2.2 that needs this before we would push this as a tracked 2.2 feature.  It'll get done either way though.
blocking-b2g: - → backlog
feature-b2g: 2.1 → ---
Flags: needinfo?(milan)
This blocks bug 1047158, which we punted from 2.1. We've not done 2.2 planning yet, but my understand is that we will want the task manager redesign - and this feature - for 2.2.

Comment 69

3 years ago
(In reply to Kevin Hu [:khu] from comment #66)
> Hi, Milan, it doesn't look like something we can land in 2.1. Do you think
> we could move this to 2.2? Thanks!

Please send requests for layout feature commitment to me. feature-b2g+ flags shouldn't get set on such features before we've reviewed. Thanks!
Needsinfo peterla, who is working on homescreen and scroll snapping.
Flags: needinfo?(pla)

Comment 71

3 years ago
Hi,

This is actually something Hung has been working on already.
Flags: needinfo?(pla) → needinfo?(hnguyen)

Comment 72

3 years ago
Sorry but I went through the thread and I'm not what info is needed here?

Any clarification would be appreciated. 

Thanks
Hung
Flags: needinfo?(hnguyen)
(Assignee)

Updated

3 years ago
Depends on: 1010538
(Assignee)

Comment 73

3 years ago
Created attachment 8496133 [details] [diff] [review]
Bug 945584: Part 1 - Style support for scroll snap and scroll snap edge (v2 Patch)

Bug 945584: Part 1 - Style support for scroll snap and scroll snap edge (v2 patch)

Work in progress
Attachment #8465111 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8496133 - Attachment description: bug945584_part1.patch → Bug 945584: Part 1 - Style support for scroll snap and scroll snap edge (v2 Patch)
(Assignee)

Comment 74

3 years ago
Created attachment 8496135 [details] [diff] [review]
Bug 945584: Part 2 - Add scroll-snap to ScrollbarStyles (v2 Patch)

Bug 945584: Part 2 - Add scroll-snap to ScrollbarStyles

Work in progress
Attachment #8465112 - Attachment is obsolete: true
(Assignee)

Comment 75

3 years ago
Created attachment 8496136 [details] [diff] [review]
Bug 945584: Part 3 - Implementation of scroll snapping (v2 Patch)

Bug 945584: Part 3 - Implementation of scroll snapping

Work in progress
Attachment #8465113 - Attachment is obsolete: true
(Assignee)

Comment 76

3 years ago
Created attachment 8496138 [details] [diff] [review]
Bug 945584: Part 4 - Tests for scroll snapping (v2 Patch)

Bug 945584: Part 4 - Tests for scroll snapping

Work in progress
Attachment #8465114 - Attachment is obsolete: true
(Assignee)

Comment 77

2 years ago
Created attachment 8531656 [details] [diff] [review]
Bug 945584: Part 1 - Style support for scroll snap and scroll snap edge (v3 Patch)

V3 Patch: Un-bitrotted
Attachment #8496133 - Attachment is obsolete: true
(Assignee)

Comment 78

2 years ago
Created attachment 8531659 [details] [diff] [review]
Bug 945584: Part 2 - Add scroll-snap to ScrollbarStyles (v3 Patch)

V3 Patch:

Un-bitrotted
Attachment #8496135 - Attachment is obsolete: true
(Assignee)

Comment 79

2 years ago
Created attachment 8531661 [details] [diff] [review]
Bug 945584: Part 3 - Implementation of scroll snapping (v3 Patch)

V3 Patch:

Un-bitrotted
Attachment #8496136 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8531659 - Attachment description: Part 2 - Add scroll-snap to ScrollbarStyles (v3 Patch) → Bug 945584: Part 2 - Add scroll-snap to ScrollbarStyles (v3 Patch)
(Assignee)

Comment 80

2 years ago
Created attachment 8531662 [details] [diff] [review]
Bug 945584: Part 4 - Tests for scroll snapping (v3 Patch)

V3 Patch:

Un-bitrotted
Attachment #8496138 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
(Assignee)

Comment 81

2 years ago
Created attachment 8548479 [details] [diff] [review]
Bug 945584: Part 1 - Style support for scroll snap and scroll snap edge (v4 Patch)

V3 Patch:
- Implemented CSS support for updated W3 specification attributes - scroll-snap-type, scroll-snap-type-x, scroll-snap-type-y, scroll-snap-points-x, scroll-snap-points-y, scroll-snap-destination, and scroll-snap-coordinate
Attachment #8531656 - Attachment is obsolete: true
(Assignee)

Comment 82

2 years ago
Created attachment 8548480 [details] [diff] [review]
Bug 945584: Part 2 - Add scroll-snap to ScrollbarStyles (v4 Patch)

V4 Patch:
- Implemented support for updated W3 specification attributes - scroll-snap-type, scroll-snap-type-x, scroll-snap-type-y, scroll-snap-points-x, scroll-snap-points-y, scroll-snap-destination, and scroll-snap-coordinate
Attachment #8531659 - Attachment is obsolete: true
(Assignee)

Comment 83

2 years ago
Created attachment 8548482 [details] [diff] [review]
Bug 945584: Part 3 - Implementation of scroll snapping (v4 Patch)

V4 Patch:
- Implemented support for updated W3 specification attributes - scroll-snap-type, scroll-snap-type-x, scroll-snap-type-y, scroll-snap-destination, and scroll-snap-coordinate

- Work In progress: scroll-snap-points-x, scroll-snap-points-y
Attachment #8531661 - Attachment is obsolete: true
(Assignee)

Comment 84

2 years ago
Created attachment 8551966 [details] [diff] [review]
Bug 945584: Part 1 - Style support for scroll snap and scroll snap edge (v5 Patch)

V5 Patch:
- Unbitrotted
(Assignee)

Comment 85

2 years ago
Created attachment 8551967 [details] [diff] [review]
Bug 945584: Part 2 - Add scroll-snap to ScrollbarStyles (v5 Patch)

v5 Patch:
- Implemented scroll-snap-points-x and scroll-snap-points-y
Attachment #8548479 - Attachment is obsolete: true
Attachment #8548480 - Attachment is obsolete: true
(Assignee)

Comment 86

2 years ago
Created attachment 8551968 [details] [diff] [review]
Bug 945584: Part 3 - Implementation of scroll snapping (v5 Patch)

v5 Patch:
- Implemented scroll-snap-points-x and scroll-snap-points-y
- Main thread implementation of scroll velocity sampling corrected,
  fixing issues with incorrect snap points being selected when flinging soon
  after a prior scroll in the opposite direction.
Attachment #8548482 - Attachment is obsolete: true
(Assignee)

Comment 87

2 years ago
Created attachment 8552060 [details] [diff] [review]
Bug 945584: Part 1 - Style support for scroll snapping attributes (v6 Patch)

- Implemented style support for new attributes:
  - scroll-snap-type
  - scroll-snap-type-x
  - scroll-snap-type-y
  - scroll-snap-points-x
  - scroll-snap-points-y
  - scroll-snap-destination
  - scroll-snap-coordinate

V6 Patch:
- scroll-snap-type shorthand property is now displayed in the inspector.
- scroll-snap-type, scroll-snap-type-x, and scroll-snap-type-y no longer accept "inherit".
- scroll-snap-coordinate now accepts "none" value.
- Added additional CSS parsing values used by mochitests.
- Cleaned up formatting
- Updated patch description.
Attachment #8551966 - Attachment is obsolete: true
(Assignee)

Comment 88

2 years ago
Comment on attachment 8552060 [details] [diff] [review]
Bug 945584: Part 1 - Style support for scroll snapping attributes (v6 Patch)

Would you have some cycles to give this patch a review?
Attachment #8552060 - Flags: review?(mstange)
(Assignee)

Comment 89

2 years ago
Created attachment 8552068 [details] [diff] [review]
Bug 945584: Part 2 - Add CSS scroll snapping attributes to ScrollbarStyles (v6 Patch)

- ScrollbarStyles now carries additional variables to support new
  CSS scroll snapping attributes:
  - scroll-snap-type / scroll-snap-type-x / scroll-snap-type-y
  - scroll-snap-points-x / scroll-snap-points-y
  - scroll-snap-destination
  - (scroll-snap-coordinate does not apply to the scroll container)
- Simplified the constructor and operator== for ScrollbarStyles.

v6 Patch:
- Cleaned up formatting
- Updated patch description and title.
Attachment #8551967 - Attachment is obsolete: true
Attachment #8552068 - Flags: review?(mstange)

Updated

2 years ago
Blocks: 1115098
(Assignee)

Updated

2 years ago
Blocks: 1124324
(Assignee)

Comment 90

2 years ago
I have added Bug 1124324 to track implementation needed to guarantee snap point constraints are met when content changes.
Comment on attachment 8552068 [details] [diff] [review]
Bug 945584: Part 2 - Add CSS scroll snapping attributes to ScrollbarStyles (v6 Patch)

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

::: layout/base/nsCSSFrameConstructor.cpp
@@ +2307,5 @@
> +      aDisplay->mScrollSnapPointsX == nsStyleCoord(eStyleUnit_None) &&
> +      aDisplay->mScrollSnapPointsY == nsStyleCoord(eStyleUnit_None) &&
> +      !aDisplay->mScrollSnapDestination.mXPosition.mHasPercent &&
> +      !aDisplay->mScrollSnapDestination.mYPosition.mHasPercent &&
> +      aDisplay->mScrollSnapDestination.mXPosition.mLength == nscoord(0.0f) &&

Why not "== 0"? nscoord isn't float.
Attachment #8552068 - Flags: review?(mstange) → review+
Comment on attachment 8552060 [details] [diff] [review]
Bug 945584: Part 1 - Style support for scroll snapping attributes (v6 Patch)

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

Let's have somebody with more style system experience review this.
Attachment #8552060 - Flags: review?(mstange) → review?(cam)
Comment on attachment 8552060 [details] [diff] [review]
Bug 945584: Part 1 - Style support for scroll snapping attributes (v6 Patch)

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

::: layout/style/nsCSSParser.cpp
@@ +14837,5 @@
>  bool
> +CSSParserImpl::ParseScrollSnapType()
> +{
> +  nsCSSValue value;
> +  if (!ParseVariant(value, VARIANT_KEYWORD, nsCSSProps::kScrollSnapTypeKTable)) {

VARIANT_HK

@@ +14849,5 @@
> +bool
> +CSSParserImpl::ParseScrollSnapPoints(nsCSSValue& aValue, nsCSSProperty aPropID)
> +{
> +  nsCSSValue value;
> +  if (ParseVariant(aValue, VARIANT_NONE, nullptr)) {

VARIANT_INHERIT | VARIANT_NONE

@@ +14859,5 @@
> +  if (mToken.mType == eCSSToken_Function &&
> +      nsCSSKeywords::LookupKeyword(mToken.mIdent) == eCSSKeyword_repeat) {
> +    nsCSSValue lengthValue;
> +    if (!ParseNonNegativeVariant(lengthValue,
> +                                 VARIANT_LENGTH | VARIANT_PERCENT | VARIANT_CALC,

Can you raise an issue on the spec that it doesn't mention <percent> in the property grammar?

Report a PEExpectedNonnegativeNP if we don't get one.

@@ +14861,5 @@
> +    nsCSSValue lengthValue;
> +    if (!ParseNonNegativeVariant(lengthValue,
> +                                 VARIANT_LENGTH | VARIANT_PERCENT | VARIANT_CALC,
> +                                 nullptr)
> +        || ExpectSymbol(')', true)) {

Is this missing a "!"?  Also, "||" at the end of the previous line please.

Report a PEExpectedCloseParen if we don't find the ')' for a better error console message.

@@ +14876,5 @@
> +
> +// This function is very similar to ParseBackgroundPosition, ParseBackgroundList
> +// and ParseBackgroundSize.
> +bool
> +CSSParserImpl::ParseScrollSnapDestination(nsCSSValue& aValue)

Need to try parsing a VARIANT_INHERIT at the top of this function.

This function is parsing a list, but the spec only allows a single <position> value?

@@ +14879,5 @@
> +bool
> +CSSParserImpl::ParseScrollSnapDestination(nsCSSValue& aValue)
> +{
> +  nsCSSValue itemValue;
> +  if (!ParsePositionValue(itemValue)) {

Report PEExpectedPosition in here?

@@ +14889,5 @@
> +    if (!ExpectSymbol(',', true)) {
> +      break;
> +    }
> +    if (!ParsePositionValue(itemValue)) {
> +      return false;

Report PEExpectedPosition in here?

@@ +14900,5 @@
> +
> +bool
> +CSSParserImpl::ParseScrollSnapCoordinate(nsCSSValue& aValue)
> +{
> +  if (ParseVariant(aValue, VARIANT_NONE, nullptr)) {

VARIANT_INHERIT | VARIANT_NONE

@@ +14904,5 @@
> +  if (ParseVariant(aValue, VARIANT_NONE, nullptr)) {
> +    return true;
> +  }
> +  nsCSSValue itemValue;
> +  if (!ParsePositionValue(itemValue)) {

Report PEExpectedPosition in here?

@@ +14913,5 @@
> +    item->mValue = itemValue;
> +    if (!ExpectSymbol(',', true)) {
> +      break;
> +    }
> +    if (!ParsePositionValue(itemValue)) {

Report PEExpectedPosition in here?

::: layout/style/nsCSSPropList.h
@@ +3034,5 @@
> +    scroll_snap_type_x,
> +    ScrollSnapTypeX,
> +    CSS_PROPERTY_PARSE_VALUE,
> +    "layout.css.scroll-snap.enabled",
> +    VARIANT_KEYWORD,

This should be VARIANT_HK so that it accepts inherit/initial/unset too.

@@ +3044,5 @@
> +    scroll_snap_type_y,
> +    ScrollSnapTypeY,
> +    CSS_PROPERTY_PARSE_VALUE,
> +    "layout.css.scroll-snap.enabled",
> +    VARIANT_KEYWORD,

And here.

@@ +3058,5 @@
> +CSS_PROP_DISPLAY(
> +    scroll-snap-points-x,
> +    scroll_snap_points_x,
> +    ScrollSnapPointsX,
> +    CSS_PROPERTY_PARSE_VALUE | CSS_PROPERTY_VALUE_PARSER_FUNCTION |

Keep it to one flag per line (and in the three following property definitions).

@@ +3059,5 @@
> +    scroll-snap-points-x,
> +    scroll_snap_points_x,
> +    ScrollSnapPointsX,
> +    CSS_PROPERTY_PARSE_VALUE | CSS_PROPERTY_VALUE_PARSER_FUNCTION |
> +        CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |

::first-letter and ::first-line can't be scroll containers (since you can't set overflow on them).  We should default to not applying properties to these two pseudos unless they feel like the set of properties allowed by the last paragraph of http://www.w3.org/TR/CSS21/selector.html#first-line-pseudo so I think CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE should be removed from all of these properties.

@@ +3060,5 @@
> +    scroll_snap_points_x,
> +    ScrollSnapPointsX,
> +    CSS_PROPERTY_PARSE_VALUE | CSS_PROPERTY_VALUE_PARSER_FUNCTION |
> +        CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |
> +        CSS_PROPERTY_APPLIES_TO_PLACEHOLDER |

In general, things that are allowed on ::first-line should also be allowed on ::placeholder.  I don't think we need to support this here either.

::: layout/style/nsComputedDOMStyle.cpp
@@ +2791,5 @@
> +
> +CSSValue*
> +nsComputedDOMStyle::DoGetScrollSnapDestination()
> +{
> +  nsDOMCSSValueList *valueList = GetROCSSValueList(false);

"*" next to type.

@@ +2800,5 @@
> +CSSValue*
> +nsComputedDOMStyle::DoGetScrollSnapCoordinate()
> +{
> +  const nsStyleDisplay* sd = StyleDisplay();
> +  if (sd->mScrollSnapCoordinate.Length() == 0) {

IsEmpty() rather than Length() == 0.

@@ +2806,5 @@
> +    nsROCSSPrimitiveValue* val = new nsROCSSPrimitiveValue;
> +    val->SetIdent(eCSSKeyword_none);
> +    return val;
> +  } else {
> +    nsDOMCSSValueList *valueList = GetROCSSValueList(true);

"*" next to type.

@@ +2807,5 @@
> +    val->SetIdent(eCSSKeyword_none);
> +    return val;
> +  } else {
> +    nsDOMCSSValueList *valueList = GetROCSSValueList(true);
> +    for (uint32_t i = 0, i_end = sd->mScrollSnapCoordinate.Length(); i < i_end; ++i) {

Prefer size_t for array indexes unless there's a specific reason to use uint32_t.

@@ +2808,5 @@
> +    return val;
> +  } else {
> +    nsDOMCSSValueList *valueList = GetROCSSValueList(true);
> +    for (uint32_t i = 0, i_end = sd->mScrollSnapCoordinate.Length(); i < i_end; ++i) {
> +      nsDOMCSSValueList *itemList = GetROCSSValueList(false);

"*" next to type.

::: layout/style/nsComputedDOMStylePropertyList.h
@@ +198,5 @@
>  COMPUTED_STYLE_PROP(resize,                        Resize)
>  COMPUTED_STYLE_PROP(right,                         Right)
>  COMPUTED_STYLE_PROP(ruby_position,                 RubyPosition)
>  COMPUTED_STYLE_PROP(scroll_behavior,               ScrollBehavior)
> +COMPUTED_STYLE_PROP(scroll_snap_type,              ScrollSnapType)

I think we should hold off exposing scroll-snap-type the shorthand on computed style objects, and instead do them all at once in bug 137688.  (Some shorthands are exposed on here but they are properties that used to be longhands but were split up, such as overflow.)

::: layout/style/nsRuleNode.cpp
@@ +82,5 @@
> +static void
> +  ComputePositionValue(nsStyleContext* aStyleContext,
> +                       const nsCSSValue& aValue,
> +                       nsStyleBackground::Position& aComputedValue,
> +                        bool& aCanStoreInRuleTree);

Fix indentation.

@@ +5262,5 @@
>                SETDSC_ENUMERATED | SETDSC_UNSET_INITIAL,
>                parentDisplay->mScrollBehavior, NS_STYLE_SCROLL_BEHAVIOR_AUTO,
>                0, 0, 0, 0);
>  
> +  // scroll-snap-type-x: none, enum, initial

You can mention "inherit" in these comments too.

@@ +5278,5 @@
> +              0, 0, 0, 0);
> +
> +  // scroll-snap-points-x: none, initial
> +  const nsCSSValue& scrollSnapPointsX = *aRuleData->ValueForScrollSnapPointsX();
> +  switch (scrollSnapPointsX.GetUnit()) {

You need to handle inherit in this switch statement.

@@ +5291,5 @@
> +      NS_ASSERTION(func->Item(0).GetKeywordValue() == eCSSKeyword_repeat,
> +                   "Expected repeat(), got another function name");
> +      nsStyleCoord coord;
> +      if (SetCoord(func->Item(1), coord, nsStyleCoord(),
> +                            SETCOORD_LPO | SETCOORD_CALC_LENGTH_ONLY |

Just SETCOORD_LP; we won't have repeat(none).

Fix indentation.

Why SETCOORD_CALC_LENGTH_ONLY?  I think we should be using SETCOORD_STORE_CALC so that we can later use nsRuleNode::ComputeCoordPercentCalc to resolve it against the scroll container padding box.

@@ +5295,5 @@
> +                            SETCOORD_LPO | SETCOORD_CALC_LENGTH_ONLY |
> +                            SETCOORD_CALC_CLAMP_NONNEGATIVE,
> +                            aContext, mPresContext, canStoreInRuleTree)) {
> +        NS_ASSERTION(coord.GetUnit() == eStyleUnit_Coord
> +                     || coord.GetUnit() == eStyleUnit_Percent,

"||" on previous line.

@@ +5336,5 @@
> +  }
> +
> +  // scroll-snap-destination: initial
> +  const nsCSSValue& snapDestination = *aRuleData->ValueForScrollSnapDestination();
> +  switch (snapDestination.GetUnit()) {

Handle inherit.

@@ +5360,5 @@
> +  }
> +
> +  // scroll-snap-coordinate: none, initial
> +  typedef nsStyleBackground::Position Position;
> +  nsTArray<Position> snapCoordinateArray;

No need for this extra array and then having to copy its values.  Just refer to display->mScrollSnapCoordinate in the eCSSUnit_List case.

::: layout/style/nsStyleStruct.cpp
@@ +2606,5 @@
> +  mScrollSnapPointsX.SetNoneValue();
> +  mScrollSnapPointsY.SetNoneValue();
> +  // Initial value for mScrollSnapDestination is "0px 0px"
> +  mScrollSnapDestination.SetInitialLengthValues(nscoord(0.0f));
> +  mScrollSnapCoordinate.Clear();

The Clear() is not needed.

@@ +2713,1 @@
>        || mResize != aOther.mResize)

ReconstructFrame seems heavy.  Where do we handle the scroll container properties, and where do we handle scroll-snap-coordinate?

::: layout/style/nsStyleStruct.h
@@ +391,5 @@
>      void SetInitialPercentValues(float aPercentVal);
>  
> +    // Sets both mXPosition and mYPosition to the given length value for the
> +    // initial property-value (e.g. 0.0f for "0 0", or 10 for "10 10")
> +    void SetInitialLengthValues(nscoord aLengthVal);

I can't see us needing nscoord values other than 0, so maybe just make SetInitialZeroValues().  And note that 10 would mean 10 app units.

::: layout/style/test/property_database.js
@@ +6399,5 @@
> +    domProp: "scrollSnapPointsX",
> +    inherited: false,
> +    type: CSS_TYPE_LONGHAND,
> +    initial_values: [ "none" ],
> +    other_values: [ "repeat(100%)", "repeat(120px), repeat(calc(3*25px))" ],

The third value should be invalid, yes?
Attachment #8552060 - Flags: review?(cam)
(Assignee)

Comment 94

2 years ago
(In reply to Cameron McCormack (:heycam) from comment #93)
> 
> @@ +14859,5 @@
> > +  if (mToken.mType == eCSSToken_Function &&
> > +      nsCSSKeywords::LookupKeyword(mToken.mIdent) == eCSSKeyword_repeat) {
> > +    nsCSSValue lengthValue;
> > +    if (!ParseNonNegativeVariant(lengthValue,
> > +                                 VARIANT_LENGTH | VARIANT_PERCENT | VARIANT_CALC,
> 
> Can you raise an issue on the spec that it doesn't mention <percent> in the
> property grammar?
> 
I have raised this issue with a w3.org bug:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=27882

Thanks for your quick review.  I am applying your feedback and will send an update patch shortly.
(Assignee)

Comment 95

2 years ago
Created attachment 8554034 [details] [diff] [review]
Bug 945584: Part 1 - Style support for scroll snapping attributes (v7 Patch)

- Implemented style support for new attributes:
  - scroll-snap-type
  - scroll-snap-type-x
  - scroll-snap-type-y
  - scroll-snap-points-x
  - scroll-snap-points-y
  - scroll-snap-destination
  - scroll-snap-coordinate

V7 Patch:
- Updated with feedback from review in Comment 93
Attachment #8552060 - Attachment is obsolete: true
Attachment #8554034 - Flags: review?(cam)
(Assignee)

Comment 96

2 years ago
(In reply to Cameron McCormack (:heycam) from comment #93)
> 
> @@ +2713,1 @@
> >        || mResize != aOther.mResize)
> 
> ReconstructFrame seems heavy.  Where do we handle the scroll container
> properties, and where do we handle scroll-snap-coordinate?
> 

Thanks again for your detailed review.

ReconstructFrame is being used in order to ensure that nsCSSFrameConstructor::PropagateScrollToViewport is called to update the values stored in ScrollbarStyles.  As scroll-snap-coordinate is not being stored in there with the rest of the scroll-behavior and scroll-snap values, I have removed mScrollSnapCoordinate from the condition.  In the updated patch I have assigned for review, I have updated the comment below the if statement to match:

  /* Note: When mScrollBehavior, mScrollSnapTypeX, mScrollSnapTypeY,
   * mScrollSnapPointsX, mScrollSnapPointsY, or mScrollSnapDestination are
   * changed, nsChangeHint_NeutralChange is not sufficient to enter
   * nsCSSFrameConstructor::PropagateScrollToViewport. By using the same hint
   * as used when the overflow css property changes,
   * nsChangeHint_ReconstructFrame, PropagateScrollToViewport will be called.
   *
   * The scroll-behavior css property is not expected to change often (the
   * CSSOM-View DOM methods are likely to be used in those cases); however,
   * if this does become common perhaps a faster-path might be worth while.
   */

Would it be appropriate to file a bug to track this for later improvement, also applying to mScrollBehavior?
(Assignee)

Comment 97

2 years ago
Created attachment 8554041 [details] [diff] [review]
Bug 945584: Part 2 - Add CSS scroll snapping attributes to ScrollbarStyles (v7 Patch),r=mstange

- ScrollbarStyles now carries additional variables to support new
  CSS scroll snapping attributes:
  - scroll-snap-type / scroll-snap-type-x / scroll-snap-type-y
  - scroll-snap-points-x / scroll-snap-points-y
  - scroll-snap-destination
  - (scroll-snap-coordinate does not apply to the scroll container)
- Simplified the constructor and operator== for ScrollbarStyles.

v7 Patch:
- Added correction in Comment 91, reflecting that nscoords will always be
  integers.
Attachment #8552068 - Attachment is obsolete: true
(Assignee)

Comment 98

2 years ago
Created attachment 8555521 [details] [diff] [review]
Bug 945584: Part 3 - Implementation of scroll snapping (v6 Patch)

- Implemented CSS scroll snapping (http://dev.w3.org/csswg/css-snappoints/)
- Mouse wheel events synthesized by OSX for momentum scrolling can now
be interrupted by DOM triggered and CSS scroll snapping triggered scroll
events for consistent behavior with the scrolling and fling gestures
in the APZC.
- Added preferences to allow trackpad and mousewheel flinging between
snap points to be tuned:
- layout.css.scroll-snap.prediction-max-velocity
- layout.css.scroll-snap.prediction-sensitivity

V6 Patch:
- Implemented prefs to replace hard coded constants that may need tuning
- Cleaned up formatting and added comments
- scroll-snap-points-{x|y} now works correctly when combined with scroll-snap-destination
- Fixed issues with nsGfxScrollFrame::GetSnapPoints found while writing tests


:heycam - Would you have some cycles to review this patch as well?
Attachment #8551968 - Attachment is obsolete: true
Attachment #8555521 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8555521 - Attachment description: Part 3 - Implementation of scroll snapping (v6 Patch) → Bug 945584: Part 3 - Implementation of scroll snapping (v6 Patch)
(In reply to :kip (Kearwood Gilbert) from comment #96)
> ReconstructFrame is being used in order to ensure that
> nsCSSFrameConstructor::PropagateScrollToViewport is called to update the
> values stored in ScrollbarStyles.  As scroll-snap-coordinate is not being
> stored in there with the rest of the scroll-behavior and scroll-snap values,
> I have removed mScrollSnapCoordinate from the condition.  In the updated
> patch I have assigned for review, I have updated the comment below the if
> statement to match:

I'm not sure what you mean by scroll-snap-coordinate not being stored there.  If changing scroll-snap-coordinate does not need any processing behaviour, then return nsChangeHint_NeutralChange for it.  If it does, then whatever change hint is required (is it ReconstructFrame, like the others?).

>   /* Note: When mScrollBehavior, mScrollSnapTypeX, mScrollSnapTypeY,
>    * mScrollSnapPointsX, mScrollSnapPointsY, or mScrollSnapDestination are
>    * changed, nsChangeHint_NeutralChange is not sufficient to enter
>    * nsCSSFrameConstructor::PropagateScrollToViewport. By using the same hint
>    * as used when the overflow css property changes,
>    * nsChangeHint_ReconstructFrame, PropagateScrollToViewport will be called.
>    *
>    * The scroll-behavior css property is not expected to change often (the
>    * CSSOM-View DOM methods are likely to be used in those cases); however,
>    * if this does become common perhaps a faster-path might be worth while.
>    */
> 
> Would it be appropriate to file a bug to track this for later improvement,
> also applying to mScrollBehavior?

Yes I think that's fine.
Comment on attachment 8554034 [details] [diff] [review]
Bug 945584: Part 1 - Style support for scroll snapping attributes (v7 Patch)

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

r=me with comment 99 addressed.
Attachment #8554034 - Flags: review?(cam) → review+
Comment on attachment 8555521 [details] [diff] [review]
Bug 945584: Part 3 - Implementation of scroll snapping (v6 Patch)

(In reply to :kip (Kearwood Gilbert) from comment #98)
> :heycam - Would you have some cycles to review this patch as well?

Sorry, someone more familiar with layout/gfx stuff should look at part.
Attachment #8555521 - Flags: review?(cam) → review?(roc)
(Assignee)

Comment 102

2 years ago
(In reply to Cameron McCormack (:heycam) from comment #99)
> (In reply to :kip (Kearwood Gilbert) from comment #96)
> > ReconstructFrame is being used in order to ensure that
> > nsCSSFrameConstructor::PropagateScrollToViewport is called to update the
> > values stored in ScrollbarStyles.  As scroll-snap-coordinate is not being
> > stored in there with the rest of the scroll-behavior and scroll-snap values,
> > I have removed mScrollSnapCoordinate from the condition.  In the updated
> > patch I have assigned for review, I have updated the comment below the if
> > statement to match:
> 
> I'm not sure what you mean by scroll-snap-coordinate not being stored there.
> If changing scroll-snap-coordinate does not need any processing behaviour,
> then return nsChangeHint_NeutralChange for it.  If it does, then whatever
> change hint is required (is it ReconstructFrame, like the others?).
The value of scroll-snap-coordinate is not stored in the ScrollBarStyles class like the others.  Changing its value does not have an immediate effect on layout, but is accessed when calculating snap points during scrolling.  I have updated it to return nsChangeHint_NeutralChange.
> 
> >   /* Note: When mScrollBehavior, mScrollSnapTypeX, mScrollSnapTypeY,
> >    * mScrollSnapPointsX, mScrollSnapPointsY, or mScrollSnapDestination are
> >    * changed, nsChangeHint_NeutralChange is not sufficient to enter
> >    * nsCSSFrameConstructor::PropagateScrollToViewport. By using the same hint
> >    * as used when the overflow css property changes,
> >    * nsChangeHint_ReconstructFrame, PropagateScrollToViewport will be called.
> >    *
> >    * The scroll-behavior css property is not expected to change often (the
> >    * CSSOM-View DOM methods are likely to be used in those cases); however,
> >    * if this does become common perhaps a faster-path might be worth while.
> >    */
> > 
> > Would it be appropriate to file a bug to track this for later improvement,
> > also applying to mScrollBehavior?
> 
> Yes I think that's fine.

I have added a Bug 1128102 to track the optimization.
(Assignee)

Comment 103

2 years ago
Created attachment 8557390 [details] [diff] [review]
Bug 945584: Part 1 - Style support for scroll snapping attributes (v8 Patch), r=cam

- Implemented style support for new attributes:
- scroll-snap-type
- scroll-snap-type-x
- scroll-snap-type-y
- scroll-snap-points-x
- scroll-snap-points-y
- scroll-snap-destination
- scroll-snap-coordinate

v8 Patch:
- scroll-snap-coordinate changes now result in nsChangeHint_nsChangeHint_NeutralChange
Attachment #8554034 - Attachment is obsolete: true
(Assignee)

Comment 104

2 years ago
Created attachment 8557399 [details] [diff] [review]
Bug 945584: Part 3 - Implementation of scroll snapping (v7 Patch)

- Implemented CSS scroll snapping (http://dev.w3.org/csswg/css-snappoints/)
- Mouse wheel events synthesized by OSX for momentum scrolling can now
  be interrupted by DOM triggered and CSS scroll snapping triggered scroll
  events for consistent behavior with the scrolling and fling gestures
  in the APZC.
- Added preferences to allow trackpad and mousewheel flinging between
  snap points to be tuned:
  - layout.css.scroll-snap.prediction-max-velocity
  - layout.css.scroll-snap.prediction-sensitivity

v7 Patch:

- Renamed preference from scroll-snap-proximity-threshold to scroll-snap.proximity-threshold
Attachment #8555521 - Attachment is obsolete: true
Attachment #8555521 - Flags: review?(roc)
Attachment #8557399 - Flags: review?(roc)
Comment on attachment 8557399 [details] [diff] [review]
Bug 945584: Part 3 - Implementation of scroll snapping (v7 Patch)

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

this generally looks great.

::: gfx/layers/FrameMetrics.h
@@ +653,5 @@
>  
> +  // mFlingSnapGeneration is used to signal a request to perform CSS scroll
> +  // snapping by recording the scroll generation of the last request.
> +  // A value of 0xffffffff indicates that no request has ever been made.
> +  uint32_t mFlingSnapGeneration;

How about we make the "no request has ever been made" value 0? That seems less surprising.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +3467,5 @@
> +  }
> +}
> +
> +void
> +ScrollFrameHelper::SampleVelocityQueue()

This is a bit complicated. A very precise comment about what this does (in nsGfxScrollFrame.h) would help.

@@ +5570,5 @@
> +
> +    // Clamp between the minimum and maximum values
> +    if (edge > maxEdge) {
> +      edge = maxEdge;
> +    } else if(edge < minEdge) {

space before (

@@ +5605,5 @@
> +
> +    // Clamp between the minimum and maximum values
> +    if (edge > maxEdge) {
> +      edge = maxEdge;
> +    } else if(edge < minEdge) {

here too

::: layout/generic/nsGfxScrollFrame.h
@@ +512,5 @@
> +  // GetScrollVelocity until move scroll position updates.
> +  void ResetVelocityQueue();
> +
> +  // Get scroll velocity averaged from recent movement, in appunits / ms
> +  nsPoint GetScrollVelocity();

I think it might be worth separating out this controller stuf, and most of the new code, into its own object.

::: layout/generic/nsIScrollableFrame.h
@@ +263,5 @@
>    virtual void ScrollBy(nsIntPoint aDelta, ScrollUnit aUnit, ScrollMode aMode,
>                          nsIntPoint* aOverflow = nullptr,
>                          nsIAtom* aOrigin = nullptr,
> +                        bool aIsMomentum = false,
> +                        bool aSnap = false) = 0;

Let's switch to a flags word now to avoid unreadable call sites. Or separate enums if you prefer.

@@ +273,5 @@
> +   * snap point, allowing touchscreen fling gestures to navigate between
> +   * snap points.
> +   */
> +  virtual void FlingSnap(const mozilla::CSSPoint& aDestination,
> +                         uint32_t aFlingSnapGeneration) = 0;

Please document aFlingSnapGeneration

::: modules/libpref/init/all.js
@@ +2092,5 @@
> +// When selecting the snap point for CSS scroll snapping, the velocity of the
> +// scroll frame is clamped to this speed, in app units / ms.
> +pref("layout.css.scroll-snap.prediction-max-velocity", 250000);
> +
> +// When selecting hte snap point for CSS scroll snapping, the velocity of the

"the"
Attachment #8557399 - Flags: review?(roc)
(Assignee)

Comment 106

2 years ago
Created attachment 8558788 [details] [diff] [review]
Bug 945584: Part 3 - Implementation of scroll snapping (v8 Patch)

- Implemented CSS scroll snapping (http://dev.w3.org/csswg/css-snappoints/)
- Mouse wheel events synthesized by OSX for momentum scrolling can now
be interrupted by DOM triggered and CSS scroll snapping triggered scroll
events for consistent behavior with the scrolling and fling gestures
in the APZC.
- Added preferences to allow trackpad and mousewheel flinging between
snap points to be tuned:
- layout.css.scroll-snap.prediction-max-velocity
- layout.css.scroll-snap.prediction-sensitivity

v8 Patch:
- Adjustments from Roc's review in Comment 105 in progress
- Implemented scroll snapping at the end of a middle-mouse-button autoscroll.  As this scrolling occurs within chrome Javascript, chrome-only DOM methods had to be added: window.MozScrollSnap and element.MozScrollSnap
- Fixed broken scroll snapping on click-wheel mice.  It now selects the scroll snap points using the same logic as arrow key scroll snapping, while preserving the normal mouse wheel scrolling distance when no snapping occurs.
- Fixed assertion that would occur when the first scroll of a scroll frame results in scroll snapping.
Attachment #8557399 - Attachment is obsolete: true
(Assignee)

Comment 107

2 years ago
I am in progress of updating the Part 4 patch to match the requirements of the updated specification.

Here are some test cases I am adding:

- Proximity + Within proximity => Snaps to point

- Proximity + Beyond proximity => Does not snap to point

- Mandatory + Beyond proximity => Snaps to point

- Scroll Snapping Enabled + No snap points => Does not snap or scroll

- Mandatory + scroll-snap-points-{x|y} + scroll-snap-destination + Start of page + No snap point at start of page => Does not snap to top of page
  - Keyboard Up Arrow
  - Keyboard Page Up
  - Keyboard Home
  - Mousewheel Up
  - Scrollbar up button
  - Scrollbar drag handle up
  - Scrollbar page up

- Mandatory + scroll-snap-points-{x|y} + negative scroll-snap-destination + End of page + No snap point at end of page => Does not snap to end of page
  - Keyboard Down Arrow
  - Keyboard Page Down
  - Keyboard End
  - Mousewheel down button
  - Scrollbar drag handle down
  - Scrollbar page down

- Mandatory + scroll-snap-coordinate + Start of page + No snap point at start of page => Does not snap to top of page
  - Keyboard Up Arrow
  - Keyboard Page Up
  - Keyboard Home
  - Mousewheel Up
  - Scrollbar up button
  - Scrollbar drag handle up
  - Scrollbar page up

- Mandatory + scroll-snap-coordinate + End of page + No snap point at end of page => Does not snap to end of page
  - Keyboard Down Arrow
  - Keyboard Page Down
  - Keyboard End
  - Mousewheel down button
  - Scrollbar drag handle down
  - Scrollbar page down


- Mandatory + scroll-snap-points-{x|y} greater than scrolling rect + positive non-zero scroll-snap-destination => No snap points, does not scroll or snap

- OSX Momentum scrolling is interrupted by scroll snapping and will not "fight" snapping force
Mandatory + scroll-snap-points-{x|y} + scroll-snap-coordinate with two points => Interval and element snapping points are combined. (Test 1 snap point generated by scroll-snap-points-{x|y} and two generated by scroll-snap-coordinate, ensure that they are evaluated as snap points)

- Ensure that NS_WHEEL_STOP triggers scroll snapping (Trackpad touches lifted up)

- Ensure that end of drag / fling on touch screen triggers scroll snapping

- Ensure that momentum of trackpad fling gestures contributes to snap point selection

- Ensure that momentum of touch screen fling gestures contributes to snap point selection

- Ensure that when paging down/up/left/right that the next snap point in the direction of the scroll is selected rather than a closer point in the opposite direction

- Ensure that when paging down/up/left/right that the farthest snap point before the destination is selected and prioritized over a snap point that is past the destination, even if the snap point past the destination is closer to the destination.  Setup - two snap points between current position and destination and one snap point past the destination which is closer than any of the other points.

- Ensure that when paging down/up/left/right that the closest snap point past the destination is selected when no snap points exist between the starting position and the destination.  Additionally, a snap point closer to the destination than the one past the snap point, but not in the scrolling direction, must not be selected.  Setup - Two snap points beyond the destination and one snap point in the opposite direction of scrolling which is closest to the destination.

- Ensure that when scrolling by lines up,down,left,or right, that the closest snap point to the destination in the direction of travel is selected.  Setup - Two snap points in the direction of travel and one in the opposite direction.  Snap point in opposite direction is closest to the destination but must not be selected.
(Assignee)

Comment 108

2 years ago
Created attachment 8559520 [details] [diff] [review]
Bug 945584: Part 3 - Implementation of scroll snapping (v9 Patch)

- Implemented CSS scroll snapping (http://dev.w3.org/csswg/css-snappoints/)
- Mouse wheel events synthesized by OSX for momentum scrolling can now
  be interrupted by DOM triggered and CSS scroll snapping triggered scroll
  events for consistent behavior with the scrolling and fling gestures
  in the APZC.
- Added preferences to allow trackpad and mousewheel flinging between
  snap points to be tuned:
  - layout.css.scroll-snap.prediction-max-velocity
  - layout.css.scroll-snap.prediction-sensitivity

v9 Patch:
- Updated with changes from Roc's review in Comment 105
Attachment #8558788 - Attachment is obsolete: true
Attachment #8559520 - Flags: review?(roc)
(Assignee)

Comment 109

2 years ago
Created attachment 8562464 [details] [diff] [review]
Bug 945584: Part 4 - Tests for scroll snapping (v4 Patch)

V4 Patch:
- Implemented testcases applicable to the new CSS scroll snapping specification.
- Test cases are now stored in an array to be more easily manageable
- Tests now wait for scrolling to stop rather than force smooth scrolling to be disabled.
Attachment #8531662 - Attachment is obsolete: true
(Assignee)

Comment 110

2 years ago
Pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=82bb52fa8e39
Comment on attachment 8559520 [details] [diff] [review]
Bug 945584: Part 3 - Implementation of scroll snapping (v9 Patch)

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

Sorry about the delay. If I don't reply within a few days, please ping me.

Can we break this patch up a bit? Looks like the new IDL interfaces and the browser-content.js change can be broken out into their own patch. Maybe the APZ flinging changes can be broken out into their own patch at as well? Could we move the new VelocityQueue object to its own patch (and file!)?
Attachment #8559520 - Flags: review?(roc)
The following example appears to only work only if apz is disable:

```
<main>
  <h1>foobar</h1>
  <div>content</div>
</main>

body, h1, div {
  margin: 0;
}

main {
  height: 400px;
  background-color: white;
  overflow: scroll;
  scroll-snap-type: mandatory;
  scroll-snap-destination: 0 0;
}

h1 {
  height: 50px;
  scroll-snap-coordinate: 0 0;
}

div {
  height: 400px;
  background-color: #FEE;
  scroll-snap-coordinate: 0 0;
}
```
Blocks: 1133666
(Assignee)

Comment 113

2 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #111)
> Comment on attachment 8559520 [details] [diff] [review]
> Bug 945584: Part 3 - Implementation of scroll snapping (v9 Patch)
> 
> Review of attachment 8559520 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry about the delay. If I don't reply within a few days, please ping me.
> 
> Can we break this patch up a bit? Looks like the new IDL interfaces and the
> browser-content.js change can be broken out into their own patch. Maybe the
> APZ flinging changes can be broken out into their own patch at as well?
> Could we move the new VelocityQueue object to its own patch (and file!)?

Thanks Roc,

I'll split it up and get this back to you.
(Assignee)

Comment 114

2 years ago
Created attachment 8566160 [details] [diff] [review]
Bug 945584: Part 1 - Style support for scroll snapping attributes (v9 Patch), r=cam

Bug 945584: Part 1 - Style support for scroll snapping attributes, r=cam
- Implemented style support for new attributes:
  - scroll-snap-type
  - scroll-snap-type-x
  - scroll-snap-type-y
  - scroll-snap-points-x
  - scroll-snap-points-y
  - scroll-snap-destination
  - scroll-snap-coordinate

v9 Patch:
- Un-bitrotted
Attachment #8557390 - Attachment is obsolete: true
(Assignee)

Comment 115

2 years ago
Created attachment 8566218 [details] [diff] [review]
Bug 945584: Part 2 - Add CSS scroll snapping attributes to ScrollbarStyles (v8 Patch),r=mstange

- ScrollbarStyles now carries additional variables to support new
  CSS scroll snapping attributes:
  - scroll-snap-type / scroll-snap-type-x / scroll-snap-type-y
  - scroll-snap-points-x / scroll-snap-points-y
  - scroll-snap-destination
  - (scroll-snap-coordinate does not apply to the scroll container)
- Simplified the constructor and operator== for ScrollbarStyles.

v8 Patch:
- Un-bitrotted
- Fixed unified build failure
- Corrected "bad implicit conversion constructor" warning in ScrollbarStyles.h
Attachment #8554041 - Attachment is obsolete: true
(Assignee)

Comment 116

2 years ago
Created attachment 8566891 [details] [diff] [review]
Bug 945584: Part 1 - Style support for scroll snapping attributes (v10 Patch), r=cam

Bug 945584: Part 1 - Style support for scroll snapping attributes, r=cam
- Implemented style support for new attributes:
  - scroll-snap-type
  - scroll-snap-type-x
  - scroll-snap-type-y
  - scroll-snap-points-x
  - scroll-snap-points-y
  - scroll-snap-destination
  - scroll-snap-coordinate

v10 Patch:

- Moved layout.css.scroll-snap-proximity-threshold preference to later patch.
Attachment #8566160 - Attachment is obsolete: true
(Assignee)

Comment 117

2 years ago
Created attachment 8566893 [details] [diff] [review]
Bug 945584: Part 1 - Style support for scroll snapping attributes (v11 Patch), r=cam

Bug 945584: Part 1 - Style support for scroll snapping attributes
- Implemented style support for new attributes:
  - scroll-snap-type
  - scroll-snap-type-x
  - scroll-snap-type-y
  - scroll-snap-points-x
  - scroll-snap-points-y
  - scroll-snap-destination
  - scroll-snap-coordinate

v11 Patch:
- Updated patch user name
Attachment #8566891 - Attachment is obsolete: true
(Assignee)

Comment 118

2 years ago
Created attachment 8566894 [details] [diff] [review]
Bug 945584: Part 2 - Add CSS scroll snapping attributes to ScrollbarStyles (v9 Patch),r=mstange

- ScrollbarStyles now carries additional variables to support new
  CSS scroll snapping attributes:
  - scroll-snap-type / scroll-snap-type-x / scroll-snap-type-y
  - scroll-snap-points-x / scroll-snap-points-y
  - scroll-snap-destination
  - (scroll-snap-coordinate does not apply to the scroll container)
- Simplified the constructor and operator== for ScrollbarStyles.

v9 Patch:
- Updated patch user name
Attachment #8566218 - Attachment is obsolete: true
(Assignee)

Comment 119

2 years ago
Created attachment 8566898 [details] [diff] [review]
Bug 945584: Part 3 - Enable cancellation of OSX synthesized mousewheel scrolling events

Bug 945584: Part 3 - Enable cancellation of OSX synthesized mousewheel scrolling events
- Mouse wheel events synthesized by OSX for momentum scrolling can now
be interrupted by DOM triggered and CSS scroll snapping triggered scroll
events for consistent behavior with the scrolling and fling gestures
in the APZC.
Attachment #8559520 - Attachment is obsolete: true
Attachment #8566898 - Flags: review?(roc)
(Assignee)

Comment 120

2 years ago
Created attachment 8566899 [details] [diff] [review]
Bug 945584: Part 4 - Add scroll snapping preferences

- Added preferences to allow trackpad and mousewheel flinging between
snap points to be tuned:
- layout.css.scroll-snap.prediction-max-velocity
- layout.css.scroll-snap.prediction-sensitivity
Attachment #8562464 - Attachment is obsolete: true
Attachment #8566899 - Flags: review?(roc)
(Assignee)

Comment 121

2 years ago
Created attachment 8566900 [details] [diff] [review]
Bug 945584: Part 5 - Implement ScrollVelocityQueue

- Implemented ScrollVelocityQueue class to calculate the velocity of a scroll
when given periodic samples of scroll position.
Attachment #8566900 - Flags: review?(roc)
(Assignee)

Comment 122

2 years ago
Created attachment 8566903 [details] [diff] [review]
Bug 945584: Part 6 - Implementation of scroll snapping

- Implemented CSS scroll snapping (http://dev.w3.org/csswg/css-snappoints/)
Attachment #8566903 - Flags: review?(roc)
(Assignee)

Comment 123

2 years ago
Created attachment 8566904 [details] [diff] [review]
Bug 945584: Part 7 - Implement Scroll Snapping for Autoscroll

- Triggering scroll snapping at the end of an autoscroll.
- This enables text selection to be unencumbered by scroll snapping, while
restoring the scroll position to a valid snapping position when the drag
operation is completed.
Attachment #8566904 - Flags: review?(roc)
(Assignee)

Comment 124

2 years ago
Created attachment 8566905 [details] [diff] [review]
Bug 945584: Part 8 - Implement Scroll Snapping for Middle Mouse Button Scrolls

- Implemented scroll snapping at the end of a middle-mouse-button scroll.
- As this scrolling occurs within chrome Javascript, chrome-only DOM methods
had to be added: window.MozScrollSnap and element.MozScrollSnap
Attachment #8566905 - Flags: review?(roc)
(Assignee)

Comment 125

2 years ago
Created attachment 8566906 [details] [diff] [review]
Bug 945584: Part 9 - Tests for scroll snapping

- Implemented tests for scroll snapping, described within mochitest source
Attachment #8566906 - Flags: review?(roc)
Attachment #8566898 - Flags: review?(roc) → review+
(Assignee)

Comment 126

2 years ago
Created attachment 8567313 [details] [diff] [review]
Bug 945584: Part 5 - Implement ScrollVelocityQueue (v2 Patch)

- Implemented ScrollVelocityQueue class to calculate the velocity of a scroll
  when given periodic samples of scroll position.

v2 Patch:
- Added "explicit" keyword to ScrollVelocityQueue constructor to eliminate static analysis warning
Attachment #8566900 - Attachment is obsolete: true
Attachment #8566900 - Flags: review?(roc)
Attachment #8567313 - Flags: review?(roc)
Comment on attachment 8566899 [details] [diff] [review]
Bug 945584: Part 4 - Add scroll snapping preferences

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

r+ with those changes

::: modules/libpref/init/all.js
@@ +2093,5 @@
>  
>  // Is support for scroll-snap enabled?
>  pref("layout.css.scroll-snap.enabled", false);
>  
> +// Set the threshold distance in pixels below which scrolling will snap to an

This should read "in CSS pixels" I presume

@@ +2099,5 @@
> +pref("layout.css.scroll-snap.proximity-threshold", 200);
> +
> +// When selecting the snap point for CSS scroll snapping, the velocity of the
> +// scroll frame is clamped to this speed, in app units / ms.
> +pref("layout.css.scroll-snap.prediction-max-velocity", 250000);

This should be in CSS pixels too. appunits should not be exposed in prefs.
Attachment #8566899 - Flags: review?(roc) → review+
Comment on attachment 8567313 [details] [diff] [review]
Bug 945584: Part 5 - Implement ScrollVelocityQueue (v2 Patch)

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

::: layout/generic/ScrollVelocityQueue.cpp
@@ +17,5 @@
> +{
> +  float flingSensitivity = gfxPrefs::ScrollSnapPredictionSensitivity();
> +  int maxVelocity = gfxPrefs::ScrollSnapPredictionMaxVelocity();
> +  int maxOffset = maxVelocity * flingSensitivity;
> +  TimeStamp currentRefereshTime = mPresContext->RefreshDriver()->MostRecentRefresh();

typo: currentRefreshTime

@@ +33,5 @@
> +      if (velocity.x > maxVelocity) {
> +        velocity.x = maxVelocity;
> +      } else if (velocity.x < -maxVelocity) {
> +        velocity.x = -maxVelocity;
> +      }

velocity.x = std::max(std::min(velocity.x, maxVelocity), -maxVelocity);

@@ +38,5 @@
> +      if (velocity.y > maxVelocity) {
> +        velocity.y = maxVelocity;
> +      } else if (velocity.y < -maxVelocity) {
> +        velocity.y = -maxVelocity;
> +      }

ditto. Actually move this to a helper function "nsPoint Clamp(nsPoint, nscoord aMaxAbsValue)" so you can call it on mAccumulator below too.

@@ +45,5 @@
> +    }
> +  }
> +  if (mQueue.Length() > gfxPrefs::APZMaxVelocityQueueSize()) {
> +    mQueue.RemoveElementAt(0);
> +  }

Why do we need APZMaxVelocityQueueSize? Wouldn't it be simpler to just remove queue elements which can no longer contribute to GetVelocity because they're older than VelocityRelevanceTime?

@@ +83,5 @@
> +  TimeStamp currentRefereshTime = mPresContext->RefreshDriver()->MostRecentRefresh();
> +  nsPoint velocity;
> +  uint32_t timeDelta = (currentRefereshTime - mSampleTime).ToMilliseconds();
> +  int count = 0;
> +  for(int i=mQueue.Length() - 1; i>= 0; i--) {

Spacing: for (int i = mQueue.Length() - 1; i >= 0; i--)

::: layout/generic/ScrollVelocityQueue.h
@@ +15,5 @@
> +
> +namespace mozilla {
> +namespace layout {
> +
> +class ScrollVelocityQueue MOZ_FINAL {

Need a comment explaining what this class is for. Maybe you should just move the Sample() comment up here and make it explain the entire purpose and operation of the class.

@@ +20,5 @@
> +public:
> +  explicit ScrollVelocityQueue(nsPresContext *aPresContext)
> +    : mPresContext(aPresContext) {}
> +
> +  ~ScrollVelocityQueue() {}

This isn't needed.

@@ +59,5 @@
> +  // Get scroll velocity averaged from recent movement, in appunits / ms
> +  nsPoint GetVelocity();
> +private:
> +  // A queue of (timestamp, velocity) pairs; these are the historical
> +  // velocities at the given timestamps. Timestamps are in milliseconds,

These are durations, not timestamps.
Attachment #8567313 - Flags: review?(roc)
Comment on attachment 8567313 [details] [diff] [review]
Bug 945584: Part 5 - Implement ScrollVelocityQueue (v2 Patch)

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

::: layout/generic/ScrollVelocityQueue.h
@@ +61,5 @@
> +private:
> +  // A queue of (timestamp, velocity) pairs; these are the historical
> +  // velocities at the given timestamps. Timestamps are in milliseconds,
> +  // velocities are in app units per ms.
> +  nsTArray<std::pair<uint32_t, nsPoint> > mQueue;

Seems to me that storing velocities in app units per second would reduce roundoff errors and eliminate some conversions in nsGfxScrollFrame
Comment on attachment 8566903 [details] [diff] [review]
Bug 945584: Part 6 - Implementation of scroll snapping

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

::: dom/events/EventStateManager.cpp
@@ +2434,3 @@
>        break;
>      case nsIDOMWheelEvent::DOM_DELTA_PAGE:
>        origin = nsGkAtoms::pages;

Why doesn't DOM_DELTA_PAGE apply scroll snapping?

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +2041,5 @@
> +  float friction = gfxPrefs::APZFlingFriction();
> +  ParentLayerPoint velocity(mX.GetVelocity(), mY.GetVelocity());
> +  ParentLayerPoint predictedDelta;
> +  if (velocity.x != 0.0f) {
> +    predictedDelta.x = -velocity.x / log(1.0 - friction);

Can you add a comment explaining the derivation of this formula?

::: layout/generic/nsGfxScrollFrame.cpp
@@ +3498,5 @@
> +  if (predictedOffset.y > maxOffset) {
> +    predictedOffset.y = maxOffset;
> +  } else if (predictedOffset.y < -maxOffset) {
> +    predictedOffset.y = -maxOffset;
> +  }

Use that clamping helper function here

@@ +5445,5 @@
> +  }
> +}
> +
> +void
> +CalcSnapPoints::AddEdgeInterval(nscoord aInterval, nscoord aMaxPos,

Can we unify AddEdgeInterval and AddEdge?

It seems to me that AddEdgeInterval can be written as two AddEdges, one on each side of the destination point. If a point from the interval is used, it must be one of those. Then AddEdgeInterval won't need to deal with the different scroll types etc.

::: layout/generic/nsGfxScrollFrame.h
@@ +128,5 @@
>      ScrollFrameHelper *mHelper;
>    };
>  
> +
> +

Remove this hunk
Attachment #8566903 - Flags: review?(roc)
Attachment #8566904 - Flags: review?(roc) → review+
One thing these patches don't do is reapply scroll snapping when elements move due to reflow.

That's going to be quite hard to do properly since it won't be obvious which snap-point is the right one to snap to when an element moves. We'd really need to store somewhere information about which snap-point we've snapped to (e.g. "the Nth point in this interval" or "the edge of this element") and try to snap to that point again (instantly?). Better file a new bug for that.
Comment on attachment 8566905 [details] [diff] [review]
Bug 945584: Part 8 - Implement Scroll Snapping for Middle Mouse Button Scrolls

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

This is OK but I think we will actually want a Web-accessible API here, and I'm not sure this is what I'd standardize. Would it make more sense to add a "snapping" parameter to the ScrollOptions dictionary? http://dev.w3.org/csswg/cssom-view/#dictdef-scrolltooptions

If we did that, I guess we could use a 0,0 scrollBy to trigger snapping. Or would that be too much of a hack?
Attachment #8566905 - Flags: review?(roc)
Comment on attachment 8566906 [details] [diff] [review]
Bug 945584: Part 9 - Tests for scroll snapping

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

::: layout/base/tests/mochitest.ini
@@ +209,5 @@
>  [test_mozPaintCount.html]
>  skip-if = buildapp == 'mulet' || buildapp == 'b2g' || toolkit == 'android' || e10s # b2g(depends on plugins support) b2g-debug(depends on plugins support) b2g-desktop(depends on plugins support)
>  [test_scroll_event_ordering.html]
> +[test_scroll_snapping.html]
> +skip-if = buildapp == 'android' # bug 1041833 

trailing whitespace
Attachment #8566906 - Flags: review?(roc) → review+
Comment on attachment 8566903 [details] [diff] [review]
Bug 945584: Part 6 - Implementation of scroll snapping

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

Some questions and drive-by comments.

::: gfx/layers/FrameMetrics.h
@@ +352,5 @@
> +    mFlingSnapOffset = aFlingSnapOffset;
> +    // We add 1 to mScrollGeneration to create the next mFlingSnapGeneration.
> +    // This ensures that only one fling snap happens per scroll generation while
> +    // also enabling a fling snap to occur even if mScrollGeneration has not yet
> +    // been incremented by the first scroll.

I don't fully understand the model here. As far as I can tell the idea is that the APZ drives fling-snapping in that it tells layout where it expects to end up, and then layout will send a smooth-scroll request back with the snap point as the destination.

However, limiting to a single fling-snap per scroll generation seems wrong. Consider the case where the user flings upwards; the APZ sends a fling-snap request to layout; then the user flings downwards (before the smooth-scroll request comes back from layout). At this point the APZ will compute a new fling-snap destination but that new destination will never get sent to layout because it has the same generation as the old one. In the meantime layout will send the smooth-scroll request corresponding to the first fling back to APZ, and APZ will then effectively discard the second fling the user did and snap to the snap point corresponding to the first fling. Am I missing something here?

::: gfx/layers/LayersLogging.cpp
@@ +179,1 @@
>    AppendToString(aStream, m.GetDisplayPort(), "] [dp=");

Please log the fling snap offset as well.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +2038,2 @@
>        aOverscrollHandoffChain,
> +      !aHandoff);  // only apply acceleration if this is an initial fling

Let's say you have a scrollable frame with a nested scrollable frame inside it. The inside scrollable frame has a scroll offset of y=100 and a single snap point at y=50. The user flings the inner scrollable towards y=0 as hard as possible. This results in the inner scrollable scrolling to y=0 and the remaining velocity carrying over as overscroll to the outer scrollable frame, which starts scrolling. Will the inner scrollable now reverse direction and scroll downwards in order to snap at y=50? If so, would the inner scrollable and outer scrollable be scrolling in opposite directions at the same time?

@@ +2048,5 @@
> +    predictedDelta.y = -velocity.y / log(1.0 - friction);
> +  }
> +  CSSPoint predictedDestination = mFrameMetrics.GetScrollOffset() + predictedDelta / mFrameMetrics.GetZoom();
> +
> +  APZC_LOG("%p fling snapping.  friction: %f velocity: %f, %f predictedDelta: %f, %f position: %f, %f predictedDestination: %f, %f\n", this, friction, velocity.x, velocity.y, (float)predictedDelta.x, (float)predictedDelta.y, (float)mFrameMetrics.GetScrollOffset().x, (float)mFrameMetrics.GetScrollOffset().y, (float)predictedDestination.x, (float)predictedDestination.y);

nit: break this up over multiple lines
(Assignee)

Comment 135

2 years ago
Created attachment 8568088 [details] [diff] [review]
Bug 945584: Part 4 - Add scroll snapping preferences (v2 Patch), r=roc

- Added preferences to allow trackpad and mousewheel flinging between
snap points to be tuned:
- layout.css.scroll-snap.prediction-max-velocity
- layout.css.scroll-snap.prediction-sensitivity

V2 Patch:
- Updated with review in Comment 127
Attachment #8566899 - Attachment is obsolete: true
(Assignee)

Comment 136

2 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #131)
> One thing these patches don't do is reapply scroll snapping when elements
> move due to reflow.
> 
> That's going to be quite hard to do properly since it won't be obvious which
> snap-point is the right one to snap to when an element moves. We'd really
> need to store somewhere information about which snap-point we've snapped to
> (e.g. "the Nth point in this interval" or "the edge of this element") and
> try to snap to that point again (instantly?). Better file a new bug for that.
Bug 1124324 tracks this (and for the case when content changes).
(Assignee)

Comment 137

2 years ago
Created attachment 8568254 [details] [diff] [review]
Bug 945584: Part 5 - Implement ScrollVelocityQueue (v3 Patch)

- Implemented ScrollVelocityQueue class to calculate the velocity of a scroll
  when given periodic samples of scroll position.
- Added BasePoint::Clamp to simplify code.

V3 Patch:
- Updated with feedback from review in Comment 128 and Comment 129

Note:
- The Part 6 patch will be updated to accommodate the change of units returned by GetVelocity()
Attachment #8567313 - Attachment is obsolete: true
Attachment #8568254 - Flags: review?(roc)
Attachment #8568254 - Flags: review?(roc) → review+
(Assignee)

Comment 138

2 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #130)
> Comment on attachment 8566903 [details] [diff] [review]
> > +void
> > +CalcSnapPoints::AddEdgeInterval(nscoord aInterval, nscoord aMaxPos,
> 
> Can we unify AddEdgeInterval and AddEdge?
> 
> It seems to me that AddEdgeInterval can be written as two AddEdges, one on
> each side of the destination point. If a point from the interval is used, it
> must be one of those. Then AddEdgeInterval won't need to deal with the
> different scroll types etc.
> 

AddEdgeInterval can be implemented to call AddEdges multiple times; however, it would still require special conditions for each scroll type.  For example, page scrolling would require calling AddEdges three times for all possible candidate edges.  Would you feel that refactoring this would still improve readability?
Flags: needinfo?(roc)
Why do we need 3 candidate edges for page scrolling? That would imply we can select a snap point that has another snap point between itself and the destination point.

Assuming you have a good answer to that, if we make AddEdgeInterval call AddEdge 4 times --- 2 edges on each side of the destination point --- does that work for all scroll types? If so, let's just do that.
Flags: needinfo?(roc)
(Assignee)

Comment 140

2 years ago
Created attachment 8568796 [details] [diff] [review]
Bug 945584: Part 6 - Implementation of scroll snapping (v2 Patch)

- Implemented CSS scroll snapping (http://dev.w3.org/csswg/css-snappoints/)

V2 Patch:
- Made changes based on review in Comment 134 and Comment 139
Attachment #8566903 - Attachment is obsolete: true
Attachment #8568796 - Flags: review?(roc)
(Assignee)

Comment 141

2 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #139)
> Why do we need 3 candidate edges for page scrolling? That would imply we can
> select a snap point that has another snap point between itself and the
> destination point.
It is possible that the candidate edges closest to the destination are not viable due to setting the scroll position out of bounds, in which case an edge between the destination and the starting position may be selected in the case of page scrolling.
> 
> Assuming you have a good answer to that, if we make AddEdgeInterval call
> AddEdge 4 times --- 2 edges on each side of the destination point --- does
> that work for all scroll types? If so, let's just do that.
I have updated the patch to do this.  In the process, I have found a bug that caused the interval scroll snap point selection to differ than in the normal path, when applying a scroll-snap-destination value that would place the scroll position out of range.  In the case of interval scrolling, it resulted in a selection of the nearest snap point to the range, even if it would result in scrolling in the opposite direction.  In the normal non-interval path, it was resulting in no scrolling in the opposite direction.

I have updated the normal AddEdge() code path to allow opposite direction scrolling to the first or last snap point only in the event of WHOLE (home, end keys) scrolling.  When not using WHOLE scrolling it behaves as before for the normal AddEdge() code path, resulting in no scroll in the opposite direction.  This behavior is now consistent with the interval and non-interval snap points.

I'll update the coordinates for these test cases in the existing tests (patch 9) to reflect this and ensure it stays consistent.
(Assignee)

Comment 142

2 years ago
(In reply to :kip (Kearwood Gilbert) from comment #140)
> Created attachment 8568796 [details] [diff] [review]
> Bug 945584: Part 6 - Implementation of scroll snapping (v2 Patch)
> 
> - Implemented CSS scroll snapping (http://dev.w3.org/csswg/css-snappoints/)
> 
> V2 Patch:
> - Made changes based on review in Comment 134 and Comment 139
Sorry, I mis-typed.  The changes in Comment 130 and Comment 139 have been applied for review only.  I missed Comment 134.

Comment 134's changes will be addressed and the patch updated again.
(In reply to :kip (Kearwood Gilbert) from comment #141)
> It is possible that the candidate edges closest to the destination are not
> viable due to setting the scroll position out of bounds, in which case an
> edge between the destination and the starting position may be selected in
> the case of page scrolling.

OK, but in that case, we could have to look arbitrarily far back through the snappoints, if the interval period is small --- right?

What if we clamp the destination to the scroll bounds first and then choose the two interval points on either side of the clamped destination? Would those two points always be the correct ones?
(Assignee)

Comment 144

2 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #143)
> (In reply to :kip (Kearwood Gilbert) from comment #141)
> > It is possible that the candidate edges closest to the destination are not
> > viable due to setting the scroll position out of bounds, in which case an
> > edge between the destination and the starting position may be selected in
> > the case of page scrolling.
> 
> OK, but in that case, we could have to look arbitrarily far back through the
> snappoints, if the interval period is small --- right?
> 
> What if we clamp the destination to the scroll bounds first and then choose
> the two interval points on either side of the clamped destination? Would
> those two points always be the correct ones?
I can't think of any cases this would not cover.  I'll give it a try and run it through the tests.

Updated

2 years ago
Blocks: 1071326
(Assignee)

Comment 145

2 years ago
Created attachment 8568885 [details] [diff] [review]
Bug 945584: Part 9 - Tests for scroll snapping (v2 Patch), r=roc

v2 Patch:
- Updated coordinates in test data to match bug fix described in Comment 141
Attachment #8566906 - Attachment is obsolete: true
(Assignee)

Comment 146

2 years ago
Created attachment 8569537 [details] [diff] [review]
Bug 945584: Part 4 - Add scroll snapping preferences (v3 Patch), r=roc

- Added preferences to allow trackpad and mousewheel flinging between
snap points to be tuned:
- layout.css.scroll-snap.prediction-max-velocity
- layout.css.scroll-snap.prediction-sensitivity

v3 Patch:
- Fine tuned default values of preferences
- layout.css.scroll-snap.prediction-max-velocity is now in CSSPixels/s (was CSSPixels/ms)
- layout.css.scroll-snap.prediction-sensitivity is now in seconds (was milliseconds)
Attachment #8568088 - Attachment is obsolete: true
(Assignee)

Comment 147

2 years ago
Created attachment 8569541 [details] [diff] [review]
Bug 945584: Part 5 - Implement ScrollVelocityQueue (v4 Patch), r=roc

- Implemented ScrollVelocityQueue class to calculate the velocity of a scroll
when given periodic samples of scroll position.
- Added BasePoint::Clamp to simplify code.

v4 Patch:
- nsPresContext::CSSPixelsToAppUnits is now being used to correctly convert
  the "layout.css.scroll-snap.prediction-max-velocity" preference.
Attachment #8568254 - Attachment is obsolete: true
(Assignee)

Comment 148

2 years ago
Created attachment 8569543 [details] [diff] [review]
Bug 945584: Part 6 - Implementation of scroll snapping (v3 Patch)

- Implemented CSS scroll snapping (http://dev.w3.org/csswg/css-snappoints/)

v3 Patch:
- Updated with recommendation in Comment 143
- Corrected unit conversion and rounding issues with scroll-snapping preferences.
Attachment #8568796 - Attachment is obsolete: true
Attachment #8568796 - Flags: review?(roc)
Attachment #8569543 - Flags: review?(roc)
(Assignee)

Comment 149

2 years ago
(In reply to :kip (Kearwood Gilbert) from comment #144)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
> #143)
> > (In reply to :kip (Kearwood Gilbert) from comment #141)
> > > It is possible that the candidate edges closest to the destination are not
> > > viable due to setting the scroll position out of bounds, in which case an
> > > edge between the destination and the starting position may be selected in
> > > the case of page scrolling.
> > 
> > OK, but in that case, we could have to look arbitrarily far back through the
> > snappoints, if the interval period is small --- right?
> > 
> > What if we clamp the destination to the scroll bounds first and then choose
> > the two interval points on either side of the clamped destination? Would
> > those two points always be the correct ones?
> I can't think of any cases this would not cover.  I'll give it a try and run
> it through the tests.
This seems to be handling all cases fine and is passing the tests.  I have updated the patch.
(Assignee)

Comment 150

2 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #132)
> Comment on attachment 8566905 [details] [diff] [review]
> Bug 945584: Part 8 - Implement Scroll Snapping for Middle Mouse Button
> Scrolls
> 
> Review of attachment 8566905 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is OK but I think we will actually want a Web-accessible API here, and
> I'm not sure this is what I'd standardize. Would it make more sense to add a
> "snapping" parameter to the ScrollOptions dictionary?
> http://dev.w3.org/csswg/cssom-view/#dictdef-scrolltooptions
> 
> If we did that, I guess we could use a 0,0 scrollBy to trigger snapping. Or
> would that be too much of a hack?

I like this a lot.  It would also be possible to perform a scrollTo without specifying "left" and "top" in the dictionary that would include a "snapping" parameter.  Combined with "behavior", it would also enable control over whether the snapping movement would be instant or animated as though a user gesture was completed.

Would it be acceptable to use the MozScrollSnap method (exposed as ChromeOnly and commented as deprecated) to solve this one instance, and land a separate update afterwards that replaces this with the web accessible API as you have described?  If so, we could get the scroll snapping in everyone's hands while w3c reviews this recommendation.
(In reply to :kip (Kearwood Gilbert) from comment #150)
> Would it be acceptable to use the MozScrollSnap method (exposed as
> ChromeOnly and commented as deprecated) to solve this one instance, and land
> a separate update afterwards that replaces this with the web accessible API
> as you have described?  If so, we could get the scroll snapping in
> everyone's hands while w3c reviews this recommendation.

Yes!
Comment on attachment 8569543 [details] [diff] [review]
Bug 945584: Part 6 - Implementation of scroll snapping (v3 Patch)

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

Getting pretty close here. Just this RTL issue to work out, from my point of view. But, please reply to Kats' comments and request review from him too on the next round.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +5467,5 @@
> +  // aDestination must be clamped to the scroll
> +  // range in order to handle cases where the best matching snap point would
> +  // result in scrolling out of bounds.  This clamping must be prior to
> +  // selecting the two interval edges.
> +  nscoord clamped = std::max(std::min(aDestination, aMaxPos), 0);

In RTL situations we can have negative scroll offsets (scroll offset starts at 0 and decreases as you scroll left). You probably need to pass in an aMinPos here to handle that.

I guess we'll need a test for scroll snapping with RTL.
Attachment #8569543 - Flags: review?(roc) → review-
(Assignee)

Comment 153

2 years ago
Created attachment 8570129 [details] [diff] [review]
Bug 945584: Part 3 - Enable cancellation of OSX synthesized mousewheel scrolling events (v2 Patch),r=roc

- Mouse wheel events synthesized by OSX for momentum scrolling can now
be interrupted by DOM triggered and CSS scroll snapping triggered scroll
events for consistent behavior with the scrolling and fling gestures
in the APZC.

v2 Patch:
- Un-bitrotted
Attachment #8566898 - Attachment is obsolete: true
(Assignee)

Comment 154

2 years ago
Created attachment 8570178 [details] [diff] [review]
Bug 945584: Part 6 - Implementation of scroll snapping (v4 Patch)

- Implemented CSS scroll snapping (http://dev.w3.org/csswg/css-snappoints/)

v4 Patch:
- Un-bitrotted
- Now working with rtl layout
Attachment #8569543 - Attachment is obsolete: true
(Assignee)

Comment 155

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #134)
> Comment on attachment 8566903 [details] [diff] [review]
> Bug 945584: Part 6 - Implementation of scroll snapping
> 
> Review of attachment 8566903 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some questions and drive-by comments.
> 
> ::: gfx/layers/FrameMetrics.h
> @@ +352,5 @@
> > +    mFlingSnapOffset = aFlingSnapOffset;
> > +    // We add 1 to mScrollGeneration to create the next mFlingSnapGeneration.
> > +    // This ensures that only one fling snap happens per scroll generation while
> > +    // also enabling a fling snap to occur even if mScrollGeneration has not yet
> > +    // been incremented by the first scroll.
> 
> I don't fully understand the model here. As far as I can tell the idea is
> that the APZ drives fling-snapping in that it tells layout where it expects
> to end up, and then layout will send a smooth-scroll request back with the
> snap point as the destination.
> 
> However, limiting to a single fling-snap per scroll generation seems wrong.
> Consider the case where the user flings upwards; the APZ sends a fling-snap
> request to layout; then the user flings downwards (before the smooth-scroll
> request comes back from layout). At this point the APZ will compute a new
> fling-snap destination but that new destination will never get sent to
> layout because it has the same generation as the old one. In the meantime
> layout will send the smooth-scroll request corresponding to the first fling
> back to APZ, and APZ will then effectively discard the second fling the user
> did and snap to the snap point corresponding to the first fling. Am I
> missing something here?
The intent was to ensure that only the latest fling-snap be executed per scroll generation.  Thanks to your careful review, it appears that my logic may be incorrect and will be skipping subsequent fling snaps within the same scroll generation.  I'll dig a bit deeper and see if this is working as expected and update the patch as needed.

> 
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> @@ +2038,2 @@
> >        aOverscrollHandoffChain,
> > +      !aHandoff);  // only apply acceleration if this is an initial fling
> 
> Let's say you have a scrollable frame with a nested scrollable frame inside
> it. The inside scrollable frame has a scroll offset of y=100 and a single
> snap point at y=50. The user flings the inner scrollable towards y=0 as hard
> as possible. This results in the inner scrollable scrolling to y=0 and the
> remaining velocity carrying over as overscroll to the outer scrollable
> frame, which starts scrolling. Will the inner scrollable now reverse
> direction and scroll downwards in order to snap at y=50?
In the case of an OverscrollAnimation, the fling snap will be triggered at the end of the animation.  This appears to be occuring, but I will do some experiments where this animation is not being triggered.  The intent is that the inner scroll frame will scroll downwards after passing its momentum to the parent to reach its trigger point.

> If so, would the
> inner scrollable and outer scrollable be scrolling in opposite directions at
> the same time?
> 
In the case of the OverscrollAnimation, the animation should complete before the fling snap triggers a smooth scroll, so they would not be moving in opposite directions.  In the case of nested scroll frames, I would expect them to move in opposite directions.

I'll do some more experiments to ensure it is working well in the scenarios you have described.  Thanks for your detailed review and feedback.
(Assignee)

Comment 156

2 years ago
(In reply to :kip (Kearwood Gilbert) from comment #155)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #134)

> I'll do some more experiments to ensure it is working well in the scenarios
> you have described.  Thanks for your detailed review and feedback.
I have been created a test page to experiment with some of these edge cases involving overscroll:

http://kearwood.com/test9.html

It appears to be behaving as I described in Comment 155 regarding the overscroll handoff, fling snapping, and OverscrollAnimation; however, I have encountered cases where the smooth scroll from a fling snap did not fire, even though the APZC_LOG showed that mFrameMetrics.DoFlingSnap was called.  Perhaps they were stomped due to the effect you described with the mScrollSnapGeneration values.

I will fix this and the other changes you listed in Comment 134 and assign the patch for your review.
(Assignee)

Comment 157

2 years ago
Created attachment 8570739 [details] [diff] [review]
Bug 945584: Part 6 - Implementation of scroll snapping (v5 Patch)

- Implemented CSS scroll snapping (http://dev.w3.org/csswg/css-snappoints/)

v5 Patch:
- APZC now sends a dedicated event to trigger scroll snapping rather than piggybacking on FrameMetrics.
- Corrects issues found in Comment 134
Attachment #8570178 - Attachment is obsolete: true
Attachment #8570739 - Flags: review?(bugmail.mozilla)
(Assignee)

Updated

2 years ago
Blocks: 1137937
(Assignee)

Comment 158

2 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #151)
> (In reply to :kip (Kearwood Gilbert) from comment #150)
> > Would it be acceptable to use the MozScrollSnap method (exposed as
> > ChromeOnly and commented as deprecated) to solve this one instance, and land
> > a separate update afterwards that replaces this with the web accessible API
> > as you have described?  If so, we could get the scroll snapping in
> > everyone's hands while w3c reviews this recommendation.
> 
> Yes!
I have created Bug 1137937 to track this.
(Assignee)

Comment 159

2 years ago
Created attachment 8570755 [details] [diff] [review]
Bug 945584: Part 8 - Implement Scroll Snapping for Middle Mouse Button Scrolls (v2 Patch)

- Implemented scroll snapping at the end of a middle-mouse-button scroll.
- As this scrolling occurs within chrome Javascript, chrome-only DOM methods
had to be added: window.MozScrollSnap and element.MozScrollSnap

v2 Patch:
- Added comments in the webidl files indicating that the MozScrollSnap methods are deprecated.
- Bug 1137937 tracks implementation of a replacement for these chome-only DOM methods, to be replaced with a web accessible API.
Attachment #8566905 - Attachment is obsolete: true
Attachment #8570755 - Flags: review?(roc)
(Assignee)

Comment 160

2 years ago
Created attachment 8570817 [details] [diff] [review]
Bug 945584: Part 9 - Tests for scroll snapping (v3 Patch)

v3 Patch:
- Now running tests twice.  Once in LTR direction and once in RTL direction.

ROC - You have r+'ed this earlier, but it has changed a bit for the RTL tests.
Attachment #8568885 - Attachment is obsolete: true
Attachment #8570817 - Flags: review?(roc)
(Assignee)

Comment 161

2 years ago
I have pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dddf205354e0
Attachment #8570755 - Flags: review?(roc) → review+
Comment on attachment 8570817 [details] [diff] [review]
Bug 945584: Part 9 - Tests for scroll snapping (v3 Patch)

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

Great thanks!!!

::: layout/base/tests/mochitest.ini
@@ +215,5 @@
>  [test_mozPaintCount.html]
>  skip-if = buildapp == 'mulet' || buildapp == 'b2g' || toolkit == 'android' || e10s # b2g(depends on plugins support) b2g-debug(depends on plugins support) b2g-desktop(depends on plugins support)
>  [test_scroll_event_ordering.html]
> +[test_scroll_snapping.html]
> +skip-if = buildapp == 'android' # bug 1041833 

trailing whitespace
Attachment #8570817 - Flags: review?(roc) → review+
Comment on attachment 8570739 [details] [diff] [review]
Bug 945584: Part 6 - Implementation of scroll snapping (v5 Patch)

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

I reviewed all the APZC changes and FlingSnap() propagation, but didn't fully review EventStateManager.cpp, nsPresShell.cpp, and nsGfxScrollFrame.cpp changes - I'm assuming roc has reviewed those bits already. One thing you may want to keep in mind is that dvander has been working on making the APZ handle scrollwheel events on desktop (when APZ is enabled there), and the codepaths for that will need to implement snapping too. See the AsyncPanZoomController::OnScrollWheel function.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +53,5 @@
>  #include "StickyScrollContainer.h"
>  #include "nsIFrameInlines.h"
>  #include "gfxPlatform.h"
>  #include "gfxPrefs.h"
> +#include "ScrollVelocityQueue.h"

This was already included in nsGfxScrollFrame.h so you shouldn't need it here.

::: layout/generic/nsGfxScrollFrame.h
@@ +235,5 @@
>     */
>    void ScrollToRestoredPosition();
> +  /**
> +   * GetSnapPointForDestination determines which point to snap to after
> +   * scrolling. aCurrPos gives the position before scrolling and aDestination

s/aCurrPos/aStartPos/. Also specify what the return bool indicates.

::: layout/generic/nsIScrollableFrame.h
@@ +282,5 @@
> +                        ScrollMomentum aMomentum = NOT_MOMENTUM,
> +                        ScrollSnapMode aSnap = DISABLE_SNAP) = 0;
> +
> +  /**
> +   * @note Perform scroll snapping, possibly resulting in a smooth scroll to

Drop the "@note"

::: widget/windows/winrt/APZController.cpp
@@ +193,5 @@
> +{
> +#ifdef DEBUG_CONTROLLER
> +  WinUtils::Log("APZController::RequestFlingSnap scrollid=%I64d destination: %lu %lu %lu %lu",
> +    aScrollId, aDestination.left, aDestination.top, aDestination.right,
> +    aDestination.bottom);

aDestination has x and y, not left/top/right/bottom
Attachment #8570739 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 164

2 years ago
Created attachment 8571515 [details] [diff] [review]
Bug 945584: Part 6 - Implementation of scroll snapping (v6 Patch),r=roc+kats

- Implemented CSS scroll snapping (http://dev.w3.org/csswg/css-snappoints/)
v6 Patch:

- Made adjustments from review in Comment 163
- Corrected Android build breakage found in Try run.
Attachment #8570739 - Attachment is obsolete: true
(Assignee)

Comment 165

2 years ago
Created attachment 8571524 [details] [diff] [review]
Bug 945584: Part 9 - Tests for scroll snapping (v4 Patch),r=roc

v4 Patch:
- Removed trailing white space as per Comment 162
Attachment #8570817 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Blocks: 1138651
(Assignee)

Comment 166

2 years ago
This can be enabled on Firefox OS before Bug 969250 is landed, as Firefox OS does not depend on scrollbar support.

I have added Bug 1138651 to track this.
(Assignee)

Updated

2 years ago
Blocks: 1138658
(Assignee)

Comment 167

2 years ago
I have added Bug 1138658 to track enabling by default on desktop platforms.  Scroll bar support must be landed (Bug 969250) before enabling this on desktop.
(Assignee)

Comment 168

2 years ago
I have added Bug 1138668 to track enabling by default on Android.  

Before this can be enabled on Android, Bug 1041833 must be landed to enable support for CSSOM-View scroll-behavior and smooth scrolling on Android.
Android builds on that try push all failed with https://treeherder.mozilla.org/logviewer.html#?job_id=5275992&repo=try

Are these patches at fault?
Flags: needinfo?(kgilbert)
Keywords: checkin-needed
(Assignee)

Comment 170

2 years ago
(In reply to Wes Kocher (:KWierso) from comment #169)
> Android builds on that try push all failed with
> https://treeherder.mozilla.org/logviewer.html#?job_id=5275992&repo=try
> 
> Are these patches at fault?
Yes, the part 6 patch was at fault.  I have corrected this with an update to that patch and confirmed that the bustage was fixed with a local android build.

To be certain, I have also pushed to try again for the android builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2575ee82338f
Flags: needinfo?(kgilbert)
(Assignee)

Comment 171

2 years ago
An intent to ship email, for B2G only, has been posted to dev-platform:

https://groups.google.com/forum/#!topic/mozilla.dev.platform/Yoj0vlP8C2w
(Assignee)

Comment 172

2 years ago
Created attachment 8571679 [details] [diff] [review]
Bug 945584: Part 2 - Add CSS scroll snapping attributes to ScrollbarStyles (v10 Patch),r=mstange

- ScrollbarStyles now carries additional variables to support new
CSS scroll snapping attributes:
- scroll-snap-type / scroll-snap-type-x / scroll-snap-type-y
- scroll-snap-points-x / scroll-snap-points-y
- scroll-snap-destination
- (scroll-snap-coordinate does not apply to the scroll container)
- Simplified the constructor and operator== for ScrollbarStyles.

v10 Patch:
- Un-bitrotted
Attachment #8566894 - Attachment is obsolete: true
(Assignee)

Comment 173

2 years ago
Created attachment 8571683 [details] [diff] [review]
Bug 945584: Part 6 - Implementation of scroll snapping (v7 Patch),r=roc,kats

- Implemented CSS scroll snapping (http://dev.w3.org/csswg/css-snappoints/)

v7 Patch:
- Un-Bitrotted
Attachment #8571515 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Part 6 failed to apply:

1 out of 1 hunks FAILED -- saving rejects to file dom/ipc/PBrowser.ipdl.rej
patching file dom/ipc/TabChild.h
Hunk #1 FAILED at 313
1 out of 1 hunks FAILED -- saving rejects to file dom/ipc/TabChild.h.rej
patching file dom/ipc/TabParent.h
Hunk #1 FAILED at 243
1 out of 1 hunks FAILED -- saving rejects to file dom/ipc/TabParent.h.rej
patching file gfx/layers/apz/util/ChromeProcessController.h
Hunk #1 FAILED at 31
1 out of 1 hunks FAILED -- saving rejects to file gfx/layers/apz/util/ChromeProcessController.h.rej
patching file gfx/tests/gtest/TestAsyncPanZoomController.cpp
Hunk #1 FAILED at 55
1 out of 1 hunks FAILED -- saving rejects to file gfx/tests/gtest/TestAsyncPanZoomController.cpp.rej
patching file widget/android/APZCCallbackHandler.h
Hunk #1 FAILED at 37
1 out of 1 hunks FAILED -- saving rejects to file widget/android/APZCCallbackHandler.h.rej
patching file widget/windows/winrt/APZController.h
Hunk #1 FAILED at 31
1 out of 1 hunks FAILED -- saving rejects to file widget/windows/winrt/APZController.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bug945584_part6.patch

could you take a look, thanks!
Flags: needinfo?(kgilbert)
Keywords: checkin-needed
(Assignee)

Comment 175

2 years ago
Created attachment 8572130 [details] [diff] [review]
Bug 945584: Part 6 - Implementation of scroll snapping (v8 Patch),r=roc,kats

- Implemented CSS scroll snapping (http://dev.w3.org/csswg/css-snappoints/)

v8 Patch:
- Un-bitrotted
Attachment #8571683 - Attachment is obsolete: true
Flags: needinfo?(kgilbert)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 176

2 years ago
(In reply to Carsten Book [:Tomcat] from comment #174)
> Part 6 failed to apply:
> 
> 1 out of 1 hunks FAILED -- saving rejects to file dom/ipc/PBrowser.ipdl.rej
> patching file dom/ipc/TabChild.h
> Hunk #1 FAILED at 313
> 1 out of 1 hunks FAILED -- saving rejects to file dom/ipc/TabChild.h.rej
> patching file dom/ipc/TabParent.h
> Hunk #1 FAILED at 243
> 1 out of 1 hunks FAILED -- saving rejects to file dom/ipc/TabParent.h.rej
> patching file gfx/layers/apz/util/ChromeProcessController.h
> Hunk #1 FAILED at 31
> 1 out of 1 hunks FAILED -- saving rejects to file
> gfx/layers/apz/util/ChromeProcessController.h.rej
> patching file gfx/tests/gtest/TestAsyncPanZoomController.cpp
> Hunk #1 FAILED at 55
> 1 out of 1 hunks FAILED -- saving rejects to file
> gfx/tests/gtest/TestAsyncPanZoomController.cpp.rej
> patching file widget/android/APZCCallbackHandler.h
> Hunk #1 FAILED at 37
> 1 out of 1 hunks FAILED -- saving rejects to file
> widget/android/APZCCallbackHandler.h.rej
> patching file widget/windows/winrt/APZController.h
> Hunk #1 FAILED at 31
> 1 out of 1 hunks FAILED -- saving rejects to file
> widget/windows/winrt/APZController.h.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working dir
> errors during apply, please fix and refresh bug945584_part6.patch
> 
> could you take a look, thanks!
The Part 6 patch was bit-rotted mid flight.  I have updated it, thanks.
Part 8 makes webidl changes that don't have DOM peer review AFAICT.
Flags: needinfo?(kgilbert)
Keywords: checkin-needed
(Assignee)

Comment 178

2 years ago
Created attachment 8572774 [details] [diff] [review]
Bug 945584: Part 6 - Implementation of scroll snapping (v9 Patch),r=roc,kats

- Implemented CSS scroll snapping (http://dev.w3.org/csswg/css-snappoints/)

v9 Patch:
- Un-bitrotted
Attachment #8572130 - Attachment is obsolete: true
Comment on attachment 8570755 [details] [diff] [review]
Bug 945584: Part 8 - Implement Scroll Snapping for Middle Mouse Button Scrolls (v2 Patch)

r=me on the IDL bits.
Attachment #8570755 - Flags: review+
(Assignee)

Comment 180

2 years ago
Created attachment 8572790 [details] [diff] [review]
Bug 945584: Part 8 - Implement Scroll Snapping for Middle Mouse Button Scrolls (v2 Patch), r=roc, r=bz

- Implemented scroll snapping at the end of a middle-mouse-button scroll.
- As this scrolling occurs within chrome Javascript, chrome-only DOM methods
had to be added: window.MozScrollSnap and element.MozScrollSnap
- Bug 1137937 tracks implementation of a replacement for these chome-only DOM methods,
to be replaced with a web accessible API.
Attachment #8570755 - Attachment is obsolete: true
(Assignee)

Comment 181

2 years ago
Created attachment 8572792 [details] [diff] [review]
Bug 945584: Part 7 - Implement Scroll Snapping for Autoscroll, r=roc

- Triggering scroll snapping at the end of an autoscroll.
- This enables text selection to be unencumbered by scroll snapping, while
restoring the scroll position to a valid snapping position when the drag
operation is completed.
Attachment #8566904 - Attachment is obsolete: true
Flags: needinfo?(kgilbert)
(Assignee)

Comment 182

2 years ago
Created attachment 8572795 [details] [diff] [review]
Bug 945584: Part 6 - Implementation of scroll snapping (v9 Patch), r=roc, r=kats

- Implemented CSS scroll snapping (http://dev.w3.org/csswg/css-snappoints/)
Attachment #8572774 - Attachment is obsolete: true
(Assignee)

Comment 183

2 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #177)
> Part 8 makes webidl changes that don't have DOM peer review AFAICT.

I have received additional review from bz and updated the patch descriptions to reflect the reviewers.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
part 6 still fail to apply here:

Hunk #1 succeeded at 509 with fuzz 1 (offset 1 lines).
patching file dom/ipc/TabChild.h
Hunk #1 FAILED at 313
1 out of 1 hunks FAILED -- saving rejects to file dom/ipc/TabChild.h.rej
patching file dom/ipc/TabParent.h
Hunk #1 FAILED at 239
1 out of 1 hunks FAILED -- saving rejects to file dom/ipc/TabParent.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bug945584_part6.patch
Flags: needinfo?(kgilbert)
Keywords: checkin-needed
(Assignee)

Comment 185

2 years ago
Created attachment 8573437 [details] [diff] [review]
Bug 945584: Part 6 - Implementation of scroll snapping (v10 Patch), r=roc, r=kats

- Implemented CSS scroll snapping (http://dev.w3.org/csswg/css-snappoints/)

v10 Patch:
- Un-bitrotted (again)
Attachment #8572795 - Attachment is obsolete: true
Flags: needinfo?(kgilbert)
(Assignee)

Comment 186

2 years ago
(In reply to Carsten Book [:Tomcat] from comment #184)
> part 6 still fail to apply here:
> 
> Hunk #1 succeeded at 509 with fuzz 1 (offset 1 lines).
> patching file dom/ipc/TabChild.h
> Hunk #1 FAILED at 313
> 1 out of 1 hunks FAILED -- saving rejects to file dom/ipc/TabChild.h.rej
> patching file dom/ipc/TabParent.h
> Hunk #1 FAILED at 239
> 1 out of 1 hunks FAILED -- saving rejects to file dom/ipc/TabParent.h.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working dir
> errors during apply, please fix and refresh bug945584_part6.patch

It appears that the patches were bit-rotted mid-flight again.  I have updated the patch. Hopefully it sticks this time.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fb93b485e4c
https://hg.mozilla.org/integration/mozilla-inbound/rev/c86b570fbb78
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f18d0348d0c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2cfe7191a07
https://hg.mozilla.org/integration/mozilla-inbound/rev/a325a7b90b91
https://hg.mozilla.org/integration/mozilla-inbound/rev/b96d3d51d4b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/79989474e408
https://hg.mozilla.org/integration/mozilla-inbound/rev/18c59100d528
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e3ba87c7800
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6fb93b485e4c
https://hg.mozilla.org/mozilla-central/rev/c86b570fbb78
https://hg.mozilla.org/mozilla-central/rev/4f18d0348d0c
https://hg.mozilla.org/mozilla-central/rev/a2cfe7191a07
https://hg.mozilla.org/mozilla-central/rev/a325a7b90b91
https://hg.mozilla.org/mozilla-central/rev/b96d3d51d4b4
https://hg.mozilla.org/mozilla-central/rev/79989474e408
https://hg.mozilla.org/mozilla-central/rev/18c59100d528
https://hg.mozilla.org/mozilla-central/rev/1e3ba87c7800
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Updated

2 years ago
Blocks: 1140623

Updated

2 years ago
Depends on: 1141884
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
Release Note Request (optional, but appreciated)
[Why is this notable]:  Of interest to web developers. 
[Suggested wording]: Implement CSS scroll snapping
[Links (documentation, blog post, etc)]:  Please need-info or email me when developer documentation exists for this feature so I can link to it from the release notes, or if you'd like to improve the wording for this note.
relnote-firefox: --- → 39+
I documented the new properties. See https://developer.mozilla.org/en-US/docs/tag/CSS%20Scroll%20Snap%20Points.

I noted that with this bug two properties, scroll-snap-type-x and scroll-snap-type-y, were introduced, which are not part of the spec yet. Therefore I didn't know whether to document those already.

Sebastian
:sebo I think we should. We sould add a non-standard_header banner until they got added to the spec
Flags: needinfo?(sebastianzartner)
Keywords: dev-doc-needed → dev-doc-complete
Keywords: dev-doc-complete → dev-doc-needed
I already documented them lately.[1][2] Sorry for not updating this bug earlier!

Sebastian

[1] https://developer.mozilla.org/en-US/docs/Web/CSS/scroll-snap-type-x
[2] https://developer.mozilla.org/en-US/docs/Web/CSS/scroll-snap-type-y
Flags: needinfo?(sebastianzartner)
Keywords: dev-doc-needed → dev-doc-complete
My bad, I looked only at the Quick Links in a page that wasn't up-to-date :-)
Depends on: 1180030
Depends on: 1180167
The release note for this feature contains a link to a specific property. It would be better to link to https://developer.mozilla.org/en-US/docs/tag/CSS%20Scroll%20Snap%20Points.

Liz, are you the one that can change that?

Sebastian
Flags: needinfo?(lhenry)
Thanks Sebastian. I changed it; that does seem like a better and more general link.
Flags: needinfo?(lhenry)
Depends on: 1183770
You need to log in before you can comment on or make changes to this bug.