Closed Bug 951793 Opened 10 years ago Closed 6 years ago

Add support for 'overscroll-behavior'

Categories

(Core :: Panning and Zooming, defect, P3)

Product:
Core
Component:
Panning and Zooming
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
platform-rel --- +
firefox48 --- unaffected
firefox59 --- fixed

People

(Reporter: botond, Assigned: botond)

References

(
URL
)

Details

(Keywords: dev-doc-complete, feature, Whiteboard: [gfx-noted][platform-rel-Facebook])

Attachments

(15 files, 3 obsolete files)

Bug 951793 - Style support for overscroll-behavior.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 951793 - Add overscroll-behavior attributes to ScrollbarStyles.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 951793 - Store the overscroll behavior in ScrollMetadata and propagate it to APZ.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 951793 - Add a few utility functions to expose the overscroll behavior in relevant places in APZ.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 951793 - Obey overscroll-behavior for immediate scroll handoff.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 951793 - Obey overscroll-behavior for fling handoff.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 951793 - Obey overscroll-behavior for any overscroll effect.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 951793 - Obey overscroll-behavior for swipe navigation.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 951793 - Add overscroll-behavior to the layer dump.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 951793 - Light refactoring to the fling handoff code.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 951793 - Obey overscroll-behavior for wheel and pan gesture events.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 951793 - Disable the timeout in the web-platform-test for overscroll-behavior.
59 bytes, text/x-review-board-request
jgraham
: review+
Details
Bug 951793 - Add a test for the pref that controls overscroll-behavior.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 951793 - Do not reconstruct the frame when overscroll-behavior has changed.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 951793 - Export a servo binding to StyleOverscrollBehavior.
59 bytes, text/x-review-board-request
emilio
: review+
Details 
Bug 898478 enabled handing off overscroll from a child APZC to a parent APZC.

There are some cases, however, where this is not desirable. For example, if the child APZC is for a text input field, it is unlikely that you want to hand off overscroll to the surrounding page.

I'm not sure yet what an appropriate mechanism is for controlling where overscroll handoff is enabled and where it isn't.
dvander has some code in bug 1142866 to disable handoff for wheel events. If we ever need to fix this bug we should consider piggybacking extra conditions on the code he's adding.
See Also: → 1142866
Whiteboard: gfx-noted
Google proposed an API for controlling scroll handoff (they call it "scroll chaining"):

https://discourse.wicg.io/t/generic-scroll-chaining-prevention-mechanism-or-expand-standardize-ms-scroll-chaining/1811
Tracking under 'platform-rel-Facebook'. This would let Facebook remove scroll listeners. It's currently used to block scrolling in chat frames from being handed off to scrolling the facebook news feed.
Whiteboard: gfx-noted → gfx-noted, platform-rel-Facebook
Whiteboard: gfx-noted, platform-rel-Facebook → [gfx-noted][platform-rel-Facebook]
platform-rel: --- → ?
platform-rel: ? → +
See Also: → 1238985
See Also: → 1050812
Summary: Disable overscroll handoff in places where it's not desirable → Add a way to disable scroll handoff
This is currently being discussion as scroll-boundary-behavior under the WICG:
https://github.com/WICG/scroll-boundary-behavior/issues

All discussion is welcome in this repo. I'll start a spec draft in the repo after a bit more discussion (or lack of) has happened.
I've posted a draft here:
https://wicg.github.io/scroll-boundary-behavior/

Feedback is welcome
The spec has received positive signals from Google, Microsoft and Mozilla and is unlikely to receive anything other than clarification edits baring implementation difficulties. The next step is to get at least two implementations to ship.

Is anyone interested in working on this?
At the moment I don't think anybody has cycles to pick this up. We're going full steam on Quantum stuff until the end of the year, pretty much.
kats@ I understand that your team may not have the cycle to prioritize the implementation of this feature but I appreciate if you can provide us a public signal on the current specification. 

We at Chromium like this feature and have an implementation landed in Canary (behind a feature flag**). So far feedback has been positive and if we don't receive any strong negative signal we plan to ship in M63. As mentioned in #8, EdgeHTML [1] we developers feedback [2,3] has been positive on the API and usecases.


** --enable-web-platform-experimental-features or --enable-blink-features=CSSScrollBoundaryBehavior
[1] https://github.com/w3c/csswg-drafts/issues/769#issuecomment-270228948
[2] https://github.com/w3c/csswg-drafts/issues/769#issuecomment-279832555
[3] https://github.com/w3c/csswg-drafts/issues/769#issuecomment-288878908
Assuming https://wicg.github.io/scroll-boundary-behavior/ is the latest version of the proposal, I think it sounds pretty reasonable. I'd be happy to implement it in Firefox once I have the time.
URL: https://wicg.github.io/scroll-boundar...
Summary: Add a way to disable scroll handoff → Add support for 'scroll-boundary-behavior'
See Also: 1238985, 1050812 → 
This seems like a high-value, straightforward to implement spec. I'm going to try implementing it.
Assignee: nobody → botond
This is a WIP patch series. Untested and probably not yet functional.
In the latest version of the spec, the property name has been changed to 'overscroll-behavior'.
Summary: Add support for 'scroll-boundary-behavior' → Add support for 'overscroll-behavior'
I've been testing and refining the implementation; it works in many cases now, but the support for trackpad scrolling (which uses wheel events) is incomplete: an element with "contain" or "none" is one direction but "auto" in the other can end up doing handoff in the forbidden direction anyways, if an event that starts a wheel transaction only has a component in the allowed direction, but a later event in the same transaction has a component in the forbidden direction.

