Bug 228302 - macOS key-driven smooth scrolling does not stop when focus changes
Summary: macOS key-driven smooth scrolling does not stop when focus changes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-26 14:44 PDT by Dana Estra
Modified: 2021-10-20 14:50 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.87 KB, patch)
2021-08-05 11:34 PDT, Dana Estra
no flags Details | Formatted Diff | Diff
WIP (69.00 KB, patch)
2021-08-13 17:05 PDT, Dana Estra
no flags Details | Formatted Diff | Diff
Manual testing works, LayoutTest still broken (62.40 KB, patch)
2021-08-17 21:45 PDT, Beth Dakin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
LayoutTest still broken, trying to fix broken builds (65.15 KB, patch)
2021-08-18 13:00 PDT, Beth Dakin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
New LayoutTest still fails, other tests hopefully don't crash (65.15 KB, patch)
2021-08-18 15:33 PDT, Beth Dakin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (64.94 KB, patch)
2021-09-24 17:26 PDT, Beth Dakin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (66.69 KB, patch)
2021-09-27 15:49 PDT, Beth Dakin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (66.59 KB, patch)
2021-09-28 15:20 PDT, Beth Dakin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (70.65 KB, patch)
2021-09-30 21:32 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (80.01 KB, patch)
2021-10-01 16:29 PDT, Beth Dakin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (80.06 KB, patch)
2021-10-04 10:08 PDT, Beth Dakin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (80.13 KB, patch)
2021-10-05 15:55 PDT, Beth Dakin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (80.13 KB, patch)
2021-10-05 16:03 PDT, Beth Dakin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (35.40 KB, patch)
2021-10-11 17:24 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (35.38 KB, patch)
2021-10-11 20:28 PDT, Beth Dakin
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (34.30 KB, patch)
2021-10-15 21:28 PDT, Beth Dakin
simon.fraser: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (34.35 KB, patch)
2021-10-19 15:52 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Estra 2021-07-26 14:44:41 PDT
macOS key-driven smooth scrolling does not stop when focus changes
Comment 1 Radar WebKit Bug Importer 2021-07-26 14:47:19 PDT
<rdar://problem/81131501>
Comment 2 Dana Estra 2021-08-05 11:34:27 PDT
Created attachment 435011 [details]
Patch
Comment 3 Tim Horton 2021-08-05 14:07:38 PDT
Comment on attachment 435011 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=435011&action=review

> Source/WebCore/ChangeLog:8
> +        No tests yet.

This seems like a grand time to rectify this, this one should be eminently testable (long page, send keydown, change focus, make sure the scroll stops... eventually (it won't be immediately, this will be a tricky part))

> Source/WebCore/page/Page.cpp:2356
> +        reportUnfocusedToScrollableAreas();

Something is weird about the name, but I can't quite place it. I think it's because we usually say didX or xChanged (didLoseFocus?). I'm not sure the "to scrollable areas" part belongs in the Page method name; you could imagine using it for other things on focus loss.

(though I do see some precedent for this style too, in e.g. `notifyPageThatContentAreaWillPaint`).

Maybe someone else has opinions :)

> Source/WebCore/platform/ScrollController.cpp:234
> +        m_client.keyboardScrollingAnimator()->handleKeyUpEvent();

Probably would be ideal if this called a function something like `stopScrollingWithAnimation`, and make `handleKeyUpEvent`'s implementation also call that. Not great to call something named `keyUp` not in response to a `keyUp` event, that's just confusing.
Comment 4 Tim Horton 2021-08-05 14:08:08 PDT
You seem to also have broken ... all of the tests.
Comment 5 Dana Estra 2021-08-13 17:05:38 PDT
Created attachment 435527 [details]
WIP
Comment 6 Beth Dakin 2021-08-17 12:43:01 PDT
Steps to reproduce:

1. Have 2 browser windows open. The focused window should be on a page that scrolls, such as https://trac.webkit.org/changeset/280492/webkit , the second window can be on any page.

2. Start scrolling the focused window by holding down the up or down arrow.

3. Still holding the key down, with scrolling still happening, switch to the other window.

Expected: scrolling stops.
Actual: scrolling keeps happening in the background window until the edge of the document is reached.
Comment 7 Beth Dakin 2021-08-17 21:45:44 PDT
Created attachment 435745 [details]
Manual testing works, LayoutTest still broken

I still haven't dug into the testing code Dana wrote, and the test is still failing locally for me. Posting to get any feedback on the WebCore changes (and just to keep track of the WebCore changes in case I don't get to fixing the test very soon, in case someone else picks this up).

Manually testing, I see the following behaviors, which all match shipping:

- If you are keyboard scrolling the main frame and switch focus to a different browser window while still holding the arrow key, scrolling in the now inactive window animates to a stop right away, while still holding the key.

