Attachment 373063 Details for Bug 197950 – To land

[patch] To land

bug-197950-20190627160507.patch (text/plain), 6.50 KB, created by Daniel Bates on 2019-06-27 16:05:08 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Daniel Bates

Created: 2019-06-27 16:05:08 PDT

Size: 6.50 KB

patch

obsolete

>Subversion Revision: 246884
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 7ca0b72822fdec42bedd3e3b6e478166f50f5386..22ecd94f2237cc896f794c325c8811765ee0122c 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,26 @@
>+2019-06-27  Daniel Bates  <dabates@apple.com>
>+
>+        [iOS] Select all with existing range selection replaces range instead of selecting all text
>+        https://bugs.webkit.org/show_bug.cgi?id=197950
>+        <rdar://problem/50245131>
>+
>+        Reviewed by Wenson Hsieh.
>+
>+        Following <rdar://problem/47333786>, UIKit now asks WebKit whether it can handle Command + A as
>+        "select all" instead of just demanding that we handle it. So, WebKit needs to be able to correctly
>+        tell UIKit in advance whether it can handle it. Currenlty WebKit tells UIKit it cannot handle a
>+        "select all" whenever there is an existing range selection. So, UIKit does not tell WebKit to
>+        perform the "select all". Moreover, since UIKit has no other means to handle this key command
>+        itself it tells WebKit the key command was not handled. So, WebKit tells the keyboard to insert
>+        the "a". Instead, WebKit should tell UIKit it can handle a "select all" even when this is an
>+        existing range selection. However we need to keep the current logic just for when UIKit is
>+        asking us with respect to populating the callout menu to not regress platform behavior.
>+
>+        * UIProcess/ios/WKContentViewInteraction.mm:
>+        (-[WKContentView canPerformActionForWebView:withSender:]): Do what we do now if we are called
>+        when populating the callout menu and action is Select All. Otherwise, return YES for the Select All
>+        action if we have a non-empty selection.
>+
> 2019-06-27  Youenn Fablet  <youenn@apple.com>
> 
>         Fix build after revision 246877
>diff --git a/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm b/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
>index 771a901d484d5279122436fff9fff06f3506980b..5d0d263aecc00a3505b058b9d82bb010d60ce8b6 100644
>--- a/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
>+++ b/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
>@@ -3050,9 +3050,10 @@ - (BOOL)canPerformActionForWebView:(SEL)action withSender:(id)sender
>     }
> 
>     if (action == @selector(selectAll:)) {
>-        if (!editorState.selectionIsNone && !editorState.selectionIsRange)
>-            return YES;
>-        return NO;
>+        // By platform convention we don't show Select All in the callout menu for a range selection.
>+        if ([sender isKindOfClass:UIMenuController.class])
>+            return !editorState.selectionIsNone && !editorState.selectionIsRange;
>+        return !editorState.selectionIsNone && self.hasContent;
>     }
> 
>     if (action == @selector(replace:))
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 602a12c7f4ecbdf6b459c0894f5fada2bb95c70d..9fe8680e00971af4be0f669969f9f0b7e67557c4 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2019-06-27  Daniel Bates  <dabates@apple.com>
>+
>+        [iOS] Select all with existing range selection replaces range instead of selecting all text
>+        https://bugs.webkit.org/show_bug.cgi?id=197950
>+        <rdar://problem/50245131>
>+
>+        Reviewed by Wenson Hsieh.
>+
>+        Add a test to ensure that pressing Command + A performs a "select all" even when there
>+        is an existing range selection.
>+
>+        * fast/events/ios/select-all-with-existing-selection-expected.txt: Added.
>+        * fast/events/ios/select-all-with-existing-selection.html: Added.
>+
> 2019-06-27  Antti Koivisto  <antti@apple.com>
> 
>         REGRESSION (touch-action): Can't scroll vertically when touching a horizontally-scrollable element on instagram.com
>diff --git a/LayoutTests/fast/events/ios/select-all-with-existing-selection-expected.txt b/LayoutTests/fast/events/ios/select-all-with-existing-selection-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..d5573f148cff1ed4f05f0176a56806e3536e0c41
>--- /dev/null
>+++ b/LayoutTests/fast/events/ios/select-all-with-existing-selection-expected.txt
>@@ -0,0 +1,11 @@
>+This tests that pressing Command + A selects all the text even when there is an existing range selction. To run this test by hand, press Command + A.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+PASS document.getElementById("test").selectionStart is 0
>+PASS document.getElementById("test").selectionEnd is document.getElementById("test").value.length
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/fast/events/ios/select-all-with-existing-selection.html b/LayoutTests/fast/events/ios/select-all-with-existing-selection.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..2262e8e67cd54e4260037509bd3b46b478664258
>--- /dev/null
>+++ b/LayoutTests/fast/events/ios/select-all-with-existing-selection.html
>@@ -0,0 +1,49 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<script src="../../../resources/js-test.js"></script>
>+<script src="../../../resources/ui-helper.js"></script>
>+</head>
>+<body>
>+<p id="description"></p>
>+<div id="console"></div>
>+<input type="text" id="test" value="Select the last word">
>+<script>
>+window.jsTestIsAsync = true;
>+
>+function testSelectAll()
>+{
>+    function checkResultAndDone() {
>+        shouldBeZero('document.getElementById("test").selectionStart');
>+        shouldBe('document.getElementById("test").selectionEnd', 'document.getElementById("test").value.length');
>+
>+        document.body.removeChild(document.getElementById("test"));
>+
>+        finishJSTest();
>+    }
>+    document.addEventListener("selectionchange", checkResultAndDone, { once: true });
>+    if (window.testRunner)
>+        UIHelper.keyDown("a", ["metaKey"]);
>+}
>+
>+function runTest()
>+{
>+    let testElement = document.getElementById("test");
>+    console.assert(testElement.value.indexOf("word") !== -1);
>+    function handleFocus() {
>+        document.addEventListener("selectionchange", testSelectAll, { once: true });
>+        testElement.setSelectionRange(testElement.value.indexOf("word"), testElement.value.length);
>+    }
>+    testElement.addEventListener("focus", handleFocus, { once: true });
>+
>+    if (window.testRunner)
>+        UIHelper.activateElement(testElement);
>+    else
>+        testElement.focus();
>+}
>+
>+description("This tests that pressing Command + A selects all the text even when there is an existing range selction. To run this test by hand, press Command + A.");
>+runTest();
>+</script>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 197950: 370048 | 373063