Bug 1715603 - part 1: Don't extend selection into a link r=edgar

If middle button click with `Shift` key occurs, Chrome and Safari extend the selection in most cases. However, if the clicked position is in a link, Chrome does: * If editable, collapse selection into the link instead of extending selection. * If not editable, not extending selection and open tabs. We should follow this behavior for both backward compatibility and web-compat. Differential Revision: https://phabricator.services.mozilla.com/D119252
dothq · Jul 13, 2021 · 1125b62 · 1125b62
1 parent bbfaac5
commit 1125b62
Show file tree

Hide file tree

Showing 6 changed files with 114 additions and 46 deletions.
diff --git a/dom/base/Element.cpp b/dom/base/Element.cpp
@@ -3036,10 +3036,16 @@ nsresult Element::PostHandleEventForLinks(EventChainPostVisitor& aVisitor) {
 
   switch (aVisitor.mEvent->mMessage) {
     case eMouseDown: {
-      if (aVisitor.mEvent->AsMouseEvent()->mButton == MouseButton::ePrimary &&
-          OwnerDoc()->LinkHandlingEnabled()) {
-        aVisitor.mEvent->mFlags.mMultipleActionsPrevented = true;
+      if (!OwnerDoc()->LinkHandlingEnabled()) {
+        break;
+      }
+
+      WidgetMouseEvent* const mouseEvent = aVisitor.mEvent->AsMouseEvent();
+      mouseEvent->mFlags.mMultipleActionsPrevented |=
+          mouseEvent->mButton == MouseButton::ePrimary ||
+          mouseEvent->mButton == MouseButton::eMiddle;
 
+      if (mouseEvent->mButton == MouseButton::ePrimary) {
         if (IsInComposedDoc()) {
           if (RefPtr<nsFocusManager> fm = nsFocusManager::GetFocusManager()) {
             RefPtr<Element> kungFuDeathGrip(this);

diff --git a/layout/generic/nsIFrame.cpp b/layout/generic/nsIFrame.cpp
@@ -17,6 +17,7 @@
 #include "mozilla/ComputedStyle.h"
 #include "mozilla/DebugOnly.h"
 #include "mozilla/DisplayPortUtils.h"
+#include "mozilla/dom/AncestorIterator.h"
 #include "mozilla/dom/ElementInlines.h"
 #include "mozilla/dom/ImageTracker.h"
 #include "mozilla/dom/Selection.h"
@@ -4578,61 +4579,57 @@ nsIFrame::HandlePress(nsPresContext* aPresContext, WidgetGUIEvent* aEvent,
     return NS_OK;
   }
 
-  // We often get out of sync state issues with mousedown events that
-  // get interrupted by alerts/dialogs.
-  // Check with the ESM to see if we should process this one
-  if (!aPresContext->EventStateManager()->EventStatusOK(aEvent)) return NS_OK;
+  return MoveCaretToEventPoint(aPresContext, aEvent->AsMouseEvent(),
+                               aEventStatus);
+}
+
+nsresult nsIFrame::MoveCaretToEventPoint(nsPresContext* aPresContext,
+                                         WidgetMouseEvent* aMouseEvent,
+                                         nsEventStatus* aEventStatus) {
+  MOZ_ASSERT(aPresContext);
+  MOZ_ASSERT(aMouseEvent);
+  MOZ_ASSERT(aMouseEvent->mMessage == eMouseDown);
+  MOZ_ASSERT(aMouseEvent->mButton == MouseButton::ePrimary ||
+             aMouseEvent->mButton == MouseButton::eMiddle);
+  MOZ_ASSERT(aEventStatus);
+  MOZ_ASSERT(nsEventStatus_eConsumeNoDefault != *aEventStatus);
 
   mozilla::PresShell* presShell = aPresContext->GetPresShell();
   if (!presShell) {
     return NS_ERROR_FAILURE;
   }
 
-  WidgetMouseEvent* mouseEvent = aEvent->AsMouseEvent();
+  // We often get out of sync state issues with mousedown events that
+  // get interrupted by alerts/dialogs.
+  // Check with the ESM to see if we should process this one
+  if (!aPresContext->EventStateManager()->EventStatusOK(aMouseEvent)) {
+    return NS_OK;
+  }
 
-  if (!mouseEvent->IsAlt()) {
+  if (!aMouseEvent->IsAlt()) {
     for (nsIContent* content = mContent; content;
          content = content->GetFlattenedTreeParent()) {
       if (nsContentUtils::ContentIsDraggable(content) &&
           !content->IsEditable()) {
         // coordinate stuff is the fix for bug #55921
         if ((mRect - GetPosition())
                 .Contains(nsLayoutUtils::GetEventCoordinatesRelativeTo(
-                    mouseEvent, RelativeTo{this}))) {
+                    aMouseEvent, RelativeTo{this}))) {
           return NS_OK;
         }
       }
     }
   }
 
-  return MoveCaretToEventPoint(aPresContext, mouseEvent, aEventStatus);
-}
-
-nsresult nsIFrame::MoveCaretToEventPoint(nsPresContext* aPresContext,
-                                         WidgetMouseEvent* aMouseEvent,
-                                         nsEventStatus* aEventStatus) {
-  MOZ_ASSERT(aPresContext);
-  MOZ_ASSERT(aMouseEvent);
-  MOZ_ASSERT(aMouseEvent->mMessage == eMouseDown);
-  MOZ_ASSERT(aMouseEvent->mButton == MouseButton::ePrimary ||
-             aMouseEvent->mButton == MouseButton::eMiddle);
-  MOZ_ASSERT(aEventStatus);
-
-  mozilla::PresShell* presShell = aPresContext->GetPresShell();
-  if (!presShell) {
-    return NS_ERROR_FAILURE;
-  }
-
-  // if we are in Navigator and the click is in a draggable node, we don't want
+  // If we are in Navigator and the click is in a draggable node, we don't want
   // to start selection because we don't want to interfere with a potential
   // drag of said node and steal all its glory.
-  int16_t isEditor = presShell->GetSelectionFlags();
-  // weaaak. only the editor can display frame selection not just text and
-  // images
-  isEditor = isEditor == nsISelectionDisplay::DISPLAY_ALL;
+  const bool isEditor =
+      presShell->GetSelectionFlags() == nsISelectionDisplay::DISPLAY_ALL;
 
-  // Don't do something if it's moddle button down event.
-  bool isPrimaryButtonDown = aMouseEvent->mButton == MouseButton::ePrimary;
+  // Don't do something if it's middle button down event.
+  const bool isPrimaryButtonDown =
+      aMouseEvent->mButton == MouseButton::ePrimary;
 
   // check whether style allows selection
   // if not, don't tell selection the mouse event even occurred.
@@ -4768,6 +4765,16 @@ nsresult nsIFrame::MoveCaretToEventPoint(nsPresContext* aPresContext,
     // If "Shift" and "Ctrl" are both pressed, "Shift" is given precedence. This
     // mimics the old behaviour.
     if (aMouseEvent->IsShift()) {
+      // If clicked in a link when focused content is editable, we should
+      // collapse selection in the link for compatibility with Blink.
+      if (isEditor) {
+        nsCOMPtr<nsIURI> uri;
+        for (Element* element : mContent->InclusiveAncestorsOfType<Element>()) {
+          if (element->IsLink(getter_AddRefs(uri))) {
+            return nsFrameSelection::FocusMode::kCollapseToNewPoint;
+          }
+        }
+      }
       return nsFrameSelection::FocusMode::kExtendSelection;
     }
 

diff --git a/layout/generic/test/test_selection_changes_with_middle_mouse_button.html b/layout/generic/test/test_selection_changes_with_middle_mouse_button.html
@@ -21,7 +21,8 @@
 
 <div id="container">
 <span id="span1">first span.</span>
-<span id="span2">second span.</span>
+<span id="span2">second span.</span><br>
+<a id="link" href="#top">link.</a>
 <table>
 <tr><td id="td1">first td.</td></tr>
 <tr><td id="td2">second td.</td></tr>
@@ -107,7 +108,7 @@
       } else {
         is(pasteEvents.length, 1,
            aDescription + aAdditionalDescription + "paste event should be fired only once at mouse up");
-        is(pasteEvents[0].target, aExpectedPastEventTarget,
+        is(pasteEvents[0]?.target, aExpectedPastEventTarget,
            aDescription + aAdditionalDescription + "paste event should be fired on start of selection");
       }
     } else {
@@ -118,6 +119,7 @@
 
   let span1 = document.getElementById("span1");
   let span2 = document.getElementById("span2");
+  let link = document.getElementById("link");
 
   selection.removeAllRanges();
   doTest({target: span1}, {target: span1},
@@ -135,6 +137,22 @@
   doTest({target: span1, shiftKey: true}, {target: span1, shiftKey: true},
          span2.firstChild, span1.firstChild, span1,
          "Expanding selection with Shift key from span2 to span1: ");
+  selection.collapse(span1.firstChild, 3);
+  if (aEditable) {
+    // Collapse link into editable link.
+    doTest({target: link, shiftKey: true}, {target: link, shiftKey: true},
+          link.firstChild, link.firstChild,
+          link /* TODO: Perhaps, the "paste" event target should be the link */,
+          "Clicking an editable link with middle-button with Shift key when selection is collapsed in span1: ");
+  } else {
+    // Don't extend selection into a link.
+    link.onauxclick = event => event.preventDefault();
+    doTest({target: link, shiftKey: true}, {target: link, shiftKey: true},
+          span1.firstChild, span1.firstChild,
+          null /* due to the call of preventDefault */,
+          "Clicking a link with middle-button with Shift key when selection is collapsed in span1: ");
+    link.onauxclick = null;
+  }
   // "paste" event should be fired in the "start" of selection.
   selection.collapse(span1.firstChild, 3);
   doTest({target: span2, shiftKey: true}, {target: span2, shiftKey: true},

diff --git a/...election/contenteditable/modifying-selection-with-primary-mouse-button.tentative.html.ini b/...election/contenteditable/modifying-selection-with-primary-mouse-button.tentative.html.ini
@@ -1,10 +1,5 @@
 [modifying-selection-with-primary-mouse-button.tentative.html]
   [Primary click should move caret in an editable element]
     expected:
-      if not webrender and (os == "linux") and not fission and not debug and (processor == "x86_64"): [FAIL, PASS]
       FAIL
 
-  [Shift + Primary click should extend the selection]
-    expected:
-      if (os == "linux") and not fission and (processor == "x86_64") and debug and webrender and not swgl: [PASS, FAIL]
-      if (os == "linux") and not fission and (processor == "x86_64") and not debug and not webrender: [PASS, FAIL]
diff --git a/...sts/selection/contenteditable/modifying-selection-with-middle-mouse-button.tentative.html b/...sts/selection/contenteditable/modifying-selection-with-middle-mouse-button.tentative.html
@@ -20,7 +20,7 @@
 "use strict";
 
 var editor = document.querySelector("div[contenteditable]");
-var span1, span2;
+var span1, span2, link;
 var selection = getSelection();
 
 function preventDefault(event) {
@@ -30,9 +30,10 @@
 
 function resetEditor() {
   editor.innerHTML =
-    '<span id="span1">first span.</span><br><span id="span2">second span.</span>';
+    '<span id="span1">first span.</span><br><span id="span2">second span.</span><br><a id="link" href="#top">link.</a>';
   span1 = document.getElementById("span1");
   span2 = document.getElementById("span2");
+  link = document.getElementById("link");
 }
 
 promise_test(async () => {
@@ -136,6 +137,26 @@
     "Selection#focusNode should be in the second <span> element which was clicked by middle button");
 }, "Shift + Middle click should extend the selection");
 
+promise_test(async () => {
+  resetEditor();
+  editor.focus();
+  selection.collapse(span1.firstChild, 2);
+  let actions = new test_driver.Actions();
+  await actions
+    .pointerMove(0, 0)
+    .pointerMove(0, 0, {origin: link})
+    .keyDown("\uE008")
+    .pointerDown({button: actions.ButtonType.MIDDLE})
+    .pointerUp({button: actions.ButtonType.MIDDLE})
+    .keyUp("\uE008")
+    .send();
+
+  assert_equals(selection.focusNode, link.firstChild,
+    "Selection#focusNode should be in the <a href> element which was clicked by middle button");
+  assert_true(selection.isCollapsed,
+    "Selection#isCollapsed should be true");
+}, "Shift + Middle click in a link shouldn't extend the selection");
+
 promise_test(async () => {
   resetEditor();
   editor.focus();

diff --git a/...ts/selection/contenteditable/modifying-selection-with-primary-mouse-button.tentative.html b/...ts/selection/contenteditable/modifying-selection-with-primary-mouse-button.tentative.html
@@ -20,7 +20,7 @@
 "use strict";
 
 var editor = document.querySelector("div[contenteditable]");
-var span1, span2;
+var span1, span2, link;
 var selection = getSelection();
 
 function preventDefault(event) {
@@ -30,9 +30,10 @@
 
 function resetEditor() {
   editor.innerHTML =
-    '<span id="span1">first span.</span><br><span id="span2">second span.</span>';
+    '<span id="span1">first span.</span><br><span id="span2">second span.</span><br><a id="link" href="#top">link.</a>';
   span1 = document.getElementById("span1");
   span2 = document.getElementById("span2");
+  link = document.getElementById("link");
 }
 
 promise_test(async () => {
@@ -136,6 +137,26 @@
     "Selection#focusNode should be in the second <span> element which was clicked by primary button");
 }, "Shift + Primary click should extend the selection");
 
+promise_test(async () => {
+  resetEditor();
+  editor.focus();
+  selection.collapse(span1.firstChild, 2);
+  let actions = new test_driver.Actions();
+  await actions
+    .pointerMove(0, 0)
+    .pointerMove(0, 0, {origin: link})
+    .keyDown("\uE008")
+    .pointerDown({button: actions.ButtonType.MIDDLE})
+    .pointerUp({button: actions.ButtonType.MIDDLE})
+    .keyUp("\uE008")
+    .send();
+
+  assert_equals(selection.focusNode, link.firstChild,
+    "Selection#focusNode should be in the <a href> element which was clicked by primary button");
+  assert_true(selection.isCollapsed,
+    "Selection#isCollapsed should be true");
+}, "Shift + Primary click in a link shouldn't extend the selection");
+
 promise_test(async () => {
   resetEditor();
   editor.focus();