Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Do not do smooth keyboard scrolling on containers with scroll snaps
https://bugs.webkit.org/show_bug.cgi?id=242523
rdar://96681582

Reviewed by Tim Horton.

Tests:
LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-
horizontal-with-keyboard-smooth-scroll-enabled.html
LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-
vertical-with-keyboard-smooth-scroll-enabled.html

Currently, smooth keyboard scrolling does not work with scroll snapping. This
patch makes it so when the focused scrollable container uses scroll snapping,
smooth keyboard scrolling is not used. Anywhere where it is checked that
smooth scrolling is enabled, it is now additionally checked that the focused
container does not have scroll snaps.

Before this change, when the pageUp or pageDown key was pressed, it was checked
whether smooth scroll was enabled in WKWebViewMac. To make for easier access to
EventHandler::focusedScrollableAreaUsesScrollSnap, checking whether to do smooth
scroll for the pageUp and pageDown keys is now performed in WebCore::EditorCommand.

Additionally, this patch refactors the smooth keyboard scrolling code so that
EventHandler::keyboardScrollRecursively takes ScrollDirection and ScrollGranularity
As parameters instead of a KeyboardEvent. This makes it simpler to call
EventHandler::keyboardScrollRecursively from EditorCommand because or else an
Artificial KeyboardEvent would need to be created.

* LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-
horizontal-with-keyboard-smooth-scroll-enabled-expected.txt: Added.
* LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-
horizontal-with-keyboard-smooth-scroll-enabled.html: Added.

Tests that when smooth scroll is turned on, scroll snapping works with the upArrow,
downArrow, pageUp, and pageDown keys.

* LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-
vertical-with-keyboard-smooth-scroll-enabled-expected.txt: Added.
* LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-
vertical-with-keyboard-smooth-scroll-enabled.html: Added.

Tests that when smooth scroll is turned on, scroll snapping works with the leftArrow
and rightArrow keys.

LayoutTests/TestExpectations
LayoutTests/fast/scrolling/keyboard-scrolling-last-timestamp-expected.txt: Deleted.
LayoutTests/fast/scrolling/keyboard-scrolling-last-timestamp.html: Deleted.
LayoutTests/platform/wk2/TestExpectations

This test was added by bugs.webkit.org/show_bug.cgi?id=229784. This patch
Reverts the changes to pageUp/pageDown scrolling introduced by it,
So the test no longer applies.

* Source/WebCore/editing/EditorCommand.cpp:
(WebCore::EditorCommand::executeScrollPageBackward):
(WebCore::EditorCommand::executeScrollPageForward):

If smooth scroll is enabled and focused container has no scroll snaps, calls
EventHandler::keyboardScrollRecursively, which starts the smooth scroll animation.

* Source/WebCore/page/EventHandler.cpp:
(WebCore::EventHandler::defaultKeyboardEventHandler):

Refactoring of pageUp and pageDown scrolling.

(WebCore::EventHandler::defaultSpaceEventHandler):
(WebCore::EventHandler::defaultArrowEventHandler):

Now call above two functions to obtain arguments to pass to keyboardScrollRecursively.

(WebCore::EventHandler::beginKeyboardScrollGesture):
(WebCore::EventHandler::startKeyboardScrollAnimationOnDocument):
(WebCore::EventHandler::startKeyboardScrollAnimationOnRenderBoxLayer):
(WebCore::EventHandler::startKeyboardScrollAnimationOnRenderBoxAndItsAncestors):
(WebCore::EventHandler::startKeyboardScrollAnimationOnEnclosingScrollableContainer):
(WebCore::EventHandler::keyboardScrollRecursively):

The above functions were refactored to take ScrollDirection and ScrollGranularity as
Parameters rather than a KeyboardEvent.

(WebCore::EventHandler::focusedScrollableAreaUsesScrollSnap):

Returns true if focused container has scroll snaps.

(WebCore::EventHandler::shouldUseSmoothKeyboardScrollingOnFocusedScrollableArea):

Returns true if setting is turned on and focused container does not have scroll snaps.

(WebCore::EventHandler::keyboardScrollRecursively):

Now checks if focused container has scroll snaps.

* Source/WebCore/page/EventHandler.h:

* Source/WebCore/platform/KeyboardScrollingAnimator.cpp:

(WebCore::KeyboardScrollingAnimator::keyboardScrollingKeyForKeyboardEvent): Added.

Helper function for scrollDirectionForKeyboardEvent and scrollGranularityForKeyboardEvent.

(WebCore::KeyboardScrollingAnimator::scrollDirectionForKeyboardEvent): Added.
(WebCore::KeyboardScrollingAnimator::scrollGranularityForKeyboardEvent): Added.

These functions are called by callers of EventHandler::keyboardScrollRecursively.