To fix this, we'll need to annotate wheel transactions as potentially only allowing scrolling along one axis.
Comment on attachment 8922121 [details]
Bug 951793 - Additional style support for overscroll-behavior in Stylo.

https://reviewboard.mozilla.org/r/193112/#review206452

r=me, with the missing `gecko_pref`s added, and the rest of the comments addressed at your will. Ideally a test to ensure the pref works too :)

Let me know if you need any help to land the Servo bits, since you need to update the autogenerated bindings and that kind of stuff. Happy to help with that.

::: servo/components/style/properties/longhand/box.mako.rs:603
(Diff revision 3)
> +% for axis in ["x", "y"]:
> +    ${helpers.predefined_type(
> +        "overscroll-behavior-" + axis,
> +        "OverscrollBehavior",
> +        "computed::OverscrollBehavior::Auto",
> +        products="gecko",

`gecko_pref="..."`.

::: servo/components/style/properties/shorthand/box.mako.rs:358
(Diff revision 3)
>  </%helpers:shorthand>
>  
> +<%helpers:shorthand name="overscroll-behavior" products="gecko"

Needs `gecko_pref="layout.css.overscroll-behavior.enabled"`, otherwise it'd be unconditionally parsed.

If you want to add a test for this it'd be nice.

::: servo/components/style/properties/shorthand/box.mako.rs:365
(Diff revision 3)
> +<%helpers:shorthand name="overscroll-behavior" products="gecko"
> +                    sub_properties="overscroll-behavior-x overscroll-behavior-y"
> +                    spec="https://wicg.github.io/scroll-boundary-behavior/#overscroll-behavior-properties">
> +    use properties::longhands::overscroll_behavior_x;
> +
> +    pub fn parse_value<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>)

nit: I think the formatting we use lately to match https://github.com/rust-lang-nursery/fmt-rfcs would be something like:

```rust
pub fn parse_value<'i, 't>(
    context: &ParserContext,
    input: &mut Parser<'i, 't>,
) -> Result<Longhands, ParseError<'i>> {
    ...
}
```

The other longhands haven't received much love lately so they still use the old formatting... Feel free to change it or not, as you wish.

::: servo/components/style/properties/shorthand/box.mako.rs:367
(Diff revision 3)
> +                    spec="https://wicg.github.io/scroll-boundary-behavior/#overscroll-behavior-properties">
> +    use properties::longhands::overscroll_behavior_x;
> +
> +    pub fn parse_value<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>)
> +                               -> Result<Longhands, ParseError<'i>> {
> +        let result = overscroll_behavior_x::parse(context, input)?;

This can probably be just:

```
use values::specified::OverscrollBehavior;

let result = OverscrollBehavior::parse(input)?;
```

But no big deal, this works too :)

::: servo/components/style/properties/shorthand/box.mako.rs:376
(Diff revision 3)
> +        })
> +    }
> +
> +    impl<'a> ToCss for LonghandsToSerialize<'a> {
> +        // Serializes into the single keyword value if both overscroll-behavior-x and overscroll-behavior-y are same.
> +        // Otherwise into an empty string. This is done to match Gecko's behavior.

You can remove the "This is done to match Gecko's behavior". This needs to be done because otherwise we wouldn't roundtrip the value. I'm not sure the comment is super-useful :)
Attachment #8922121 - Flags: review?(emilio) → review+
Comment on attachment 8921267 [details]
Bug 951793 - Style support for overscroll-behavior.

https://reviewboard.mozilla.org/r/192282/#review206458

::: layout/style/nsStyleConsts.h:1192
(Diff revision 4)
>  // See nsStyleDisplay::mScrollBehavior
>  #define NS_STYLE_SCROLL_BEHAVIOR_AUTO       0
>  #define NS_STYLE_SCROLL_BEHAVIOR_SMOOTH     1
>  
> +// See nsStyleDisplay::mOverscrollBehavior{X,Y}
> +#define NS_STYLE_OVERSCROLL_BEHAVIOR_AUTO      0

(Just FYI, there's a bug[1] for moving all the se to enum classes, maybe it's worth to make these an enum class, so maybe it's useful to do that... After we kill the old style system we may want to autogenerate them from rust somehow, we'll see how that goes...)

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1277133
Comment on attachment 8930187 [details]
Bug 951793 - Disable the timeout in the web-platform-test for overscroll-behavior.

https://reviewboard.mozilla.org/r/201342/#review206460
Attachment #8930187 - Flags: review?(james) → review+
Rebased the patch series, addressed comments on the Stylo patch, and added the suggested test.
Comment on attachment 8930251 [details]
Bug 951793 - Add a test for the pref that controls overscroll-behavior.

https://reviewboard.mozilla.org/r/201376/#review206578

r=me, thanks!
Attachment #8930251 - Flags: review?(emilio) → review+
Fixed various failing builds and tests.

Added :jgraham as a reviewer for the web-platform-test change to make sure the change to testing/web-platform/meta (and more generally, renaming the test) is ok.
Now with the NS_STYLE_OVERSCROLL_* macro constants replaced with an enum class, as suggested in comment 61.

One follow-up simplification I'd like to make is axing the mozilla::layers::OverscrollBehavior enum; since the style constants have been replaced by a mozilla::StyleOverscrollBehavior enum, I think we might as well just use that in layers code.
Comment on attachment 8930186 [details]
Bug 951793 - Update the manual web-platform-test to reflect the name change from scroll-boundary-behavior for overscroll-behavior.

https://reviewboard.mozilla.org/r/201340/#review206838
Attachment #8930186 - Flags: review?(james) → review+
Comment on attachment 8921267 [details]
Bug 951793 - Style support for overscroll-behavior.

