Changeset 245285 in webkit

Timestamp:

May 14, 2019, 9:43:51 AM (7 years ago)

Author:

commit-queue@webkit.org

Message:

[iOS] Cannot scroll to beginning of document after scrolling to end of document and vice versa via key commands
​https://bugs.webkit.org/show_bug.cgi?id=197848
<rdar://problem/49523065>

Patch by Daniel Bates <​dabates@apple.com> on 2019-05-14
Reviewed by Brent Fulgham.

Source/WebKit:

Following the fix for <rdar://problem/49523065>, UIKit no longer emits a keyup event for a Command-
modified key. This breaks WebKit's own implementation of key command handling for scrolling to the
beginning or end of the document (triggered using Command + Arrow Up and Command + Arrow Down,
respectively) because it watches for keyup events to reset state after initiating a scroll. If state
is not reset then the scroll key command logic becomes confused and may not perform a subsequent scroll.
It seems like we can actually get away with supporting these key commands and future Command modified
commands by preemptively reseting state on keydown if the Command modifier is held down. If this does
not work out then we can do something more complicated.

• UIProcess/ios/WKKeyboardScrollingAnimator.mm:

(-[WKKeyboardScrollingAnimator handleKeyEvent:]):

LayoutTests:

Add a test to ensure that key commands can be used to scroll to the end of the page and then
to the beginning of the page.

• fast/scrolling/ios/scroll-to-beginning-and-end-of-document-expected.txt: Added.
• fast/scrolling/ios/scroll-to-beginning-and-end-of-document.html: Added.
• resources/ui-helper.js:

(window.UIHelper.callFunctionAndWaitForScrollToFinish): Added. Convenience function that invokes the
specified function and returns a Promise that is resolved once the page has finished scrolling. To know
if the page has finished scrolling we listen for DOM scroll events and repeatedly reset a 300ms timer.
The delay of 300ms was chosen to be > 250ms (to give some margin of error), which is the upper bound
delay between scroll event firings, last I recall. When the timer expires we assume that page has
finished scrolling.
(window.UIHelper):

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/scrolling/ios/scroll-to-beginning-and-end-of-document-expected.txt (added)
• LayoutTests/fast/scrolling/ios/scroll-to-beginning-and-end-of-document.html (added)
• LayoutTests/resources/ui-helper.js (modified) (1 diff)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2019-05-14  Daniel Bates  <dabates@apple.com>
+
+        [iOS] Cannot scroll to beginning of document after scrolling to end of document and vice versa via key commands
+        https://bugs.webkit.org/show_bug.cgi?id=197848
+        <rdar://problem/49523065>
+
+        Reviewed by Brent Fulgham.
+
+        Add a test to ensure that key commands can be used to scroll to the end of the page and then
+        to the beginning of the page.
+
+        * fast/scrolling/ios/scroll-to-beginning-and-end-of-document-expected.txt: Added.
+        * fast/scrolling/ios/scroll-to-beginning-and-end-of-document.html: Added.
+        * resources/ui-helper.js:
+        (window.UIHelper.callFunctionAndWaitForScrollToFinish): Added. Convenience function that invokes the
+        specified function and returns a Promise that is resolved once the page has finished scrolling. To know
+        if the page has finished scrolling we listen for DOM scroll events and repeatedly reset a 300ms timer.
+        The delay of 300ms was chosen to be > 250ms (to give some margin of error), which is the upper bound
+        delay between scroll event firings, last I recall. When the timer expires we assume that page has
+        finished scrolling.
+        (window.UIHelper):
+
 2019-05-14  Said Abou-Hallawa  <sabouhallawa@apple.com>
 

trunk/LayoutTests/resources/ui-helper.js

@@ -885 +885 @@
             await this.activateAt(menuRect.left + menuRect.width / 2, menuRect.top + menuRect.height / 2);
     }
+
+    static callFunctionAndWaitForScrollToFinish(functionToCall, ...theArguments)
+    {
+        return new Promise((resolved) => {
+            function scrollDidFinish() {
+                window.removeEventListener("scroll", handleScroll, true);
+                resolved();
+            }
+
+            let lastScrollTimerId = 0; // When the timer with this id fires then the page has finished scrolling.
+            function handleScroll() {
+                if (lastScrollTimerId) {
+                    window.clearTimeout(lastScrollTimerId);
+                    lastScrollTimerId = 0;
+                }
+                lastScrollTimerId = window.setTimeout(scrollDidFinish, 300); // Over 250ms to give some room for error.
+            }
+            window.addEventListener("scroll", handleScroll, true);
+
+            functionToCall.apply(this, theArguments);
+        });
+    }
 }

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-05-14  Daniel Bates  <dabates@apple.com>
+
+        [iOS] Cannot scroll to beginning of document after scrolling to end of document and vice versa via key commands
+        https://bugs.webkit.org/show_bug.cgi?id=197848
+        <rdar://problem/49523065>
+
+        Reviewed by Brent Fulgham.
+
+        Following the fix for <rdar://problem/49523065>, UIKit no longer emits a keyup event for a Command-
+        modified key. This breaks WebKit's own implementation of key command handling for scrolling to the
+        beginning or end of the document (triggered using Command + Arrow Up and Command + Arrow Down,
+        respectively) because it watches for keyup events to reset state after initiating a scroll. If state
+        is not reset then the scroll key command logic becomes confused and may not perform a subsequent scroll.
+        It seems like we can actually get away with supporting these key commands and future Command modified
+        commands by preemptively reseting state on keydown if the Command modifier is held down. If this does
+        not work out then we can do something more complicated.
+
+        * UIProcess/ios/WKKeyboardScrollingAnimator.mm:
+        (-[WKKeyboardScrollingAnimator handleKeyEvent:]):
+
 2019-05-14  Brent Fulgham  <bfulgham@apple.com>
 

trunk/Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm

@@ -347 +347 @@
 
     auto scroll = [self keyboardScrollForEvent:event];
-    if (!scroll || event.type == WebEventKeyUp) {
+
+    // UIKit does not emit a keyup event when the Command key is down. See <rdar://problem/49523065>.
+    // For recognized key commands that include the Command key (e.g. Command + Arrow Up) we reset our
+    // state on keydown.
+    if (!scroll || event.type == WebEventKeyUp || (event.modifierFlags & WebEventFlagMaskCommandKey)) {
         [self stopAnimatedScroll];
         _scrollTriggeringKeyIsPressed = NO;