<attachment> shouldn't use "user-select: all"
authortimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 12 Mar 2015 00:33:07 +0000 (00:33 +0000)
committertimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 12 Mar 2015 00:33:07 +0000 (00:33 +0000)
https://bugs.webkit.org/show_bug.cgi?id=142453

Reviewed by Darin Adler.

It turns out that "user-select: all" is rife with bugs; in lieu of fixing them
all (at least for now), let's not use "user-select: all" in the default stylesheet
for <attachment>. It's really overkill anyway, since <attachment> can't have children.
The only "user-select: all" behavior we actually want is select-on-click.
So, we'll implement that in a slightly different way.

Tests: fast/attachment/attachment-select-on-click-inside-user-select-all.html
       fast/attachment/attachment-select-on-click.html

* css/html.css:
(attachment):
No more "user-select: all".

(attachment:focus): Deleted.
We stopped using attachment focus a while back and forgot to remove this.

* dom/Node.h:
(WebCore::Node::shouldSelectOnMouseDown):
Add a virtual function that Node subclasses can override to indicate they
should be selected on mouse down.

* html/HTMLAttachmentElement.h:
Override the aforementioned virtual function; <attachment> should always
be selected on mouse down.

* page/EventHandler.cpp:
(WebCore::nodeToSelectOnMouseDownForNode):
Determine which node should be selected when a mousedown hits the given node.
If there's any "user-select: all", we go with the outermost "user-select: all".
Otherwise, we give the node a chance to say that it wants to be selected itself.

(WebCore::expandSelectionToRespectSelectOnMouseDown):
Rename this function, it's not just about "user-select: all" anymore.
Make use of nodeToSelectOnMouseDownForNode.

(WebCore::EventHandler::selectClosestWordFromHitTestResult):
(WebCore::EventHandler::selectClosestWordOrLinkFromMouseEvent):
(WebCore::EventHandler::handleMousePressEventTripleClick):
(WebCore::EventHandler::handleMousePressEventSingleClick):
(WebCore::expandSelectionToRespectUserSelectAll): Deleted.
Adjust to the new names.

* fast/attachment/attachment-select-on-click-inside-user-select-all.html: Added.
* fast/attachment/attachment-select-on-click.html: Added.
* platform/mac/fast/attachment/attachment-select-on-click-expected.png: Added.
* platform/mac/fast/attachment/attachment-select-on-click-expected.txt: Added.
* platform/mac/fast/attachment/attachment-select-on-click-inside-user-select-all-expected.png: Added.
* platform/mac/fast/attachment/attachment-select-on-click-inside-user-select-all-expected.txt: Added.
* platform/mac-mavericks/fast/attachment/attachment-select-on-click-inside-user-select-all-expected.txt: Added.
* platform/mac-mavericks/fast/attachment/attachment-select-on-click-expected.txt: Added.
Add two tests. One, for the basic functionality of clicking on an
<attachment> to select it. The second, to test that clicking on an
<attachment> inside a larger "user-select: all" element still selects
the whole "user-select: all" element.

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

