WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
220139
Implement CSS overscroll-behavior for asynchronous scroll on Mac
https://bugs.webkit.org/show_bug.cgi?id=220139
Summary
Implement CSS overscroll-behavior for asynchronous scroll on Mac
cathiechen
Reported
2020-12-24 01:52:41 PST
Implement CSS property overscroll-behavior on Mac
Attachments
Patch
(85.34 KB, patch)
2020-12-24 03:21 PST
,
cathiechen
no flags
Details
Formatted Diff
Diff
Patch
(85.35 KB, patch)
2020-12-24 03:25 PST
,
cathiechen
no flags
Details
Formatted Diff
Diff
Patch
(85.25 KB, patch)
2020-12-24 03:37 PST
,
cathiechen
no flags
Details
Formatted Diff
Diff
Patch
(85.25 KB, patch)
2020-12-24 03:46 PST
,
cathiechen
no flags
Details
Formatted Diff
Diff
Patch
(85.27 KB, patch)
2020-12-24 04:38 PST
,
cathiechen
no flags
Details
Formatted Diff
Diff
Patch
(85.29 KB, patch)
2020-12-24 08:28 PST
,
cathiechen
no flags
Details
Formatted Diff
Diff
Patch
(79.06 KB, patch)
2021-01-15 10:16 PST
,
cathiechen
no flags
Details
Formatted Diff
Diff
Patch
(81.33 KB, patch)
2021-01-18 00:06 PST
,
cathiechen
no flags
Details
Formatted Diff
Diff
Patch
(81.57 KB, patch)
2021-01-18 00:41 PST
,
cathiechen
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(81.57 KB, patch)
2021-01-18 00:50 PST
,
cathiechen
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(81.56 KB, patch)
2021-01-18 01:00 PST
,
cathiechen
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(81.46 KB, patch)
2021-01-18 01:43 PST
,
cathiechen
no flags
Details
Formatted Diff
Diff
async-overscroll-behavior-for-ews
(95.78 KB, patch)
2021-03-15 06:22 PDT
,
cathiechen
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
async-overscroll-behavior-for-review
(38.51 KB, patch)
2021-03-15 06:23 PDT
,
cathiechen
no flags
Details
Formatted Diff
Diff
async-overscroll-behavior-for-ews
(97.06 KB, patch)
2021-03-15 22:48 PDT
,
cathiechen
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
async-overscroll-behavior-for-ews
(96.55 KB, patch)
2021-03-22 08:47 PDT
,
cathiechen
no flags
Details
Formatted Diff
Diff
Patch
(18.42 KB, patch)
2021-11-17 16:26 PST
,
Nikos Mouchtaris
no flags
Details
Formatted Diff
Diff
Patch
(32.14 KB, patch)
2021-11-30 23:22 PST
,
Nikos Mouchtaris
no flags
Details
Formatted Diff
Diff
Patch
(32.79 KB, patch)
2021-12-01 11:21 PST
,
Nikos Mouchtaris
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(32.79 KB, patch)
2021-12-01 11:38 PST
,
Nikos Mouchtaris
no flags
Details
Formatted Diff
Diff
Patch
(32.39 KB, patch)
2021-12-09 16:37 PST
,
Nikos Mouchtaris
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(32.40 KB, patch)
2021-12-09 17:10 PST
,
Nikos Mouchtaris
no flags
Details
Formatted Diff
Diff
Patch
(32.40 KB, patch)
2021-12-10 14:28 PST
,
Nikos Mouchtaris
no flags
Details
Formatted Diff
Diff
Patch
(34.20 KB, patch)
2022-01-21 17:09 PST
,
Nikos Mouchtaris
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(34.73 KB, patch)
2022-01-21 19:21 PST
,
Nikos Mouchtaris
no flags
Details
Formatted Diff
Diff
Patch
(35.47 KB, patch)
2022-01-24 15:58 PST
,
Nikos Mouchtaris
no flags
Details
Formatted Diff
Diff
Patch
(35.56 KB, patch)
2022-01-25 13:11 PST
,
Nikos Mouchtaris
no flags
Details
Formatted Diff
Diff
Patch
(34.52 KB, patch)
2022-01-27 17:43 PST
,
Nikos Mouchtaris
no flags
Details
Formatted Diff
Diff
Patch
(34.68 KB, patch)
2022-01-28 16:55 PST
,
Nikos Mouchtaris
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(28)
View All
Add attachment
proposed patch, testcase, etc.
cathiechen
Comment 1
2020-12-24 03:21:03 PST
Comment hidden (obsolete)
Created
attachment 416742
[details]
Patch
cathiechen
Comment 2
2020-12-24 03:25:15 PST
Comment hidden (obsolete)
Created
attachment 416743
[details]
Patch
cathiechen
Comment 3
2020-12-24 03:37:32 PST
Comment hidden (obsolete)
Created
attachment 416744
[details]
Patch
cathiechen
Comment 4
2020-12-24 03:46:39 PST
Comment hidden (obsolete)
Created
attachment 416745
[details]
Patch
cathiechen
Comment 5
2020-12-24 04:38:16 PST
Comment hidden (obsolete)
Created
attachment 416747
[details]
Patch
cathiechen
Comment 6
2020-12-24 08:28:51 PST
Comment hidden (obsolete)
Created
attachment 416751
[details]
Patch
Radar WebKit Bug Importer
Comment 7
2020-12-31 01:53:13 PST
<
rdar://problem/72751742
>
cathiechen
Comment 8
2021-01-15 10:16:52 PST
Comment hidden (obsolete)
Created
attachment 417713
[details]
Patch
cathiechen
Comment 9
2021-01-18 00:06:50 PST
Comment hidden (obsolete)
Created
attachment 417806
[details]
Patch
cathiechen
Comment 10
2021-01-18 00:41:50 PST
Comment hidden (obsolete)
Created
attachment 417809
[details]
Patch
cathiechen
Comment 11
2021-01-18 00:50:30 PST
Comment hidden (obsolete)
Created
attachment 417811
[details]
Patch
cathiechen
Comment 12
2021-01-18 01:00:25 PST
Comment hidden (obsolete)
Created
attachment 417813
[details]
Patch
cathiechen
Comment 13
2021-01-18 01:43:11 PST
Comment hidden (obsolete)
Created
attachment 417818
[details]
Patch
cathiechen
Comment 14
2021-01-18 03:25:48 PST
Comment on
attachment 417818
[details]
Patch Hi, I think this patch is ready to review. Please take a look, thanks:)
cathiechen
Comment 15
2021-03-15 06:22:51 PDT
Created
attachment 423169
[details]
async-overscroll-behavior-for-ews
cathiechen
Comment 16
2021-03-15 06:23:26 PDT
Created
attachment 423170
[details]
async-overscroll-behavior-for-review
cathiechen
Comment 17
2021-03-15 22:48:29 PDT
Created
attachment 423301
[details]
async-overscroll-behavior-for-ews
cathiechen
Comment 18
2021-03-22 08:47:58 PDT
Created
attachment 423891
[details]
async-overscroll-behavior-for-ews
Nikos Mouchtaris
Comment 19
2021-11-17 16:26:37 PST
Comment hidden (obsolete)
Created
attachment 444598
[details]
Patch
Nikos Mouchtaris
Comment 20
2021-11-30 23:22:34 PST
Comment hidden (obsolete)
Created
attachment 445528
[details]
Patch
Nikos Mouchtaris
Comment 21
2021-12-01 11:21:02 PST
Comment hidden (obsolete)
Created
attachment 445595
[details]
Patch
Nikos Mouchtaris
Comment 22
2021-12-01 11:38:34 PST
Comment hidden (obsolete)
Created
attachment 445600
[details]
Patch
cathiechen
Comment 23
2021-12-02 01:14:57 PST
Comment on
attachment 445600
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=445600&action=review
Hi Nikolaos, Thanks for working on this!
> Source/WebCore/page/scrolling/ScrollingTree.cpp:211 > + if (scrollingNode.hasHorizontalOverscrollBehavior() || scrollingNode.hasVerticalOverscrollBehavior()) {
It seems if we only check the overscroll behavior of the current scrollingNode here, the wheelEvent might skip the scroll chaining blocker. For instance, <div id="parent"> <div id="scroller" style="overscroll-behavior-y: none;"> </div> </div> First, let's horizontally scroll "scroller", for overscroll-behavior-x is auto and it's horizontal scroll, so the wheel event can pass on the tree to "parent". Then we start to scroll vertically, because the current scrolling node is "parent", so it won't break the scroll chaining.
Simon Fraser (smfr)
Comment 24
2021-12-09 10:52:19 PST
Comment on
attachment 445600
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=445600&action=review
> Source/WebCore/page/scrolling/ScrollingTree.h:237 > + bool shouldBlockScrollChainingWithNode(const ScrollingTreeScrollingNode&, PlatformWheelEvent&, bool&);
We don't use the term "scroll chaining" anywhere else. I would prefer something like "scroll propagation". Can this function be const? What does the the bool& param mean? Why isn't this a function on ScrollingTreeScrollingNode?
> Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm:218 > + if (scrollingNode().horizontalOverscrollBehavior() == OverscrollBehavior::None) > + return false; > + > switch (horizontalScrollElasticity()) {
Does OverscrollBehavior not change the value of elasticity?
> Source/WebCore/rendering/RenderLayerCompositor.cpp:3953 > + if (oldStyle && (oldStyle->overscrollBehaviorX() != renderer.style().overscrollBehaviorX() || oldStyle->overscrollBehaviorY() != renderer.style().overscrollBehaviorY())) { > + if (auto* layer = m_renderView.layer()) > + layer->setNeedsCompositingGeometryUpdate();
Is this the signal we use elsewhere for updating the scrolling tree? RenderLayerCompositor::updateBackingAndHierarchy() calls updateScrollCoordinationForLayer() unconditionally, so it's not clear that this is the best signal.
Nikos Mouchtaris
Comment 25
2021-12-09 16:37:52 PST
Comment hidden (obsolete)
Created
attachment 446631
[details]
Patch
Nikos Mouchtaris
Comment 26
2021-12-09 17:10:03 PST
Comment hidden (obsolete)
Created
attachment 446637
[details]
Patch
Nikos Mouchtaris
Comment 27
2021-12-10 14:28:39 PST
Comment hidden (obsolete)
Created
attachment 446816
[details]
Patch
Simon Fraser (smfr)
Comment 28
2022-01-20 21:02:26 PST
Comment on
attachment 446816
[details]
Patch Testing this patch on macOS, I see a bug where `overscroll-behavior: contain;` on an overflow:scroll also prevents rubberbanding when it should not.
Simon Fraser (smfr)
Comment 29
2022-01-20 21:22:12 PST
With an `overscroll-behavior: contain` node, we need to allow rubberbanding if propagation will stop at this node. That means that we need to enter ScrollingEffectsController::handleWheelEvent() for such a node, and instead of the "root node always rubber bands" code that we have, the logic needs to be "if scroll propagation stops at this node, and if `overscroll-behavior` is not `none`, rubberband.
Nikos Mouchtaris
Comment 30
2022-01-21 17:09:23 PST
Comment hidden (obsolete)
Created
attachment 449713
[details]
Patch
Darin Adler
Comment 31
2022-01-21 17:19:34 PST
Comment on
attachment 449713
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=449713&action=review
> Source/WebCore/page/scrolling/ScrollingTree.cpp:205 > + PlatformWheelEvent adjustedWheelEvent = PlatformWheelEvent(wheelEvent);
Should just be this: auto adjustedWheelEvent = wheelEvent; At the very least, there is no need to list the class name twice. But I think you don’t need it even once.
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:127 > + PlatformWheelEvent adjustedWheelEvent = PlatformWheelEvent(wheelEvent);
Same.
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:132 > + if (isLatchedNode() || eventTargeting == EventTargeting::NodeOnly || (isRootNode() && !wheelEvent.isNonGestureEvent()) || (shouldBlockScrollPropagation(adjustedWheelEvent, handled) && overscrollBehaviorAllowRubberBand())) > + return true; > + return false;
Can we just say: return xxx; Instead of: if (xxx) return true; return false;
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:167 > + bool shouldBlockScrollPropagation(PlatformWheelEvent&, bool&) const;
This seems confusing. What are the bools for? How would I know that? Why is the wheel event passed in as a reference? Very much "not self explanatory". If we have two return values, I suggest we consider returning a structure with two named booleans, making it easier to understand at the call site. Reading the code, it seems neither of the callers are taking advantages of the adjustments made to the passed-in wheel event; they never look at the adjusted event. Given that, why does this function do the adjustment?
Nikos Mouchtaris
Comment 32
2022-01-21 19:21:04 PST
Created
attachment 449718
[details]
Patch
Nikos Mouchtaris
Comment 33
2022-01-21 19:25:45 PST
> Reading the code, it seems neither of the callers are taking advantages of > the adjustments made to the passed-in wheel event; they never look at the > adjusted event. Given that, why does this function do the adjustment?
The caller in ScrollingTree::handleWheelEventWithNode uses this adjusted wheel event in the case where we block one axis of the wheel event (based on the value of the overscroll behavior) and the wheel event is propagated up the scroll tree. The other caller doesn't make use of the adjusted event but I think that's ok.
Simon Fraser (smfr)
Comment 34
2022-01-21 21:16:20 PST
Comment on
attachment 449718
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=449718&action=review
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:128 > + bool shouldBlock = shouldBlockScrollPropagation(adjustedWheelEvent).first;
Sucks that we have to copy the PlatformWheelEvent just to ask whether propagation is prevented.
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:134 > +
Blank line.
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:407 > +// Returns if it should block scroll propogation, and if the wheel event is handled > +std::pair<bool, bool> ScrollingTreeScrollingNode::shouldBlockScrollPropagation(PlatformWheelEvent& wheelEvent) const
If you need a comment to explain the return value, perhaps it should be a simple struct instead.
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:418 > + if (hasHorizontalOverscrollBehavior() && !biasedDelta.height() && biasedDelta.width()) > + return std::make_pair<bool, bool>(true, false);
I'm confused by the lack of symmetry between checking horizontal and vertical over scroll behavior. Also, this would be easier to read if instead of hasHorizontalOverscrollBehavior() it was overscrollBehaviorPreventsPropagation().
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:162 > +
Whitespace
> LayoutTests/fast/scrolling/resources/overscroll-behavior-support.js:39 > +async function mouseWheelScrollAndWait(x, y, beginX, beginY, deltaX, deltaY) > +{ > + if (beginX === undefined) > + beginX = 0; > + if (beginY === undefined) > + beginY = -1; > + if (deltaX === undefined) > + deltaX = 0; > + if (deltaY === undefined) > + deltaY = -10; > + > + eventSender.monitorWheelEvents(); > + eventSender.mouseMoveTo(x, y); > + eventSender.mouseScrollByWithWheelAndMomentumPhases(beginX, beginY, "began", "none"); > + eventSender.mouseScrollByWithWheelAndMomentumPhases(deltaX, deltaY, "changed", "none"); > + eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, "ended", "none"); > + return new Promise(resolve => { > + setTimeout(() => { > + requestAnimationFrame(resolve); > + }, 500); > + }); > +}
This looks very similar to code in UIHelpers.js. Please use that.
Nikos Mouchtaris
Comment 35
2022-01-24 15:58:57 PST
Created
attachment 449877
[details]
Patch
Nikos Mouchtaris
Comment 36
2022-01-24 16:04:55 PST
(In reply to Simon Fraser (smfr) from
comment #34
)
> Comment on
attachment 449718
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=449718&action=review
> > > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:418 > > + if (hasHorizontalOverscrollBehavior() && !biasedDelta.height() && biasedDelta.width()) > > + return std::make_pair<bool, bool>(true, false); > > I'm confused by the lack of symmetry between checking horizontal and > vertical over scroll behavior.
This has to do with allowing horizontal swipe gestures for navigation. Firefox and Chrome don't allow navigations in this case, so we could make that the case as well.
> > LayoutTests/fast/scrolling/resources/overscroll-behavior-support.js:39 > > +async function mouseWheelScrollAndWait(x, y, beginX, beginY, deltaX, deltaY) > > +{ > > + if (beginX === undefined) > > + beginX = 0; > > + if (beginY === undefined) > > + beginY = -1; > > + if (deltaX === undefined) > > + deltaX = 0; > > + if (deltaY === undefined) > > + deltaY = -10; > > + > > + eventSender.monitorWheelEvents(); > > + eventSender.mouseMoveTo(x, y); > > + eventSender.mouseScrollByWithWheelAndMomentumPhases(beginX, beginY, "began", "none"); > > + eventSender.mouseScrollByWithWheelAndMomentumPhases(deltaX, deltaY, "changed", "none"); > > + eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, "ended", "none"); > > + return new Promise(resolve => { > > + setTimeout(() => { > > + requestAnimationFrame(resolve); > > + }, 500); > > + }); > > +} > > This looks very similar to code in UIHelpers.js. Please use that.
Unfortunately the tests timeout when using the original function in ui-helpers.js, so this one is needed.
Darin Adler
Comment 37
2022-01-24 16:22:14 PST
Comment on
attachment 449877
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=449877&action=review
Looking good; some thoughts
> Source/WebCore/page/scrolling/ScrollingTree.cpp:220 > + auto ret = scrollingNode.shouldBlockScrollPropagationFilter(adjustedWheelEvent);
WebKit coding style says we use words for local variable names. We could use a name like shouldBlock, blockingPolicy, policy, or propagation. If it doesn’t make the line too long, could also write it this way to scope the variable: if (auto propagation = scrollingNode.shouldBlockScrollPropagationFilter(adjustedWheelEvent); propagation.shouldBlockScrollPropogation)
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:35 > +#import "ScrollingEffectsController.h"
Needs to be "include", not "import".
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:412 > + BlockScrollPropogation ret = shouldBlockScrollPropagation(biasedDelta);
Same "ret" feedback here. I also suggest using auto instead of writing the name BlockScrollPropogation.
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:426 > + BlockScrollPropogation ret = { false, false };
Here it would be good to have these false values just default in the structures’s definition.
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:40 > struct WheelEventHandlingResult;
Forward declarations should not be right up next to structures defined in this file. I suggest a blank line.
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:44 > +struct BlockScrollPropogation { > + bool shouldBlockScrollPropogation; > + bool isHandled; > +};
There's some misspelling here, "propogation", with an "o" instead of an "a". Also, it’s best to give these booleans default values, even if they are never needed in practice. Just better to not have uninitialized variables.
Nikos Mouchtaris
Comment 38
2022-01-25 13:11:31 PST
Created
attachment 449960
[details]
Patch
Simon Fraser (smfr)
Comment 39
2022-01-27 16:00:03 PST
Comment on
attachment 449960
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=449960&action=review
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:404 > +BlockScrollPropagation ScrollingTreeScrollingNode::shouldBlockScrollPropagationFilter(PlatformWheelEvent& wheelEvent) const
This function has two tasks: computing BlockScrollPropagation, and filtering the wheel event, but at call sites, it's hard to see that the event is being modified. It would be nice to separate these; at the call site, it could look something like: auto scrollPropagation = computeScrollPropagation(delta); if (scrollPropagation.propagationBlocked) filteredWheelEvent = eventForPropagation(wheelEvent, scrollPropagation) or something.
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:406 > + FloatSize filteredDelta(wheelEvent.deltaX(), wheelEvent.deltaY());
auto filteredDelta = wheelEvent.delta();
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:413 > + if (hasHorizontalOverscrollBehavior() || hasVerticalOverscrollBehavior()) {
I still think hasHorizontalOverscrollBehavior() needs a better name.
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:424 > +BlockScrollPropagation ScrollingTreeScrollingNode::shouldBlockScrollPropagation(const FloatSize& delta) const
Since this no longer returns a bool, call it `computeScrollPropagation`
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:438 > + if (hasHorizontalOverscrollBehavior() && !delta.height() && delta.width()) { > + propagation.shouldBlockScrollPropagation = true; > + propagation.isHandled = false; > + return propagation; > + } > + > + if ((hasHorizontalOverscrollBehavior() && hasVerticalOverscrollBehavior()) || (hasHorizontalOverscrollBehavior() && !delta.height()) || (hasVerticalOverscrollBehavior() && !delta.width())) { > + propagation.shouldBlockScrollPropagation = true; > + propagation.isHandled = true; > + return propagation; > + }
In a previous response, you said that the lack of symmetry between vertical and horizontal was about history swiping. But this function is called for non-root nodes, and there's no code here that looks at whether the node is a root node, or consults scrollingTree->mainFrameCanRubberBandOnSide().
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:42 > +struct BlockScrollPropagation {
Let's just call this `ScrollPropagation` or `ScrollPropagationInfo`
Nikos Mouchtaris
Comment 40
2022-01-27 17:43:40 PST
Created
attachment 450197
[details]
Patch
Nikos Mouchtaris
Comment 41
2022-01-27 17:46:49 PST
(In reply to Simon Fraser (smfr) from
comment #39
)
> Comment on
attachment 449960
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=449960&action=review
> > > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:438 > > + if (hasHorizontalOverscrollBehavior() && !delta.height() && delta.width()) { > > + propagation.shouldBlockScrollPropagation = true; > > + propagation.isHandled = false; > > + return propagation; > > + } > > + > > + if ((hasHorizontalOverscrollBehavior() && hasVerticalOverscrollBehavior()) || (hasHorizontalOverscrollBehavior() && !delta.height()) || (hasVerticalOverscrollBehavior() && !delta.width())) { > > + propagation.shouldBlockScrollPropagation = true; > > + propagation.isHandled = true; > > + return propagation; > > + } > > In a previous response, you said that the lack of symmetry between vertical > and horizontal was about history swiping. But this function is called for > non-root nodes, and there's no code here that looks at whether the node is a > root node, or consults scrollingTree->mainFrameCanRubberBandOnSide().
Adjusted this area so that we have same behavior as Firefox and Chrome.
Simon Fraser (smfr)
Comment 42
2022-01-27 19:20:47 PST
(In reply to Nikos Mouchtaris from
comment #36
)
> > > LayoutTests/fast/scrolling/resources/overscroll-behavior-support.js:39 > > > +async function mouseWheelScrollAndWait(x, y, beginX, beginY, deltaX, deltaY) > > > +{ > > > + if (beginX === undefined) > > > + beginX = 0; > > > + if (beginY === undefined) > > > + beginY = -1; > > > + if (deltaX === undefined) > > > + deltaX = 0; > > > + if (deltaY === undefined) > > > + deltaY = -10; > > > + > > > + eventSender.monitorWheelEvents(); > > > + eventSender.mouseMoveTo(x, y); > > > + eventSender.mouseScrollByWithWheelAndMomentumPhases(beginX, beginY, "began", "none"); > > > + eventSender.mouseScrollByWithWheelAndMomentumPhases(deltaX, deltaY, "changed", "none"); > > > + eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, "ended", "none"); > > > + return new Promise(resolve => { > > > + setTimeout(() => { > > > + requestAnimationFrame(resolve); > > > + }, 500); > > > + }); > > > +} > > > > This looks very similar to code in UIHelpers.js. Please use that. > Unfortunately the tests timeout when using the original function in > ui-helpers.js, so this one is needed.
Did you figure out why?
Simon Fraser (smfr)
Comment 43
2022-01-27 19:21:05 PST
(In reply to Nikos Mouchtaris from
comment #41
)
> > In a previous response, you said that the lack of symmetry between vertical > > and horizontal was about history swiping. But this function is called for > > non-root nodes, and there's no code here that looks at whether the node is a > > root node, or consults scrollingTree->mainFrameCanRubberBandOnSide(). > > Adjusted this area so that we have same behavior as Firefox and Chrome.
Is there a test for this?
Simon Fraser (smfr)
Comment 44
2022-01-28 16:29:59 PST
Comment on
attachment 450197
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=450197&action=review
Please file followup bugs on the testing-related tasks: 1. Testing history swipe. 2. Removing mouseWheelScrollAndWait(). 3. Making the tests more like other fast/scrolling/ tests
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:413 > + filteredDelta.setWidth(horizontalOverscrollBehaviorPreventsPropagation() || (verticalOverscrollBehaviorPreventsPropagation() && !biasedDelta.width()) ? 0 : filteredDelta.width()); > + filteredDelta.setHeight(verticalOverscrollBehaviorPreventsPropagation() || (horizontalOverscrollBehaviorPreventsPropagation() && !biasedDelta.height()) ? 0 : filteredDelta.height());
I'd write these with an if (). It's odd to set the value to itself.
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:166 > + PlatformWheelEvent eventForPropogation(const PlatformWheelEvent&) const;
eventForPropogation -> eventForPropagation
> LayoutTests/fast/scrolling/mac/async-overscroll-behavior-element.html:41 > +var overscrollDatas = [["auto", "auto", true, true], > + ["contain", "auto", false, true], > + ["none", "auto", false, true], > + ["auto", "contain", true, false], > + ["contain", "contain", false, false], > + ["none", "contain", false, false], > + ["auto", "none", true, false], > + ["contain", "none", false, false], > + ["none", "none", false, false]];
This is really hard to read. Variable can be a const. Might be better to rewrite this as a js-test-pre/post-style test, not a WPT style test.
> LayoutTests/fast/scrolling/mac/async-overscroll-behavior-element.html:47 > + // Try various methods to ensure the element position is reset immediately. > + scroller.scrollLeft = 300; > + scroller.scrollTop = 300; > + scroller.scrollTo(300, 300);
Why the uncertainty? Look at other scrolling tests to see how to do this.
> LayoutTests/fast/scrolling/mac/async-overscroll-behavior-element.html:66 > + assert_equals(root.scrollLeft > 0, propagateX, 'Continue up the horizontal scroll chaining');
Mentions chaining.
> LayoutTests/fast/scrolling/mac/async-overscroll-behavior-iframe.html:35 > + var overscrollDatas = [["auto", "auto", true, true],
Same comments as the previous test.
Simon Fraser (smfr)
Comment 45
2022-01-28 16:37:11 PST
Comment on
attachment 450197
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=450197&action=review
> LayoutTests/fast/scrolling/mac/async-overscroll-behavior-iframe.html:2 > +
Blank line here, not in the other test.
Nikos Mouchtaris
Comment 46
2022-01-28 16:55:10 PST
Created
attachment 450289
[details]
Patch
EWS
Comment 47
2022-01-28 18:57:50 PST
Committed
r288777
(
246558@main
): <
https://commits.webkit.org/246558@main
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 450289
[details]
.
Darin Adler
Comment 48
2022-02-06 12:18:20 PST
Comment on
attachment 450289
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=450289&action=review
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:415 > + if(horizontalOverscrollBehaviorPreventsPropagation() || (verticalOverscrollBehaviorPreventsPropagation() && !biasedDelta.width())) > + filteredDelta.setWidth(0); > + if(verticalOverscrollBehaviorPreventsPropagation() || (horizontalOverscrollBehaviorPreventsPropagation() && !biasedDelta.height())) > + filteredDelta.setHeight(0);
Missing spaces here after the "if".
> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:163 > + OverscrollBehavior horizontalOverscrollBehavior() const { return m_scrollableAreaParameters.horizontalOverscrollBehavior; } > + OverscrollBehavior verticalOverscrollBehavior() const { return m_scrollableAreaParameters.verticalOverscrollBehavior; }
Not new to this patch, but this design where we have separate getters for each of the ScrollableAreaParameters isn’t really good. We literally have 11 of these. One small thing that’s good about them is that some are public and some protected. But for the protected one at least, instead a single getter that returns a const& would be better. The callers don’t need a cover function for each to abstract away that these are part of the scrollable area parameters, does it. This patch leads to difficult-to-maintain code where things are repeated.
Darin Adler
Comment 49
2022-02-06 12:18:43 PST
Comment on
attachment 450289
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=450289&action=review
>> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:163 >> + OverscrollBehavior verticalOverscrollBehavior() const { return m_scrollableAreaParameters.verticalOverscrollBehavior; } > > Not new to this patch, but this design where we have separate getters for each of the ScrollableAreaParameters isn’t really good. We literally have 11 of these. One small thing that’s good about them is that some are public and some protected. But for the protected one at least, instead a single getter that returns a const& would be better. The callers don’t need a cover function for each to abstract away that these are part of the scrollable area parameters, does it. > > This patch leads to difficult-to-maintain code where things are repeated.
I meant "this pattern", not "this patch".
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug