- Revision
- 255603
- Author
- wenson_hs...@apple.com
- Date
- 2020-02-03 15:09:19 -0800 (Mon, 03 Feb 2020)
Log Message
[iOS 13] Dragging on-screen volume control on a YouTube video selects text around the panel
https://bugs.webkit.org/show_bug.cgi?id=207140
<rdar://problem/58852938>
Reviewed by Tim Horton.
Source/WebKit:
This bug occurs on iPadOS when long pressing the volume controls on the desktop version of YouTube, and then
performing a pan gesture; this activates the highlight text selection gesture recognizer added in iOS 13,
causing text to be selected while adjusting the volume using these custom controls. On macOS, we avoid this
because `Node::canStartSelection()` returns `false`, due to a container node having both `user-drag: element`
and `user-select: none`; in this scenario, we allow dragging to take precendence over text selection, so the
volume slider can be moved using a mouse without simultaneously selecting text.
This logic is currently absent on iOS, where UIKit text interaction gesture recognizers ask us for information
about the DOM at given locations, and we determine whether to allow text interaction gestures (such as long
pressing) to begin in `-textInteractionGesture:shouldBeginAtPoint:` and `-hasSelectablePositionAtPoint:`.
Ideally, we'd want to eventually unify these two codepaths for triggering text selection (and preferably just
have both go through EventHandler); in lieu of this, simply make the iOS codepath behave a little more like
macOS by adding a bit to allow text interaction gestures to bail in the case where at least one container of the
hit-tested element is both draggable and unselectable.
Test: editing/selection/ios/prefer-drag-over-text-selection.html
* Shared/ios/InteractionInformationAtPosition.h:
* Shared/ios/InteractionInformationAtPosition.mm:
(WebKit::InteractionInformationAtPosition::encode const):
(WebKit::InteractionInformationAtPosition::decode):
Add the new bit to InteractionInformationAtPosition.
* UIProcess/ios/WKContentViewInteraction.mm:
(-[WKContentView textInteractionGesture:shouldBeginAtPoint:]):
Avoid beginning text selection if the bit is set.
* WebProcess/WebPage/ios/WebPageIOS.mm:
(WebKit::selectionPositionInformation):
Move a comment about the ">= 97% of the document's visible area" heuristic next to the relevant code, and also
leave a FIXME mentioning that we should reconsider whether this is really needed; it seems that this was
originally intended to avoid ending up with an excessively large block selection, but block selection has been
removed entirely since iOS 12. That said, this logic may still be helping avoid situations where the text
selection is too aggressive on iOS and ends up selecting too much text.
LayoutTests:
Add a layout test to verify that we don't allow text selection gestures inside of containers that have both
`user-drag: element` and `user-select: none`.
* editing/selection/ios/prefer-drag-over-text-selection-expected.txt: Added.
* editing/selection/ios/prefer-drag-over-text-selection.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (255602 => 255603)
--- trunk/LayoutTests/ChangeLog 2020-02-03 23:04:20 UTC (rev 255602)
+++ trunk/LayoutTests/ChangeLog 2020-02-03 23:09:19 UTC (rev 255603)
@@ -1,3 +1,17 @@
+2020-02-03 Wenson Hsieh <wenson_hs...@apple.com>
+
+ [iOS 13] Dragging on-screen volume control on a YouTube video selects text around the panel
+ https://bugs.webkit.org/show_bug.cgi?id=207140
+ <rdar://problem/58852938>
+
+ Reviewed by Tim Horton.
+
+ Add a layout test to verify that we don't allow text selection gestures inside of containers that have both
+ `user-drag: element` and `user-select: none`.
+
+ * editing/selection/ios/prefer-drag-over-text-selection-expected.txt: Added.
+ * editing/selection/ios/prefer-drag-over-text-selection.html: Added.
+
2020-02-03 Jacob Uphoff <jacob_uph...@apple.com>
[ macOS iOS ] fast/dom/connected-subframe-counter-overflow.html is flaky timing out
Added: trunk/LayoutTests/editing/selection/ios/prefer-drag-over-text-selection-expected.txt (0 => 255603)
--- trunk/LayoutTests/editing/selection/ios/prefer-drag-over-text-selection-expected.txt (rev 0)
+++ trunk/LayoutTests/editing/selection/ios/prefer-drag-over-text-selection-expected.txt 2020-02-03 23:09:19 UTC (rev 255603)
@@ -0,0 +1,11 @@
+WebKit
+This test verifies that text selection does not begin when long pressing on an element that is inside a container with both 'user-select: none' and 'user-drag: element'. To manually run the test, try to select the word 'WebKit' below; the text selection should not change.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS getSelection().toString() is ""
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/editing/selection/ios/prefer-drag-over-text-selection.html (0 => 255603)
--- trunk/LayoutTests/editing/selection/ios/prefer-drag-over-text-selection.html (rev 0)
+++ trunk/LayoutTests/editing/selection/ios/prefer-drag-over-text-selection.html 2020-02-03 23:09:19 UTC (rev 255603)
@@ -0,0 +1,36 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+<html>
+<head>
+<script src=""
+<script src=""
+<meta name=viewport content="width=device-width, initial-scale=1, user-scalable=no">
+<style>
+#target {
+ font-size: 40px;
+ -webkit-user-select: none;
+ -webkit-user-drag: element;
+ display: inline;
+}
+
+body, html {
+ margin: 0;
+}
+</style>
+<script>
+jsTestIsAsync = true;
+
+addEventListener("load", async () => {
+ description("This test verifies that text selection does not begin when long pressing on an element that is inside a container with both 'user-select: none' and 'user-drag: element'. To manually run the test, try to select the word 'WebKit' below; the text selection should not change.");
+
+ await UIHelper.longPressElement(document.getElementById("target"));
+ shouldBeEqualToString("getSelection().toString()", "");
+ finishJSTest();
+});
+</script>
+</head>
+<body>
+<p id="target">WebKit</p>
+ <p id="description"></p>
+ <p id="console"></p>
+</body>
+</html>
Modified: trunk/Source/WebKit/ChangeLog (255602 => 255603)
--- trunk/Source/WebKit/ChangeLog 2020-02-03 23:04:20 UTC (rev 255602)
+++ trunk/Source/WebKit/ChangeLog 2020-02-03 23:09:19 UTC (rev 255603)
@@ -1,3 +1,49 @@
+2020-02-03 Wenson Hsieh <wenson_hs...@apple.com>
+
+ [iOS 13] Dragging on-screen volume control on a YouTube video selects text around the panel
+ https://bugs.webkit.org/show_bug.cgi?id=207140
+ <rdar://problem/58852938>
+
+ Reviewed by Tim Horton.
+
+ This bug occurs on iPadOS when long pressing the volume controls on the desktop version of YouTube, and then
+ performing a pan gesture; this activates the highlight text selection gesture recognizer added in iOS 13,
+ causing text to be selected while adjusting the volume using these custom controls. On macOS, we avoid this
+ because `Node::canStartSelection()` returns `false`, due to a container node having both `user-drag: element`
+ and `user-select: none`; in this scenario, we allow dragging to take precendence over text selection, so the
+ volume slider can be moved using a mouse without simultaneously selecting text.
+
+ This logic is currently absent on iOS, where UIKit text interaction gesture recognizers ask us for information
+ about the DOM at given locations, and we determine whether to allow text interaction gestures (such as long
+ pressing) to begin in `-textInteractionGesture:shouldBeginAtPoint:` and `-hasSelectablePositionAtPoint:`.
+ Ideally, we'd want to eventually unify these two codepaths for triggering text selection (and preferably just
+ have both go through EventHandler); in lieu of this, simply make the iOS codepath behave a little more like
+ macOS by adding a bit to allow text interaction gestures to bail in the case where at least one container of the
+ hit-tested element is both draggable and unselectable.
+
+ Test: editing/selection/ios/prefer-drag-over-text-selection.html
+
+ * Shared/ios/InteractionInformationAtPosition.h:
+ * Shared/ios/InteractionInformationAtPosition.mm:
+ (WebKit::InteractionInformationAtPosition::encode const):
+ (WebKit::InteractionInformationAtPosition::decode):
+
+ Add the new bit to InteractionInformationAtPosition.
+
+ * UIProcess/ios/WKContentViewInteraction.mm:
+ (-[WKContentView textInteractionGesture:shouldBeginAtPoint:]):
+
+ Avoid beginning text selection if the bit is set.
+
+ * WebProcess/WebPage/ios/WebPageIOS.mm:
+ (WebKit::selectionPositionInformation):
+
+ Move a comment about the ">= 97% of the document's visible area" heuristic next to the relevant code, and also
+ leave a FIXME mentioning that we should reconsider whether this is really needed; it seems that this was
+ originally intended to avoid ending up with an excessively large block selection, but block selection has been
+ removed entirely since iOS 12. That said, this logic may still be helping avoid situations where the text
+ selection is too aggressive on iOS and ends up selecting too much text.
+
2020-02-03 Chris Dumez <cdu...@apple.com>
Regression(r253224) WKUIDelegate.webViewDidClose may get called twice after calling _tryClose on the WKWebView
Modified: trunk/Source/WebKit/Shared/ios/InteractionInformationAtPosition.h (255602 => 255603)
--- trunk/Source/WebKit/Shared/ios/InteractionInformationAtPosition.h 2020-02-03 23:04:20 UTC (rev 255602)
+++ trunk/Source/WebKit/Shared/ios/InteractionInformationAtPosition.h 2020-02-03 23:09:19 UTC (rev 255603)
@@ -54,6 +54,7 @@
bool canBeValid { true };
bool nodeAtPositionHasDoubleClickHandler { false };
bool isSelectable { false };
+ bool prefersDraggingOverTextSelection { false };
bool isNearMarkedText { false };
bool touchCalloutEnabled { true };
bool isLink { false };
Modified: trunk/Source/WebKit/Shared/ios/InteractionInformationAtPosition.mm (255602 => 255603)
--- trunk/Source/WebKit/Shared/ios/InteractionInformationAtPosition.mm 2020-02-03 23:04:20 UTC (rev 255602)
+++ trunk/Source/WebKit/Shared/ios/InteractionInformationAtPosition.mm 2020-02-03 23:09:19 UTC (rev 255603)
@@ -46,6 +46,7 @@
encoder << canBeValid;
encoder << nodeAtPositionHasDoubleClickHandler;
encoder << isSelectable;
+ encoder << prefersDraggingOverTextSelection;
encoder << isNearMarkedText;
encoder << touchCalloutEnabled;
encoder << isLink;
@@ -101,6 +102,9 @@
if (!decoder.decode(result.isSelectable))
return false;
+ if (!decoder.decode(result.prefersDraggingOverTextSelection))
+ return false;
+
if (!decoder.decode(result.isNearMarkedText))
return false;
Modified: trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm (255602 => 255603)
--- trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm 2020-02-03 23:04:20 UTC (rev 255602)
+++ trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm 2020-02-03 23:09:19 UTC (rev 255603)
@@ -2528,6 +2528,9 @@
if (self.isFocusingElement)
return _positionInformation.elementContext && _positionInformation.elementContext->isSameElement(_focusedElementInformation.elementContext);
+ if (_positionInformation.prefersDraggingOverTextSelection)
+ return NO;
+
// If we're selecting something, don't activate highlight.
if (gesture == UIWKGestureLoupe && [self hasSelectablePositionAtPoint:point])
[self _cancelLongPressGestureRecognizer];
Modified: trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm (255602 => 255603)
--- trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm 2020-02-03 23:04:20 UTC (rev 255602)
+++ trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm 2020-02-03 23:09:19 UTC (rev 255603)
@@ -121,6 +121,7 @@
#import <WebCore/RenderView.h>
#import <WebCore/RuntimeApplicationChecks.h>
#import <WebCore/Settings.h>
+#import <WebCore/ShadowRoot.h>
#import <WebCore/SharedBuffer.h>
#import <WebCore/StyleProperties.h>
#import <WebCore/TextIndicator.h>
@@ -2709,7 +2710,6 @@
RenderObject* renderer = hitNode->renderer();
info.bounds = renderer->absoluteBoundingBoxRect(true);
- // We don't want to select blocks that are larger than 97% of the visible area of the document.
if (is<HTMLAttachmentElement>(*hitNode)) {
info.isAttachment = true;
HTMLAttachmentElement& attachment = downcast<HTMLAttachmentElement>(*hitNode);
@@ -2719,10 +2719,22 @@
info.url = ""
} else {
info.isSelectable = renderer->style().userSelect() != UserSelect::None;
+ // We don't want to select blocks that are larger than 97% of the visible area of the document.
+ // FIXME: Is this heuristic still needed, now that block selection has been removed?
if (info.isSelectable && !hitNode->isTextNode())
info.isSelectable = !isAssistableElement(*downcast<Element>(hitNode)) && !rectIsTooBigForSelection(info.bounds, *result.innerNodeFrame());
}
+ for (auto currentNode = makeRefPtr(hitNode); currentNode; currentNode = currentNode->parentOrShadowHostNode()) {
+ auto* renderer = currentNode->renderer();
+ if (!renderer)
+ continue;
+ auto& style = renderer->style();
+ if (style.userSelect() == UserSelect::None && style.userDrag() == UserDrag::Element) {
+ info.prefersDraggingOverTextSelection = true;
+ break;
+ }
+ }
#if PLATFORM(MACCATALYST)
bool isInsideFixedPosition;
VisiblePosition caretPosition(renderer->positionForPoint(request.point, nullptr));
