Skip to content
Permalink
Browse files
Vertical scroll with mouse wheel in horizontal scroller fails to prop…
…agate to the document

https://bugs.webkit.org/show_bug.cgi?id=228891
<rdar://81640239>

Reviewed by Antti Koivisto.

Source/WebCore:

If a page has a horizontal scrolling carousel with a non-passive wheel event handler, and a
user with a clicky scroll wheel mouse tries to vertically scroll over that carousel, then
we'd fail to propagate the scroll to the page. This affects apple.com/store.

This happened because we don't do latching for legacy mouse wheel events, so we hit the code
in EventHandler::handleWheelEventInAppropriateEnclosingBox(); this finds the carousel and
calls handleWheelEventInScrollableArea(), but that dispatches to the scrolling thread in way
that does not propagate the event to the nearest ancestor that can handle it.

The fix is to check that the ScrollableArea can handle the event, sharing some code that
already exists and was used by findEnclosingScrollableContainer(). This is a conservative
fix; it's possible that scrollableAreaCanHandleEvent() could be called down in
handleWheelEventInScrollableArea() but that will affect other call sites.

Test: fast/scrolling/mac/vertical-scroll-in-horizontal-scroller.html

* page/EventHandler.cpp:
(WebCore::EventHandler::handleWheelEventInAppropriateEnclosingBox):
(WebCore::EventHandler::scrollableAreaCanHandleEvent):
* page/EventHandler.h:
* page/mac/EventHandlerMac.mm:
(WebCore::findEnclosingScrollableContainer):

LayoutTests:

* fast/scrolling/mac/vertical-scroll-in-horizontal-scroller-expected.txt: Added.
* fast/scrolling/mac/vertical-scroll-in-horizontal-scroller.html: Added.
* tiled-drawing/scrolling/overflow-scroll-reduced-content.html: Convert this test to use
async scrolling. webkit.org/b/228898 tracks an existing problem that caused the test
to fail with non-async scrolling.


Canonical link: https://commits.webkit.org/240377@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@280807 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
smfr committed Aug 9, 2021
1 parent 25cb6cb commit 226390f3bcac1d01eeac694d0535eaac84a90827
@@ -1,3 +1,17 @@
2021-08-09 Simon Fraser <simon.fraser@apple.com>

Vertical scroll with mouse wheel in horizontal scroller fails to propagate to the document
https://bugs.webkit.org/show_bug.cgi?id=228891
<rdar://81640239>

Reviewed by Antti Koivisto.

* fast/scrolling/mac/vertical-scroll-in-horizontal-scroller-expected.txt: Added.
* fast/scrolling/mac/vertical-scroll-in-horizontal-scroller.html: Added.
* tiled-drawing/scrolling/overflow-scroll-reduced-content.html: Convert this test to use
async scrolling. webkit.org/b/228898 tracks an existing problem that caused the test
to fail with non-async scrolling.

2021-08-09 Ayumi Kojima <ayumi_kojima@apple.com>

[ MacOS wk2 ] tiled-drawing/scrolling/scroll-snap/scroll-snap-momentum-in-non-snapping-axis.html is a flaky timeout.
@@ -0,0 +1,9 @@

Test that a non-momentum scroll over a horizontal scroller with a non-passive wheel event listener scrolls the document.
Received wheel event
PASS overflowScrollEventCount is 0
PASS windowScrollEventCount > 0 is true
PASS successfullyParsed is true

TEST COMPLETE

@@ -0,0 +1,83 @@
<!DOCTYPE html> <!-- webkit-test-runner [ AsyncOverflowScrollingEnabled=true ] -->
<html>
<head>
<style>
body {
height: 2000px;
}
.scroller {
margin: 50px;
height: 300px;
width: 300px;
border: 20px solid gray;
padding: 5px;
overflow: scroll;
}
.content {
width: 500%;
height: 100%;
background-image: repeating-linear-gradient(to right, silver, white 250px);
}
</style>
<script src="../../../resources/js-test-pre.js"></script>
<script src="../../../resources/ui-helper.js"></script>
<script>
var jsTestIsAsync = true;

let overflowScrollEventCount = 0;
let windowScrollEventCount = 0;

async function mouseWheelScrollAt(x, y, deltaX, deltaY)
{
eventSender.monitorWheelEvents();
eventSender.mouseMoveTo(x, y);
eventSender.mouseScrollByWithWheelAndMomentumPhases(deltaX, deltaY, "none", "none");
return new Promise(resolve => {
eventSender.callAfterScrollingCompletes(() => {
requestAnimationFrame(resolve);
});
});
}

