[iOS] REGRESSION (r239441): Tab cycling to offscreen <select> may not scroll it into...
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 3 Jan 2019 15:38:33 +0000 (15:38 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 3 Jan 2019 15:38:33 +0000 (15:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=193084
<rdar://problem/47006882>

Reviewed by Simon Fraser.

Source/WebKit:

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):

LayoutTests:

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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@239590 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/forms/ios/scroll-to-reveal-focused-select-expected.txt [new file with mode: 0644]
LayoutTests/fast/forms/ios/scroll-to-reveal-focused-select.html [new file with mode: 0644]
Source/WebKit/ChangeLog
Source/WebKit/Shared/FocusedElementInformation.cpp
Source/WebKit/Shared/FocusedElementInformation.h
Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm

index 6304aa0..6e5fbf3 100644 (file)
@@ -1,3 +1,17 @@
+2019-01-03  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 Simon Fraser.
+
+        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  Devin Rousso  <webkit@devinrousso.com>
 
         Web Inspector: Implement `queryObjects` Command Line API
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 (file)
index 0000000..9d2671b
--- /dev/null
@@ -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 (file)
index 0000000..1847580
--- /dev/null
@@ -0,0 +1,79 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+
+<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>
index a9590e8..83f5006 100644 (file)
@@ -1,5 +1,58 @@
 2019-01-03  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 Simon Fraser.
+
+        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-03  Wenson Hsieh  <wenson_hsieh@apple.com>
+
         WebUndoStep's monotonically increasing identifier should be a WebUndoStepID instead of uint64_t
         https://bugs.webkit.org/show_bug.cgi?id=193100
 
index 380bb49..9ee815f 100644 (file)
@@ -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))
index 1a5b469..0530703 100644 (file)
@@ -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 };
index cdfe5db..f5162b0 100644 (file)
@@ -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;
 
index b0f2d18..ebdc370 100644 (file)
@@ -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();