Attachment 370917 Details for Bug 198356 – Patch

[patch] Patch

bug-198356-20190529203743.patch (text/plain), 9.97 KB, created by Wenson Hsieh on 2019-05-29 20:37:44 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Wenson Hsieh

Created: 2019-05-29 20:37:44 PDT

Size: 9.97 KB

patch

obsolete

>Subversion Revision: 245847
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index f7a0e5cceea299b59e81f70bd4484aca184e95cb..db953c25645484eba773bcb8ff889e7ba2fbea0d 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,45 @@
>+2019-05-29  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        Missing caret when focusing an editable field if the selection was set when WKWebView wasn't first responder
>+        https://bugs.webkit.org/show_bug.cgi?id=198356
>+        <rdar://problem/50798593>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        In this bug, the DOM selection is initially set by script in a web view that is not the first responder. Then,
>+        either the user begins editing by tapping somewhere such that the selection does not change, or an editable
>+        element is programmatically focused and the client allows programmatic focus to show the keyboard. This is
>+        because the selection clipping rect used by the UI process when computing the bounds of the caret view is empty,
>+        causing the entire caret to be clipped.
>+
>+        This is due to two related issues: first, no updated editor state is sent to the UI process after the element is
>+        focused, if the selection has not also changed. This means that while the selection geometry is sent over to the
>+        UI process, the selection clipping rect (a member of the EditorState's PostLayoutData called
>+        "focusedElementRect") becomes stale in the UI process, since the there was no focused element when the
>+        previously computed editor state was sent to the UI process. To fix this, we schedule a full editor state update
>+        when an element is focused, to ensure that the selection is eventually updated in the UI process.
>+
>+        Secondly, even once the editor state update is sent to the UI process, we will actually avoid updating any text
>+        selection views, since there is no change in WKSelectionDrawingInfo, which currently consists of a selection
>+        type, and either a caret rect or a list of selection rects. However, since selection drawing is also affected by
>+        the selection clipping rect, it seems reasonable to add the selection clipping rect to the drawing info, and
>+        trigger a selection update if this selection clipping rect has changed.
>+
>+        * UIProcess/ios/WKContentViewInteraction.h:
>+        * UIProcess/ios/WKContentViewInteraction.mm:
>+        (WebKit::WKSelectionDrawingInfo::WKSelectionDrawingInfo):
>+
>+        Add selectionClippingRect to WKSelectionDrawingInfo, and check against it when comparing two drawing infos.
>+
>+        (WebKit::operator==):
>+        (WebKit::operator<<):
>+        * WebProcess/WebPage/WebPage.cpp:
>+        (WebKit::WebPage::elementDidFocus):
>+
>+        Schedule an editor state update when focusing an element. In many cases, an editor state update has already been
>+        scheduled when focusing an element, so this becomes a no-op; however, in this scenario, it delivers updated
>+        selection clipping rects (i.e. the focused element rect) and other updated information to the UI process.
>+
> 2019-05-29  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         Remove some logic to suppress the text selection assistant during drop
>diff --git a/Source/WebKit/UIProcess/ios/WKContentViewInteraction.h b/Source/WebKit/UIProcess/ios/WKContentViewInteraction.h
>index 31be4b5d733fa855c6bf68539c4bec9c7c8ff023..7917db6da7ef25e84997fda5876daeff2a12ab5e 100644
>--- a/Source/WebKit/UIProcess/ios/WKContentViewInteraction.h
>+++ b/Source/WebKit/UIProcess/ios/WKContentViewInteraction.h
>@@ -175,6 +175,7 @@ struct WKSelectionDrawingInfo {
>     SelectionType type;
>     WebCore::IntRect caretRect;
>     Vector<WebCore::SelectionRect> selectionRects;
>+    WebCore::IntRect selectionClipRect;
> };
> 
> WTF::TextStream& operator<<(WTF::TextStream&, const WKSelectionDrawingInfo&);
>diff --git a/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm b/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
>index 0d2d62135f1dea74ed4e9c1d8464b3825302f07c..d7840b9f691c2710ae0f3b6fa9a29bdbeca20bf9 100644
>--- a/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
>+++ b/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
>@@ -195,6 +195,7 @@ WKSelectionDrawingInfo::WKSelectionDrawingInfo(const EditorState& editorState)
>     auto& postLayoutData = editorState.postLayoutData();
>     caretRect = postLayoutData.caretRectAtEnd;
>     selectionRects = postLayoutData.selectionRects;
>+    selectionClipRect = postLayoutData.focusedElementRect;
> }
> 
> inline bool operator==(const WKSelectionDrawingInfo& a, const WKSelectionDrawingInfo& b)
>@@ -215,6 +216,9 @@ inline bool operator==(const WKSelectionDrawingInfo& a, const WKSelectionDrawing
>         }
>     }
> 
>+    if (a.type != WKSelectionDrawingInfo::SelectionType::None && a.selectionClipRect != b.selectionClipRect)
>+        return false;
>+
>     return true;
> }
> 
>@@ -240,6 +244,7 @@ TextStream& operator<<(TextStream& stream, const WKSelectionDrawingInfo& info)
>     stream.dumpProperty("type", info.type);
>     stream.dumpProperty("caret rect", info.caretRect);
>     stream.dumpProperty("selection rects", info.selectionRects);
>+    stream.dumpProperty("selection clip rect", info.selectionClipRect);
>     return stream;
> }
> 
>diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>index 69b5fce4ebd5f53a9daefc7d75f47916eb69201a..436ff63468ecc9fc18502b32499c692c82d780a4 100644
>--- a/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>+++ b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>@@ -5467,6 +5467,8 @@ void WebPage::elementDidFocus(WebCore::Element& element)
>         send(Messages::WebPageProxy::SetEditableElementIsFocused(!element.hasTagName(WebCore::HTMLNames::selectTag)));
> #endif
>         m_recentlyBlurredElement = nullptr;
>+
>+        scheduleFullEditorStateUpdate();
>     }
> }
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index e24fbeaf0957d7e56acc8aa3004e4f22bdadcc4e..1e2691a2b741309c61056b8a30bdb1d95df64505 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2019-05-29  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        Missing caret when focusing an editable field if the selection was set when WKWebView wasn't first responder
>+        https://bugs.webkit.org/show_bug.cgi?id=198356
>+        <rdar://problem/50798593>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add a new layout test to exercise this scenario.
>+
>+        * editing/selection/ios/caret-when-focusing-editable-element-with-selection-expected.txt: Added.
>+        * editing/selection/ios/caret-when-focusing-editable-element-with-selection.html: Added.
>+
> 2019-05-28  Saam Barati  <sbarati@apple.com>
> 
>         [WHLSL] Type of dereference is the type of the thing we point to, not a pointer to that type
>diff --git a/LayoutTests/editing/selection/ios/caret-when-focusing-editable-element-with-selection-expected.txt b/LayoutTests/editing/selection/ios/caret-when-focusing-editable-element-with-selection-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..3a9ab5c9d86fd1516e55332732d3ee885abaaf9d
>--- /dev/null
>+++ b/LayoutTests/editing/selection/ios/caret-when-focusing-editable-element-with-selection-expected.txt
>@@ -0,0 +1,13 @@
>+This test verifies that when setting the selection inside an editable element while the web view is not first responder, the caret should appear after tapping the editable element to show the keyboard. This test requires WebKitTestRunner.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+PASS caretRect.left is 9
>+PASS caretRect.top is 21
>+PASS caretRect.width is 2
>+PASS caretRect.height is 24
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/editing/selection/ios/caret-when-focusing-editable-element-with-selection.html b/LayoutTests/editing/selection/ios/caret-when-focusing-editable-element-with-selection.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..c503f3e91ea6c32a83b38a0d068f86933c9d7583
>--- /dev/null
>+++ b/LayoutTests/editing/selection/ios/caret-when-focusing-editable-element-with-selection.html
>@@ -0,0 +1,55 @@
>+<!DOCTYPE html> 
>+<html>
>+    <head>
>+        <meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no">
>+        <script src="../../../resources/ui-helper.js"></script>
>+        <script src="../../../resources/js-test.js"></script>
>+        <style>
>+            html, body {
>+                width: 100%;
>+                height: 100%;
>+                font-size: 20px;
>+            }
>+
>+            #editor {
>+                width: 100%;
>+                height: 100px;
>+                border: 1px solid tomato;
>+            }
>+        </style>
>+    </head>
>+    <body>
>+        <p contenteditable id="editor"></p>
>+        <p id="description"></p>
>+        <p id="console"></p>
>+    </body>
>+    <script>
>+        jsTestIsAsync = true;
>+
>+        addEventListener("load", runTest);
>+
>+        async function runTest()
>+        {
>+            description("This test verifies that when setting the selection inside an editable element while the web view is not first responder, the caret should appear after tapping the editable element to show the keyboard. This test requires WebKitTestRunner.");
>+
>+            const editor = document.getElementById("editor");
>+            await UIHelper.setHardwareKeyboardAttached(false);
>+            await UIHelper.resignFirstResponder();
>+            getSelection().setPosition(editor);
>+            await UIHelper.activateElement(editor);
>+            do {
>+                caretRect = await UIHelper.getUICaretViewRect();
>+            } while (!caretRect.width || !caretRect.height);
>+
>+            shouldBe("caretRect.left", "9");
>+            shouldBe("caretRect.top", "21");
>+            shouldBe("caretRect.width", "2");
>+            shouldBe("caretRect.height", "24");
>+
>+            editor.blur();
>+            await UIHelper.waitForKeyboardToHide();
>+
>+            finishJSTest();
>+        }
>+    </script>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 198356: 370917