- Revision
- 239590
- Author
- [email protected]
- Date
- 2019-01-03 07:38:33 -0800 (Thu, 03 Jan 2019)
Log Message
[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.
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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (239589 => 239590)
--- trunk/LayoutTests/ChangeLog 2019-01-03 15:34:34 UTC (rev 239589)
+++ trunk/LayoutTests/ChangeLog 2019-01-03 15:38:33 UTC (rev 239590)
@@ -1,3 +1,17 @@
+2019-01-03 Wenson Hsieh <[email protected]>
+
+ [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 <[email protected]>
Web Inspector: Implement `queryObjects` Command Line API
Added: trunk/LayoutTests/fast/forms/ios/scroll-to-reveal-focused-select-expected.txt (0 => 239590)
--- trunk/LayoutTests/fast/forms/ios/scroll-to-reveal-focused-select-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/forms/ios/scroll-to-reveal-focused-select-expected.txt 2019-01-03 15:38:33 UTC (rev 239590)
@@ -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
+
Added: trunk/LayoutTests/fast/forms/ios/scroll-to-reveal-focused-select.html (0 => 239590)
--- trunk/LayoutTests/fast/forms/ios/scroll-to-reveal-focused-select.html (rev 0)
+++ trunk/LayoutTests/fast/forms/ios/scroll-to-reveal-focused-select.html 2019-01-03 15:38:33 UTC (rev 239590)
@@ -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=""
+<script src=""
+<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>
Modified: trunk/Source/WebKit/ChangeLog (239589 => 239590)
--- trunk/Source/WebKit/ChangeLog 2019-01-03 15:34:34 UTC (rev 239589)
+++ trunk/Source/WebKit/ChangeLog 2019-01-03 15:38:33 UTC (rev 239590)
@@ -1,5 +1,58 @@
2019-01-03 Wenson Hsieh <[email protected]>
+ [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 <[email protected]>
+
WebUndoStep's monotonically increasing identifier should be a WebUndoStepID instead of uint64_t
https://bugs.webkit.org/show_bug.cgi?id=193100
Modified: trunk/Source/WebKit/Shared/FocusedElementInformation.cpp (239589 => 239590)
--- trunk/Source/WebKit/Shared/FocusedElementInformation.cpp 2019-01-03 15:34:34 UTC (rev 239589)
+++ trunk/Source/WebKit/Shared/FocusedElementInformation.cpp 2019-01-03 15:38:33 UTC (rev 239590)
@@ -64,7 +64,7 @@
void FocusedElementInformation::encode(IPC::Encoder& encoder) const
{
encoder << elementRect;
- encoder << elementInteractionLocation;
+ encoder << lastInteractionLocation;
encoder << minimumScaleFactor;
encoder << maximumScaleFactor;
encoder << maximumScaleFactorIgnoringAlwaysScalable;
@@ -112,7 +112,7 @@
if (!decoder.decode(result.elementRect))
return false;
- if (!decoder.decode(result.elementInteractionLocation))
+ if (!decoder.decode(result.lastInteractionLocation))
return false;
if (!decoder.decode(result.minimumScaleFactor))
Modified: trunk/Source/WebKit/Shared/FocusedElementInformation.h (239589 => 239590)
--- trunk/Source/WebKit/Shared/FocusedElementInformation.h 2019-01-03 15:34:34 UTC (rev 239589)
+++ trunk/Source/WebKit/Shared/FocusedElementInformation.h 2019-01-03 15:38:33 UTC (rev 239590)
@@ -97,7 +97,7 @@
struct FocusedElementInformation {
WebCore::IntRect elementRect;
- WebCore::IntPoint elementInteractionLocation;
+ WebCore::IntPoint lastInteractionLocation;
double minimumScaleFactor { -INFINITY };
double maximumScaleFactor { INFINITY };
double maximumScaleFactorIgnoringAlwaysScalable { INFINITY };
Modified: trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm (239589 => 239590)
--- trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm 2019-01-03 15:34:34 UTC (rev 239589)
+++ trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm 2019-01-03 15:38:33 UTC (rev 239590)
@@ -4369,7 +4369,10 @@
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;
Modified: trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm (239589 => 239590)
--- trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm 2019-01-03 15:34:34 UTC (rev 239589)
+++ trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm 2019-01-03 15:38:33 UTC (rev 239590)
@@ -2362,7 +2362,7 @@
{
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 @@
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();
