macOS key-driven smooth scrolling does not stop when focus changes

[Dana Estra]

macOS key-driven smooth scrolling does not stop when focus changes

[Radar WebKit Bug Importer]

<rdar://problem/81131501>

[Dana Estra]

Created attachment 435011 [details]
Patch

[Tim Horton]

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.

[Tim Horton]

You seem to also have broken ... all of the tests.

[Dana Estra]

Created attachment 435527 [details]
WIP

[Beth Dakin]

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.

[Beth Dakin]

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.

[Beth Dakin]

Created attachment 435790 [details]
LayoutTest still broken, trying to fix broken builds

[Beth Dakin]

Created attachment 435806 [details]
New LayoutTest still fails, other tests hopefully don't crash

[Beth Dakin]

Created attachment 439219 [details]
Patch

[Beth Dakin]

Created attachment 439405 [details]
Patch

[Tim Horton]

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.

[Beth Dakin]

Created attachment 439531 [details]
Patch

[Aditya Keerthi]

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.

[Simon Fraser (smfr)]

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?

[Beth Dakin]

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?

[Beth Dakin]

Created attachment 439930 [details]
Patch

This patch shares all the duplicated testing code with TestRunnerShared.

[Beth Dakin]

Created attachment 440071 [details]
Patch

[Beth Dakin]

Created attachment 440286 [details]
Patch

[Beth Dakin]

Created attachment 440287 [details]
Patch

[Beth Dakin]

Created attachment 440864 [details]
Patch

[Beth Dakin]

Created attachment 440879 [details]
Patch

[Tim Horton]

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

[Simon Fraser (smfr)]

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.

[Tim Horton]

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

[Beth Dakin]

Created attachment 441472 [details]
Patch

[Simon Fraser (smfr)]

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.

[Beth Dakin]

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.

[Simon Fraser (smfr)]

That iOS crash is not caused by this patch.

[EWS]

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].

[Brent Fulgham]

This change should be present in STP 139, iOS 15.4 Beta, and macOS 12.3 Beta.