- If you are keyboard scrolling the main frame and switch focus to a different app while still holding the arrow key, scrolling in the now inactive window stops when you release the key (but continues until then).

- If you are keyboard scrolling a subframe and switch focus to another subframe or the mainframe, scrolling in the now blurred frame animates to a stop, and the focused frames starts scrolling while the key is still held down.
Comment 8 Beth Dakin 2021-08-18 13:00:46 PDT
Created attachment 435790 [details]
LayoutTest still broken, trying to fix broken builds
Comment 9 Beth Dakin 2021-08-18 15:33:10 PDT
Created attachment 435806 [details]
New LayoutTest still fails, other tests hopefully don't crash
Comment 10 Beth Dakin 2021-09-24 17:26:25 PDT
Created attachment 439219 [details]
Patch
Comment 11 Beth Dakin 2021-09-27 15:49:38 PDT
Created attachment 439405 [details]
Patch
Comment 12 Tim Horton 2021-09-28 01:30:42 PDT
Comment on attachment 439405 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439405&action=review

> Source/WebCore/page/Page.cpp:2381
> +            scrollableArea->updateScrollAnimationIfNecessary();

Can anything downstream of `updateScrollAnimationIfNecessary` run script and invalidate your iterator? (I pray the answer is no, but you never know)

> Source/WebCore/page/Page.h:890
> +    void updateScrollAnimationIfNecessary();

I /expect/ smfr to object to the vague name, maybe this should be named about the specific animation?

> Source/WebCore/platform/ScrollingEffectsController.cpp:93
> +void ScrollingEffectsController::updateScrollAnimationIfNecessary()
> +{
> +    if (m_isAnimatingKeyboardScrolling)
> +        m_client.keyboardScrollingAnimator()->handleKeyUpEvent();
> +}

"update scroll animation if necessary" /always/ sends a key up if you're scrolling? That sounds even less like "update if necessary" than I realized. Is it really like "hey please stop any outstanding keyboard scrolling animations for this Page/Frame/ScrollableArea"?

