Bug 220139

<rdar://problem/72751742>

Comment on attachment 417818 [details] Patch Hi, I think this patch is ready to review. Please take a look, thanks:)

Created attachment 423169 [details] async-overscroll-behavior-for-ews

Created attachment 423170 [details] async-overscroll-behavior-for-review

Created attachment 423301 [details] async-overscroll-behavior-for-ews

Created attachment 423891 [details] async-overscroll-behavior-for-ews

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.

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.

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.

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.

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?

Created attachment 449718 [details] Patch

> 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.

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.

Created attachment 449877 [details] Patch

(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.

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.

Created attachment 449960 [details] Patch

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`

Created attachment 450197 [details] Patch

(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.

(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?

(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?

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.

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.

Created attachment 450289 [details] Patch

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].

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.

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".