Context menu doesn't account for selection semantics
authorbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 22 Apr 2015 22:10:29 +0000 (22:10 +0000)
committerbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 22 Apr 2015 22:10:29 +0000 (22:10 +0000)
https://bugs.webkit.org/show_bug.cgi?id=143958
<rdar://problem/19735706>

Reviewed by Tim Horton.

Source/WebCore:

Before using the default word-only selection for context menus, check with the
lookup service to see if we can get a semantically appropriate selection.

* editing/EditingBehavior.h:
(WebCore::EditingBehavior::shouldSelectBasedOnDictionaryLookup): Added, so that we can
behavior correctly when using non-Mac editing behavior.
* editing/mac/DictionaryLookup.mm:
(WebCore::rangeForDictionaryLookupAtHitTestResult): Modified to honor standard WebKit
behavior when hit testing at end-of-line/end-of-paragraph, etc.
* page/EventHandler.cpp:
(WebCore::EventHandler::shouldAppendTrailingWhitespace): New helper function to share code.
(WebCore::EventHandler::selectClosestWordFromHitTestResultBasedOnLookup): Added.
(WebCore::EventHandler::selectClosestContextualWordFromMouseEvent): Added.
(WebCore::EventHandler::selectClosestContextualWordOrLinkFromMouseEvent): Renamed from selectClosestWordOrLinkFromMouseEvent.
Have this call the new 'selectClosestContextualWordFromMouseEvent' instead of the vanilla 'selectClosestWordFromMouseEvent'.
* page/EventHandler.h:
* page/mac/EventHandlerMac.mm:
(WebCore::EventHandler::selectClosestWordFromHitTestResultBasedOnLookup): Added.

LayoutTests:

* editing/selection/context-menu-text-selection-lookup-expected.txt: Added.
* editing/selection/context-menu-text-selection-lookup.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/editing/selection/context-menu-text-selection-lookup-expected.txt [new file with mode: 0644]
LayoutTests/editing/selection/context-menu-text-selection-lookup.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/editing/EditingBehavior.h
Source/WebCore/editing/mac/DictionaryLookup.mm
Source/WebCore/page/EventHandler.cpp
Source/WebCore/page/EventHandler.h
Source/WebCore/page/mac/EventHandlerMac.mm

index df4a6cf..4317745 100644 (file)
@@ -1,3 +1,14 @@
+2015-04-22  Brent Fulgham  <bfulgham@apple.com>
+
+        Context menu doesn't account for selection semantics
+        https://bugs.webkit.org/show_bug.cgi?id=143958
+        <rdar://problem/19735706>
+
+        Reviewed by Tim Horton.
+
+        * editing/selection/context-menu-text-selection-lookup-expected.txt: Added.
+        * editing/selection/context-menu-text-selection-lookup.html: Added.
+
 2015-04-22  Jeremy Jones  <jeremyj@apple.com>
 
         Expect failure on windows for treeitem-child-exposed test.