https://reviewboard.mozilla.org/r/192282/#review206878

::: modules/libpref/init/all.js:3094
(Diff revision 7)
>  // Are inter-character ruby annotations enabled?
>  pref("layout.css.ruby.intercharacter.enabled", false);
>  
> +// Is support for overscroll-behavior enabled?
> +#ifdef RELEASE_OR_BETA
> +pref("layout.css.overscroll-behavior.enabled", false);

You should probably reply to your intent-to-implement email an update on the name change for the CSS property.
Comment on attachment 8921271 [details]
Bug 951793 - Obey overscroll-behavior for immediate scroll handoff.

https://reviewboard.mozilla.org/r/192290/#review206884

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:2915
(Diff revision 7)
> +  ParentLayerPoint newStartPoint = startPoint;
>    treeManagerLocal->DispatchScroll(this,
> -                                   aStartPoint, aEndPoint,
> +                                   newStartPoint, endPoint,
>                                     aOverscrollHandoffState);
> +
> +  // DispatchScroll() updates the start point to reflect the portion of the
> +  // scroll consumed by downchain APZCs. Apply that update to |aStartPoint| so
> +  // the caller is informed about it.
> +  aStartPoint += (newStartPoint - startPoint);

It seems to me that this is equivalent to "aStartPoint = newStartPoint" (because prior to this line aStartPoint is exactly startPoint). And in that case there doesn't seem to be a point (ha!) to even having "newStartPoint" as a local var, you can just keep the original code and it will do the right thing by updating aStartPoint directly.
Attachment #8921271 - Flags: review?(bugmail) → review+
Comment on attachment 8921270 [details]
Bug 951793 - Add a few utility functions to expose the overscroll behavior in relevant places in APZ.

https://reviewboard.mozilla.org/r/192288/#review206886
Attachment #8921270 - Flags: review?(bugmail) → review+
Comment on attachment 8925166 [details]
Bug 951793 - Light refactoring to the fling handoff code.

https://reviewboard.mozilla.org/r/196408/#review206888

Minusing for now, I believe there's a correctness error where you're using aHandoffState instead of newHandoffState. See below.

::: gfx/layers/apz/src/APZCTreeManager.cpp:2073
(Diff revision 5)
>      }
>    } else {
>      startIndex = 0;
>    }
>  
> +  ParentLayerPoint currentVelocity = aHandoffState.mVelocity;

I'd like to move the declaration of |finalResidualVelocity| down to this point as well, since it's not used before this loop.

::: gfx/layers/apz/src/APZCTreeManager.cpp:2077
(Diff revision 5)
>  
> +  ParentLayerPoint currentVelocity = aHandoffState.mVelocity;
>    for (; startIndex < overscrollHandoffChainLength; startIndex++) {
>      current = chain->GetApzcAtIndex(startIndex);
>  
>      // Make sure the apcz about to be handled can be handled

s/apcz/apzc/ here as well

::: gfx/layers/apz/src/APZCTreeManager.cpp:2095
(Diff revision 5)
>                                   endPoint)) {
> -        return;
> +        break;
>        }
>      }
>  
> -    ParentLayerPoint transformedVelocity = endPoint - startPoint;
> +    FlingHandoffState newHandoffState = aHandoffState;

Would prefer calling this |transformedHandoffState| instead of |newHandoffState| so it's a little clearer that it holds the transformedVelocity.

::: gfx/layers/apz/src/APZCTreeManager.cpp:2105
(Diff revision 5)
> -      // If there is residual velocity, subtract the proportion of used
> -      // velocity from finalResidualVelocity and continue handoff along the
> -      // chain.
> +    if (!FuzzyEqualsAdditive(aHandoffState.mVelocity.x,
> +    // If any of the velocity available to be handed off was consumed,
> +    // subtract the proportion of consumed velocity from finalResidualVelocity.
> -      if (!FuzzyEqualsAdditive(transformedVelocity.x,
> -                               residualVelocity.x, COORDINATE_EPSILON)) {
> +                             residualVelocity.x, COORDINATE_EPSILON)) {
> -        finalResidualVelocity.x *= (residualVelocity.x / transformedVelocity.x);
> +      finalResidualVelocity.x *= (residualVelocity.x / aHandoffState.mVelocity.x);
> -      }
> +    }
> -      if (!FuzzyEqualsAdditive(transformedVelocity.y,
> +    if (!FuzzyEqualsAdditive(aHandoffState.mVelocity.y,
> -                               residualVelocity.y, COORDINATE_EPSILON)) {
> +                             residualVelocity.y, COORDINATE_EPSILON)) {
> -        finalResidualVelocity.y *= (residualVelocity.y / transformedVelocity.y);
> +      finalResidualVelocity.y *= (residualVelocity.y / aHandoffState.mVelocity.y);
> -      }

It seems to me that this code should be using newHandoffState (or transformedHandoffState) instead of aHandoffState. The old code used |tranformedVelocity| and |residualVelocity| which are now in |newHandoffState.mVelocity| and |residualVelocity| respectively.
Attachment #8925166 - Flags: review?(bugmail) → review-
Comment on attachment 8921272 [details]
Bug 951793 - Obey overscroll-behavior for fling handoff.

https://reviewboard.mozilla.org/r/192292/#review206910

I see this corrects the problem I pointed out in the previous patch. Still, it would be good to fix the previous patch.
Attachment #8921272 - Flags: review?(bugmail) → review+
Comment on attachment 8921272 [details]
Bug 951793 - Obey overscroll-behavior for fling handoff.

https://reviewboard.mozilla.org/r/192292/#review206912

::: gfx/layers/apz/src/APZCTreeManager.cpp:2099
(Diff revision 7)
>                                   endPoint)) {
>          break;
>        }
>      }
>  
> +    ParentLayerPoint availableVelocity = (endPoint - startPoint);