> Tools/DumpRenderTree/mac/EventSendingController.mm:1440
> +    if ([character isEqualToString:@"leftArrow"]) {

I assume all of this code is copied from somewhere? Is it possible to at least share the keycode/character mapping stuff?

> Tools/WebKitTestRunner/mac/EventSenderProxy.mm:857
> +    NSString *eventCharacter = character;

Ditto my question from the WK1 version.
Comment 13 Beth Dakin 2021-09-28 15:20:51 PDT
Created attachment 439531 [details]
Patch
Comment 14 Aditya Keerthi 2021-09-28 15:52:57 PDT
Comment on attachment 439531 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439531&action=review

> Source/WebCore/page/FocusController.cpp:363
> +        if (FrameView* oldFrameView = oldFrame->view()) {

Consider `if (auto* oldFrameView = oldFrame ? oldFrame->view() : nullptr)` instead of the nested if.

> Source/WebCore/page/Page.cpp:2369
> +    for (Frame* frame = &mainFrame(); frame; frame = frame->tree().traverseNext()) {

Nit: "auto* frame".

> Source/WebCore/page/Page.cpp:2370
> +        FrameView* frameView = frame->view();

Nit: "auto* frameView".

> LayoutTests/fast/scrolling/unfocusing-page-while-keyboard-scrolling.html:5
> +    <script src="../../resources/js-test-pre.js"></script>

This can be `<script src="../../resources/js-test.js"></script>`.

And then the `js-test-post.js` below can be removed.
Comment 15 Simon Fraser (smfr) 2021-09-28 16:48:21 PDT
Comment on attachment 439531 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439531&action=review

> Source/WebCore/platform/ScrollingEffectsController.h:146
> +    void updateScrollAnimationIfNecessary();

If "update" is always "stop" then we should call it "stop" everywhere. I don't think it will automatically start under any circumstances.

> Tools/DumpRenderTree/mac/EventSendingController.mm:1230
> +    NSString *eventCharacter = character;
> +    unsigned short keyCode = 0;
> +    if ([character isEqualToString:@"leftArrow"]) {
> +        const unichar ch = NSLeftArrowFunctionKey;
> +        eventCharacter = [NSString stringWithCharacters:&ch length:1];
> +        keyCode = 0x7B;
> +    } else if ([character isEqualToString:@"rightArrow"]) {
> +        const unichar ch = NSRightArrowFunctionKey;
> +        eventCharacter = [NSString stringWithCharacters:&ch length:1];

This is the 3rd copy of this code in the tree. I think we really need to share it (see TestRunnerShared).

> Tools/DumpRenderTree/mac/EventSendingController.mm:1447
> +    NSString *eventCharacter = character;
> +    unsigned short keyCode = 0;
> +    if ([character isEqualToString:@"leftArrow"]) {
> +        const unichar ch = NSLeftArrowFunctionKey;
> +        eventCharacter = [NSString stringWithCharacters:&ch length:1];
> +        keyCode = 0x7B;
> +    } else if ([character isEqualToString:@"rightArrow"]) {
> +        const unichar ch = NSRightArrowFunctionKey;
> +        eventCharacter = [NSString stringWithCharacters:&ch length:1];
> +        keyCode = 0x7C;

Same.

> Tools/WebKitTestRunner/mac/EventSenderProxy.mm:864
> +    unsigned short keyCode = 0;
> +    if ([character isEqualToString:@"leftArrow"]) {
> +        const unichar ch = NSLeftArrowFunctionKey;
> +        eventCharacter = [NSString stringWithCharacters:&ch length:1];
> +        keyCode = 0x7B;
> +    } else if ([character isEqualToString:@"rightArrow"]) {
> +        const unichar ch = NSRightArrowFunctionKey;

4th copy now?

> LayoutTests/resources/ui-helper.js:355
> +            eventSender.rawKeyDown(key, modifiers);
> +            return Promise.resolve();
> +        }
> +
> +        return new Promise((resolve) => {
> +            testRunner.runUIScript(`uiController.rawKeyDown("${key}", ${JSON.stringify(modifiers)});`, resolve);

Can we just make uiController.rawKeyDown() work on macOS?
Comment 16 Beth Dakin 2021-09-30 21:32:31 PDT
Created attachment 439819 [details]
Patch

This patch addresses many comments above. This attempts to share more code in WebKitTestRunner, but it doesn't yet share any code with or within DumpRenderTree. Before I try to address that, I wanted to throw this on the bots to see that it doesn't introduce failures (seems okay with just fast/ on my machine), and to get feedback on whether using a little class to share code is too ridiculous. Should it just be a static function?
Comment 17 Beth Dakin 2021-10-01 16:29:16 PDT
Created attachment 439930 [details]
Patch

This patch shares all the duplicated testing code with TestRunnerShared.
Comment 18 Beth Dakin 2021-10-04 10:08:09 PDT
Created attachment 440071 [details]
Patch
Comment 19 Beth Dakin 2021-10-05 15:55:19 PDT
Created attachment 440286 [details]
Patch
Comment 20 Beth Dakin 2021-10-05 16:03:36 PDT
Created attachment 440287 [details]
Patch
Comment 21 Beth Dakin 2021-10-11 17:24:13 PDT
Created attachment 440864 [details]
Patch
Comment 22 Beth Dakin 2021-10-11 20:28:36 PDT
Created attachment 440879 [details]
Patch
Comment 23 Tim Horton 2021-10-13 11:41:11 PDT
Comment on attachment 440879 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440879&action=review

> Source/WebCore/page/FocusController.cpp:363
> +        oldFrameView->stopScrollAnimations();

I think this is specifically about keyboard animations? It won't stop the rubber-band or a scroll-to, right? Maybe the name should be more specific?

> Tools/ChangeLog:6
> +

extraneous newline
Comment 24 Simon Fraser (smfr) 2021-10-13 11:47:13 PDT
Comment on attachment 440879 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440879&action=review

> Source/WebCore/page/Page.cpp:2384
> +        for (auto& scrollableArea : *scrollableAreas)
> +            scrollableArea->stopScrollAnimations();

This will stop smooth scroll animations and possibly rubberband animations too, which I guess is OK (they are short).

You'll also need to call stopAsyncAnimatedScroll() to prepare for the keyboard scroller running on the scrolling thread (the code is not well factored; ScrollableArea should have a common function for stopping sync and async scrolls).

> Source/WebCore/platform/ScrollingEffectsController.cpp:97
> +void ScrollingEffectsController::stopScrollAnimations()
> +{
> +    if (m_isAnimatingKeyboardScrolling)
> +        m_client.keyboardScrollingAnimator()->handleKeyUpEvent();
> +}

I think it's wrong for ScrollingEffectsController::stopScrollAnimations() to only stop keyboard animations. This class now has smooth scroll and will have rubberband animations.

Also, ScrollingEffectsController::stopAnimatedScroll() and ScrollingEffectsController::stopKeyboardScrolling() already exist.

Either change the whole call chain from Page down to be about keyboard scrolling, or keep the names and call the existing stopAnimatedScroll().

> Tools/DumpRenderTree/mac/EventSendingController.mm:1088
> +    RetainPtr<ModifierKeys> modifierKeys = [ModifierKeys modifierKeysWithKey:character modifiers:buildModifierFlags(modifiers) keyLocation:location];
> +
> +    [[[mainFrame frameView] documentView] layout];
> +
> +#if !PLATFORM(IOS_FAMILY)
> +    auto event = retainPtr([NSEvent keyEventWithType:NSEventTypeKeyUp
> +        location:NSMakePoint(5, 5)
> +        modifierFlags:modifierKeys->modifierFlags
> +        timestamp:[self currentEventTime]
> +        windowNumber:[[[mainFrame webView] window] windowNumber]
> +        context:[NSGraphicsContext currentContext]
> +        characters:modifierKeys->eventCharacter.get()
> +        charactersIgnoringModifiers:modifierKeys->charactersIgnoringModifiers.get()
> +        isARepeat:NO
> +        keyCode:modifierKeys->keyCode]);
> +#else
> +    auto event = adoptNS([[WebEvent alloc] initWithKeyEventType:WebEventKeyUp timeStamp:[self currentEventTime] characters:modifierKeys->eventCharacter.get() charactersIgnoringModifiers:modifierKeys->charactersIgnoringModifiers.get() modifiers:(WebEventFlags)modifierKeys->modifierFlags isRepeating:NO withFlags:0 withInputManagerHint:nil keyCode:[character characterAtIndex:0] isTabKey:([character characterAtIndex:0] == '\t')]);
> +#endif

Lots of sharable code here.

> Tools/WebKitTestRunner/InjectedBundle/Bindings/EventSendingController.idl:49
> +    undefined rawKeyDown(DOMString key, object modifierArray, long location);

I'd like to see a comment explaining what "raw" means.

> Tools/WebKitTestRunner/mac/EventSenderProxy.mm:695
> +    RetainPtr<ModifierKeys> modifierKeys = [ModifierKeys modifierKeysWithKey:toWTFString(key) modifiers:buildModifierFlags(modifiers) keyLocation:keyLocation];
> +
> +    NSEvent *event = [NSEvent keyEventWithType:NSEventTypeKeyUp
> +        location:NSMakePoint(5, 5)
> +        modifierFlags:modifierKeys->modifierFlags
> +        timestamp:absoluteTimeForEventTime(currentEventTime())
> +        windowNumber:[m_testController->mainWebView()->platformWindow() windowNumber]
> +        context:[NSGraphicsContext currentContext]
> +        characters:modifierKeys->eventCharacter.get()
> +        charactersIgnoringModifiers:modifierKeys->charactersIgnoringModifiers.get()
> +        isARepeat:NO
> +        keyCode:modifierKeys->keyCode];

Ditto for code sharing.

> LayoutTests/fast/scrolling/unfocusing-page-while-keyboard-scrolling.html:1
> +<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true EventHandlerDrivenSmoothKeyboardScrollingEnabled=true ] -->

Has useFlexibleViewport but you're only enabling the test on macOS? useFlexibleViewport only applies to iOS.
Comment 25 Tim Horton 2021-10-13 13:58:39 PDT
Comment on attachment 440879 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440879&action=review

>> Tools/WebKitTestRunner/InjectedBundle/Bindings/EventSendingController.idl:49
>> +    undefined rawKeyDown(DOMString key, object modifierArray, long location);
> 
> I'd like to see a comment explaining what "raw" means.

Or maybe a FIXME that says "keydown should really be called 'keyDownAndUp', rawKeyDown just sends a keyDown" :P
Comment 26 Beth Dakin 2021-10-15 21:28:50 PDT
Created attachment 441472 [details]
Patch
Comment 27 Simon Fraser (smfr) 2021-10-16 10:32:35 PDT
Comment on attachment 441472 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=441472&action=review

> Source/WebCore/page/Page.cpp:2383
> +        const HashSet<ScrollableArea*>* scrollableAreas = frameView->scrollableAreas();

auto?

> Tools/DumpRenderTree/mac/EventSendingController.mm:1033
> +

Extra line.

> LayoutTests/ChangeLog:8
> +        useFlexibleViewport and EventHandlerDrivenSmoothKeyboardScrollingEnabled keys are WK2 only 

The useFlexibleViewport thing isn't relevant any more.
Comment 28 Beth Dakin 2021-10-19 15:52:16 PDT
Created attachment 441810 [details]
Patch

Should address Simon's comments. I'm v confused by the layout test failure on iOS in the last patch, and I could not find the layout test results to correspond to it on the bots even. If anyone can point me in the right direction, it would be appreciated.
Comment 29 Simon Fraser (smfr) 2021-10-19 16:07:30 PDT
That iOS crash is not caused by this patch.
Comment 30 EWS 2021-10-20 14:50:26 PDT
Committed r284575 (243315@main): <https://commits.webkit.org/243315@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441810 [details].