diff --git a/LayoutTests/editing/selection/context-menu-text-selection-lookup-expected.txt b/LayoutTests/editing/selection/context-menu-text-selection-lookup-expected.txt
new file mode 100644 (file)
index 0000000..30eaac5
--- /dev/null
@@ -0,0 +1,7 @@
+CONSOLE MESSAGE: line 31: ReferenceError: Can't find variable: expectedText
+This test checks that triggering the context menu selects/doesn't select using surround text as context (as platform-appropriate).
+
+New York
+RUNNING
+RUNNING
+RUNNING
diff --git a/LayoutTests/editing/selection/context-menu-text-selection-lookup.html b/LayoutTests/editing/selection/context-menu-text-selection-lookup.html
new file mode 100644 (file)
index 0000000..1c9920d
--- /dev/null
@@ -0,0 +1,56 @@
+<body onload="load()">
+<p>This test checks that triggering the context menu selects/doesn't select using surround text as context (as platform-appropriate).</p>
+<div id="text">New York</div>
+<div id="resultmac">RUNNING</div>
+<div id="resultwin">RUNNING</div>
+<div id="resultunix">RUNNING</div>
+</body>
+<script>
+function test(platform, selectionExpected, expectedText, result)
+{
+    // clear selection
+    window.getSelection().removeAllRanges();
+
+    internals.settings.setEditingBehavior(platform);
+
+    var text = document.getElementById("text");
+
+    var x = text.offsetParent.offsetLeft + text.offsetLeft + 4;
+    var y = text.offsetParent.offsetTop + text.offsetTop + text.offsetHeight / 2;
+
+    eventSender.mouseMoveTo(x, y);
+    eventSender.contextClick();
+    // esc key to kill the context menu
+    eventSender.keyDown(String.fromCharCode(0x001B), null);
+
+    var resultElement = document.getElementById(result);
+    var selectionType = window.getSelection().type;
+    if (selectionExpected) {
+        if (selectionType == "Range") {
+            var selectionText = window.getSelection().toString();
+            if (selectionText == expectedText)
+                resultElement.innerHTML = "SUCCESS";
+            else
+                resultElement.innerHTML = "FAILURE: We expected " + expectedText + ", but we got " + selectionText + ".";
+        } else
+            resultElement.innerHTML = "FAILURE: There should be a selection.";
+    } else {
+        if (selectionType == "Range")
+            resultElement.innerHTML = "FAILURE: There shouldn't be a selection.";
+        else
+            resultElement.innerHTML = "SUCCESS";
+    }
+}
+
+function load()
+{
+    if (!window.eventSender || !window.testRunner || !window.internals)
+        return;
+
+    testRunner.dumpAsText();
+
+    test('mac', true, 'New York', 'resultmac');
+    test('win', false, '', 'resultwin');
+    test('unix', false, '', 'resultunix');
+}
+</script>
index 2ac590e..ddde341 100644 (file)
@@ -1,3 +1,30 @@
+2015-04-22  Brent Fulgham  <bfulgham@apple.com>
+
+        Context menu doesn't account for selection semantics
+        https://bugs.webkit.org/show_bug.cgi?id=143958
+        <rdar://problem/19735706>
+
+        Reviewed by Tim Horton.
+
+        Before using the default word-only selection for context menus, check with the
+        lookup service to see if we can get a semantically appropriate selection.
+
+        * editing/EditingBehavior.h:
+        (WebCore::EditingBehavior::shouldSelectBasedOnDictionaryLookup): Added, so that we can
+        behavior correctly when using non-Mac editing behavior.
+        * editing/mac/DictionaryLookup.mm:
+        (WebCore::rangeForDictionaryLookupAtHitTestResult): Modified to honor standard WebKit
+        behavior when hit testing at end-of-line/end-of-paragraph, etc.
+        * page/EventHandler.cpp:
+        (WebCore::EventHandler::shouldAppendTrailingWhitespace): New helper function to share code.
+        (WebCore::EventHandler::selectClosestWordFromHitTestResultBasedOnLookup): Added.
+        (WebCore::EventHandler::selectClosestContextualWordFromMouseEvent): Added.
+        (WebCore::EventHandler::selectClosestContextualWordOrLinkFromMouseEvent): Renamed from selectClosestWordOrLinkFromMouseEvent.
+        Have this call the new 'selectClosestContextualWordFromMouseEvent' instead of the vanilla 'selectClosestWordFromMouseEvent'.
+        * page/EventHandler.h:
+        * page/mac/EventHandlerMac.mm:
+        (WebCore::EventHandler::selectClosestWordFromHitTestResultBasedOnLookup): Added.
+
 2015-04-21  Brent Fulgham  <bfulgham@apple.com>
 
         [Mac] Extend action menus to support PDF
index cfed2bc..56841dc 100644 (file)
@@ -89,6 +89,8 @@ public:
     // space when deleting password cause space been showed insecurely.
     bool shouldRebalanceWhiteSpacesInSecureField() const { return m_type != EditingIOSBehavior; }
 
+    bool shouldSelectBasedOnDictionaryLookup() const { return m_type == EditingMacBehavior; }
+
 private:
     EditingBehaviorType m_type;
 };
index f2fe624..b7e85d0 100644 (file)
@@ -143,6 +143,10 @@ PassRefPtr<Range> rangeForDictionaryLookupAtHitTestResult(const HitTestResult& h
     if (shouldUseSelection(position, selection))
         return rangeForDictionaryLookupForSelection(selection, options);
 
+    VisibleSelection selectionAccountingForLineRules = VisibleSelection(position);
+    selectionAccountingForLineRules.expandUsingGranularity(WordGranularity);
+    position = selectionAccountingForLineRules.start();
+
     // As context, we are going to use 250 characters of text before and after the point.
     RefPtr<Range> fullCharacterRange = rangeExpandedAroundPositionByCharacters(position, 250);
     if (!fullCharacterRange)
index c8bee9e..10b69d6 100644 (file)
@@ -588,19 +588,53 @@ void EventHandler::selectClosestWordFromHitTestResult(const HitTestResult& resul
     }
 }
 
+static AppendTrailingWhitespace shouldAppendTrailingWhitespace(const MouseEventWithHitTestResults& result, const Frame& frame)
+{
+    return (result.event().clickCount() == 2 && frame.editor().isSelectTrailingWhitespaceEnabled()) ? ShouldAppendTrailingWhitespace : DontAppendTrailingWhitespace;
+}
+
 void EventHandler::selectClosestWordFromMouseEvent(const MouseEventWithHitTestResults& result)
 {
-    if (m_mouseDownMayStartSelect) {
-        selectClosestWordFromHitTestResult(result.hitTestResult(),
-            (result.event().clickCount() == 2 && m_frame.editor().isSelectTrailingWhitespaceEnabled()) ? ShouldAppendTrailingWhitespace : DontAppendTrailingWhitespace);
-    }
+    if (m_mouseDownMayStartSelect)
+        selectClosestWordFromHitTestResult(result.hitTestResult(), shouldAppendTrailingWhitespace(result, m_frame));
 }
 
