Attachment 346688 Details for Bug 188374 – Patch

[patch] Patch

bug-188374-20180806232440.patch (text/plain), 15.20 KB, created by Wenson Hsieh on 2018-08-06 23:24:41 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Wenson Hsieh

Created: 2018-08-06 23:24:41 PDT

Size: 15.20 KB

patch

obsolete

>Subversion Revision: 234614
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 8a6cf390b7d1e4fea00a997f98634f1567ac0749..96ee800c770b3b3eab03aac70acf5ad157a3a583 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,34 @@
>+2018-08-06  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        REGRESSION (r233778): Text selection sometimes cannot be extended in iframes
>+        https://bugs.webkit.org/show_bug.cgi?id=188374
>+        <rdar://problem/42928657>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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-06  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed, rolling out r234569.
>diff --git a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
>index bd9553359296f315577853e47217902ac0a2d9d4..518e38e32d24378f2047cb3130e0c1a1e1c04abe 100644
>--- a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
>+++ b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
>@@ -1215,17 +1215,19 @@ void WebPage::selectWithGesture(const IntPoint& point, uint32_t granularity, uin
>     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,14 +1237,14 @@ static RefPtr<Range> rangeForPoint(Frame* frame, const IntPoint& point, bool bas
>         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();
>         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 @@ void WebPage::updateSelectionWithTouches(const IntPoint& point, uint32_t touches
>             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 @@ void WebPage::updateSelectionWithTouches(const IntPoint& point, uint32_t touches
>         break;
> 
>     case SelectionTouch::Moved:
>-        range = rangeForPoint(&frame, point, baseIsStart);
>+        range = rangeForPointInRootViewCoordinates(frame, point, baseIsStart);
>         break;
>     }
>     if (range)
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 0a16baa82ed47127e2529345d498d415f5017770..49b8aee9615ab0bf1ff2e40163f437f9cd0c5806 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,29 @@
>+2018-08-06  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        REGRESSION (r233778): Text selection sometimes cannot be extended in iframes
>+        https://bugs.webkit.org/show_bug.cgi?id=188374
>+        <rdar://problem/42928657>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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.
>+
>+        * platform/ios-wk2/TestExpectations:
>+
>+        Enable the tests in editing/selection/ios by default, but disable selection-handles-after-touch-end.html by
>+        default due to reliance on touch event support.
>+
> 2018-08-03  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed test gardening for mac-wk1.
>diff --git a/LayoutTests/editing/selection/ios/selection-handles-in-iframe-expected.txt b/LayoutTests/editing/selection/ios/selection-handles-in-iframe-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..60c933f0388ed339d314a597b1cc3dd2398c760c
>--- /dev/null
>+++ b/LayoutTests/editing/selection/ios/selection-handles-in-iframe-expected.txt
>@@ -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.
>diff --git a/LayoutTests/editing/selection/ios/selection-handles-in-iframe.html b/LayoutTests/editing/selection/ios/selection-handles-in-iframe.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..fa6a2310665d58da723ab61d971a899fefd6eee0
>--- /dev/null
>+++ b/LayoutTests/editing/selection/ios/selection-handles-in-iframe.html
>@@ -0,0 +1,83 @@
>+<!DOCTYPE html> 
>+<html>
>+<head>
>+<script src="../../../resources/ui-helper.js"></script>
>+<script src="../../../resources/basic-gestures.js"></script>
>+<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="data:text/html,
>+    <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
>diff --git a/LayoutTests/editing/selection/ios/selection-handles-in-readonly-input-expected.txt b/LayoutTests/editing/selection/ios/selection-handles-in-readonly-input-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..2e19f6a60187aa0fb3c024d3234e106296eafece
>--- /dev/null
>+++ b/LayoutTests/editing/selection/ios/selection-handles-in-readonly-input-expected.txt
>@@ -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.
>diff --git a/LayoutTests/editing/selection/ios/selection-handles-in-readonly-input.html b/LayoutTests/editing/selection/ios/selection-handles-in-readonly-input.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..7582b0bb77ca2cca367e67a901ee4d94e39598f7
>--- /dev/null
>+++ b/LayoutTests/editing/selection/ios/selection-handles-in-readonly-input.html
>@@ -0,0 +1,65 @@
>+<!DOCTYPE html> 
>+<html>
>+<head>
>+<script src="../../../resources/ui-helper.js"></script>
>+<script src="../../../resources/basic-gestures.js"></script>
>+<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
>diff --git a/LayoutTests/platform/ios-wk2/TestExpectations b/LayoutTests/platform/ios-wk2/TestExpectations
>index c1edfec111c4bfd5eb30a301abe7a74149b0f60d..02841884db175f93cf70fc2651025ba54a3bfaaa 100644
>--- a/LayoutTests/platform/ios-wk2/TestExpectations
>+++ b/LayoutTests/platform/ios-wk2/TestExpectations
>@@ -13,8 +13,12 @@ fast/viewport/ios [ Pass ]
> fast/visual-viewport/ios/ [ Pass ]
> scrollingcoordinator/ios [ Pass ]
> tiled-drawing/ios [ Pass ]
>+editing/selection/ios [ Pass ]
> editing/selection/character-granularity-rect.html [ Failure ]
> 
>+# Requires touch events to work.
>+editing/selection/ios/selection-handles-after-touch-end.html [ Skip ]
>+
> accessibility/smart-invert.html [ Pass ]
> accessibility/smart-invert-reference.html [ Pass ]
> fast/media/mq-inverted-colors-live-update.html [ Pass ]

Actions: View | Formatted Diff | Diff

Attachments on bug 188374: 346688 | 346693 | 346709