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
Patch (85.35 KB, patch)
2020-12-24 03:25 PST, cathiechen
no flags
Patch (85.25 KB, patch)
2020-12-24 03:37 PST, cathiechen
no flags
Patch (85.25 KB, patch)
2020-12-24 03:46 PST, cathiechen
no flags
Patch (85.27 KB, patch)
2020-12-24 04:38 PST, cathiechen
no flags
Patch (85.29 KB, patch)
2020-12-24 08:28 PST, cathiechen
no flags
Patch (79.06 KB, patch)
2021-01-15 10:16 PST, cathiechen
no flags
Patch (81.33 KB, patch)
2021-01-18 00:06 PST, cathiechen
no flags
Patch (81.57 KB, patch)
2021-01-18 00:41 PST, cathiechen
ews-feeder: commit-queue-
Patch (81.57 KB, patch)
2021-01-18 00:50 PST, cathiechen
ews-feeder: commit-queue-
Patch (81.56 KB, patch)
2021-01-18 01:00 PST, cathiechen
ews-feeder: commit-queue-
Patch (81.46 KB, patch)
2021-01-18 01:43 PST, cathiechen
no flags
async-overscroll-behavior-for-ews (95.78 KB, patch)
2021-03-15 06:22 PDT, cathiechen
ews-feeder: commit-queue-
async-overscroll-behavior-for-review (38.51 KB, patch)
2021-03-15 06:23 PDT, cathiechen
no flags
async-overscroll-behavior-for-ews (97.06 KB, patch)
2021-03-15 22:48 PDT, cathiechen
ews-feeder: commit-queue-
async-overscroll-behavior-for-ews (96.55 KB, patch)
2021-03-22 08:47 PDT, cathiechen
no flags
Patch (18.42 KB, patch)
2021-11-17 16:26 PST, Nikos Mouchtaris
no flags
Patch (32.14 KB, patch)
2021-11-30 23:22 PST, Nikos Mouchtaris
no flags
Patch (32.79 KB, patch)
2021-12-01 11:21 PST, Nikos Mouchtaris
ews-feeder: commit-queue-
Patch (32.79 KB, patch)
2021-12-01 11:38 PST, Nikos Mouchtaris
no flags
Patch (32.39 KB, patch)
2021-12-09 16:37 PST, Nikos Mouchtaris
ews-feeder: commit-queue-
Patch (32.40 KB, patch)
2021-12-09 17:10 PST, Nikos Mouchtaris
no flags
Patch (32.40 KB, patch)
2021-12-10 14:28 PST, Nikos Mouchtaris
no flags
Patch (34.20 KB, patch)
2022-01-21 17:09 PST, Nikos Mouchtaris
ews-feeder: commit-queue-
Patch (34.73 KB, patch)
2022-01-21 19:21 PST, Nikos Mouchtaris
no flags
Patch (35.47 KB, patch)
2022-01-24 15:58 PST, Nikos Mouchtaris
no flags
Patch (35.56 KB, patch)
2022-01-25 13:11 PST, Nikos Mouchtaris
no flags
Patch (34.52 KB, patch)
2022-01-27 17:43 PST, Nikos Mouchtaris
no flags
Patch (34.68 KB, patch)
2022-01-28 16:55 PST, Nikos Mouchtaris
ews-feeder: commit-queue-
cathiechen
Comment 1 2020-12-24 03:21:03 PST Comment hidden (obsolete)
cathiechen
Comment 2 2020-12-24 03:25:15 PST Comment hidden (obsolete)
cathiechen
Comment 3 2020-12-24 03:37:32 PST Comment hidden (obsolete)
cathiechen
Comment 4 2020-12-24 03:46:39 PST Comment hidden (obsolete)
cathiechen
Comment 5 2020-12-24 04:38:16 PST Comment hidden (obsolete)
cathiechen
Comment 6 2020-12-24 08:28:51 PST Comment hidden (obsolete)
Radar WebKit Bug Importer
Comment 7 2020-12-31 01:53:13 PST
cathiechen
Comment 8 2021-01-15 10:16:52 PST Comment hidden (obsolete)
cathiechen
Comment 9 2021-01-18 00:06:50 PST Comment hidden (obsolete)
cathiechen
Comment 10 2021-01-18 00:41:50 PST Comment hidden (obsolete)
cathiechen
Comment 11 2021-01-18 00:50:30 PST Comment hidden (obsolete)
cathiechen
Comment 12 2021-01-18 01:00:25 PST Comment hidden (obsolete)
cathiechen
Comment 13 2021-01-18 01:43:11 PST Comment hidden (obsolete)
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)
Nikos Mouchtaris
Comment 20 2021-11-30 23:22:34 PST Comment hidden (obsolete)
Nikos Mouchtaris
Comment 21 2021-12-01 11:21:02 PST Comment hidden (obsolete)
Nikos Mouchtaris
Comment 22 2021-12-01 11:38:34 PST Comment hidden (obsolete)
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)
Nikos Mouchtaris
Comment 26 2021-12-09 17:10:03 PST Comment hidden (obsolete)
Nikos Mouchtaris
Comment 27 2021-12-10 14:28:39 PST Comment hidden (obsolete)
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)
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
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
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
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
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
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.