Source/WebCore:
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 30 Oct 2019 02:48:45 +0000 (02:48 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 30 Oct 2019 02:48:45 +0000 (02:48 +0000)
REGRESSION (r251693): [iOS] Unable to change selection after focusing an element with keyboard attached
https://bugs.webkit.org/show_bug.cgi?id=203582

Reviewed by Tim Horton.

Introduces a new helper method to check whether two ElementContexts refer to the same element. Importantly, this
ignores any information on ElementContext that is not either the element, document, or page identifier (for now,
this only includes the element's bounding rect, which may change over time).

Test: editing/selection/ios/set-selection-by-tapping-after-changing-focused-element-bounds.html

* dom/ElementContext.h:
(WebCore::ElementContext::isSameElement const):
(WebCore::operator==):

Source/WebKit:
REGRESSION (r251693): [iOS] Unable to change selection in a focused element if the element's bounds change
https://bugs.webkit.org/show_bug.cgi?id=203582

Reviewed by Tim Horton.

The refactoring in r251693 broke the ability to change selection in an editable area by tapping in iOS Safari,
in the case where the editable element's bounds change after focus. This is because the aforementioned change
now compares position informations' element context against the focused element information's element context to
check whether or not the position information request was inside the focused element. However, if the bounds of
the focused element change in between the position information request and when the element is initially
focused, the `operator==` comparison will fail, causing us to prevent text selection.

To fix this, only check whether or not the two element contexts refer to the same element in the DOM by
comparing page, document and element identifiers, but not the element's bounding rect.

* UIProcess/ios/WKContentViewInteraction.mm:
(-[WKContentView _didGetTapHighlightForRequest:color:quads:topLeftRadius:topRightRadius:bottomLeftRadius:bottomRightRadius:nodeHasBuiltInClickHandling:]):
(-[WKContentView gestureRecognizerShouldBegin:]):
(-[WKContentView textInteractionGesture:shouldBeginAtPoint:]):

LayoutTests:
REGRESSION (r251693): [iOS] Unable to change selection after focusing an element with keyboard attached
https://bugs.webkit.org/show_bug.cgi?id=203582

Reviewed by Tim Horton.

Add a new layout test to cover this scenario.

* editing/selection/ios/set-selection-by-tapping-after-changing-focused-element-bounds-expected.txt: Added.
* editing/selection/ios/set-selection-by-tapping-after-changing-focused-element-bounds.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/editing/selection/ios/set-selection-by-tapping-after-changing-focused-element-bounds-expected.txt [new file with mode: 0644]
LayoutTests/editing/selection/ios/set-selection-by-tapping-after-changing-focused-element-bounds.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/ElementContext.h
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm

index cc5a67e..55155fd 100644 (file)
@@ -1,3 +1,15 @@
+2019-10-29  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION (r251693): [iOS] Unable to change selection after focusing an element with keyboard attached
+        https://bugs.webkit.org/show_bug.cgi?id=203582
+
+        Reviewed by Tim Horton.
+
+        Add a new layout test to cover this scenario.
+
+        * editing/selection/ios/set-selection-by-tapping-after-changing-focused-element-bounds-expected.txt: Added.
+        * editing/selection/ios/set-selection-by-tapping-after-changing-focused-element-bounds.html: Added.
+
 2019-10-29  Chris Dumez  <cdumez@apple.com>
 
         UserMediaRequest should not prevent entering the back/forward cache
diff --git a/LayoutTests/editing/selection/ios/set-selection-by-tapping-after-changing-focused-element-bounds-expected.txt b/LayoutTests/editing/selection/ios/set-selection-by-tapping-after-changing-focused-element-bounds-expected.txt
new file mode 100644 (file)
index 0000000..3565201
--- /dev/null
@@ -0,0 +1,11 @@
+Verifies that tapping to change selection works when the focused element's bounds change after focusing the element. To manually run the test, first tap the editable area to focus it, then tap the button to shrink the editable area, and finally, verify that it's possible to change selection in the editable area by tapping it.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS getSelection().rangeCount is 1
+PASS Successfully changed selection after shrinking editable area.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/editing/selection/ios/set-selection-by-tapping-after-changing-focused-element-bounds.html b/LayoutTests/editing/selection/ios/set-selection-by-tapping-after-changing-focused-element-bounds.html
new file mode 100644 (file)
index 0000000..5c7a95c
--- /dev/null
@@ -0,0 +1,71 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+<html>
+<head>
+<script src="../../../resources/js-test.js"></script>
+<script src="../../../resources/ui-helper.js"></script>
+<meta name=viewport content="width=device-width, initial-scale=1, user-scalable=no">
+<style>
+body, html {
+    width: 100%;
+    height: 100%;
+    margin: 0;
+}
+
+#editor {
+    width: 300px;
+    height: 300px;
+    font-size: 18px;
+    overflow: scroll;
+}
+</style>
+<script>
+jsTestIsAsync = true;
+
+function tapAndWaitForSelectionChange(x, y) {
+    return new Promise(async resolve => {
+        let doneCount = 0;
+        const checkDone = () => {
+            if (++doneCount == 2)
+                resolve();
+        }
+        document.addEventListener("selectionchange", checkDone, { once: true });
+        await UIHelper.activateAt(x, y);
+        checkDone();
+    });
+}
+
+addEventListener("load", async () => {
+    description("Verifies that tapping to change selection works when the focused element's bounds change after focusing the element. To manually run the test, first tap the editable area to focus it, then tap the button to shrink the editable area, and finally, verify that it's possible to change selection in the editable area by tapping it.");
+
+    button = document.querySelector("button");
+    editor = document.getElementById("editor");
+
+    button.addEventListener("mousedown", event => {
+        editor.style.width = "160px";
+        editor.style.height = "160px";
+        event.preventDefault();
+    });
+
+    if (!window.testRunner)
+        return;
+
+    await UIHelper.activateElementAndWaitForInputSession(editor);
+    await UIHelper.activateElement(button);
+    await tapAndWaitForSelectionChange(30, 30);
+
+    shouldBe("getSelection().rangeCount", "1");
+    testPassed("Successfully changed selection after shrinking editable area.");
+
+    button.remove();
+    editor.remove();
+    finishJSTest();
+});
+</script>
+</head>
+<body>
+<p contenteditable id="editor">Here's to the crazy ones, the misfits, the rebels, the trouble makers, the round pegs in the square holes, the ones who see things differently. There not fond of rules, and they have no respect for the status quo, you can quote then, disagree with them, glorify or vilify them, about the only thing you can't do is ignore them.  Because they change things. They push the human race forward. And while some may see them as the crazy ones, we see genius. Because the people who are crazy enough to think they can change the world are the ones who do.</p>
+<button>Shrink</button>
+<p id="description"></p>
+<p id="console"></p>
+</body>
+</html>
index 4ec42fe..ef0e772 100644 (file)
@@ -1,3 +1,20 @@
+2019-10-29  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION (r251693): [iOS] Unable to change selection after focusing an element with keyboard attached
+        https://bugs.webkit.org/show_bug.cgi?id=203582
+
+        Reviewed by Tim Horton.
+
+        Introduces a new helper method to check whether two ElementContexts refer to the same element. Importantly, this
+        ignores any information on ElementContext that is not either the element, document, or page identifier (for now,
+        this only includes the element's bounding rect, which may change over time).
+
+        Test: editing/selection/ios/set-selection-by-tapping-after-changing-focused-element-bounds.html
+
+        * dom/ElementContext.h:
+        (WebCore::ElementContext::isSameElement const):
+        (WebCore::operator==):
+
 2019-10-29  Chris Dumez  <cdumez@apple.com>
 
         UserMediaRequest should not prevent entering the back/forward cache
index 59f65e9..689b061 100644 (file)
@@ -41,16 +41,18 @@ struct ElementContext {
 
     ~ElementContext() = default;
 
+    bool isSameElement(const ElementContext& other) const
+    {
+        return webPageIdentifier == other.webPageIdentifier && documentIdentifier == other.documentIdentifier && elementIdentifier == other.elementIdentifier;
+    }
+
     template<class Encoder> void encode(Encoder&) const;
     template<class Decoder> static Optional<ElementContext> decode(Decoder&);
 };
 
 inline bool operator==(const ElementContext& a, const ElementContext& b)
 {
-    return a.boundingRect == b.boundingRect
-        && a.webPageIdentifier == b.webPageIdentifier
-        && a.documentIdentifier == b.documentIdentifier
-        && a.elementIdentifier == b.elementIdentifier;
+    return a.boundingRect == b.boundingRect && a.isSameElement(b);
 }
 
 template<class Encoder>
index 19da265..f692ace 100644 (file)
@@ -1,3 +1,25 @@
+2019-10-29  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION (r251693): [iOS] Unable to change selection in a focused element if the element's bounds change
+        https://bugs.webkit.org/show_bug.cgi?id=203582
+
+        Reviewed by Tim Horton.
+
+        The refactoring in r251693 broke the ability to change selection in an editable area by tapping in iOS Safari,
+        in the case where the editable element's bounds change after focus. This is because the aforementioned change
+        now compares position informations' element context against the focused element information's element context to
+        check whether or not the position information request was inside the focused element. However, if the bounds of
+        the focused element change in between the position information request and when the element is initially
+        focused, the `operator==` comparison will fail, causing us to prevent text selection.
+
+        To fix this, only check whether or not the two element contexts refer to the same element in the DOM by
+        comparing page, document and element identifiers, but not the element's bounding rect.
+
+        * UIProcess/ios/WKContentViewInteraction.mm:
+        (-[WKContentView _didGetTapHighlightForRequest:color:quads:topLeftRadius:topRightRadius:bottomLeftRadius:bottomRightRadius:nodeHasBuiltInClickHandling:]):
+        (-[WKContentView gestureRecognizerShouldBegin:]):
+        (-[WKContentView textInteractionGesture:shouldBeginAtPoint:]):
+
 2019-10-07  Jer Noble  <jer.noble@apple.com>
 
         Implement the Remote Playback API.
index 9ba3758..e93895f 100644 (file)
@@ -1696,7 +1696,7 @@ static NSValue *nsSizeForTapHighlightBorderRadius(WebCore::IntSize borderRadius,
     if (!_isTapHighlightIDValid || _latestTapID != requestID)
         return;
 
-    if (hasFocusedElement(_focusedElementInformation) && _positionInformation.elementContext == _focusedElementInformation.elementContext)
+    if (hasFocusedElement(_focusedElementInformation) && _positionInformation.elementContext && _positionInformation.elementContext->isSameElement(_focusedElementInformation.elementContext))
         return;
 
     _isTapHighlightIDValid = NO;
@@ -2268,7 +2268,7 @@ static inline bool isSamePair(UIGestureRecognizer *a, UIGestureRecognizer *b, UI
             // If the focused element is the same, prevent the gesture.
             if (![self ensurePositionInformationIsUpToDate:WebKit::InteractionInformationRequest(WebCore::roundedIntPoint(point))])
                 return NO;
-            if (_positionInformation.elementContext == _focusedElementInformation.elementContext)
+            if (_positionInformation.elementContext && _positionInformation.elementContext->isSameElement(_focusedElementInformation.elementContext))
                 return NO;
         }
     }
@@ -2314,7 +2314,7 @@ static inline bool isSamePair(UIGestureRecognizer *a, UIGestureRecognizer *b, UI
 
         if (hasFocusedElement(_focusedElementInformation)) {
             // Prevent the gesture if it is the same node.
-            if (_positionInformation.elementContext == _focusedElementInformation.elementContext)
+            if (_positionInformation.elementContext && _positionInformation.elementContext->isSameElement(_focusedElementInformation.elementContext))
                 return NO;
         } else {
             // Prevent the gesture if there is no action for the node.
@@ -2449,7 +2449,7 @@ static inline bool isSamePair(UIGestureRecognizer *a, UIGestureRecognizer *b, UI
 
     // If we're currently focusing an editable element, only allow the selection to move within that focused element.
     if (self.isFocusingElement)
-        return _positionInformation.elementContext == _focusedElementInformation.elementContext;
+        return _positionInformation.elementContext && _positionInformation.elementContext->isSameElement(_focusedElementInformation.elementContext);
 
     // If we're selecting something, don't activate highlight.
     if (gesture == UIWKGestureLoupe && [self hasSelectablePositionAtPoint:point])