Attachment 358184 Details for Bug 193070 – Patch

[patch] Patch

bug-193070-20190102104115.patch (text/plain), 11.10 KB, created by Wenson Hsieh on 2019-01-02 10:41:16 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Wenson Hsieh

Created: 2019-01-02 10:41:16 PST

Size: 11.10 KB

patch

obsolete

>Subversion Revision: 239558
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 6604bac572539b1c3a38ee9a4ab96ca31b7e7cf8..7bb44130737e84853b5750da8e6e3a2653539378 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,49 @@
>+2019-01-01  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        REGRESSION (r239441): [iOS] Selection UI sometimes doesn't change after tapping "select all" in the callout bar
>+        https://bugs.webkit.org/show_bug.cgi?id=193070
>+        <rdar://problem/46921508>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        r239441 added logic to include an EditorState in the next layer tree commit when refocusing an element; this was
>+        done to ensure that after tapping an element that has already been programmatically focused, we still send up-
>+        to-date editor information to the UI process for the purposes of zooming to the selection rect, even if the
>+        selection in the DOM is unchanged, since other aspects of the editor state may have changed since the element
>+        was initially focused.
>+
>+        We currently try to flag the next layer tree commit by setting `m_hasPendingEditorStateUpdate` to `true`.
>+        However, this is problematic since we aren't guaranteed in all cases that a compositing flush has been
>+        scheduled. In the case where it hasn't, we'll end up in a state where the editor state update flag has been set,
>+        yet the update will not make it over to the UI process until something happens that forces a layer tree commit
>+        (e.g. scrolling, pinch zooming). Worse still, if the selection is then programmatically changed from the web
>+        process, we will bail from sending a subsequent editor state update to the UI process because `WebPage` thinks
>+        that a pending editor state has already been scheduled. This manifests in selection UI not updating after
>+        tapping "Select All" in the callout bar, if the callout bar was brought up by tapping near the selection (since
>+        this refocuses the element).
>+
>+        To fix this, we adjust this logic in `WebPage::elementDidRefocus` so that it sets the flag and then schedules a
>+        compositing flush, but only if the user is actually interacting with the focused element (i.e., if the page
>+        calls `focus` repeatedly, we won't continue to schedule compositing flushes).
>+
>+        Test: editing/selection/ios/change-selection-after-tapping-focused-element.html
>+
>+        * WebProcess/WebPage/WebPage.cpp:
>+        (WebKit::WebPage::elementDidRefocus):
>+        (WebKit::WebPage::sendEditorStateUpdate):
>+        (WebKit::WebPage::scheduleFullEditorStateUpdate):
>+
>+        Add a private helper method to schedule an editor state update by setting `m_hasPendingEditorStateUpdate` to
>+        `true` and then scheduling a compositing layer flush. Also, add a FIXME aluding to the fact that scheduling an
>+        entire compositing layer flush to send an editor state update is somewhat wasteful, and should be replaced by
>+        just scheduling this work to be done before the next frame (see: <rdar://problem/36523583> for more detail).
>+
>+        We also use this helper method in a few places where we currently turn on the editor state flag and schedule a
>+        subsequent compositing flush.
>+
>+        (WebKit::WebPage::sendPartialEditorStateAndSchedulePostLayoutUpdate):
>+        * WebProcess/WebPage/WebPage.h:
>+
> 2018-12-29  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         Add support for using the current text selection as the find string on iOS
>diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>index f65026d2192373215885668eb987e06b3e143a73..d1616244b2bf23ed290fa3cf4ad15b155f9fd5f5 100644
>--- a/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>+++ b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>@@ -5281,7 +5281,9 @@ void WebPage::resetFocusedElementForFrame(WebFrame* frame)
> void WebPage::elementDidRefocus(WebCore::Element& element)
> {
>     elementDidFocus(element);
>-    m_hasPendingEditorStateUpdate = true;
>+
>+    if (m_isFocusingElementDueToUserInteraction)
>+        scheduleFullEditorStateUpdate();
> }
> 
> void WebPage::elementDidFocus(WebCore::Element& element)
>@@ -5874,10 +5876,19 @@ void WebPage::sendEditorStateUpdate()
>     auto state = editorState();
>     send(Messages::WebPageProxy::EditorStateChanged(state), pageID());
> 
>-    if (state.isMissingPostLayoutData) {
>-        m_hasPendingEditorStateUpdate = true;
>-        m_drawingArea->scheduleCompositingLayerFlush();
>-    }
>+    if (state.isMissingPostLayoutData)
>+        scheduleFullEditorStateUpdate();
>+}
>+
>+void WebPage::scheduleFullEditorStateUpdate()
>+{
>+    if (m_hasPendingEditorStateUpdate)
>+        return;
>+
>+    m_hasPendingEditorStateUpdate = true;
>+    // FIXME: Scheduling a compositing layer flush here can be more expensive than necessary.
>+    // Instead, we should just compute and send post-layout editor state during the next frame.
>+    m_drawingArea->scheduleCompositingLayerFlush();
> }
> 
> #if PLATFORM(COCOA)
>@@ -5909,13 +5920,7 @@ void WebPage::sendPartialEditorStateAndSchedulePostLayoutUpdate()
>         return;
> 
>     send(Messages::WebPageProxy::EditorStateChanged(editorState(IncludePostLayoutDataHint::No)), pageID());
>-
>-    if (m_hasPendingEditorStateUpdate)
>-        return;
>-
>-    // Flag the next layer flush to compute and propagate an EditorState to the UI process.
>-    m_hasPendingEditorStateUpdate = true;
>-    m_drawingArea->scheduleCompositingLayerFlush();
>+    scheduleFullEditorStateUpdate();
> }
> 
> void WebPage::flushPendingEditorStateUpdate()
>diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.h b/Source/WebKit/WebProcess/WebPage/WebPage.h
>index 34ddff5150b0a6212ba2b6087f5d796bc57065e5..075d75c1e66044acd00fa088498c78cb4b1190e7 100644
>--- a/Source/WebKit/WebProcess/WebPage/WebPage.h
>+++ b/Source/WebKit/WebProcess/WebPage/WebPage.h
>@@ -1149,6 +1149,7 @@ private:
>     void platformDetach();
>     void platformEditorState(WebCore::Frame&, EditorState& result, IncludePostLayoutDataHint) const;
>     void sendEditorStateUpdate();
>+    void scheduleFullEditorStateUpdate();
> 
> #if PLATFORM(COCOA)
>     void sendTouchBarMenuDataAddedUpdate(WebCore::HTMLMenuElement&);
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 76c38d92500820641145ee49c7c8d94fec3584c1..c49353743ef462862ee122e053b2818c83576d9e 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2019-01-01  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        REGRESSION (r239441): [iOS] Selection UI sometimes doesn't change after tapping "select all" in the callout bar
>+        https://bugs.webkit.org/show_bug.cgi?id=193070
>+        <rdar://problem/46921508>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add a test to ensure that selection UI is shown after tapping on a focused element and then changing the
>+        selection programmatically.
>+
>+        * editing/selection/ios/change-selection-after-tapping-focused-element-expected.txt: Added.
>+        * editing/selection/ios/change-selection-after-tapping-focused-element.html: Added.
>+
> 2018-12-23  Carlos Garcia Campos  <cgarcia@igalia.com>
> 
>         Unreviewed GTK+ gardening. Rebaseline fast/text/zero-font-size.html after r239539.
>diff --git a/LayoutTests/editing/selection/ios/change-selection-after-tapping-focused-element-expected.txt b/LayoutTests/editing/selection/ios/change-selection-after-tapping-focused-element-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..28ed18544781e607bd8ba58dbcd2b8620b320d68
>--- /dev/null
>+++ b/LayoutTests/editing/selection/ios/change-selection-after-tapping-focused-element-expected.txt
>@@ -0,0 +1,14 @@
>+WebKit
>+Verifies that after tapping a focused element to bring up the keyboard, changing the selection via script causes the native selection UI to be shown. To manually test, tap the red box above and tap select all via the callout bar; the entire text in the editable element should be selected.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+PASS selectionRects.length is 1
>+PASS selectionRects[0].left is 2
>+PASS selectionRects[0].top is 2
>+PASS selectionRects[0].width is 309
>+PASS selectionRects[0].height is 114
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/editing/selection/ios/change-selection-after-tapping-focused-element.html b/LayoutTests/editing/selection/ios/change-selection-after-tapping-focused-element.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..9f9bee52c5b50e38c79f917358b3decf78b51412
>--- /dev/null
>+++ b/LayoutTests/editing/selection/ios/change-selection-after-tapping-focused-element.html
>@@ -0,0 +1,73 @@
>+<!DOCTYPE html> 
>+<html>
>+<meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no">
>+<head>
>+    <script src="../../../resources/ui-helper.js"></script>
>+    <script src="../../../resources/js-test.js"></script>
>+    <style>
>+    body {
>+        margin: 0;
>+    }
>+
>+    #editor {
>+        width: 100%;
>+        height: 280px;
>+        border: 2px solid tomato;
>+        outline: none;
>+        box-sizing: border-box;
>+        font-size: 100px;
>+        margin: 0;
>+    }
>+    </style>
>+    <script>
>+    jsTestIsAsync = true;
>+
>+    function doAfterDetectingVisibleSelectionViewRects(callback)
>+    {
>+        const uiScript = `
>+            function waitForSelectionRects() {
>+                uiController.doAsyncTask(() => {
>+                    const rects = uiController.selectionRangeViewRects;
>+                    if (rects && rects.length)
>+                        uiController.uiScriptComplete(JSON.stringify(rects));
>+                    else
>+                        waitForSelectionRects();
>+                });
>+            }
>+            uiController.doAsyncTask(waitForSelectionRects);`;
>+        testRunner.runUIScript(uiScript, result => callback(JSON.parse(result)));
>+    }
>+
>+    async function run()
>+    {
>+        description("Verifies that after tapping a focused element to bring up the keyboard, changing the selection via "
>+            + "script causes the native selection UI to be shown. To manually test, tap the red box above and tap select "
>+            + "all via the callout bar; the entire text in the editable element should be selected.");
>+
>+        if (!window.testRunner)
>+            return;
>+
>+        await UIHelper.activateAndWaitForInputSessionAt(130, 100);
>+        await new Promise(resolve => setTimeout(resolve, 500));
>+
>+        doAfterDetectingVisibleSelectionViewRects(selectionRects => {
>+            window.selectionRects = selectionRects;
>+            shouldBe("selectionRects.length", "1");
>+            shouldBe("selectionRects[0].left", "2");
>+            shouldBe("selectionRects[0].top", "2");
>+            shouldBe("selectionRects[0].width", "309");
>+            shouldBe("selectionRects[0].height", "114");
>+            finishJSTest();
>+        });
>+
>+        await UIHelper.tapAt(130, 100);
>+        getSelection().selectAllChildren(editor);
>+    }
>+    </script>
>+</head>
>+<body onload=run()>
>+    <div id="editor" contenteditable>WebKit</div>
>+    <div id="description"></div>
>+    <div id="console"></div>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 193070: 358184