Attachment 358231 Details for Bug 193084 – Patch

[patch] Patch

bug-193084-20190102173706.patch (text/plain), 12.65 KB, created by Wenson Hsieh on 2019-01-02 17:37:07 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Wenson Hsieh

Created: 2019-01-02 17:37:07 PST

Size: 12.65 KB

patch

obsolete

>Subversion Revision: 239574
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 3207b95dd997da17c35baaddc73c1185ffd98b4f..153c41934c1ae0f360c91005c221df0a6b29c098 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,56 @@
>+2019-01-02  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        [iOS] REGRESSION (r239441): Tab cycling to offscreen <select> may not scroll it into view
>+        https://bugs.webkit.org/show_bug.cgi?id=193084
>+        <rdar://problem/47006882>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        In `WKWebView.mm`, `-_zoomToFocusRect:` will ignore the given selection rect if it is of size `{ 0, 0 }` and at
>+        the origin. Prior to r239441, when using the tab key to move focus between non-editable form controls (or any
>+        other method that doesn't involve tapping on the focused select element, with the exception of the next and
>+        previous buttons in the input accessory view), we would compute a selection rect of `{{ 0, 0 }, { 0, 0 }}`, and
>+        subsequently try to scroll the focused element to the center of the visible area, without taking the selection
>+        rect into account.
>+
>+        However, after r239441, the web process sends the element interaction location to the UI process, which then
>+        computes the selection rect by taking this location and adding a size of `{ 1, 1 }` (before r239441, this was
>+        done in `WebPage::getAssistedNodeInformation`). However, our new implementation doesn't take into account the
>+        case where the element interaction rect is null, which happens when the last interaction location is outside of
>+        the bounding rect of the element. In this case, we set the element interaction location to { 0, 0 } and end up
>+        computing a selection rect of `{{ 0, 0 }, { 1, 1 }}` instead of `{{ 0, 0 }, { 0, 0 }}` as we would have
>+        previously done. This causes us to scroll up to the origin, instead of revealing the focused element.
>+
>+        To fix this, we restore the pre-r239441 behavior. See additional comments below for details.
>+
>+        Test: fast/forms/ios/scroll-to-reveal-focused-select.html
>+
>+        * Shared/FocusedElementInformation.cpp:
>+        (WebKit::FocusedElementInformation::encode const):
>+        (WebKit::FocusedElementInformation::decode):
>+        * Shared/FocusedElementInformation.h:
>+
>+        Rename `elementInteractionLocation` to `lastInteractionLocation`. This was previously
>+        `elementInteractionLocation` due to existing logic that tries to move the interaction location into the bounding
>+        rect of the element in the case where visual viewports are disabled; however, since this feature has long been
>+        enabled by default for all modern WebKit clients (internal and external), it's simpler to just use always send
>+        the last interaction location over to the UI process, and have the UI process use `{{ 0, 0 }, { 0, 0 }}` if
>+        the interaction location is outside of the element rect.
>+
>+        In the very unlikely event that any modern WebKit client disables visual viewports, this will still behave
>+        reasonably, since we'll just use `{{ 0, 0 }, { 0, 0 }}` as the target rect and scroll to reveal the entire
>+        element rather than the top left corner of the element.
>+
>+        * UIProcess/ios/WKContentViewInteraction.mm:
>+        (rectToRevealWhenZoomingToFocusedElement):
>+        * WebProcess/WebPage/ios/WebPageIOS.mm:
>+
>+        Move the check for whether the interaction location is inside the element's bounding rect from the web process
>+        to the UI process. This relocates the logic to determine whether the selection rect should be a 1 by 1 fallback
>+        interaction rect or the zero rect (`{{ 0, 0 }, { 0, 0 }}`) closer to the code that actually uses this rect.
>+
>+        (WebKit::WebPage::getFocusedElementInformation):
>+
> 2019-01-02  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         REGRESSION (r239441): [iOS] Selection UI sometimes doesn't change after tapping "select all" in the callout bar
>diff --git a/Source/WebKit/Shared/FocusedElementInformation.cpp b/Source/WebKit/Shared/FocusedElementInformation.cpp
>index 380bb4918da8f44f25272efc01b9946c33b08ccb..9ee815f42d64b0688969b38341ac44470585c122 100644
>--- a/Source/WebKit/Shared/FocusedElementInformation.cpp
>+++ b/Source/WebKit/Shared/FocusedElementInformation.cpp
>@@ -64,7 +64,7 @@ Optional<OptionItem> OptionItem::decode(IPC::Decoder& decoder)
> void FocusedElementInformation::encode(IPC::Encoder& encoder) const
> {
>     encoder << elementRect;
>-    encoder << elementInteractionLocation;
>+    encoder << lastInteractionLocation;
>     encoder << minimumScaleFactor;
>     encoder << maximumScaleFactor;
>     encoder << maximumScaleFactorIgnoringAlwaysScalable;
>@@ -112,7 +112,7 @@ bool FocusedElementInformation::decode(IPC::Decoder& decoder, FocusedElementInfo
>     if (!decoder.decode(result.elementRect))
>         return false;
> 
>-    if (!decoder.decode(result.elementInteractionLocation))
>+    if (!decoder.decode(result.lastInteractionLocation))
>         return false;
> 
>     if (!decoder.decode(result.minimumScaleFactor))
>diff --git a/Source/WebKit/Shared/FocusedElementInformation.h b/Source/WebKit/Shared/FocusedElementInformation.h
>index 1a5b469e1ce187b45d4d361785a787d2f1b4d8c6..053070361e59c303d83bb6670d18ac43dcc16c5d 100644
>--- a/Source/WebKit/Shared/FocusedElementInformation.h
>+++ b/Source/WebKit/Shared/FocusedElementInformation.h
>@@ -97,7 +97,7 @@ using FocusedElementIdentifier = uint64_t;
> 
> struct FocusedElementInformation {
>     WebCore::IntRect elementRect;
>-    WebCore::IntPoint elementInteractionLocation;
>+    WebCore::IntPoint lastInteractionLocation;
>     double minimumScaleFactor { -INFINITY };
>     double maximumScaleFactor { INFINITY };
>     double maximumScaleFactorIgnoringAlwaysScalable { INFINITY };
>diff --git a/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm b/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
>index cdfe5dba2e3877a1183f69b8f4c100aeda7ff931..f5162b00591d497775b6779f8b0fc577ac9ec9c7 100644
>--- a/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
>+++ b/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
>@@ -4369,7 +4369,10 @@ static bool shouldZoomToRevealSelectionRect(WebKit::InputType type)
> 
> static WebCore::FloatRect rectToRevealWhenZoomingToFocusedElement(const WebKit::FocusedElementInformation& elementInfo, const WebKit::EditorState& editorState)
> {
>-    WebCore::IntRect elementInteractionRect(elementInfo.elementInteractionLocation, { 1, 1 });
>+    WebCore::IntRect elementInteractionRect;
>+    if (elementInfo.elementRect.contains(elementInfo.lastInteractionLocation))
>+        elementInteractionRect = { elementInfo.lastInteractionLocation, { 1, 1 } };
>+
>     if (!shouldZoomToRevealSelectionRect(elementInfo.elementType))
>         return elementInteractionRect;
> 
>diff --git a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
>index 90658056389737ef9caa5ffd61e8edaadf79cce9..bb1acad669724fcb6f772e771e745aeb4864f3d0 100644
>--- a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
>+++ b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
>@@ -2362,7 +2362,7 @@ void WebPage::getFocusedElementInformation(FocusedElementInformation& informatio
> {
>     layoutIfNeeded();
> 
>-    information.elementInteractionLocation = m_lastInteractionLocation;
>+    information.lastInteractionLocation = m_lastInteractionLocation;
> 
>     if (auto* renderer = m_focusedElement->renderer()) {
>         auto& elementFrame = m_page->focusController().focusedOrMainFrame();
>@@ -2381,13 +2381,6 @@ void WebPage::getFocusedElementInformation(FocusedElementInformation& informatio
>             frameView->setCustomFixedPositionLayoutRect(frameView->renderView()->documentRect());
>             information.elementRect = frameView->contentsToRootView(renderer->absoluteBoundingBoxRect());
>             frameView->setCustomFixedPositionLayoutRect(currentFixedPositionRect);
>-            
>-            if (!information.elementRect.contains(m_lastInteractionLocation))
>-                information.elementInteractionLocation = information.elementRect.location();
>-        } else {
>-            // Don't use the selection rect if interaction was outside the element rect.
>-            if (!information.elementRect.contains(m_lastInteractionLocation))
>-                information.elementInteractionLocation = { };
>         }
>     } else
>         information.elementRect = IntRect();
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 1f36ebf8576159f2b56076d5180d5cee62bd592c..3c7db20075d4711a88f2c35dbb4e0688d92531c4 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2019-01-02  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        [iOS] REGRESSION (r239441): Tab cycling to offscreen <select> may not scroll it into view
>+        https://bugs.webkit.org/show_bug.cgi?id=193084
>+        <rdar://problem/47006882>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add a layout test to verify that focusing a select element by tapping outside of it scrolls to reveal the
>+        focused select element.
>+
>+        * fast/forms/ios/scroll-to-reveal-focused-select-expected.txt: Added.
>+        * fast/forms/ios/scroll-to-reveal-focused-select.html: Added.
>+
> 2019-01-02  Simon Fraser  <simon.fraser@apple.com>
> 
>         Support css-color-4 rgb functions
>diff --git a/LayoutTests/fast/forms/ios/scroll-to-reveal-focused-select-expected.txt b/LayoutTests/fast/forms/ios/scroll-to-reveal-focused-select-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..9d2671b64797c406b7325c02d0a74995eacd5208
>--- /dev/null
>+++ b/LayoutTests/fast/forms/ios/scroll-to-reveal-focused-select-expected.txt
>@@ -0,0 +1,11 @@
>+
>+Verifies that focusing a select element after user interaction scrolls to reveal the select element. To manually test, focus the top select element and tap the red box to focus the bottom select element. The bottom select element should be scrolled into view.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+PASS document.activeElement is document.getElementById('bottom')
>+PASS pageYOffset is >= 1000
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/fast/forms/ios/scroll-to-reveal-focused-select.html b/LayoutTests/fast/forms/ios/scroll-to-reveal-focused-select.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..1847580b10271dea83234e99ea086baad8bce8b6
>--- /dev/null
>+++ b/LayoutTests/fast/forms/ios/scroll-to-reveal-focused-select.html
>@@ -0,0 +1,79 @@
>+<!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>
>+select {
>+    width: 100%;
>+    height: 80px;
>+}
>+
>+#bottom {
>+    margin-top: 1300px;
>+}
>+
>+#tapToFocus {
>+    width: 100%;
>+    height: 80px;
>+    background: tomato;
>+    border-radius: 20px;
>+    color: white;
>+    font-size: 2em;
>+    text-align: center;
>+    cursor: pointer;
>+}
>+</style>
>+<script>
>+jsTestIsAsync = true;
>+
>+async function tapAtLocationAndWaitForScrollingToEnd(x, y)
>+{
>+    const uiScript = `
>+    let doneCount = 0;
>+    function checkDone() {
>+        if (++doneCount == 2)
>+            uiController.uiScriptComplete();
>+    }
>+    uiController.didEndZoomingCallback = checkDone;
>+    uiController.singleTapAtPoint(${x}, ${y}, checkDone);`;
>+    return new Promise(resolve => testRunner.runUIScript(uiScript, resolve));
>+}
>+
>+async function run()
>+{
>+    description("Verifies that focusing a select element after user interaction scrolls to reveal the select element. "
>+        + "To manually test, focus the top select element and tap the red box to focus the bottom select element. The "
>+        + "bottom select element should be scrolled into view.");
>+
>+    if (!window.testRunner)
>+        return;
>+
>+    await UIHelper.activateAndWaitForInputSessionAt(160, 40);
>+    await tapAtLocationAndWaitForScrollingToEnd(160, 120);
>+
>+    shouldBe("document.activeElement", "document.getElementById('bottom')");
>+    shouldBeGreaterThanOrEqual("pageYOffset", "1000");
>+
>+    document.activeElement.blur();
>+    await UIHelper.waitForKeyboardToHide();
>+    tapToFocus.remove();
>+    finishJSTest();
>+}
>+</script>
>+</head>
>+<body onload=run()>
>+    <div>
>+        <select id="top"><option selected>First</option><option>Second</option></select>
>+    </div>
>+    <div id="tapToFocus" onclick="bottom.focus()">Tap to focus the<br>bottom select</div>
>+    <div id="description"></div>
>+    <div id="console"></div>
>+    <select id="bottom">
>+        <option selected>Third</option>
>+        <option>Fourth</option>
>+    </select>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 193084: 358192 | 358231