14 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/attachment/attachment-select-on-click-inside-user-select-all.html [new file with mode: 0644]
LayoutTests/fast/attachment/attachment-select-on-click.html [new file with mode: 0644]
LayoutTests/platform/mac-mavericks/fast/attachment/attachment-select-on-click-expected.txt [new file with mode: 0644]
LayoutTests/platform/mac-mavericks/fast/attachment/attachment-select-on-click-inside-user-select-all-expected.txt [new file with mode: 0644]
LayoutTests/platform/mac/fast/attachment/attachment-select-on-click-expected.png [new file with mode: 0644]
LayoutTests/platform/mac/fast/attachment/attachment-select-on-click-expected.txt [new file with mode: 0644]
LayoutTests/platform/mac/fast/attachment/attachment-select-on-click-inside-user-select-all-expected.png [new file with mode: 0644]
LayoutTests/platform/mac/fast/attachment/attachment-select-on-click-inside-user-select-all-expected.txt [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/css/html.css
Source/WebCore/dom/Node.h
Source/WebCore/html/HTMLAttachmentElement.h
Source/WebCore/page/EventHandler.cpp

index df7bb1d..af48ec6 100644 (file)
@@ -1,3 +1,23 @@
+2015-03-11  Timothy Horton  <timothy_horton@apple.com>
+
+        <attachment> shouldn't use "user-select: all"
+        https://bugs.webkit.org/show_bug.cgi?id=142453
+
+        Reviewed by Darin Adler.
+
+        * fast/attachment/attachment-select-on-click-inside-user-select-all.html: Added.
+        * fast/attachment/attachment-select-on-click.html: Added.
+        * platform/mac/fast/attachment/attachment-select-on-click-expected.png: Added.
+        * platform/mac/fast/attachment/attachment-select-on-click-expected.txt: Added.
+        * platform/mac/fast/attachment/attachment-select-on-click-inside-user-select-all-expected.png: Added.
+        * platform/mac/fast/attachment/attachment-select-on-click-inside-user-select-all-expected.txt: Added.
+        * platform/mac-mavericks/fast/attachment/attachment-select-on-click-inside-user-select-all-expected.txt: Added.
+        * platform/mac-mavericks/fast/attachment/attachment-select-on-click-expected.txt: Added.
+        Add two tests. One, for the basic functionality of clicking on an
+        <attachment> to select it. The second, to test that clicking on an
+        <attachment> inside a larger "user-select: all" element still selects
+        the whole "user-select: all" element.
+
 2015-03-11  Matthew Mirman  <mmirman@apple.com>
 
         Update windows test results
diff --git a/LayoutTests/fast/attachment/attachment-select-on-click-inside-user-select-all.html b/LayoutTests/fast/attachment/attachment-select-on-click-inside-user-select-all.html
new file mode 100644 (file)
index 0000000..57e7251
--- /dev/null
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<html>
+<body onload="runTest()">
+<div style="-webkit-user-select: all;">text before <attachment id="attachment"></attachment> text after</div>
+<script>
+var file;
+if (window.internals)
+    file = window.internals.createFile("resources/test-file.txt");
+
+var attachment = document.getElementById('attachment');
+attachment.file = file;
+
+window.onload = function () {
+    if (!window.eventSender)
+        return;
+    eventSender.mouseMoveTo(attachment.offsetLeft + 10, attachment.offsetTop + 10);
+    eventSender.mouseDown(0);
+    eventSender.mouseUp(0);
+}
+</script>
+</body>
+</html>
diff --git a/LayoutTests/fast/attachment/attachment-select-on-click.html b/LayoutTests/fast/attachment/attachment-select-on-click.html
new file mode 100644 (file)
index 0000000..bc5e18f
--- /dev/null
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<html>
+<body onload="runTest()">
+<div>text before <attachment id="attachment"></attachment> text after</div>
+<script>
+var file;
+if (window.internals)
+    file = window.internals.createFile("resources/test-file.txt");
+
+var attachment = document.getElementById('attachment');
+attachment.file = file;
+
+window.onload = function () {
+    if (!window.eventSender)
+        return;
+    eventSender.mouseMoveTo(attachment.offsetLeft + 10, attachment.offsetTop + 10);
+    eventSender.mouseDown(0);
+    eventSender.mouseUp(0);
+}
+</script>
+</body>
+</html>
diff --git a/LayoutTests/platform/mac-mavericks/fast/attachment/attachment-select-on-click-expected.txt b/LayoutTests/platform/mac-mavericks/fast/attachment/attachment-select-on-click-expected.txt
new file mode 100644 (file)
index 0000000..8266c5f
--- /dev/null
@@ -0,0 +1,13 @@
+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600
+layer at (0,0) size 800x93
+  RenderBlock {HTML} at (0,0) size 800x93
+    RenderBody {BODY} at (8,8) size 784x77
+      RenderBlock {DIV} at (0,0) size 784x77
+        RenderText {#text} at (0,54) size 73x18
+          text run at (0,54) width 73: "text before "
+        RenderAttachment {ATTACHMENT} at (72,0) size 79x77
+        RenderText {#text} at (150,54) size 63x18
+          text run at (150,54) width 63: " text after"
+selection start: position 0 of child 1 {ATTACHMENT} of child 1 {DIV} of body
+selection end:   position 1 of child 1 {ATTACHMENT} of child 1 {DIV} of body
diff --git a/LayoutTests/platform/mac-mavericks/fast/attachment/attachment-select-on-click-inside-user-select-all-expected.txt b/LayoutTests/platform/mac-mavericks/fast/attachment/attachment-select-on-click-inside-user-select-all-expected.txt
new file mode 100644 (file)
index 0000000..2f81f0d
--- /dev/null
@@ -0,0 +1,13 @@
+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600
+layer at (0,0) size 800x93
+  RenderBlock {HTML} at (0,0) size 800x93
+    RenderBody {BODY} at (8,8) size 784x77
+      RenderBlock {DIV} at (0,0) size 784x77
+        RenderText {#text} at (0,54) size 73x18
+          text run at (0,54) width 73: "text before "
+        RenderAttachment {ATTACHMENT} at (72,0) size 79x77
+        RenderText {#text} at (150,54) size 63x18
+          text run at (150,54) width 63: " text after"
+selection start: position 0 of child 0 {#text} of child 1 {DIV} of body
+selection end:   position 11 of child 2 {#text} of child 1 {DIV} of body
diff --git a/LayoutTests/platform/mac/fast/attachment/attachment-select-on-click-expected.png b/LayoutTests/platform/mac/fast/attachment/attachment-select-on-click-expected.png
new file mode 100644 (file)
index 0000000..544dd74
Binary files /dev/null and b/LayoutTests/platform/mac/fast/attachment/attachment-select-on-click-expected.png differ
diff --git a/LayoutTests/platform/mac/fast/attachment/attachment-select-on-click-expected.txt b/LayoutTests/platform/mac/fast/attachment/attachment-select-on-click-expected.txt
new file mode 100644 (file)
index 0000000..ea882ae
--- /dev/null
@@ -0,0 +1,13 @@
+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600
+layer at (0,0) size 800x93
+  RenderBlock {HTML} at (0,0) size 800x93
+    RenderBody {BODY} at (8,8) size 784x77
+      RenderBlock {DIV} at (0,0) size 784x77
+        RenderText {#text} at (0,54) size 73x18
+          text run at (0,54) width 73: "text before "
+        RenderAttachment {ATTACHMENT} at (72,0) size 75x77
+        RenderText {#text} at (146,54) size 63x18
+          text run at (146,54) width 63: " text after"
+selection start: position 0 of child 1 {ATTACHMENT} of child 1 {DIV} of body
+selection end:   position 1 of child 1 {ATTACHMENT} of child 1 {DIV} of body
diff --git a/LayoutTests/platform/mac/fast/attachment/attachment-select-on-click-inside-user-select-all-expected.png b/LayoutTests/platform/mac/fast/attachment/attachment-select-on-click-inside-user-select-all-expected.png
new file mode 100644 (file)
index 0000000..003b2a1
Binary files /dev/null and b/LayoutTests/platform/mac/fast/attachment/attachment-select-on-click-inside-user-select-all-expected.png differ
diff --git a/LayoutTests/platform/mac/fast/attachment/attachment-select-on-click-inside-user-select-all-expected.txt b/LayoutTests/platform/mac/fast/attachment/attachment-select-on-click-inside-user-select-all-expected.txt
new file mode 100644 (file)
index 0000000..65dff2b
--- /dev/null
@@ -0,0 +1,13 @@
+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600
+layer at (0,0) size 800x93
+  RenderBlock {HTML} at (0,0) size 800x93
+    RenderBody {BODY} at (8,8) size 784x77
+      RenderBlock {DIV} at (0,0) size 784x77
+        RenderText {#text} at (0,54) size 73x18
+          text run at (0,54) width 73: "text before "
+        RenderAttachment {ATTACHMENT} at (72,0) size 75x77
+        RenderText {#text} at (146,54) size 63x18
+          text run at (146,54) width 63: " text after"
+selection start: position 0 of child 0 {#text} of child 1 {DIV} of body
+selection end:   position 11 of child 2 {#text} of child 1 {DIV} of body
index abc909c..e843b4c 100644 (file)
@@ -1,3 +1,52 @@
+2015-03-11  Timothy Horton  <timothy_horton@apple.com>
+
+        <attachment> shouldn't use "user-select: all"
+        https://bugs.webkit.org/show_bug.cgi?id=142453
+
+        Reviewed by Darin Adler.
+
+        It turns out that "user-select: all" is rife with bugs; in lieu of fixing them
+        all (at least for now), let's not use "user-select: all" in the default stylesheet
+        for <attachment>. It's really overkill anyway, since <attachment> can't have children.
+        The only "user-select: all" behavior we actually want is select-on-click.
+        So, we'll implement that in a slightly different way.
+
+        Tests: fast/attachment/attachment-select-on-click-inside-user-select-all.html
+               fast/attachment/attachment-select-on-click.html
+
+        * css/html.css:
+        (attachment):
+        No more "user-select: all".
+
+        (attachment:focus): Deleted.
+        We stopped using attachment focus a while back and forgot to remove this.
+
+        * dom/Node.h:
+        (WebCore::Node::shouldSelectOnMouseDown):
+        Add a virtual function that Node subclasses can override to indicate they
+        should be selected on mouse down.
+
+        * html/HTMLAttachmentElement.h:
+        Override the aforementioned virtual function; <attachment> should always
+        be selected on mouse down.
+
+        * page/EventHandler.cpp:
+        (WebCore::nodeToSelectOnMouseDownForNode):
+        Determine which node should be selected when a mousedown hits the given node.
+        If there's any "user-select: all", we go with the outermost "user-select: all".
+        Otherwise, we give the node a chance to say that it wants to be selected itself.
+
+        (WebCore::expandSelectionToRespectSelectOnMouseDown):
+        Rename this function, it's not just about "user-select: all" anymore.
+        Make use of nodeToSelectOnMouseDownForNode.
+
+        (WebCore::EventHandler::selectClosestWordFromHitTestResult):
+        (WebCore::EventHandler::selectClosestWordOrLinkFromMouseEvent):
+        (WebCore::EventHandler::handleMousePressEventTripleClick):
+        (WebCore::EventHandler::handleMousePressEventSingleClick):
+        (WebCore::expandSelectionToRespectUserSelectAll): Deleted.
+        Adjust to the new names.
+
 2015-03-11  Geoffrey Garen  <ggaren@apple.com>
 
         Users of Heap::deprecatedReportExtraMemory should switch to reportExtraMemoryAllocated+reportExtraMemoryVisited
index e4a78f7..f61bf79 100644 (file)
@@ -1211,11 +1211,6 @@ applet, embed, object, img {
 #if defined(ENABLE_ATTACHMENT_ELEMENT) && ENABLE_ATTACHMENT_ELEMENT
 attachment {
     -webkit-appearance: attachment;
-    -webkit-user-select: all;
-}
-
-attachment:focus {
-    outline: none;
 }
 #endif
 
index 71afc4d..6e10ffc 100644 (file)
@@ -431,6 +431,8 @@ public:
     // Whether or not a selection can be started in this object
     virtual bool canStartSelection() const;
 
+    virtual bool shouldSelectOnMouseDown() { return false; }
+
     // Getting points into and out of screen space
     FloatPoint convertToPage(const FloatPoint&) const;
     FloatPoint convertFromPage(const FloatPoint&) const;
index 323c347..5829687 100644 (file)
@@ -48,6 +48,7 @@ private:
 
     virtual RenderPtr<RenderElement> createElementRenderer(Ref<RenderStyle>&&) override;
 
+    virtual bool shouldSelectOnMouseDown() override { return true; }
     virtual bool canContainRangeEndPoint() const override { return false; }
     virtual void parseAttribute(const QualifiedName&, const AtomicString&) override;
     
index a5f6831..2985947 100644 (file)
@@ -522,22 +522,30 @@ static inline bool dispatchSelectStart(Node* node)
     return node->dispatchEvent(Event::create(eventNames().selectstartEvent, true, true));
 }
 
-static VisibleSelection expandSelectionToRespectUserSelectAll(Node* targetNode, const VisibleSelection& selection)
+static Node* nodeToSelectOnMouseDownForNode(Node& targetNode)
 {
 #if ENABLE(USERSELECT_ALL)
-    Node* rootUserSelectAll = Position::rootUserSelectAllForNode(targetNode);
-    if (!rootUserSelectAll)
+    if (Node* rootUserSelectAll = Position::rootUserSelectAllForNode(&targetNode))
+        return rootUserSelectAll;
+#endif
+
+    if (targetNode.shouldSelectOnMouseDown())
+        return &targetNode;
+
+    return nullptr;
+}
+
+static VisibleSelection expandSelectionToRespectSelectOnMouseDown(Node& targetNode, const VisibleSelection& selection)
+{
+    Node* nodeToSelect = nodeToSelectOnMouseDownForNode(targetNode);
+    if (!nodeToSelect)
         return selection;
 
     VisibleSelection newSelection(selection);
-    newSelection.setBase(positionBeforeNode(rootUserSelectAll).upstream(CanCrossEditingBoundary));
-    newSelection.setExtent(positionAfterNode(rootUserSelectAll).downstream(CanCrossEditingBoundary));
+    newSelection.setBase(positionBeforeNode(nodeToSelect).upstream(CanCrossEditingBoundary));
+    newSelection.setExtent(positionAfterNode(nodeToSelect).downstream(CanCrossEditingBoundary));
 
     return newSelection;
-#else
-    UNUSED_PARAM(targetNode);
-    return selection;
-#endif
 }
 
 bool EventHandler::updateSelectionForMouseDownDispatchingSelectStart(Node* targetNode, const VisibleSelection& selection, TextGranularity granularity)
@@ -575,7 +583,7 @@ void EventHandler::selectClosestWordFromHitTestResult(const HitTestResult& resul
         if (appendTrailingWhitespace == ShouldAppendTrailingWhitespace && newSelection.isRange())
             newSelection.appendTrailingWhitespace();
 
-        updateSelectionForMouseDownDispatchingSelectStart(targetNode, expandSelectionToRespectUserSelectAll(targetNode, newSelection), WordGranularity);
+        updateSelectionForMouseDownDispatchingSelectStart(targetNode, expandSelectionToRespectSelectOnMouseDown(targetNode, newSelection), WordGranularity);
     }
 }
 
@@ -601,7 +609,7 @@ void EventHandler::selectClosestWordOrLinkFromMouseEvent(const MouseEventWithHit
         if (pos.isNotNull() && pos.deepEquivalent().deprecatedNode()->isDescendantOf(URLElement))
             newSelection = VisibleSelection::selectionFromContentsOfNode(URLElement);
 
-        updateSelectionForMouseDownDispatchingSelectStart(targetNode, expandSelectionToRespectUserSelectAll(targetNode, newSelection), WordGranularity);
+        updateSelectionForMouseDownDispatchingSelectStart(targetNode, expandSelectionToRespectSelectOnMouseDown(targetNode, newSelection), WordGranularity);
     }
 }
 
@@ -639,7 +647,7 @@ bool EventHandler::handleMousePressEventTripleClick(const MouseEventWithHitTestR
         newSelection.expandUsingGranularity(ParagraphGranularity);
     }
 
-    return updateSelectionForMouseDownDispatchingSelectStart(targetNode, expandSelectionToRespectUserSelectAll(targetNode, newSelection), ParagraphGranularity);
+    return updateSelectionForMouseDownDispatchingSelectStart(targetNode, expandSelectionToRespectSelectOnMouseDown(targetNode, newSelection), ParagraphGranularity);
 }
 
 static int textDistance(const Position& start, const Position& end)
@@ -677,7 +685,7 @@ bool EventHandler::handleMousePressEventSingleClick(const MouseEventWithHitTestR
     TextGranularity granularity = CharacterGranularity;
 
     if (extendSelection && newSelection.isCaretOrRange()) {
-        VisibleSelection selectionInUserSelectAll = expandSelectionToRespectUserSelectAll(targetNode, VisibleSelection(pos));
+        VisibleSelection selectionInUserSelectAll = expandSelectionToRespectSelectOnMouseDown(targetNode, VisibleSelection(pos));
         if (selectionInUserSelectAll.isRange()) {
             if (comparePositions(selectionInUserSelectAll.start(), newSelection.start()) < 0)
                 pos = selectionInUserSelectAll.start();
@@ -704,7 +712,7 @@ bool EventHandler::handleMousePressEventSingleClick(const MouseEventWithHitTestR
             newSelection.expandUsingGranularity(m_frame.selection().granularity());
         }
     } else
-        newSelection = expandSelectionToRespectUserSelectAll(targetNode, visiblePos);
+        newSelection = expandSelectionToRespectSelectOnMouseDown(targetNode, visiblePos);
 
     bool handled = updateSelectionForMouseDownDispatchingSelectStart(targetNode, newSelection, granularity);