(WebCore::KeyboardScrollingAnimator::keyboardScrollingKeyFromEvent): Deleted.
(WebCore::KeyboardScrollingAnimator::makeKeyboardScroll): Added.
(WebCore::KeyboardScrollingAnimator::keyboardScrollForKeyboardEvent): Deleted.
(WebCore::KeyboardScrollingAnimator::beginKeyboardScrollGesture):

Refactored to not use KeyboardEvent's.

Source/WebCore/platform/KeyboardScrollingAnimator.h:

* Source/WebCore/platform/ScrollAnimator.h:
(Webcore::ScrollAnimator::usesScrollSnap):

Gets m_scrollController.usesScrollSnap() for use by
EventHandler::focusedScrollableAreaUsesScrollSnap.

* Source/WebKit/UIProcess/API/mac/WKWebViewMac.mm:
(-[WKWebViewMac scrollPageDown:]): Deleted.
(-[WKWebViewMac scrollPageUp:]): Deleted.

Refactoring of pageUp and pageDown scrolling.

* Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:
(WebKit::WebPage::performNonEditingBehaviorForSelector):

Now checks if focused container has scroll snaps.

Canonical link: https://commits.webkit.org/252663@main
  • Loading branch information
danae404 authored and megangardner committed Jul 20, 2022
1 parent 1420da2 commit 4334fd5
Show file tree
Hide file tree
Showing 16 changed files with 352 additions and 182 deletions.
1 change: 0 additions & 1 deletion LayoutTests/TestExpectations
Expand Up @@ -102,7 +102,6 @@ printing/printing-events.html [ Skip ]
fast/forms/enterkeyhint-attribute-values.html [ Skip ]
fast/scrolling/keyboard-scrolling-distance-downArrow.html [ Skip ]
fast/scrolling/keyboard-scrolling-distance-pageDown.html [ Skip ]
fast/scrolling/keyboard-scrolling-last-timestamp.html [ Skip ]
fast/scrolling/unfocusing-page-while-keyboard-scrolling.html [ Skip ]

# Highlighting marked text ranges from layout tests is only supported in WebKit2.
Expand Down

This file was deleted.

36 changes: 0 additions & 36 deletions LayoutTests/fast/scrolling/keyboard-scrolling-last-timestamp.html

This file was deleted.

1 change: 0 additions & 1 deletion LayoutTests/platform/wk2/TestExpectations
Expand Up @@ -859,7 +859,6 @@ http/tests/websocket/tests/hybi/closed-port-delay.html [ Pass ]
js/throw-large-string-oom.html [ Pass ]
fast/scrolling/keyboard-scrolling-distance-downArrow.html [ Pass ]
fast/scrolling/keyboard-scrolling-distance-pageDown.html [ Pass ]
fast/scrolling/keyboard-scrolling-last-timestamp.html [ Pass ]
fast/speechrecognition/permission-error.html [ Pass ]
fast/speechrecognition/start-recognition-then-stop.html [ Pass ]
fast/speechrecognition/start-second-recognition.html [ Pass ]
Expand Down
@@ -0,0 +1,6 @@
PASS arrow key div scrolled to second div.
PASS arrow key div scrolled back to first div.
PASS successfullyParsed is true

TEST COMPLETE

@@ -0,0 +1,73 @@
<!DOCTYPE HTML> <!-- webkit-test-runner [ EventHandlerDrivenSmoothKeyboardScrollingEnabled=true ] -->
<html>
<head>
<style>
html {
scroll-snap-type: x mandatory;
}
.horizontalGallery {
width: 600vw;
height: 100vh;
margin: 0;
padding: 0;
}
.colorBox {
height: 100vh;
width: 100vw;
float: left;
scroll-snap-align: start;
}
#item0 { background-color: red; }
#item1 { background-color: green; }
#item2 { background-color: blue; }
#item3 { background-color: aqua; }
#item4 { background-color: yellow; }
#item5 { background-color: fuchsia; }
</style>
<script src="../../../resources/js-test.js"></script>
<script src="../../../resources/ui-helper.js"></script>
<script>
window.jsTestIsAsync = true;

async function runTests()
{
if (window.internals)
internals.settings.setScrollAnimatorEnabled(false);
try {
await UIHelper.delayFor(0);

let scrollPositionBeforeSnap = document.scrollingElement.scrollLeft;
eventSender.keyDown("rightArrow");
expectTrue(document.scrollingElement.scrollLeft == window.innerWidth, "arrow key div scrolled to second div.");

eventSender.keyDown("leftArrow");
expectTrue(document.scrollingElement.scrollLeft == scrollPositionBeforeSnap, "arrow key div scrolled back to first div.");
} catch (e) {
console.log(e);
} finally {
finishJSTest();
}
}

