[iOS] Cannot scroll to beginning of document after scrolling to end of document and...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 14 May 2019 16:43:51 +0000 (16:43 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 14 May 2019 16:43:51 +0000 (16:43 +0000)
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):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@245285 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/scrolling/ios/scroll-to-beginning-and-end-of-document-expected.txt [new file with mode: 0644]
LayoutTests/fast/scrolling/ios/scroll-to-beginning-and-end-of-document.html [new file with mode: 0644]
LayoutTests/resources/ui-helper.js
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm

index 642d5e9..ec8613e 100644 (file)
@@ -1,3 +1,25 @@
+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>
 
         [CG] Adding support for HEIF-sequence ('public.heics') images
diff --git a/LayoutTests/fast/scrolling/ios/scroll-to-beginning-and-end-of-document-expected.txt b/LayoutTests/fast/scrolling/ios/scroll-to-beginning-and-end-of-document-expected.txt
new file mode 100644 (file)
index 0000000..b1de408
--- /dev/null
@@ -0,0 +1,12 @@
+Test key commands to scroll to the end of the document and then to the beginning of the document. To run this test by hand, reload the page then press Command + Down Arrow and then press Command + Up Arrow.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS window.scrollY is INITIAL_Y_OFFSET
+PASS window.scrollY is >= INITIAL_Y_OFFSET + 1
+PASS window.scrollY is 0
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/scrolling/ios/scroll-to-beginning-and-end-of-document.html b/LayoutTests/fast/scrolling/ios/scroll-to-beginning-and-end-of-document.html
new file mode 100644 (file)
index 0000000..88737f9
--- /dev/null
@@ -0,0 +1,51 @@
+<!DOCTYPE html>
+<html>
+<head>
+<meta name="viewport" content="width=device-width">
+<script src="../../../resources/js-test.js"></script>
+<script src="../../../resources/ui-helper.js"></script>
+</head>
+<body>
+<p id="description"></p>
+<div id="console"></div>
+<div id="test" style="height: 4096px; width: 256px; background-color: blue"></div>
+<script>
+window.jsTestIsAsync = true;
+
+// This is used to detect if scrolling is completely broken.
+const INITIAL_Y_OFFSET = 256;
+
+function done()
+{
+    document.body.removeChild(document.getElementById("test"));
+    finishJSTest();
+}
+
+async function runTest()
+{
+    await UIHelper.callFunctionAndWaitForScrollToFinish(() => window.scrollTo(0, INITIAL_Y_OFFSET));
+    shouldBe("window.scrollY", "INITIAL_Y_OFFSET");
+
+    // Scroll to the end of the document.
+    await UIHelper.callFunctionAndWaitForScrollToFinish(() => {
+        if (window.testRunner)
+            UIHelper.keyDown("downArrow", ["metaKey"]);
+    });
+    shouldBeGreaterThanOrEqual("window.scrollY", "INITIAL_Y_OFFSET + 1");
+
+
+    // Scroll to the beginning of the document.
+    await UIHelper.callFunctionAndWaitForScrollToFinish(() => {
+        if (window.testRunner)
+            UIHelper.keyDown("upArrow", ["metaKey"]);
+    });
+    shouldBeZero("window.scrollY");
+
+    done();
+}
+
+description("Test key commands to scroll to the end of the document and then to the beginning of the document. To run this test by hand, reload the page then press <kbd>Command</kbd> + <kbd>Down Arrow</kbd> and then press <kbd>Command</kbd> + <kbd>Up Arrow</kbd>.");
+runTest();
+</script>
+</body>
+</html>
index eea193e..b522767 100644 (file)
@@ -884,4 +884,26 @@ window.UIHelper = class UIHelper {
         if (menuRect)
             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);
+        });
+    }
 }
index 5ba533f..65924d0 100644 (file)
@@ -1,3 +1,23 @@
+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>
 
         Protect current WebFrame during form submission
index bca982e..172810a 100644 (file)
@@ -346,7 +346,11 @@ static WebCore::PhysicalBoxSide boxSide(WebKit::ScrollingDirection direction)
         return;
 
     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;
     }