[webkit-reviews] review denied: [Bug 228302] macOS key-driven smooth scrolling does not stop when focus changes : [Attachment 440879] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied  review:
Bug 228302: macOS key-driven smooth scrolling does not stop when focus changes
https://bugs.webkit.org/show_bug.cgi?id=228302

Attachment 440879: Patch

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




--- Comment #24 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 440879
  --> https://bugs.webkit.org/attachment.cgi?id=440879
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.