function onLoad()
{
if (window.eventSender) {
runTests();
} else {
var messageLocation = document.getElementById('item0');
var message = document.createElement('div');
message.innerHTML = "<p>To run this test manually, scroll the page horizontally with the keyboard.</p>";
messageLocation.appendChild(message);
}
}
</script>
</head>
<body onload="onLoad();" class="horizontalGallery">
<div id="item0" class="colorBox"><div id="console"></div></div>
<div id="item1" class="colorBox"></div>
<div id="item2" class="colorBox"></div>
<div id="item3" class="colorBox"></div>
<div id="item4" class="colorBox"></div>
<div id="item5" class="colorBox"></div>
</body>
</html>
@@ -0,0 +1,9 @@
PASS arrow key scrolled to second div.
PASS arrow key scrolled to third div.
PASS arrow key div scrolled back to first div.
PASS page down scrolled to second div.
PASS page up div scrolled back to first div.
PASS successfullyParsed is true

TEST COMPLETE

@@ -0,0 +1,90 @@
<!DOCTYPE HTML> <!-- webkit-test-runner [ EventHandlerDrivenSmoothKeyboardScrollingEnabled=true ] -->
<html>
<head>
<style>
html {
scroll-snap-type: y mandatory;
}
.verticalGallery {
width: 100vw;
height: 600vh;
margin: 0;
padding: 0;
}
.colorBox {
height: 100vh;
width: 100vw;
float: left;
scroll-snap-align: start;
}
#item0 { background-color: red; }
#item1 { background-color: green; }
#item2 { background-color: blue; }
#item3 { background-color: aqua; }
#item4 { background-color: yellow; }
#item5 { background-color: fuchsia; }
</style>
<script src="../../../resources/js-test.js"></script>
<script src="../../../resources/ui-helper.js"></script>
<script>
window.jsTestIsAsync = true;

async function runTests()
{
if (window.internals)
internals.settings.setScrollAnimatorEnabled(false);
try {
await UIHelper.delayFor(0);

let scrollPositionBeforeSnap = document.scrollingElement.scrollTop;
let results = [];
eventSender.keyDown("downArrow");
results.push([document.scrollingElement.scrollTop == window.innerHeight, "arrow key scrolled to second div."]);

eventSender.keyDown("downArrow");
results.push([document.scrollingElement.scrollTop == (window.innerHeight * 2), "arrow key scrolled to third div."]);

eventSender.keyDown("upArrow");
eventSender.keyDown("upArrow");
results.push([document.scrollingElement.scrollTop == scrollPositionBeforeSnap, "arrow key div scrolled back to first div."]);

eventSender.keyDown("pageDown");
results.push([document.scrollingElement.scrollTop == window.innerHeight, "page down scrolled to second div."]);

eventSender.keyDown("pageUp");
results.push([document.scrollingElement.scrollTop == scrollPositionBeforeSnap, "page up div scrolled back to first div."]);

// We avoid modifying the DOM until the test is over, because sometimes
// DOM modifications can reset the main frame scroll position.
results.forEach((result) => {
expectTrue(result[0], result[1]);
});
} catch (e) {
console.log(e);
} finally {
finishJSTest();
}
}

function onLoad()
{
if (window.eventSender) {
runTests();
} else {
var messageLocation = document.getElementById('item0');
var message = document.createElement('div');
message.innerHTML = "<p>To run this test manually, scroll the page vertically with the keyboard.</p>";
messageLocation.appendChild(message);
}
}
</script>
</head>
<body onload="onLoad();" class="verticalGallery">
<div id="item0" class="colorBox"><div id="console"></div></div>
<div id="item1" class="colorBox"></div>
<div id="item2" class="colorBox"></div>
<div id="item3" class="colorBox"></div>
<div id="item4" class="colorBox"></div>
<div id="item5" class="colorBox"></div>
</body>
</html>
6 changes: 6 additions & 0 deletions Source/WebCore/editing/EditorCommand.cpp
Expand Up @@ -1013,11 +1013,17 @@ static bool executeRemoveFormat(Frame& frame, Event*, EditorCommandSource, const

static bool executeScrollPageBackward(Frame& frame, Event*, EditorCommandSource, const String&)
{
if (frame.eventHandler().shouldUseSmoothKeyboardScrollingForFocusedScrollableArea())
return frame.eventHandler().keyboardScrollRecursively(ScrollDirection::ScrollUp, ScrollGranularity::Page, nullptr);

return frame.eventHandler().logicalScrollRecursively(ScrollBlockDirectionBackward, ScrollGranularity::Page);
}

static bool executeScrollPageForward(Frame& frame, Event*, EditorCommandSource, const String&)
{
if (frame.eventHandler().shouldUseSmoothKeyboardScrollingForFocusedScrollableArea())
return frame.eventHandler().keyboardScrollRecursively(ScrollDirection::ScrollDown, ScrollGranularity::Page, nullptr);

return frame.eventHandler().logicalScrollRecursively(ScrollBlockDirectionForward, ScrollGranularity::Page);
}

Expand Down

0 comments on commit 4334fd5

Please sign in to comment.