[webkit-reviews] review granted: [Bug 220139] Implement CSS overscroll-behavior for asynchronous scroll on Mac : [Attachment 450197] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 28 16:29:59 PST 2022


Simon Fraser (smfr) <simon.fraser at apple.com> has granted Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for review:
Bug 220139: Implement CSS overscroll-behavior for asynchronous scroll on Mac
https://bugs.webkit.org/show_bug.cgi?id=220139

Attachment 450197: Patch

https://bugs.webkit.org/attachment.cgi?id=450197&action=review




--- Comment #44 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 450197
  --> https://bugs.webkit.org/attachment.cgi?id=450197
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.


More information about the webkit-reviews mailing list