On second thought it's not really clear to me why |availableVelocity| is needed as a separate variable here. If you just use |newHandoffState.mVelocity| (or transformedHandoffState.mVelocity) instead of |availableVelocity| that would work. Is it just for readability?
Comment on attachment 8930179 [details]
Bug 951793 - Remove ScrollDirection::NONE.

https://reviewboard.mozilla.org/r/201334/#review206914

::: gfx/layers/LayerAttributes.h:23
(Diff revision 4)
>  namespace layers {
>  
>  // Data stored for scroll thumb container layers.
>  struct ScrollThumbData {
>    ScrollThumbData()
> -    : mDirection(ScrollDirection::NONE)
> +    : mDirection()

You can drop mDirection from the initializer entirely, Maybe<> defaults to Nothing()

::: gfx/layers/apz/src/AsyncDragMetrics.h:32
(Diff revision 4)
>    AsyncDragMetrics()
>      : mViewId(0)
>      , mPresShellId(0)
>      , mDragStartSequenceNumber(0)
>      , mScrollbarDragOffset(0)
> -    , mDirection(ScrollDirection::NONE)
> +    , mDirection()

Ditto, drop initializer
Attachment #8930179 - Flags: review?(bugmail) → review+
Comment on attachment 8925167 [details]
Bug 951793 - Obey overscroll-behavior for wheel and pan gesture events.

https://reviewboard.mozilla.org/r/196410/#review206922
Attachment #8925167 - Flags: review?(bugmail) → review+
Comment on attachment 8921273 [details]
Bug 951793 - Obey overscroll-behavior for any overscroll effect.

https://reviewboard.mozilla.org/r/192294/#review206924

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:2904
(Diff revision 7)
> +      }
> +      if (!mY.OverscrollBehaviorAllowsOverscrollEffect()) {
> +        residualVelocity.y = 0;
> +      }
> +
>        mOverscrollEffect->HandleFlingOverscroll(residualVelocity);

Maybe wrap this in another !IsZero(residualVelocity) check?
Attachment #8921273 - Flags: review?(bugmail) → review+
Comment on attachment 8922122 [details]
Bug 951793 - Add overscroll-behavior to the layer dump.

https://reviewboard.mozilla.org/r/193114/#review206926
Attachment #8922122 - Flags: review?(bugmail) → review+
Comment on attachment 8930186 [details]
Bug 951793 - Update the manual web-platform-test to reflect the name change from scroll-boundary-behavior for overscroll-behavior.

https://reviewboard.mozilla.org/r/201340/#review206928
Attachment #8930186 - Flags: review?(bugmail) → review+
Attachment #8921267 - Flags: review?(mstange) → review?(cam)
Comment on attachment 8921268 [details]
Bug 951793 - Add overscroll-behavior attributes to ScrollbarStyles.

https://reviewboard.mozilla.org/r/192284/#review207002
Attachment #8921268 - Flags: review?(mstange) → review+
Comment on attachment 8921269 [details]
Bug 951793 - Store the overscroll behavior in ScrollMetadata and propagate it to APZ.

https://reviewboard.mozilla.org/r/192286/#review207004

::: gfx/layers/ipc/LayersMessageUtils.h:299
(Diff revision 7)
>              ReadBoolForBitfield(aMsg, aIter, aResult, &paramType::SetHasScrollgrab) &&
>              ReadBoolForBitfield(aMsg, aIter, aResult, &paramType::SetAllowVerticalScrollWithWheel) &&
>              ReadBoolForBitfield(aMsg, aIter, aResult, &paramType::SetIsLayersIdRoot) &&
>              ReadBoolForBitfield(aMsg, aIter, aResult, &paramType::SetUsesContainerScrolling) &&
> -            ReadBoolForBitfield(aMsg, aIter, aResult, &paramType::SetForceDisableApz));
> +            ReadBoolForBitfield(aMsg, aIter, aResult, &paramType::SetForceDisableApz) &&
> +            ReadParam(aMsg, aIter, &aResult->mOverscrollBehavior));

No enum validation?
Attachment #8921269 - Flags: review?(mstange) → review+
Comment on attachment 8921274 [details]
Bug 951793 - Obey overscroll-behavior for swipe navigation.

https://reviewboard.mozilla.org/r/192296/#review207008
Attachment #8921274 - Flags: review?(mstange) → review+
Comment on attachment 8921267 [details]
Bug 951793 - Style support for overscroll-behavior.

https://reviewboard.mozilla.org/r/192282/#review207186

Couple of small changes to make.

::: layout/style/Declaration.cpp:1479
(Diff revision 7)
> +      // If overscroll-behavior-x and overscroll-behavior-y are not
> +      // equal, we don't have a shorthand that can express. Bail.

The spec seems to define the overscroll-behavior shorthand as taking two values.  That would mean we can serialize two different values here.

::: layout/style/nsCSSParser.cpp:17502
(Diff revision 7)
> +  if (!ParseSingleTokenVariant(value, VARIANT_HK,
> +                               nsCSSProps::kOverscrollBehaviorKTable)) {
> +    return false;
> +  }
> +  AppendValue(eCSSProperty_overscroll_behavior_x, value);
> +  AppendValue(eCSSProperty_overscroll_behavior_y, value);

This needs to be able to parse two values.

