Attachment 369760 Details for Bug 197848 – Patch and layout test

[patch] Patch and layout test

bug-197848-20190513123002.patch (text/plain), 8.30 KB, created by Daniel Bates on 2019-05-13 12:30:03 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Daniel Bates

Created: 2019-05-13 12:30:03 PDT

Size: 8.30 KB

patch

obsolete

>Subversion Revision: 245179
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index a39e57372e1fe299418dc6295eb18840c6e15541..ae0e2beb490d87ee6691ba2eb3872c9ac8c7983d 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,23 @@
>+2019-05-13  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 NOBODY (OOPS!).
>+
>+        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-10  Michael Catanzaro  <mcatanzaro@igalia.com>
> 
>         Fix a bunch of compiler warnings
>diff --git a/Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm b/Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm
>index bca982e0e542bc13996a4da89a02168f57ffec78..172810aec105ce944ad767d06b26289510c46961 100644
>--- a/Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm
>+++ b/Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm
>@@ -346,7 +346,11 @@ - (void)handleKeyEvent:(::WebEvent *)event
>         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;
>     }
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 251358afd8bd7c527b6df0fb657fd32f33f49070..782386f1fec04357beab7931d18ddf8e626f3440 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,25 @@
>+2019-05-13  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 NOBODY (OOPS!).
>+
>+        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):
>+
> 2018-12-05  Daniel Bates  <dabates@apple.com>
> 
>         [iOS] Add test to ensure that a web page can prevent the default for Command + A
>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
>index 0000000000000000000000000000000000000000..b1de408a200f1b6135404efd54c6dbc8a2dc3f80
>--- /dev/null
>+++ b/LayoutTests/fast/scrolling/ios/scroll-to-beginning-and-end-of-document-expected.txt
>@@ -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
>index 0000000000000000000000000000000000000000..88737f9e0404962fd9c551732bbed729ed85c245
>--- /dev/null
>+++ b/LayoutTests/fast/scrolling/ios/scroll-to-beginning-and-end-of-document.html
>@@ -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>
>diff --git a/LayoutTests/resources/ui-helper.js b/LayoutTests/resources/ui-helper.js
>index eea193e38ec8e7ed853a09368938c2d4b13ee360..b52276735964fb9df108b0569c782b0f51dbc2dc 100644
>--- a/LayoutTests/resources/ui-helper.js
>+++ b/LayoutTests/resources/ui-helper.js
>@@ -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);
>+        });
>+    }
> }

Actions: View | Formatted Diff | Diff

Attachments on bug 197848: 369760