- Revision
- 234661
- Author
- [email protected]
- Date
- 2018-08-07 11:32:42 -0700 (Tue, 07 Aug 2018)
Log Message
REGRESSION (r233778): Text selection sometimes cannot be extended in iframes
https://bugs.webkit.org/show_bug.cgi?id=188374
<rdar://problem/42928657>
Reviewed by Simon Fraser.
Source/WebKit:
rangeForPoint contains logic for converting a selection handle location in root view coordinates to an updated
selection. In doing so, we first convert the selection handle location to content coordinates; however, the call
site to EventHandler::hitTestResultAtPoint still hit-tests using the location in root view coordinates rather
than content coordinates, which means that when the focused frame is a subframe, hit-testing will fail to find
nodes within the subframe under the selection handle. This manifests in behaviors such as snapping to a single
character when selecting text in subframes.
To fix this, we just need to pass in the point in the frame's content coordinates when hit-testing.
Tests: editing/selection/ios/selection-handles-in-iframe.html
editing/selection/ios/selection-handles-in-readonly-input.html
* WebProcess/WebPage/ios/WebPageIOS.mm:
(WebKit::rangeForPointInRootViewCoordinates):
Make a couple of other minor adjustments:
1. Take a Frame& instead of a Frame*, since Frame& is assumed to be non-null here.
2. Rename rangeForPoint to rangeForPointInRootViewCoordinates, as well as the point argument to
pointInRootViewCoordinates.
(WebKit::WebPage::updateSelectionWithTouches):
(WebKit::rangeForPoint): Deleted.
LayoutTests:
Add 2 new layout tests to cover the original bug that r233778 fixed, as well as the regression in this bug.
* editing/selection/ios/selection-handles-in-iframe-expected.txt: Added.
* editing/selection/ios/selection-handles-in-iframe.html: Added.
Add a test to verify that the user can select text in an iframe by dragging selection handles.
* editing/selection/ios/selection-handles-in-readonly-input-expected.txt: Added.
* editing/selection/ios/selection-handles-in-readonly-input.html: Added.
Add a test to verify that dragging a selection handle outside of a readonly input does not cause the selection
to jump outside of the input and clear out the selection in the input.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (234660 => 234661)
--- trunk/LayoutTests/ChangeLog 2018-08-07 18:12:59 UTC (rev 234660)
+++ trunk/LayoutTests/ChangeLog 2018-08-07 18:32:42 UTC (rev 234661)
@@ -1,3 +1,24 @@
+2018-08-07 Wenson Hsieh <[email protected]>
+
+ REGRESSION (r233778): Text selection sometimes cannot be extended in iframes
+ https://bugs.webkit.org/show_bug.cgi?id=188374
+ <rdar://problem/42928657>
+
+ Reviewed by Simon Fraser.
+
+ Add 2 new layout tests to cover the original bug that r233778 fixed, as well as the regression in this bug.
+
+ * editing/selection/ios/selection-handles-in-iframe-expected.txt: Added.
+ * editing/selection/ios/selection-handles-in-iframe.html: Added.
+
+ Add a test to verify that the user can select text in an iframe by dragging selection handles.
+
+ * editing/selection/ios/selection-handles-in-readonly-input-expected.txt: Added.
+ * editing/selection/ios/selection-handles-in-readonly-input.html: Added.
+
+ Add a test to verify that dragging a selection handle outside of a readonly input does not cause the selection
+ to jump outside of the input and clear out the selection in the input.
+
2018-08-07 Alex Christensen <[email protected]>
Fix things after r234640
Added: trunk/LayoutTests/editing/selection/ios/selection-handles-in-iframe-expected.txt (0 => 234661)
--- trunk/LayoutTests/editing/selection/ios/selection-handles-in-iframe-expected.txt (rev 0)
+++ trunk/LayoutTests/editing/selection/ios/selection-handles-in-iframe-expected.txt 2018-08-07 18:32:42 UTC (rev 234661)
@@ -0,0 +1,9 @@
+selectionchange: 'M'
+selectionchange: 'M '
+selectionchange: 'M M'
+
+Verifies that the user can select text in an iframe by dragging selection handles. This test is best run under WebKitTestRunner.
+
+To manually run the test, select "M" in the iframe above and drag the selection handles.
+
+The output above should log selection changes in the subframe.
Added: trunk/LayoutTests/editing/selection/ios/selection-handles-in-iframe.html (0 => 234661)
--- trunk/LayoutTests/editing/selection/ios/selection-handles-in-iframe.html (rev 0)
+++ trunk/LayoutTests/editing/selection/ios/selection-handles-in-iframe.html 2018-08-07 18:32:42 UTC (rev 234661)
@@ -0,0 +1,83 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+<html>
+<head>
+<script src=""
+<script src=""
+<meta name=viewport content="width=device-width">
+<style>
+body, html {
+ width: 100%;
+ height: 100%;
+ margin: 0;
+}
+
+#output {
+ width: 320px;
+ height: 160px;
+ overflow: scroll;
+ color: green;
+ border: 1px green solid;
+}
+
+#target {
+ width: 320px;
+ height: 160px;
+}
+</style>
+<script>
+if (window.testRunner) {
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+}
+
+function appendOutput(s) {
+ const paragraph = document.createElement("div");
+ paragraph.textContent = s;
+ output.appendChild(paragraph);
+}
+
+function checkForCompletion() {
+ doneCount = window.doneCount ? doneCount : 0;
+ if (++doneCount == 4 && window.testRunner)
+ testRunner.notifyDone();
+}
+
+addEventListener("message", event => {
+ appendOutput(event.data);
+ checkForCompletion();
+});
+
+async function runTest() {
+ // Wait for both the main frame and the subframe to finish loading.
+ loadCount = window.loadCount ? loadCount : 0;
+ if (++loadCount != 2)
+ return;
+
+ await longPressAtPoint(30, 240);
+ let grabberEndRect = null;
+ while (!grabberEndRect || !grabberEndRect.width || !grabberEndRect.height)
+ grabberEndRect = await UIHelper.getSelectionEndGrabberViewRect();
+
+ const [midX, midY] = [grabberEndRect.left + (grabberEndRect.width / 2), grabberEndRect.top + (grabberEndRect.height / 2)];
+ await touchAndDragFromPointToPoint(midX, midY, midX + 150, midY);
+ await liftUpAtPoint(midX + 150, midY);
+ checkForCompletion();
+}
+</script>
+</head>
+
+<body _onload_="runTest()">
+<pre id="output"></pre>
+<iframe _onload_="runTest()" src=""
+ <span id='text' style='font-size: 140px;'>M M</span>
+ <script>
+ document.addEventListener('selectionchange', () => {
+ const eventData = 'selectionchange: \'' + getSelection().toString() + '\'';
+ parent.postMessage(eventData, '*');
+ });
+ </script>" id="target"></iframe>
+<p>Verifies that the user can select text in an iframe by dragging selection handles. This test is best run under WebKitTestRunner.</p>
+<p>To manually run the test, select "M" in the iframe above and drag the selection handles.</p>
+<p>The output above should log selection changes in the subframe.</p>
+</body>
+</html>
\ No newline at end of file
Added: trunk/LayoutTests/editing/selection/ios/selection-handles-in-readonly-input-expected.txt (0 => 234661)
--- trunk/LayoutTests/editing/selection/ios/selection-handles-in-readonly-input-expected.txt (rev 0)
+++ trunk/LayoutTests/editing/selection/ios/selection-handles-in-readonly-input-expected.txt 2018-08-07 18:32:42 UTC (rev 234661)
@@ -0,0 +1,10 @@
+
+Initial selected range: (0, 2)
+Final selected range: (0, 2)
+Verifies that dragging a selection handle outside of a readonly input does not cause the selection to jump outside of the input.
+
+To manually test, double-tap to select the word "aa", drag the selection end handle out of the bounds of the input, and then drag the selection handle back in.
+
+"aa" should remain selected.
+
+This test is best run under WebKitTestRunner.
Added: trunk/LayoutTests/editing/selection/ios/selection-handles-in-readonly-input.html (0 => 234661)
--- trunk/LayoutTests/editing/selection/ios/selection-handles-in-readonly-input.html (rev 0)
+++ trunk/LayoutTests/editing/selection/ios/selection-handles-in-readonly-input.html 2018-08-07 18:32:42 UTC (rev 234661)
@@ -0,0 +1,65 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+<html>
+<head>
+<script src=""
+<script src=""
+<meta name=viewport content="width=device-width">
+<style>
+body, html {
+ width: 100%;
+ height: 100%;
+ margin: 0;
+}
+
+#output {
+ width: 320px;
+ height: 160px;
+ overflow: scroll;
+ color: green;
+ border: 1px green solid;
+}
+
+input {
+ font-size: 150px;
+ width: 300px;
+ height: 100px;
+ padding-left: 0;
+}
+</style>
+<script>
+if (window.testRunner) {
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+}
+
+function appendOutput(s) {
+ const paragraph = document.createElement("div");
+ paragraph.textContent = s;
+ output.appendChild(paragraph);
+}
+
+async function runTest() {
+ await UIHelper.activateAndWaitForInputSessionAt(100, 50);
+ input.setSelectionRange(0, 2);
+ let grabberEndRect = null;
+ while (!grabberEndRect || !grabberEndRect.width || !grabberEndRect.height)
+ grabberEndRect = await UIHelper.getSelectionEndGrabberViewRect();
+
+ const [midX, midY] = [grabberEndRect.left + (grabberEndRect.width / 2), grabberEndRect.top + (grabberEndRect.height / 2)];
+ appendOutput(`Initial selected range: (${input.selectionStart}, ${input.selectionEnd})`);
+ await touchAndDragFromPointToPoint(midX, midY, midX + 25, midY + 150);
+ await liftUpAtPoint(midX + 25, midY + 150);
+ appendOutput(`Final selected range: (${input.selectionStart}, ${input.selectionEnd})`);
+ testRunner.notifyDone();
+}
+</script>
+</head>
+
+<body _onload_="runTest()">
+<input id="input" readonly value="aa">
+<pre id="output"></pre>
+<p>Verifies that dragging a selection handle outside of a readonly input does not cause the selection to jump outside of the input.</p>
+<p>To manually test, double-tap to select the word "aa", drag the selection end handle out of the bounds of the input, and then drag the selection handle back in.</p>
+<p>"aa" should remain selected.</p>
+<p>This test is best run under WebKitTestRunner.</p></body>
+</html>
\ No newline at end of file
Modified: trunk/Source/WebKit/ChangeLog (234660 => 234661)
--- trunk/Source/WebKit/ChangeLog 2018-08-07 18:12:59 UTC (rev 234660)
+++ trunk/Source/WebKit/ChangeLog 2018-08-07 18:32:42 UTC (rev 234661)
@@ -1,3 +1,34 @@
+2018-08-07 Wenson Hsieh <[email protected]>
+
+ REGRESSION (r233778): Text selection sometimes cannot be extended in iframes
+ https://bugs.webkit.org/show_bug.cgi?id=188374
+ <rdar://problem/42928657>
+
+ Reviewed by Simon Fraser.
+
+ rangeForPoint contains logic for converting a selection handle location in root view coordinates to an updated
+ selection. In doing so, we first convert the selection handle location to content coordinates; however, the call
+ site to EventHandler::hitTestResultAtPoint still hit-tests using the location in root view coordinates rather
+ than content coordinates, which means that when the focused frame is a subframe, hit-testing will fail to find
+ nodes within the subframe under the selection handle. This manifests in behaviors such as snapping to a single
+ character when selecting text in subframes.
+
+ To fix this, we just need to pass in the point in the frame's content coordinates when hit-testing.
+
+ Tests: editing/selection/ios/selection-handles-in-iframe.html
+ editing/selection/ios/selection-handles-in-readonly-input.html
+
+ * WebProcess/WebPage/ios/WebPageIOS.mm:
+ (WebKit::rangeForPointInRootViewCoordinates):
+
+ Make a couple of other minor adjustments:
+ 1. Take a Frame& instead of a Frame*, since Frame& is assumed to be non-null here.
+ 2. Rename rangeForPoint to rangeForPointInRootViewCoordinates, as well as the point argument to
+ pointInRootViewCoordinates.
+
+ (WebKit::WebPage::updateSelectionWithTouches):
+ (WebKit::rangeForPoint): Deleted.
+
2018-08-07 Alex Christensen <[email protected]>
Fix things after r234640
Modified: trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm (234660 => 234661)
--- trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm 2018-08-07 18:12:59 UTC (rev 234660)
+++ trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm 2018-08-07 18:32:42 UTC (rev 234661)
@@ -1215,17 +1215,19 @@
send(Messages::WebPageProxy::GestureCallback(point, gestureType, gestureState, static_cast<uint32_t>(flags), callbackID));
}
-static RefPtr<Range> rangeForPoint(Frame* frame, const IntPoint& point, bool baseIsStart)
+static RefPtr<Range> rangeForPointInRootViewCoordinates(Frame& frame, const IntPoint& pointInRootViewCoordinates, bool baseIsStart)
{
- IntPoint pointInDocument = frame->view()->rootViewToContents(point);
- Position result = frame->visiblePositionForPoint(pointInDocument).deepEquivalent();
+ IntPoint pointInDocument = frame.view()->rootViewToContents(pointInRootViewCoordinates);
+ Position result;
RefPtr<Range> range;
- HitTestResult hitTest = frame->eventHandler().hitTestResultAtPoint(point, HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::AllowChildFrameContent);
+ HitTestResult hitTest = frame.eventHandler().hitTestResultAtPoint(pointInDocument, HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::AllowChildFrameContent);
if (hitTest.targetNode())
- result = frame->eventHandler().selectionExtentRespectingEditingBoundary(frame->selection().selection(), hitTest.localPoint(), hitTest.targetNode()).deepEquivalent();
+ result = frame.eventHandler().selectionExtentRespectingEditingBoundary(frame.selection().selection(), hitTest.localPoint(), hitTest.targetNode()).deepEquivalent();
+ else
+ result = frame.visiblePositionForPoint(pointInDocument).deepEquivalent();
- VisibleSelection existingSelection = frame->selection().selection();
+ VisibleSelection existingSelection = frame.selection().selection();
Position selectionStart = existingSelection.visibleStart().deepEquivalent();
Position selectionEnd = existingSelection.visibleEnd().deepEquivalent();
@@ -1235,7 +1237,7 @@
else if (&selectionStart.anchorNode()->treeScope() != &hitTest.targetNode()->treeScope())
result = VisibleSelection::adjustPositionForEnd(result, selectionStart.containerNode());
if (result.isNotNull())
- range = Range::create(*frame->document(), selectionStart, result);
+ range = Range::create(*frame.document(), selectionStart, result);
} else {
if (comparePositions(selectionEnd, result) <= 0)
result = selectionEnd.previous();
@@ -1242,7 +1244,7 @@
else if (&hitTest.targetNode()->treeScope() != &selectionEnd.anchorNode()->treeScope())
result = VisibleSelection::adjustPositionForStart(result, selectionEnd.containerNode());
if (result.isNotNull())
- range = Range::create(*frame->document(), result, selectionEnd);
+ range = Range::create(*frame.document(), result, selectionEnd);
}
return range;
@@ -1329,7 +1331,7 @@
if (result.isNotNull())
range = Range::create(*frame.document(), result, result);
} else
- range = rangeForPoint(&frame, point, baseIsStart);
+ range = rangeForPointInRootViewCoordinates(frame, point, baseIsStart);
break;
case SelectionTouch::EndedMovingForward:
@@ -1341,7 +1343,7 @@
break;
case SelectionTouch::Moved:
- range = rangeForPoint(&frame, point, baseIsStart);
+ range = rangeForPointInRootViewCoordinates(frame, point, baseIsStart);
break;
}
if (range)
