Closed
Bug 951793
Opened 11 years ago
Closed 7 years ago
Add support for 'overscroll-behavior'
Categories
(Core :: Panning and Zooming, defect, P3)
Core
Panning and Zooming
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
platform-rel | --- | + |
firefox48 | --- | unaffected |
firefox59 | --- | fixed |
People
(Reporter: botond, Assigned: botond)
References
()
Details
(Keywords: dev-doc-complete, feature, Whiteboard: [gfx-noted][platform-rel-Facebook])
Attachments
(15 files, 3 obsolete files)
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jgraham
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
emilio
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
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.
Comment 1•10 years ago
|
||
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
Updated•9 years ago
|
Whiteboard: gfx-noted
Updated•9 years ago
|
status-firefox48:
--- → unaffected
Assignee | ||
Comment 2•8 years ago
|
||
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
Comment 3•8 years ago
|
||
Comment 4•8 years ago
|
||
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
Updated•8 years ago
|
Whiteboard: gfx-noted, platform-rel-Facebook → [gfx-noted][platform-rel-Facebook]
Updated•8 years ago
|
platform-rel: --- → ?
Updated•8 years ago
|
platform-rel: ? → +
Assignee | ||
Updated•8 years ago
|
Summary: Disable overscroll handoff in places where it's not desirable → Add a way to disable scroll handoff
Comment 6•8 years ago
|
||
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.
Comment 7•8 years ago
|
||
I've posted a draft here:
https://wicg.github.io/scroll-boundary-behavior/
Feedback is welcome
Comment 8•8 years ago
|
||
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?
Comment 9•8 years ago
|
||
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.
Comment 10•8 years ago
|
||
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
Comment 11•8 years ago
|
||
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.
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Updated•8 years ago
|
Summary: Add a way to disable scroll handoff → Add support for 'scroll-boundary-behavior'
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 13•8 years ago
|
||
This seems like a high-value, straightforward to implement spec. I'm going to try implementing it.
Assignee: nobody → botond
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•8 years ago
|
||
This is a WIP patch series. Untested and probably not yet functional.
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 33•8 years ago
|
||
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'
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 46•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 60•7 years ago
|
||
mozreview-review |
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 61•7 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 64•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 81•7 years ago
|
||
Rebased the patch series, addressed comments on the Stylo patch, and added the suggested test.
Comment 82•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 99•7 years ago
|
||
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.
Assignee | ||
Comment 100•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 117•7 years ago
|
||
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 118•7 years ago
|
||
mozreview-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/#review206838
Attachment #8930186 -
Flags: review?(james) → review+
Comment 119•7 years ago
|
||
mozreview-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 120•7 years ago
|
||
mozreview-review |
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 121•7 years ago
|
||
mozreview-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 122•7 years ago
|
||
mozreview-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 123•7 years ago
|
||
mozreview-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 124•7 years ago
|
||
mozreview-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 125•7 years ago
|
||
mozreview-review |
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 126•7 years ago
|
||
mozreview-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 127•7 years ago
|
||
mozreview-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 128•7 years ago
|
||
mozreview-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 129•7 years ago
|
||
mozreview-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+
Updated•7 years ago
|
Attachment #8921267 -
Flags: review?(mstange) → review?(cam)
Comment 130•7 years ago
|
||
mozreview-review |
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 131•7 years ago
|
||
mozreview-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, ¶mType::SetHasScrollgrab) &&
> ReadBoolForBitfield(aMsg, aIter, aResult, ¶mType::SetAllowVerticalScrollWithWheel) &&
> ReadBoolForBitfield(aMsg, aIter, aResult, ¶mType::SetIsLayersIdRoot) &&
> ReadBoolForBitfield(aMsg, aIter, aResult, ¶mType::SetUsesContainerScrolling) &&
> - ReadBoolForBitfield(aMsg, aIter, aResult, ¶mType::SetForceDisableApz));
> + ReadBoolForBitfield(aMsg, aIter, aResult, ¶mType::SetForceDisableApz) &&
> + ReadParam(aMsg, aIter, &aResult->mOverscrollBehavior));
No enum validation?
Attachment #8921269 -
Flags: review?(mstange) → review+
Comment 132•7 years ago
|
||
mozreview-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 133•7 years ago
|
||
mozreview-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-
Assignee | ||
Comment 134•7 years ago
|
||
(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.
Assignee | ||
Comment 135•7 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #134)
> because it's passed over IRC
er, over IPC :)
Assignee | ||
Comment 136•7 years ago
|
||
(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.
Assignee | ||
Comment 137•7 years ago
|
||
(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.
Assignee | ||
Comment 138•7 years ago
|
||
(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.
Assignee | ||
Comment 139•7 years ago
|
||
(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, ¶mType::SetHasScrollgrab) &&
> > ReadBoolForBitfield(aMsg, aIter, aResult, ¶mType::SetAllowVerticalScrollWithWheel) &&
> > ReadBoolForBitfield(aMsg, aIter, aResult, ¶mType::SetIsLayersIdRoot) &&
> > ReadBoolForBitfield(aMsg, aIter, aResult, ¶mType::SetUsesContainerScrolling) &&
> > - ReadBoolForBitfield(aMsg, aIter, aResult, ¶mType::SetForceDisableApz));
> > + ReadBoolForBitfield(aMsg, aIter, aResult, ¶mType::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.
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8921267 -
Flags: review?(mstange)
Attachment #8925166 -
Flags: review?(bugmail)
Assignee | ||
Comment 157•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8925166 -
Flags: review?(bugmail)
Assignee | ||
Comment 158•7 years ago
|
||
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 159•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 160•7 years ago
|
||
(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 161•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 179•7 years ago
|
||
Comment 180•7 years ago
|
||
(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 181•7 years ago
|
||
mozreview-review |
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 182•7 years ago
|
||
mozreview-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
Updated•7 years ago
|
Attachment #8931175 -
Flags: review?(cam) → review?(mstange)
Comment 183•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 184•7 years ago
|
||
(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 :)
Assignee | ||
Comment 185•7 years ago
|
||
(I probably should have cleared the review flag when I realized the patch will likely change.)
Comment 186•7 years ago
|
||
Ah, no worries. r+ either way.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8922121 -
Attachment is obsolete: true
Assignee | ||
Comment 188•7 years ago
|
||
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.
Assignee | ||
Comment 189•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8930179 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8930186 -
Attachment is obsolete: true
Comment 205•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 206•7 years ago
|
||
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.
Assignee | ||
Comment 207•7 years ago
|
||
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 :)
Comment 208•7 years ago
|
||
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
Comment 209•7 years ago
|
||
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
Comment 210•7 years ago
|
||
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
Comment 211•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/473b81910dc0
https://hg.mozilla.org/mozilla-central/rev/59e720384c85
https://hg.mozilla.org/mozilla-central/rev/fd6df39b03fa
https://hg.mozilla.org/mozilla-central/rev/bbe200c9a567
https://hg.mozilla.org/mozilla-central/rev/36bb988687c9
https://hg.mozilla.org/mozilla-central/rev/611c648c4ad3
https://hg.mozilla.org/mozilla-central/rev/5ea4c69f933e
https://hg.mozilla.org/mozilla-central/rev/3059bf83c463
https://hg.mozilla.org/mozilla-central/rev/daca6c7ab82d
https://hg.mozilla.org/mozilla-central/rev/9eabd878337a
https://hg.mozilla.org/mozilla-central/rev/ddd4a6daae74
https://hg.mozilla.org/mozilla-central/rev/e41cccdf8082
https://hg.mozilla.org/mozilla-central/rev/7607f740e87b
https://hg.mozilla.org/mozilla-central/rev/6121a94e63b9
https://hg.mozilla.org/mozilla-central/rev/826766a15d6a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 212•7 years ago
|
||
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
Assignee | ||
Comment 213•7 years ago
|
||
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)
Comment 214•7 years ago
|
||
Looks good to me as well, thank you for documenting!
Comment 215•7 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•