Changeset 251761 in webkit

Timestamp:

Oct 29, 2019 7:48:45 PM (4 years ago)

Author:

Wenson Hsieh

Message:

Source/WebCore:
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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/editing/selection/ios/set-selection-by-tapping-after-changing-focused-element-bounds-expected.txt (added)
• LayoutTests/editing/selection/ios/set-selection-by-tapping-after-changing-focused-element-bounds.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/ElementContext.h (modified) (2 diffs)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm (modified) (4 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/dom/ElementContext.h

@@ -42 +42 @@
     ~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&);
@@ -48 +53 @@
 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);
 }
 

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm

@@ -1697 +1697 @@
         return;
 
-    if (hasFocusedElement(_focusedElementInformation) && _positionInformation.elementContext == _focusedElementInformation.elementContext)
+    if (hasFocusedElement(_focusedElementInformation) && _positionInformation.elementContext && _positionInformation.elementContext->isSameElement(_focusedElementInformation.elementContext))
         return;
 
@@ -2269 +2269 @@
             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;
         }
@@ -2315 +2315 @@
         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 {
@@ -2450 +2450 @@
     // 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.