-void EventHandler::selectClosestWordOrLinkFromMouseEvent(const MouseEventWithHitTestResults& result)
+#if !PLATFORM(MAC)
+VisibleSelection EventHandler::selectClosestWordFromHitTestResultBasedOnLookup(const HitTestResult&)
+{
+    return VisibleSelection();
+}
+#endif
+    
+void EventHandler::selectClosestContextualWordFromMouseEvent(const MouseEventWithHitTestResults& mouseEvent)
+{
+    Node* targetNode = mouseEvent.targetNode();
+    const HitTestResult& result = mouseEvent.hitTestResult();
+    VisibleSelection newSelection;
+    bool appendTrailingWhitespace = shouldAppendTrailingWhitespace(mouseEvent, m_frame);
+    
+    if (targetNode && targetNode->renderer()) {
+        newSelection = selectClosestWordFromHitTestResultBasedOnLookup(result);
+        if (newSelection.isNone()) {
+            VisiblePosition pos(targetNode->renderer()->positionForPoint(result.localPoint(), nullptr));
+            if (pos.isNotNull()) {
+                newSelection = VisibleSelection(pos);
+                newSelection.expandUsingGranularity(WordGranularity);
+            }
+        }
+        
+        if (appendTrailingWhitespace == ShouldAppendTrailingWhitespace && newSelection.isRange())
+            newSelection.appendTrailingWhitespace();
+        
+        updateSelectionForMouseDownDispatchingSelectStart(targetNode, expandSelectionToRespectSelectOnMouseDown(*targetNode, newSelection), WordGranularity);
+    }
+}
+    
+void EventHandler::selectClosestContextualWordOrLinkFromMouseEvent(const MouseEventWithHitTestResults& result)
 {
     Element* urlElement = result.hitTestResult().URLElement();
     if (!urlElement || !isDraggableLink(*urlElement))
-        return selectClosestWordFromMouseEvent(result);
+        return selectClosestContextualWordFromMouseEvent(result);
 
     Node* targetNode = result.targetNode();
 
@@ -2798,7 +2832,7 @@ bool EventHandler::sendContextMenuEvent(const PlatformMouseEvent& event)
         // available for text selections.  But only if we're above text.
         && (m_frame.selection().selection().isContentEditable() || (mouseEvent.targetNode() && mouseEvent.targetNode()->isTextNode()))) {
         m_mouseDownMayStartSelect = true; // context menu events are always allowed to perform a selection
-        selectClosestWordOrLinkFromMouseEvent(mouseEvent);
+        selectClosestContextualWordOrLinkFromMouseEvent(mouseEvent);
     }
 
     swallowEvent = !dispatchMouseEvent(eventNames().contextmenuEvent, mouseEvent.targetNode(), true, 0, event, false);
index 854c40e..d9cfd15 100644 (file)
@@ -325,8 +325,10 @@ private:
     bool eventActivatedView(const PlatformMouseEvent&) const;
     bool updateSelectionForMouseDownDispatchingSelectStart(Node*, const VisibleSelection&, TextGranularity);
     void selectClosestWordFromHitTestResult(const HitTestResult&, AppendTrailingWhitespace);
+    VisibleSelection selectClosestWordFromHitTestResultBasedOnLookup(const HitTestResult&);
     void selectClosestWordFromMouseEvent(const MouseEventWithHitTestResults&);
-    void selectClosestWordOrLinkFromMouseEvent(const MouseEventWithHitTestResults&);
+    void selectClosestContextualWordFromMouseEvent(const MouseEventWithHitTestResults&);
+    void selectClosestContextualWordOrLinkFromMouseEvent(const MouseEventWithHitTestResults&);
 
     bool handleMouseDoubleClickEvent(const PlatformMouseEvent&);
 
index 6caa169..b620455 100644 (file)
@@ -31,7 +31,9 @@
 #include "Chrome.h"
 #include "ChromeClient.h"
 #include "DataTransfer.h"
+#include "DictionaryLookup.h"
 #include "DragController.h"
+#include "Editor.h"
 #include "EventNames.h"
 #include "FocusController.h"
 #include "Frame.h"
@@ -45,6 +47,7 @@
 #include "Page.h"
 #include "Pasteboard.h"
 #include "PlatformEventFactoryMac.h"
+#include "Range.h"
 #include "RenderLayer.h"
 #include "RenderListBox.h"
 #include "RenderWidget.h"
@@ -1007,4 +1010,16 @@ void EventHandler::platformNotifyIfEndGesture(const PlatformWheelEvent& wheelEve
 #endif
 }
 
+VisibleSelection EventHandler::selectClosestWordFromHitTestResultBasedOnLookup(const HitTestResult& result)
+{
+    if (!m_frame.editor().behavior().shouldSelectBasedOnDictionaryLookup())
+        return VisibleSelection();
+
+    NSDictionary *options = nil;
+    if (RefPtr<Range> range = rangeForDictionaryLookupAtHitTestResult(result, &options))
+        return VisibleSelection(range.get());
+
+    return VisibleSelection();
+}
+
 }