Changeset 271090 in webkit

Timestamp:

Dec 26, 2020 10:28:39 AM (19 months ago)

Author:

Simon Fraser

Message:

Fix scrolling issues when scrolling on only one axis is enabled
​https://bugs.webkit.org/show_bug.cgi?id=220134

Reviewed by Sam Weinig.

Source/WebCore:

If an overflow:scroll has overflow on an axis, but overflow:hidden on that
axis, then there are various issues with finding the correct scroller and
latching.

This affects nested scrollers where inner and outer and scrollable on different
axes, and the inner scroller has overflow, but overflow:hidden on the cross axis.

The fix involves adding checks for scrolling being allowed in code that fetches
pinned state, and code that looks for scrollable areas for a given event delta.

Tests: fast/scrolling/mac/overflow-hidden-on-one-axis-async-overflow.html

fast/scrolling/mac/overflow-hidden-on-one-axis.html

• page/mac/EventHandlerMac.mm:

(WebCore::findEnclosingScrollableContainer):

• page/scrolling/ScrollingTreeScrollingNode.cpp:

(WebCore::ScrollingTreeScrollingNode::edgePinnedState const):

• page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm:

(WebCore::ScrollingTreeScrollingNodeDelegateMac::isPinnedForScrollDeltaOnAxis const):

• platform/ScrollableArea.cpp:

(WebCore::ScrollableArea::isPinnedForScrollDeltaOnAxis const):
(WebCore::ScrollableArea::isPinnedForScrollDelta const): Check for non-zero deltas.
isPinnedForScrollDeltaOnAxis() returns false if a delta is zero, so we don't want to say
we're not pinned if a delta is zero. The logic of this code really needs to be inverted
to talk about scrollability, not pinning.
(WebCore::ScrollableArea::edgePinnedState const):

LayoutTests:

• fast/scrolling/mac/overflow-hidden-on-one-axis-async-overflow-expected.txt: Added.
• fast/scrolling/mac/overflow-hidden-on-one-axis-async-overflow.html: Added.
• fast/scrolling/mac/overflow-hidden-on-one-axis-expected.txt: Added.
• fast/scrolling/mac/overflow-hidden-on-one-axis.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/scrolling/mac/overflow-hidden-on-one-axis-async-overflow-expected.txt (added)
• LayoutTests/fast/scrolling/mac/overflow-hidden-on-one-axis-async-overflow.html (added)
• LayoutTests/fast/scrolling/mac/overflow-hidden-on-one-axis-expected.txt (added)
• LayoutTests/fast/scrolling/mac/overflow-hidden-on-one-axis.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/mac/EventHandlerMac.mm (modified) (1 diff)
• Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp (modified) (1 diff)
• Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm (modified) (2 diffs)
• Source/WebCore/platform/ScrollableArea.cpp (modified) (4 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2020-12-26  Simon Fraser  <simon.fraser@apple.com>
+
+        Fix scrolling issues when scrolling on only one axis is enabled
+        https://bugs.webkit.org/show_bug.cgi?id=220134
+
+        Reviewed by Sam Weinig.
+
+        * fast/scrolling/mac/overflow-hidden-on-one-axis-async-overflow-expected.txt: Added.
+        * fast/scrolling/mac/overflow-hidden-on-one-axis-async-overflow.html: Added.
+        * fast/scrolling/mac/overflow-hidden-on-one-axis-expected.txt: Added.
+        * fast/scrolling/mac/overflow-hidden-on-one-axis.html: Added.
+
 2020-12-23  Ryan Haddad  <ryanhaddad@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-12-26  Simon Fraser  <simon.fraser@apple.com>
+
+        Fix scrolling issues when scrolling on only one axis is enabled
+        https://bugs.webkit.org/show_bug.cgi?id=220134
+
+        Reviewed by Sam Weinig.
+
+        If an overflow:scroll has overflow on an axis, but overflow:hidden on that
+        axis, then there are various issues with finding the correct scroller and
+        latching.
+
+        This affects nested scrollers where inner and outer and scrollable on different
+        axes, and the inner scroller has overflow, but overflow:hidden on the cross axis.
+
+        The fix involves adding checks for scrolling being allowed in code that fetches
+        pinned state, and code that looks for scrollable areas for a given event delta.
+
+        Tests: fast/scrolling/mac/overflow-hidden-on-one-axis-async-overflow.html
+               fast/scrolling/mac/overflow-hidden-on-one-axis.html
+
+        * page/mac/EventHandlerMac.mm:
+        (WebCore::findEnclosingScrollableContainer):
+        * page/scrolling/ScrollingTreeScrollingNode.cpp:
+        (WebCore::ScrollingTreeScrollingNode::edgePinnedState const):
+        * page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm:
+        (WebCore::ScrollingTreeScrollingNodeDelegateMac::isPinnedForScrollDeltaOnAxis const):
+        * platform/ScrollableArea.cpp:
+        (WebCore::ScrollableArea::isPinnedForScrollDeltaOnAxis const):
+        (WebCore::ScrollableArea::isPinnedForScrollDelta const): Check for non-zero deltas.
+        isPinnedForScrollDeltaOnAxis() returns false if a delta is zero, so we don't want to say
+        we're not pinned if a delta is zero. The logic of this code really needs to be inverted
+        to talk about scrollability, not pinning.
+        (WebCore::ScrollableArea::edgePinnedState const):
+
 2020-12-26  Sam Weinig  <weinig@apple.com>
 

trunk/Source/WebCore/page/mac/EventHandlerMac.mm

@@ -820 +820 @@
             return candidate;
 
-        if ((biasedDelta.height() > 0 && !scrollableArea->scrolledToTop()) || (biasedDelta.height() < 0 && !scrollableArea->scrolledToBottom())
-            || (biasedDelta.width() > 0 && !scrollableArea->scrolledToLeft()) || (biasedDelta.width() < 0 && !scrollableArea->scrolledToRight()))
+        if (biasedDelta.height() && !scrollableArea->isPinnedForScrollDeltaOnAxis(-biasedDelta.height(), ScrollEventAxis::Vertical))
+            return candidate;
+
+        if (biasedDelta.width() && !scrollableArea->isPinnedForScrollDeltaOnAxis(-biasedDelta.width(), ScrollEventAxis::Horizontal))
             return candidate;
     }

