Skip to content

Commit 8984da7

Browse files
rr-codesrr-codes
committed
Page scrolls more than one screenful when pressing Space or Fn+Down
https://bugs.webkit.org/show_bug.cgi?id=250598 rdar://104152802 Reviewed by Simon Fraser. Before event handler driven smooth keyboard scrolling, pressing and holding the spacebar or page up/down keys would do the following sequence of actions: 1. Scroll down the page by a "screenful". 2. On Cocoa platforms, the scrolling mechanism would wait until the `keyRepeatInterval` has passed. 3. After the interval has passed, it would continue consistently page scrolling until the key is released. With the introduction of event handler driven smooth keyboard scrolling, step two was omitted. However, because smooth scrolling was slower than previously, the behavior was effectively unchanged, as the slowness of the scrolling compensated for what would have been the key repeat interval. After #8318, smooth scrolling was adjusted which caused its velocity to increase. As a result, the lack of a delay was now noticable, and page scrolling would scroll more than a "screenful" unless preisely one key event was sent. This PR adjusts smooth scrolling such that the smooth keyboard scroll animation only starts after the `keyRepeatInterval`, with the first part of the entire scroll behaving the same as it did prior to smooth keyboard scrolling. Note that this will apply to only spacebar and page up / down scrolling, and not arrow key scrolling. * Source/WebCore/editing/EditorCommand.cpp: (WebCore::executeScrollPageBackward): (WebCore::executeScrollPageForward): * Source/WebCore/page/EventHandler.cpp: (WebCore::EventHandler::defaultKeyboardEventHandler): (WebCore::EventHandler::defaultKeyboardScrollEventHandler): (WebCore::EventHandler::defaultPageUpDownEventHandler): (WebCore::EventHandler::defaultSpaceEventHandler): (WebCore::EventHandler::beginKeyboardScrollGesture): (WebCore::EventHandler::startKeyboardScrollAnimationOnDocument): (WebCore::EventHandler::startKeyboardScrollAnimationOnRenderBoxLayer): (WebCore::EventHandler::startKeyboardScrollAnimationOnRenderBoxAndItsAncestors): (WebCore::EventHandler::startKeyboardScrollAnimationOnEnclosingScrollableContainer): (WebCore::EventHandler::keyboardScrollRecursively): (WebCore::EventHandler::keyboardScroll): * Source/WebCore/page/EventHandler.h: * Source/WebCore/platform/KeyboardScrollingAnimator.cpp: (WebCore::KeyboardScrollingAnimator::beginKeyboardScrollGesture): * Source/WebCore/platform/KeyboardScrollingAnimator.h: Canonical link: https://commits.webkit.org/259146@main
1 parent 88a0736 commit 8984da7

File tree

6 files changed

+40
-58
lines changed

6 files changed

+40
-58
lines changed

‎Source/WebCore/editing/EditorCommand.cpp‎

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1012,22 +1012,6 @@ static bool executeRemoveFormat(Frame& frame, Event*, EditorCommandSource, const
10121012
return true;
10131013
}
10141014