::: layout/style/nsComputedDOMStyle.h:517
(Diff revision 7)
>    already_AddRefed<CSSValue> DoGetBackfaceVisibility();
>    already_AddRefed<CSSValue> DoGetPerspectiveOrigin();
>    already_AddRefed<CSSValue> DoGetTransformStyle();
>    already_AddRefed<CSSValue> DoGetOrient();
>    already_AddRefed<CSSValue> DoGetScrollBehavior();
> +  already_AddRefed<CSSValue> DoGetOverscrollBehavior();

Computed style objects generally don't expose shorthands.  (We only do so if a longhand property gets turned into a shorthand and split up into longhands.)  So, you can remove DoGetOverscrollBehavior.

::: layout/style/nsStyleStruct.cpp:3635
(Diff revision 7)
> +      || mOverscrollBehaviorX != aNewData.mOverscrollBehaviorX
> +      || mOverscrollBehaviorY != aNewData.mOverscrollBehaviorY

I assume it's not worth adding a new change hint to handle changes to these properties in a cheaper way than reconstructing the frame.

::: layout/style/test/property_database.js:7478
(Diff revision 7)
> +    other_values: [ "contain", "none" ],
> +    invalid_values: [ "left", "1px" ]

Let's put some valid two-value values in other_values, and maybe an invalid three-value value in invalid_values.
Attachment #8921267 - Flags: review?(cam) → review-
(In reply to Botond Ballo [:botond] from comment #117)
> One follow-up simplification I'd like to make is axing the
> mozilla::layers::OverscrollBehavior enum; since the style constants have
> been replaced by a mozilla::StyleOverscrollBehavior enum, I think we might
> as well just use that in layers code.

I realized we can't actually do that; mozilla::layers::OverscrollBehavior needs to be defined using MOZ_DEFINE_ENUM_CLASS, because it's passed over IRC and needs to know its max value.
(In reply to Botond Ballo [:botond] from comment #134)
> because it's passed over IRC

er, over IPC :)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #120)
> Comment on attachment 8921271 [details]
> Bug 951793 - Obey overscroll-behavior for immediate scroll handoff.
> 
> https://reviewboard.mozilla.org/r/192290/#review206884
> 
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:2915
> (Diff revision 7)
> > +  ParentLayerPoint newStartPoint = startPoint;
> >    treeManagerLocal->DispatchScroll(this,
> > -                                   aStartPoint, aEndPoint,
> > +                                   newStartPoint, endPoint,
> >                                     aOverscrollHandoffState);
> > +
> > +  // DispatchScroll() updates the start point to reflect the portion of the
> > +  // scroll consumed by downchain APZCs. Apply that update to |aStartPoint| so
> > +  // the caller is informed about it.
> > +  aStartPoint += (newStartPoint - startPoint);
> 
> It seems to me that this is equivalent to "aStartPoint = newStartPoint"
> (because prior to this line aStartPoint is exactly startPoint). And in that
> case there doesn't seem to be a point (ha!) to even having "newStartPoint"
> as a local var, you can just keep the original code and it will do the right
> thing by updating aStartPoint directly.

Good catch. I think this was left over from a previous revision of this change when I was updating |startPoint| (rather than |endPoint|) to obey overscroll-behavior.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #122)
> Comment on attachment 8925166 [details]
> Bug 951793 - Light refactoring to the fling handoff code.
> 
> https://reviewboard.mozilla.org/r/196408/#review206888
> 
> ::: gfx/layers/apz/src/APZCTreeManager.cpp:2105
> (Diff revision 5)
> > -      // If there is residual velocity, subtract the proportion of used
> > -      // velocity from finalResidualVelocity and continue handoff along the
> > -      // chain.
> > +    if (!FuzzyEqualsAdditive(aHandoffState.mVelocity.x,
> > +    // If any of the velocity available to be handed off was consumed,
> > +    // subtract the proportion of consumed velocity from finalResidualVelocity.
> > -      if (!FuzzyEqualsAdditive(transformedVelocity.x,
> > -                               residualVelocity.x, COORDINATE_EPSILON)) {
> > +                             residualVelocity.x, COORDINATE_EPSILON)) {
> > -        finalResidualVelocity.x *= (residualVelocity.x / transformedVelocity.x);
> > +      finalResidualVelocity.x *= (residualVelocity.x / aHandoffState.mVelocity.x);
> > -      }
> > +    }
> > -      if (!FuzzyEqualsAdditive(transformedVelocity.y,
> > +    if (!FuzzyEqualsAdditive(aHandoffState.mVelocity.y,
> > -                               residualVelocity.y, COORDINATE_EPSILON)) {
> > +                             residualVelocity.y, COORDINATE_EPSILON)) {
> > -        finalResidualVelocity.y *= (residualVelocity.y / transformedVelocity.y);
> > +      finalResidualVelocity.y *= (residualVelocity.y / aHandoffState.mVelocity.y);
> > -      }
> 
> It seems to me that this code should be using newHandoffState (or
> transformedHandoffState) instead of aHandoffState. The old code used
> |tranformedVelocity| and |residualVelocity| which are now in
> |newHandoffState.mVelocity| and |residualVelocity| respectively.

Yep, good catch here too. As you noticed, I caught this when writing the next patch, but forgot to go back and change this one.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #124)
> Comment on attachment 8921272 [details]
> Bug 951793 - Obey overscroll-behavior for fling handoff.
> 
> https://reviewboard.mozilla.org/r/192292/#review206912
> 
> ::: gfx/layers/apz/src/APZCTreeManager.cpp:2099
> (Diff revision 7)
> >                                   endPoint)) {
> >          break;
> >        }
> >      }
> >  
> > +    ParentLayerPoint availableVelocity = (endPoint - startPoint);
> 
> On second thought it's not really clear to me why |availableVelocity| is
> needed as a separate variable here. If you just use
> |newHandoffState.mVelocity| (or transformedHandoffState.mVelocity) instead
> of |availableVelocity| that would work. Is it just for readability?