async function testScrollOverContent()
{
debug('');
debug('Test that a non-momentum scroll over a horizontal scroller with a non-passive wheel event listener scrolls the document.');
await mouseWheelScrollAt(100, 100, 0, -10);

shouldBe('overflowScrollEventCount', '0');
shouldBe('windowScrollEventCount > 0', 'true');
}

async function scrollTest()
{
await testScrollOverContent();
finishJSTest();
}

window.addEventListener('load', () => {
let scroller = document.querySelector('.scroller');
scroller.addEventListener('scroll', () => {
++overflowScrollEventCount;
}, false);

scroller.addEventListener('wheel', () => {
debug('Received wheel event');
}, { passive: false });

window.addEventListener('scroll', () => {
++windowScrollEventCount;
}, false);

setTimeout(scrollTest, 0);
}, false);
</script>
</head>
<body>
<div class="scroller">
<div class="content"></div>
</div>
<div id='console'></div>
<script src="../../../resources/js-test-post.js"></script>
</body>
</html>
@@ -1,4 +1,4 @@
<html>
<!DOCTYPE html> <!-- webkit-test-runner [ AsyncOverflowScrollingEnabled=true ] -->
<head>
<style>
.outer {
@@ -1,3 +1,34 @@
2021-08-09 Simon Fraser <simon.fraser@apple.com>

Vertical scroll with mouse wheel in horizontal scroller fails to propagate to the document
https://bugs.webkit.org/show_bug.cgi?id=228891
<rdar://81640239>

Reviewed by Antti Koivisto.

If a page has a horizontal scrolling carousel with a non-passive wheel event handler, and a
user with a clicky scroll wheel mouse tries to vertically scroll over that carousel, then
we'd fail to propagate the scroll to the page. This affects apple.com/store.

This happened because we don't do latching for legacy mouse wheel events, so we hit the code
in EventHandler::handleWheelEventInAppropriateEnclosingBox(); this finds the carousel and
calls handleWheelEventInScrollableArea(), but that dispatches to the scrolling thread in way
that does not propagate the event to the nearest ancestor that can handle it.

The fix is to check that the ScrollableArea can handle the event, sharing some code that
already exists and was used by findEnclosingScrollableContainer(). This is a conservative
fix; it's possible that scrollableAreaCanHandleEvent() could be called down in
handleWheelEventInScrollableArea() but that will affect other call sites.

Test: fast/scrolling/mac/vertical-scroll-in-horizontal-scroller.html

* page/EventHandler.cpp:
(WebCore::EventHandler::handleWheelEventInAppropriateEnclosingBox):
(WebCore::EventHandler::scrollableAreaCanHandleEvent):
* page/EventHandler.h:
* page/mac/EventHandlerMac.mm:
(WebCore::findEnclosingScrollableContainer):

2021-08-09 Kate Cheney <katherine_cheney@apple.com>

Add console logging to encourage the use of authenticated encryption in WebCrypto
@@ -3051,14 +3051,14 @@ bool EventHandler::handleWheelEventInAppropriateEnclosingBox(Node* startNode, co

RenderBox* currentEnclosingBox = &initialEnclosingBox;
while (currentEnclosingBox) {
if (auto* boxLayer = currentEnclosingBox->layer() ? currentEnclosingBox->layer()->scrollableArea() : nullptr) {
if (auto* boxScrollableArea = currentEnclosingBox->layer() ? currentEnclosingBox->layer()->scrollableArea() : nullptr) {
auto platformEvent = wheelEvent.underlyingPlatformEvent();
bool scrollingWasHandled;
if (platformEvent) {
auto copiedEvent = platformEvent->copyWithDeltasAndVelocity(filteredPlatformDelta.width(), filteredPlatformDelta.height(), filteredVelocity);
scrollingWasHandled = handleWheelEventInScrollableArea(copiedEvent, *boxLayer, eventHandling);
scrollingWasHandled = scrollableAreaCanHandleEvent(copiedEvent, *boxScrollableArea) && handleWheelEventInScrollableArea(copiedEvent, *boxScrollableArea, eventHandling);
} else
scrollingWasHandled = didScrollInScrollableArea(*boxLayer, wheelEvent);
scrollingWasHandled = didScrollInScrollableArea(*boxScrollableArea, wheelEvent);

if (scrollingWasHandled)
return true;
@@ -3071,6 +3071,23 @@ bool EventHandler::handleWheelEventInAppropriateEnclosingBox(Node* startNode, co
return false;
}

bool EventHandler::scrollableAreaCanHandleEvent(const PlatformWheelEvent& wheelEvent, ScrollableArea& scrollableArea)
{
#if PLATFORM(MAC)
auto biasedDelta = ScrollController::wheelDeltaBiasingTowardsVertical(wheelEvent);
#else
auto biasedDelta = wheelEvent.delta();
#endif

if (biasedDelta.height() && !scrollableArea.isPinnedForScrollDeltaOnAxis(-biasedDelta.height(), ScrollEventAxis::Vertical))
return true;

if (biasedDelta.width() && !scrollableArea.isPinnedForScrollDeltaOnAxis(-biasedDelta.width(), ScrollEventAxis::Horizontal))
return true;

return false;
}

bool EventHandler::handleWheelEventInScrollableArea(const PlatformWheelEvent& wheelEvent, ScrollableArea& scrollableArea, OptionSet<EventHandling> eventHandling)
{
auto gestureState = updateWheelGestureState(wheelEvent, eventHandling);
@@ -355,6 +355,8 @@ class EventHandler {

WEBCORE_EXPORT void invalidateClick();

static bool scrollableAreaCanHandleEvent(const PlatformWheelEvent&, ScrollableArea&);

private:
#if ENABLE(DRAG_SUPPORT)
static DragState& dragState();
@@ -799,8 +799,6 @@ static bool frameHasPlatformWidget(const Frame& frame)
// FIXME: This could be written in terms of ScrollableArea::enclosingScrollableArea().
static ContainerNode* findEnclosingScrollableContainer(ContainerNode* node, const PlatformWheelEvent& wheelEvent)
{
auto biasedDelta = ScrollController::wheelDeltaBiasingTowardsVertical(wheelEvent);

// Find the first node with a valid scrollable area starting with the current
// node and traversing its parents (or shadow hosts).
for (ContainerNode* candidate = node; candidate; candidate = candidate->parentOrShadowHostNode()) {
@@ -821,10 +819,7 @@ static bool frameHasPlatformWidget(const Frame& frame)
if (wheelEvent.phase() == PlatformWheelEventPhase::MayBegin || wheelEvent.phase() == PlatformWheelEventPhase::Cancelled)
return candidate;

if (biasedDelta.height() && !scrollableArea->isPinnedForScrollDeltaOnAxis(-biasedDelta.height(), ScrollEventAxis::Vertical))
return candidate;

if (biasedDelta.width() && !scrollableArea->isPinnedForScrollDeltaOnAxis(-biasedDelta.width(), ScrollEventAxis::Horizontal))
if (EventHandler::scrollableAreaCanHandleEvent(wheelEvent, *scrollableArea))
return candidate;
}

@@ -286,7 +286,7 @@ void RenderLayerScrollableArea::scrollTo(const ScrollPosition& position)
if (!box)
return;

LOG_WITH_STREAM(Scrolling, stream << "RenderLayerScrollableArea [" << scrollingNodeID() << "] scrollTo " << position << " from " << m_scrollPosition << " (is user scroll " << (currentScrollType() == ScrollType::User) << ")");
LOG_WITH_STREAM(Scrolling, stream << "RenderLayerScrollableArea [" << scrollingNodeID() << "] scrollTo " << position << " from " << m_scrollPosition << " (is user scroll " << (currentScrollType() == ScrollType::User) << ")");

ScrollPosition newPosition = position;
if (!box->isHTMLMarquee()) {
@@ -1246,6 +1246,9 @@ void RenderLayerScrollableArea::updateScrollInfoAfterLayout()

updateScrollbarsAfterLayout();

LOG_WITH_STREAM(Scrolling, stream << "RenderLayerScrollableArea [" << scrollingNodeID() << "] updateScrollInfoAfterLayout - new scroll width " << m_scrollWidth << " scroll height " << m_scrollHeight
<< " rubber banding " << isRubberBandInProgress() << " user scrolling " << isUserScrollInProgress() << " scroll position updated from " << originalScrollPosition << " to " << scrollPosition());

if (originalScrollPosition != scrollPosition())
scrollToPositionWithoutAnimation(IntPoint(scrollPosition()));

0 comments on commit 226390f

Please sign in to comment.