1015-
static bool executeScrollPageBackward(Frame& frame, Event*, EditorCommandSource, const String&)
1016-
{
1017-
if (frame.eventHandler().shouldUseSmoothKeyboardScrollingForFocusedScrollableArea())
1018-
return frame.eventHandler().keyboardScrollRecursively(ScrollDirection::ScrollUp, ScrollGranularity::Page, nullptr);
1019-
1020-
return frame.eventHandler().logicalScrollRecursively(ScrollBlockDirectionBackward, ScrollGranularity::Page);
1021-
}
1022-
1023-
static bool executeScrollPageForward(Frame& frame, Event*, EditorCommandSource, const String&)
1024-
{
1025-
if (frame.eventHandler().shouldUseSmoothKeyboardScrollingForFocusedScrollableArea())
1026-
return frame.eventHandler().keyboardScrollRecursively(ScrollDirection::ScrollDown, ScrollGranularity::Page, nullptr);
1027-
1028-
return frame.eventHandler().logicalScrollRecursively(ScrollBlockDirectionForward, ScrollGranularity::Page);
1029-
}
1030-
10311015
static bool executeScrollLineUp(Frame& frame, Event*, EditorCommandSource, const String&)
10321016
{
10331017
return frame.eventHandler().scrollRecursively(ScrollUp, ScrollGranularity::Line);
@@ -1771,8 +1755,6 @@ static const CommandMap& createCommandMap()
17711755
{ "Print"_s, { executePrint, supported, enabled, stateNone, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },
17721756
{ "Redo"_s, { executeRedo, supported, enabledRedo, stateNone, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },
17731757
{ "RemoveFormat"_s, { executeRemoveFormat, supported, enabledRangeInEditableText, stateNone, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },
1774-
{ "ScrollPageBackward"_s, { executeScrollPageBackward, supportedFromMenuOrKeyBinding, enabled, stateNone, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },
1775-
{ "ScrollPageForward"_s, { executeScrollPageForward, supportedFromMenuOrKeyBinding, enabled, stateNone, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },
17761758
{ "ScrollLineUp"_s, { executeScrollLineUp, supportedFromMenuOrKeyBinding, enabled, stateNone, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },
17771759
{ "ScrollLineDown"_s, { executeScrollLineDown, supportedFromMenuOrKeyBinding, enabled, stateNone, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },
17781760
{ "ScrollToBeginningOfDocument"_s, { executeScrollToBeginningOfDocument, supportedFromMenuOrKeyBinding, enabled, stateNone, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },

‎Source/WebCore/page/EventHandler.cpp‎

Lines changed: 30 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -3918,8 +3918,13 @@ void EventHandler::defaultKeyboardEventHandler(KeyboardEvent& event)
39183918

39193919
if (event.type() == eventNames().keydownEvent) {
39203920
m_frame.editor().handleKeyboardEvent(event);
3921+
#if PLATFORM(COCOA)
3922+
if (event.defaultHandled() && !(event.keyIdentifier() == "PageDown"_s || event.keyIdentifier() == "PageUp"_s))
3923+
#else
39213924
if (event.defaultHandled())
3925+
#endif
39223926
return;
3927+
39233928
if (event.key() == "Escape"_s) {
39243929
if (RefPtr activeModalDialog = m_frame.document()->activeModalDialog())
39253930
activeModalDialog->queueCancelTask();
@@ -4337,14 +4342,14 @@ void EventHandler::defaultTextInputEventHandler(TextEvent& event)
43374342
bool EventHandler::defaultKeyboardScrollEventHandler(KeyboardEvent& event, ScrollLogicalDirection direction, ScrollGranularity granularity)
43384343
{
43394344
if (shouldUseSmoothKeyboardScrollingForFocusedScrollableArea())
4340-
return keyboardScrollRecursively(scrollDirectionForKeyboardEvent(event), scrollGranularityForKeyboardEvent(event), nullptr);
4345+
return keyboardScrollRecursively(scrollDirectionForKeyboardEvent(event), scrollGranularityForKeyboardEvent(event), nullptr, event.repeat());
43414346

43424347
return logicalScrollRecursively(direction, granularity);
43434348
}
43444349

43454350
void EventHandler::defaultPageUpDownEventHandler(KeyboardEvent& event)
43464351
{
4347-
#if PLATFORM(GTK) || PLATFORM(WPE) || PLATFORM(WIN_CAIRO)
4352+
#if PLATFORM(GTK) || PLATFORM(WPE) || PLATFORM(WIN_CAIRO) || PLATFORM(COCOA)
43484353
ASSERT(event.type() == eventNames().keydownEvent);
43494354

43504355
if (event.ctrlKey() || event.metaKey() || event.altKey() || event.altGraphKey() || event.shiftKey())
@@ -4395,7 +4400,7 @@ void EventHandler::defaultSpaceEventHandler(KeyboardEvent& event)
43954400

43964401
bool defaultHandled = false;
43974402
if (shouldUseSmoothKeyboardScrollingForFocusedScrollableArea())
4398-
defaultHandled = keyboardScroll(scrollDirectionForKeyboardEvent(event), scrollGranularityForKeyboardEvent(event), nullptr);
4403+
defaultHandled = keyboardScroll(scrollDirectionForKeyboardEvent(event), scrollGranularityForKeyboardEvent(event), nullptr, event.repeat());
43994404
else
44004405
defaultHandled = view->logicalScroll(direction, ScrollGranularity::Page);
44014406

@@ -4438,48 +4443,48 @@ void EventHandler::stopKeyboardScrolling()
44384443
animator->handleKeyUpEvent();
44394444
}
44404445

4441-
bool EventHandler::beginKeyboardScrollGesture(KeyboardScrollingAnimator* animator, ScrollDirection direction, ScrollGranularity granularity)
4446+
bool EventHandler::beginKeyboardScrollGesture(KeyboardScrollingAnimator* animator, ScrollDirection direction, ScrollGranularity granularity, bool isKeyRepeat)
44424447
{
4443-
if (animator && animator->beginKeyboardScrollGesture(direction, granularity)) {
4448+
if (animator && animator->beginKeyboardScrollGesture(direction, granularity, isKeyRepeat)) {
44444449
m_frame.page()->setCurrentKeyboardScrollingAnimator(animator);
44454450
return true;
44464451
}
44474452

44484453
return false;
44494454
}
44504455

4451-
bool EventHandler::startKeyboardScrollAnimationOnDocument(ScrollDirection direction, ScrollGranularity granularity)
4456+
bool EventHandler::startKeyboardScrollAnimationOnDocument(ScrollDirection direction, ScrollGranularity granularity, bool isKeyRepeat)
44524457
{
44534458
auto view = m_frame.view();
44544459
if (!view)
44554460
return false;
44564461

44574462
auto* animator = view->scrollAnimator().keyboardScrollingAnimator();
4458-
return beginKeyboardScrollGesture(animator, direction, granularity);
4463+
return beginKeyboardScrollGesture(animator, direction, granularity, isKeyRepeat);
44594464
}
44604465

4461-
bool EventHandler::startKeyboardScrollAnimationOnRenderBoxLayer(ScrollDirection direction, ScrollGranularity granularity, RenderBox* renderBox)
4466+
bool EventHandler::startKeyboardScrollAnimationOnRenderBoxLayer(ScrollDirection direction, ScrollGranularity granularity, RenderBox* renderBox, bool isKeyRepeat)
44624467
{
44634468
auto* scrollableArea = renderBox->layer() ? renderBox->layer()->scrollableArea() : nullptr;
44644469
if (!scrollableArea)
44654470
return false;
44664471

44674472
auto* animator = scrollableArea->scrollAnimator().keyboardScrollingAnimator();
4468-
return beginKeyboardScrollGesture(animator, direction, granularity);
4473+
return beginKeyboardScrollGesture(animator, direction, granularity, isKeyRepeat);
44694474
}
44704475

4471-
bool EventHandler::startKeyboardScrollAnimationOnRenderBoxAndItsAncestors(ScrollDirection direction, ScrollGranularity granularity, RenderBox* renderBox)
4476+
bool EventHandler::startKeyboardScrollAnimationOnRenderBoxAndItsAncestors(ScrollDirection direction, ScrollGranularity granularity, RenderBox* renderBox, bool isKeyRepeat)
44724477
{
44734478
while (renderBox && !renderBox->isRenderView()) {
4474-
if (startKeyboardScrollAnimationOnRenderBoxLayer(direction, granularity, renderBox))
4479+
if (startKeyboardScrollAnimationOnRenderBoxLayer(direction, granularity, renderBox, isKeyRepeat))
44754480
return true;
44764481
renderBox = renderBox->containingBlock();
44774482
}
44784483

44794484
return false;
44804485
}
44814486

4482-
bool EventHandler::startKeyboardScrollAnimationOnEnclosingScrollableContainer(ScrollDirection direction, ScrollGranularity granularity, Node* startingNode)
4487+
bool EventHandler::startKeyboardScrollAnimationOnEnclosingScrollableContainer(ScrollDirection direction, ScrollGranularity granularity, Node* startingNode, bool isKeyRepeat)
44834488
{
44844489
RefPtr node = startingNode;
44854490

@@ -4495,14 +4500,17 @@ bool EventHandler::startKeyboardScrollAnimationOnEnclosingScrollableContainer(Sc
44954500
return false;
44964501

44974502
RenderBox& renderBox = renderer->enclosingBox();
4498-
if (!renderer->isListBox() && startKeyboardScrollAnimationOnRenderBoxAndItsAncestors(direction, granularity, &renderBox))
4503+
if (!renderer->isListBox() && startKeyboardScrollAnimationOnRenderBoxAndItsAncestors(direction, granularity, &renderBox, isKeyRepeat))
44994504
return true;
45004505
}
45014506
return false;
45024507
}
45034508

4504-
bool EventHandler::focusedScrollableAreaShouldUseSmoothKeyboardScrolling()
4509+
bool EventHandler::shouldUseSmoothKeyboardScrollingForFocusedScrollableArea()
45054510
{
4511+
if (!m_frame.settings().eventHandlerDrivenSmoothKeyboardScrollingEnabled())
4512+
return false;
4513+
45064514
Node* node = m_frame.document()->focusedElement();
45074515
if (!node)
45084516
node = m_mousePressNode.get();
@@ -4517,20 +4525,15 @@ bool EventHandler::focusedScrollableAreaShouldUseSmoothKeyboardScrolling()
45174525
#if PLATFORM(GTK) || PLATFORM(WPE)
45184526
if (!m_frame.settings().asyncFrameScrollingEnabled())
45194527
return false;
4528+
#endif
45204529

45214530
if (!scrollableArea->scrollAnimatorEnabled())
45224531
return false;
4523-
#endif
45244532

45254533
return true;
45264534
}
45274535

4528-
bool EventHandler::shouldUseSmoothKeyboardScrollingForFocusedScrollableArea()
4529-
{
4530-
return m_frame.settings().eventHandlerDrivenSmoothKeyboardScrollingEnabled() && focusedScrollableAreaShouldUseSmoothKeyboardScrolling() && m_frame.settings().scrollAnimatorEnabled();
4531-
}
4532-
4533-
bool EventHandler::keyboardScrollRecursively(std::optional<ScrollDirection> direction, std::optional<ScrollGranularity> granularity, Node* startingNode)
4536+
bool EventHandler::keyboardScrollRecursively(std::optional<ScrollDirection> direction, std::optional<ScrollGranularity> granularity, Node* startingNode, bool isKeyRepeat)
45344537
{
45354538
if (!direction || !granularity)
45364539
return false;
@@ -4539,10 +4542,10 @@ bool EventHandler::keyboardScrollRecursively(std::optional<ScrollDirection> dire
45394542

45404543
m_frame.document()->updateLayoutIgnorePendingStylesheets();
45414544

4542-
if (startKeyboardScrollAnimationOnEnclosingScrollableContainer(*direction, *granularity, startingNode))
4545+
if (startKeyboardScrollAnimationOnEnclosingScrollableContainer(*direction, *granularity, startingNode, isKeyRepeat))
45434546
return true;
45444547

4545-
if (startKeyboardScrollAnimationOnDocument(*direction, *granularity))
4548+
if (startKeyboardScrollAnimationOnDocument(*direction, *granularity, isKeyRepeat))
45464549
return true;
45474550

45484551
RefPtr frame = &m_frame;
@@ -4553,10 +4556,10 @@ bool EventHandler::keyboardScrollRecursively(std::optional<ScrollDirection> dire
45534556
if (!localParent)
45544557
return false;
45554558

4556-
return localParent->eventHandler().keyboardScrollRecursively(direction, granularity, m_frame.ownerElement());
4559+
return localParent->eventHandler().keyboardScrollRecursively(direction, granularity, m_frame.ownerElement(), isKeyRepeat);
45574560
}
45584561

4559-
bool EventHandler::keyboardScroll(std::optional<ScrollDirection> direction, std::optional<ScrollGranularity> granularity, Node* startingNode)
4562+
bool EventHandler::keyboardScroll(std::optional<ScrollDirection> direction, std::optional<ScrollGranularity> granularity, Node* startingNode, bool isKeyRepeat)
45604563
{
45614564
if (!direction || !granularity)
45624565
return false;
@@ -4565,10 +4568,10 @@ bool EventHandler::keyboardScroll(std::optional<ScrollDirection> direction, std:
45654568

45664569
m_frame.document()->updateLayoutIgnorePendingStylesheets();
45674570

4568-
if (startKeyboardScrollAnimationOnEnclosingScrollableContainer(*direction, *granularity, startingNode))
4571+
if (startKeyboardScrollAnimationOnEnclosingScrollableContainer(*direction, *granularity, startingNode, isKeyRepeat))
45694572
return true;
45704573

4571-
return startKeyboardScrollAnimationOnDocument(*direction, *granularity);
4574+
return startKeyboardScrollAnimationOnDocument(*direction, *granularity, isKeyRepeat);
45724575
}
45734576

45744577
void EventHandler::defaultArrowEventHandler(FocusDirection focusDirection, KeyboardEvent& event)

‎Source/WebCore/page/EventHandler.h‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -366,7 +366,7 @@ class EventHandler {
366366

367367
WEBCORE_EXPORT void selectClosestContextualWordOrLinkFromHitTestResult(const HitTestResult&, AppendTrailingWhitespace);
368368

369-
bool keyboardScrollRecursively(std::optional<ScrollDirection>, std::optional<ScrollGranularity>, Node*);
369+
bool keyboardScrollRecursively(std::optional<ScrollDirection>, std::optional<ScrollGranularity>, Node*, bool isKeyRepeat);
370370
WEBCORE_EXPORT bool shouldUseSmoothKeyboardScrollingForFocusedScrollableArea();
371371

372372
private:
@@ -389,14 +389,13 @@ class EventHandler {
389389
bool handleMousePressEventDoubleClick(const MouseEventWithHitTestResults&);
390390
bool handleMousePressEventTripleClick(const MouseEventWithHitTestResults&);
391391

392-
bool keyboardScroll(std::optional<ScrollDirection>, std::optional<ScrollGranularity>, Node*);
393-
bool beginKeyboardScrollGesture(KeyboardScrollingAnimator*, ScrollDirection, ScrollGranularity);
392+
bool keyboardScroll(std::optional<ScrollDirection>, std::optional<ScrollGranularity>, Node*, bool isKeyRepeat);
393+
bool beginKeyboardScrollGesture(KeyboardScrollingAnimator*, ScrollDirection, ScrollGranularity, bool isKeyRepeat);
394394
void stopKeyboardScrolling();
395-
bool startKeyboardScrollAnimationOnDocument(ScrollDirection, ScrollGranularity);
396-
bool startKeyboardScrollAnimationOnRenderBoxLayer(ScrollDirection, ScrollGranularity, RenderBox*);
397-
bool startKeyboardScrollAnimationOnRenderBoxAndItsAncestors(ScrollDirection, ScrollGranularity, RenderBox*);
398-
bool startKeyboardScrollAnimationOnEnclosingScrollableContainer(ScrollDirection, ScrollGranularity, Node*);
399-
bool focusedScrollableAreaShouldUseSmoothKeyboardScrolling();
395+
bool startKeyboardScrollAnimationOnDocument(ScrollDirection, ScrollGranularity, bool isKeyRepeat);
396+
bool startKeyboardScrollAnimationOnRenderBoxLayer(ScrollDirection, ScrollGranularity, RenderBox*, bool isKeyRepeat);
397+
bool startKeyboardScrollAnimationOnRenderBoxAndItsAncestors(ScrollDirection, ScrollGranularity, RenderBox*, bool isKeyRepeat);
398+
bool startKeyboardScrollAnimationOnEnclosingScrollableContainer(ScrollDirection, ScrollGranularity, Node*, bool isKeyRepeat);
400399

401400
#if ENABLE(DRAG_SUPPORT)
402401
bool handleMouseDraggedEvent(const MouseEventWithHitTestResults&, CheckDragHysteresis = ShouldCheckDragHysteresis);

‎Source/WebCore/platform/KeyboardScrollingAnimator.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,7 @@ std::optional<KeyboardScroll> KeyboardScrollingAnimator::makeKeyboardScroll(Scro
196196
return scroll;
197197
}
198198

199-
bool KeyboardScrollingAnimator::beginKeyboardScrollGesture(ScrollDirection direction, ScrollGranularity granularity)
199+
bool KeyboardScrollingAnimator::beginKeyboardScrollGesture(ScrollDirection direction, ScrollGranularity granularity, bool isKeyRepeat)
200200
{
201201
auto scroll = makeKeyboardScroll(direction, granularity);
202202
if (!scroll)
@@ -208,7 +208,7 @@ bool KeyboardScrollingAnimator::beginKeyboardScrollGesture(ScrollDirection direc
208208
if (!rubberbandableDirections().at(boxSideForDirection(direction)))
209209
return false;
210210

211-
if (granularity == ScrollGranularity::Document) {
211+
if (granularity == ScrollGranularity::Document || (!isKeyRepeat && granularity == ScrollGranularity::Page)) {
212212
m_scrollableArea.endKeyboardScroll(false);
213213
auto newPosition = IntPoint(m_scrollableArea.scrollAnimator().currentPosition() + scroll->offset);
214214
m_scrollableArea.scrollAnimator().scrollToPositionWithAnimation(newPosition);

‎Source/WebCore/platform/KeyboardScrollingAnimator.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ class KeyboardScrollingAnimator : public CanMakeWeakPtr<KeyboardScrollingAnimato
5656
public:
5757
KeyboardScrollingAnimator(ScrollableArea&);
5858

59-
bool beginKeyboardScrollGesture(ScrollDirection, ScrollGranularity);
59+
bool beginKeyboardScrollGesture(ScrollDirection, ScrollGranularity, bool isKeyRepeat);
6060
void handleKeyUpEvent();
6161
WEBCORE_EXPORT void stopScrollingImmediately();
6262

‎Source/WebKit/UIProcess/mac/WebViewImpl.mm‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2577,8 +2577,6 @@ static NSTrackingAreaOptions trackingAreaOptions()
25772577
{ @selector(pageDownAndModifySelection:), "MovePageDownAndModifySelection"_s },
25782578
{ @selector(pageUp:), "MovePageUp"_s },
25792579
{ @selector(pageUpAndModifySelection:), "MovePageUpAndModifySelection"_s },
2580-
{ @selector(scrollPageDown:), "ScrollPageForward"_s },
2581-
{ @selector(scrollPageUp:), "ScrollPageBackward"_s },
25822580
{ @selector(_pasteAsQuotation:), "PasteAsQuotation"_s },
25832581
};
25842582

0 commit comments

Comments
 (0)