It's not just for readability. |availableVelocity| is the velocity available to be handed off. |newHandoffState.mVelocity| is initialized to |availableVelocity|, but is then passed through AdjustHandoffVelocityForOverscrollBehavior(), which can chop off one or both of its components, making it the velocity that will actually be handed off.

When calculating the amount to add to |finalResidualVelocity|, we want to use |availableVelocity|; that way, if overscroll-behavior prevented handoff but allows an overscroll effect, the caller knows that there was residual velocity and so it should show the overscroll effect.

I'll add a comment to make this a bit clearer.
(In reply to Markus Stange [:mstange] from comment #131)
> Comment on attachment 8921269 [details]
> Bug 951793 - Store the overscroll behavior in ScrollMetadata and propagate
> it to APZ.
> 
> https://reviewboard.mozilla.org/r/192286/#review207004
> 
> ::: gfx/layers/ipc/LayersMessageUtils.h:299
> (Diff revision 7)
> >              ReadBoolForBitfield(aMsg, aIter, aResult, &paramType::SetHasScrollgrab) &&
> >              ReadBoolForBitfield(aMsg, aIter, aResult, &paramType::SetAllowVerticalScrollWithWheel) &&
> >              ReadBoolForBitfield(aMsg, aIter, aResult, &paramType::SetIsLayersIdRoot) &&
> >              ReadBoolForBitfield(aMsg, aIter, aResult, &paramType::SetUsesContainerScrolling) &&
> > -            ReadBoolForBitfield(aMsg, aIter, aResult, &paramType::SetForceDisableApz));
> > +            ReadBoolForBitfield(aMsg, aIter, aResult, &paramType::SetForceDisableApz) &&
> > +            ReadParam(aMsg, aIter, &aResult->mOverscrollBehavior));
> 
> No enum validation?