trunk/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp

@@ -199 +199 @@
     auto maxScrollPosition = maximumScrollPosition();
 
+    bool horizontallyUnscrollable = !allowsHorizontalScrolling();
+    bool verticallyUnscrollable = !allowsVerticalScrolling();
+
     // Top, right, bottom, left.
     return {
-        scrollPosition.y() <= minScrollPosition.y(),
-        scrollPosition.x() >= maxScrollPosition.x(),
-        scrollPosition.y() >= maxScrollPosition.y(),
-        scrollPosition.x() <= minScrollPosition.x()
+        verticallyUnscrollable || scrollPosition.y() <= minScrollPosition.y(),
+        horizontallyUnscrollable || scrollPosition.x() >= maxScrollPosition.x(),
+        verticallyUnscrollable || scrollPosition.y() >= maxScrollPosition.y(),
+        horizontallyUnscrollable || scrollPosition.x() <= minScrollPosition.x()
     };
 }

trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm

@@ -221 +221 @@
     switch (axis) {
     case ScrollEventAxis::Vertical:
+        if (!allowsVerticalScrolling())
+            return true;
+
         if (scrollDelta < 0) {
             auto topOffset = scrollPosition.y() - minimumScrollPosition().y();
@@ -232 +235 @@
         break;
     case ScrollEventAxis::Horizontal:
+        if (!allowsHorizontalScrolling())
+            return true;
+
         if (scrollDelta < 0) {
             auto leftOffset = scrollPosition.x() - minimumScrollPosition().x();

trunk/Source/WebCore/platform/ScrollableArea.cpp

@@ -635 +635 @@
     switch (axis) {
     case ScrollEventAxis::Vertical:
+        if (!allowsVerticalScrolling())
+            return true;
+
         if (scrollDelta < 0) // top
             return scrollPosition.y() <= minimumScrollPosition().y();
@@ -643 +646 @@
         break;
     case ScrollEventAxis::Horizontal:
+        if (!allowsHorizontalScrolling())
+            return true;
+
         if (scrollDelta < 0) // left
             return scrollPosition.x() <= minimumScrollPosition().x();
@@ -657 +663 @@
 bool ScrollableArea::isPinnedForScrollDelta(const FloatSize& scrollDelta) const
 {
-    return isPinnedForScrollDeltaOnAxis(scrollDelta.width(), ScrollEventAxis::Horizontal) && isPinnedForScrollDeltaOnAxis(scrollDelta.height(), ScrollEventAxis::Vertical);
+    return (!scrollDelta.width() || isPinnedForScrollDeltaOnAxis(scrollDelta.width(), ScrollEventAxis::Horizontal))
+        && (!scrollDelta.height() || isPinnedForScrollDeltaOnAxis(scrollDelta.height(), ScrollEventAxis::Vertical));
 }
 
@@ -666 +673 @@
     auto maxScrollPosition = maximumScrollPosition();
 
+    bool horizontallyUnscrollable = !allowsHorizontalScrolling();
+    bool verticallyUnscrollable = !allowsVerticalScrolling();
+
     // Top, right, bottom, left.
     return {
-        scrollPosition.y() <= minScrollPosition.y(),
-        scrollPosition.x() >= maxScrollPosition.x(),
-        scrollPosition.y() >= maxScrollPosition.y(),
-        scrollPosition.x() <= minScrollPosition.x()
+        verticallyUnscrollable || scrollPosition.y() <= minScrollPosition.y(),
+        horizontallyUnscrollable || scrollPosition.x() >= maxScrollPosition.x(),
+        verticallyUnscrollable || scrollPosition.y() >= maxScrollPosition.y(),
+        horizontallyUnscrollable || scrollPosition.x() <= minScrollPosition.x()
     };
 }