Oh whoops! I went through the trouble of using MOZ_DEFINE_ENUM_CLASS to define OverscrollBehavior so that I could use ContiguousEnumSerializer to serialize it, but then used PlainOldDataSerializer to serialize the enclosing class, OverscrollBehaviorInfo, thereby bypassing individual serialization of the members. Will fix.
(In reply to Botond Ballo [:botond] from comment #138)
> It's not just for readability. |availableVelocity| is the velocity available
> to be handed off. |newHandoffState.mVelocity| is initialized to
> |availableVelocity|, but is then passed through
> AdjustHandoffVelocityForOverscrollBehavior(), which can chop off one or both
> of its components, making it the velocity that will actually be handed off.
> 

Ah yes, that makes sense. I forgot about the in-out parameter behaviour of AdjustHandoffVelocityForOverscrollBehavior.
Attachment #8921267 - Flags: review?(mstange)
Attachment #8925166 - Flags: review?(bugmail)
Comment on attachment 8921267 [details]
Bug 951793 - Style support for overscroll-behavior.

(In reply to Cameron McCormack (:heycam) from comment #133)
> ::: layout/style/Declaration.cpp:1479
> (Diff revision 7)
> > +      // If overscroll-behavior-x and overscroll-behavior-y are not
> > +      // equal, we don't have a shorthand that can express. Bail.
> 
> The spec seems to define the overscroll-behavior shorthand as taking two
> values.  That would mean we can serialize two different values here.

Thanks for catching that. I admit, I've kind of cargo-culted a lot of this patch from the 
corresponding patch from scroll-snap-type, and overlooked the fact that that doesn't allow two values while this does.

> ::: layout/style/nsStyleStruct.cpp:3635
> (Diff revision 7)
> > +      || mOverscrollBehaviorX != aNewData.mOverscrollBehaviorX
> > +      || mOverscrollBehaviorY != aNewData.mOverscrollBehaviorY
> 
> I assume it's not worth adding a new change hint to handle changes to these
> properties in a cheaper way than reconstructing the frame.

It might be. I'll explore that in a new patch.

The other comments should be addressed.
Attachment #8921267 - Flags: review?(cam)
Attachment #8925166 - Flags: review?(bugmail)
Comment on attachment 8922121 [details]
Bug 951793 - Additional style support for overscroll-behavior in Stylo.

As part of addressing :heycam's comment about accepting two values, I made some changes to the Stylo patch as well. Asking for re-review on that.
Attachment #8922121 - Flags: review+ → review?(emilio)
Comment on attachment 8922121 [details]
Bug 951793 - Additional style support for overscroll-behavior in Stylo.

https://reviewboard.mozilla.org/r/193112/#review207598

That fixed and test added -> r=me

::: servo/components/style/properties/shorthand/box.mako.rs:366
(Diff revision 7)
> +<%helpers:shorthand name="overscroll-behavior" products="gecko"
> +                    gecko_pref="layout.css.overscroll-behavior.enabled"
> +                    sub_properties="overscroll-behavior-x overscroll-behavior-y"
> +                    spec="https://wicg.github.io/scroll-boundary-behavior/#overscroll-behavior-properties">
> +    pub fn parse_value<'i, 't>(
> +        _: &ParserContext, 

nit: whitespace.

::: servo/components/style/properties/shorthand/box.mako.rs:371
(Diff revision 7)
> +        _: &ParserContext, 
> +        input: &mut Parser<'i, 't>
> +    ) -> Result<Longhands, ParseError<'i>> {
> +        use values::specified::OverscrollBehavior;
> +        let behavior_x = OverscrollBehavior::parse(input)?;
> +        let behavior_y = OverscrollBehavior::parse(input).unwrap_or(behavior_x);

This needs to use `input.try()`, otherwise it'll eat  the next token in the input

Please add a test for this in the property database, something like `contain nonsense`, in which case with this patch we'd parse as `contain contain` instead of properly rejecting this.

Thus:

```
let behavior_y = input.try(OverscrollBehavior::parse).unwrap_or(behavior_x);
```
Attachment #8922121 - Flags: review?(emilio) → review+
(In reply to Botond Ballo [:botond] from comment #157)
> > ::: layout/style/nsStyleStruct.cpp:3635
> > (Diff revision 7)
> > > +      || mOverscrollBehaviorX != aNewData.mOverscrollBehaviorX
> > > +      || mOverscrollBehaviorY != aNewData.mOverscrollBehaviorY
> > 
> > I assume it's not worth adding a new change hint to handle changes to these
> > properties in a cheaper way than reconstructing the frame.
> 
> It might be. I'll explore that in a new patch.

I think using nsChangeHint_RepaintFrame would be appropriate, since the overflow-behavior is propagated from nsStyleDisplay to ScrollMetadata during painting.

We may be able to optimize it further and avoid actually repainting textures, for example by using an empty transaction, but I think that will involve changes elsewhere (in painting code), not using a different change hint.
Comment on attachment 8925166 [details]
Bug 951793 - Light refactoring to the fling handoff code.

https://reviewboard.mozilla.org/r/196408/#review207628
Attachment #8925166 - Flags: review?(bugmail) → review+
(In reply to Botond Ballo [:botond] from comment #160)
> (In reply to Botond Ballo [:botond] from comment #157)
> > > ::: layout/style/nsStyleStruct.cpp:3635
> > > (Diff revision 7)
> > > > +      || mOverscrollBehaviorX != aNewData.mOverscrollBehaviorX
> > > > +      || mOverscrollBehaviorY != aNewData.mOverscrollBehaviorY
> > > 
> > > I assume it's not worth adding a new change hint to handle changes to these
> > > properties in a cheaper way than reconstructing the frame.
> > 
> > It might be. I'll explore that in a new patch.
> 
> I think using nsChangeHint_RepaintFrame would be appropriate, since the
> overflow-behavior is propagated from nsStyleDisplay to ScrollMetadata during
> painting.
> 
> We may be able to optimize it further and avoid actually repainting
> textures,

I think nsChangeHint_SchedulePaint would achieve this. It will cause us to rebuild the display list (at least the part of the display list for the affected frame + its descendants) and run layerization, which I think is all that's needed here.
Comment on attachment 8921267 [details]
Bug 951793 - Style support for overscroll-behavior.

https://reviewboard.mozilla.org/r/192282/#review207960
Attachment #8921267 - Flags: review?(cam) → review+
Comment on attachment 8931175 [details]
Bug 951793 - Do not reconstruct the frame when overscroll-behavior has changed.

https://reviewboard.mozilla.org/r/202252/#review207962
Attachment #8931175 - Flags: review?(cam) → review?(mstange)
Comment on attachment 8931175 [details]
Bug 951793 - Do not reconstruct the frame when overscroll-behavior has changed.

https://reviewboard.mozilla.org/r/202252/#review207968

So you decided against SchedulePaint?
Attachment #8931175 - Flags: review?(mstange) → review+
(In reply to Markus Stange [:mstange] from comment #183)
> So you decided against SchedulePaint?

No, I posted this patch before our conversation about SchedulePaint, and haven't had a chance to revise it yet :)
(I probably should have cleared the review flag when I realized the patch will likely change.)
Ah, no worries. r+ either way.
Attachment #8922121 - Attachment is obsolete: true
Updated to use nsChangeHint_SchedulePaint as discussed with Markus. Confirmed that this is working as intended.

I axed the Stylo patch from the series because that's going to land via Github.
(In reply to Botond Ballo [:botond] from comment #188)
> I axed the Stylo patch from the series because that's going to land via
> Github.

Oh, actually, that patch contained a change outside of servo/ directory as well (to layout/style/ServoBindings.toml). I guess I need to split that out into a separate patch.
Depends on: 1420516
Attachment #8930179 - Attachment is obsolete: true
Attachment #8930186 - Attachment is obsolete: true
Comment on attachment 8931787 [details]
Bug 951793 - Export a servo binding to StyleOverscrollBehavior.

https://reviewboard.mozilla.org/r/202920/#review208240
Attachment #8931787 - Flags: review?(emilio) → review+
A few updates here:

  - Rebased patches to apply to latest m-c

  - Re-added the change to layout/style/ServoBindings.toml 
    as a separate change here

  - Moved the "Remove ScrollDirection::NONE" patch to a 
    separate bug (bug 1420516) to unblock bug 1418387.
Oh, the patch to update the w-p-t to reflect the name change is also gone, because it has meanwhile been made upstream by Blink :)
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e61ac8e48971
Style support for overscroll-behavior. r=heycam
https://hg.mozilla.org/integration/autoland/rev/867d8ea5355c
Add overscroll-behavior attributes to ScrollbarStyles. r=mstange
https://hg.mozilla.org/integration/autoland/rev/fa82cdc01408
Store the overscroll behavior in ScrollMetadata and propagate it to APZ. r=mstange
https://hg.mozilla.org/integration/autoland/rev/386f77004d89
Add a few utility functions to expose the overscroll behavior in relevant places in APZ. r=kats
https://hg.mozilla.org/integration/autoland/rev/fa6da1e723cf
Obey overscroll-behavior for immediate scroll handoff. r=kats
https://hg.mozilla.org/integration/autoland/rev/7055bd5dfc4e
Light refactoring to the fling handoff code. r=kats
https://hg.mozilla.org/integration/autoland/rev/be4e22e5c257
Obey overscroll-behavior for fling handoff. r=kats
https://hg.mozilla.org/integration/autoland/rev/50fe3c6ac486
Obey overscroll-behavior for wheel and pan gesture events. r=kats
https://hg.mozilla.org/integration/autoland/rev/c3340b84e534
Obey overscroll-behavior for any overscroll effect. r=kats
https://hg.mozilla.org/integration/autoland/rev/884913aa1668
Obey overscroll-behavior for swipe navigation. r=mstange
https://hg.mozilla.org/integration/autoland/rev/713a3c9617ce
Add overscroll-behavior to the layer dump. r=kats
https://hg.mozilla.org/integration/autoland/rev/054e837609d0
Disable the timeout in the web-platform-test for overscroll-behavior. r=jgraham
https://hg.mozilla.org/integration/autoland/rev/a5e529f52fb1
Add a test for the pref that controls overscroll-behavior. r=emilio
https://hg.mozilla.org/integration/autoland/rev/6eef6403fa71
Do not reconstruct the frame when overscroll-behavior has changed. r=mstange
https://hg.mozilla.org/integration/autoland/rev/ca8c86e229df
Export a servo binding to StyleOverscrollBehavior. r=emilio
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/d7a700707ddb
Backed out 15 changesets because it landed before the necessary servo changes. r=backout requested by emilio on a CLOSED TREE
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/473b81910dc0
Style support for overscroll-behavior. r=heycam
https://hg.mozilla.org/integration/autoland/rev/59e720384c85
Add overscroll-behavior attributes to ScrollbarStyles. r=mstange
https://hg.mozilla.org/integration/autoland/rev/fd6df39b03fa
Store the overscroll behavior in ScrollMetadata and propagate it to APZ. r=mstange
https://hg.mozilla.org/integration/autoland/rev/bbe200c9a567
Add a few utility functions to expose the overscroll behavior in relevant places in APZ. r=kats
https://hg.mozilla.org/integration/autoland/rev/36bb988687c9
Obey overscroll-behavior for immediate scroll handoff. r=kats
https://hg.mozilla.org/integration/autoland/rev/611c648c4ad3
Light refactoring to the fling handoff code. r=kats
https://hg.mozilla.org/integration/autoland/rev/5ea4c69f933e
Obey overscroll-behavior for fling handoff. r=kats
https://hg.mozilla.org/integration/autoland/rev/3059bf83c463
Obey overscroll-behavior for wheel and pan gesture events. r=kats
https://hg.mozilla.org/integration/autoland/rev/daca6c7ab82d
Obey overscroll-behavior for any overscroll effect. r=kats
https://hg.mozilla.org/integration/autoland/rev/9eabd878337a
Obey overscroll-behavior for swipe navigation. r=mstange
https://hg.mozilla.org/integration/autoland/rev/ddd4a6daae74
Add overscroll-behavior to the layer dump. r=kats
https://hg.mozilla.org/integration/autoland/rev/e41cccdf8082
Disable the timeout in the web-platform-test for overscroll-behavior. r=jgraham
https://hg.mozilla.org/integration/autoland/rev/7607f740e87b
Add a test for the pref that controls overscroll-behavior. r=emilio
https://hg.mozilla.org/integration/autoland/rev/6121a94e63b9
Do not reconstruct the frame when overscroll-behavior has changed. r=mstange
https://hg.mozilla.org/integration/autoland/rev/826766a15d6a
Export a servo binding to StyleOverscrollBehavior. r=emilio
Blocks: 1425485
Blocks: 1425573
Blocks: 1425603
I have documented this.

Created pages for the new properties:
https://developer.mozilla.org/en-US/docs/Web/CSS/overscroll-behavior
https://developer.mozilla.org/en-US/docs/Web/CSS/overscroll-behavior-x
https://developer.mozilla.org/en-US/docs/Web/CSS/overscroll-behavior-y

Created demos to show them in action:
* https://github.com/mdn/css-examples/tree/master/overscroll-behavior

Updated the necessary repos to add the specification to MDN, add the missing formal syntax info, and add the relevant browser compat data (respectively):
* https://github.com/mdn/kumascript/pull/562
* https://github.com/mdn/data/pull/167
* https://github.com/mdn/browser-compat-data/pull/898

Added a note to the Fx59 rel notes:
* https://developer.mozilla.org/en-US/Firefox/Releases/59

Let me know if this is OK. Thanks!
Flags: needinfo?(botond)
Keywords: dev-doc-needed → dev-doc-complete
That all looks great, Chris, thanks for doing this!

The only thing I'd like to note is that the spec will soon graduate from WICG to CSSWG [1]. When that happens, we should probably update the spec links accordingly.

[1] https://github.com/w3c/csswg-drafts/issues/2179
Flags: needinfo?(botond)
Looks good to me as well, thank you for documenting!
(In reply to Botond Ballo [:botond] from comment #213)
> That all looks great, Chris, thanks for doing this!
> 
> The only thing I'd like to note is that the spec will soon graduate from
> WICG to CSSWG [1]. When that happens, we should probably update the spec
> links accordingly.
> 
> [1] https://github.com/w3c/csswg-drafts/issues/2179

OK, thanks Botond. I've subscribed to that thread, and will update the spec links when I see the change has been made.


(In reply to Benoit Girard (:BenWa) from comment #214)
> Looks good to me as well, thank you for documenting!

You are welcome.
Blocks: 1453472
You need to log in before you can comment on or make changes to this bug.