[tbb-commits] [tor-browser] 207/311: Bug 1754459 - Improve caret position selection for images. r=dholbert a=dmeehan

[gitolite role]

This is an automated email from the git hooks/post-receive script.

pierov pushed a commit to branch geckoview-99.0.1-11.0-1
in repository tor-browser.

commit 8bc2b1176fa7044585d5e29cb0f18b39ad4b75ba
Author: Emilio Cobos Álvarez <emilio at crisal.io>
AuthorDate: Wed Mar 16 23:32:44 2022 +0000

    Bug 1754459 - Improve caret position selection for images. r=dholbert a=dmeehan
    
    What saves us in the non-pointer-events: none case is silly, and is
    this loop that hits the draggable image:
    
      https://searchfox.org/mozilla-central/rev/fa69d8b248e6c1df670aa6b019e30ec37e6672be/layout/generic/nsIFrame.cpp#4752
    
    Needs tests (if it doesn't affect existing ones), but uploading for some
    initial feedback. I think this makes a lot more sense than the existing
    code, and works for editing and dragging the same way, so it's nice to
    have less special-cases.
    
    Differential Revision: https://phabricator.services.mozilla.com/D141242
---
 layout/generic/nsIFrame.cpp                     | 43 +++++++++++++++++-----
 layout/generic/test/mochitest.ini               |  1 +
 layout/generic/test/test_image_selection_3.html | 48 +++++++++++++++++++++++++
 3 files changed, 83 insertions(+), 9 deletions(-)

diff --git a/layout/generic/nsIFrame.cpp b/layout/generic/nsIFrame.cpp
index c4fc8ac39f81f..be46eba2110bc 100644
--- a/layout/generic/nsIFrame.cpp
+++ b/layout/generic/nsIFrame.cpp
@@ -5638,21 +5638,45 @@ static FrameTarget GetSelectionClosestFrameForBlock(nsIFrame* aFrame,
   return DrillDownToSelectionFrame(aFrame, true, aFlags);
 }
 
-// Use frame edge for grid, flex, table, and non-draggable & non-editable
-// image frames.
+// Use frame edge for grid, flex, table, and non-editable images. Choose the
+// edge based on the point position past the frame rect. If past the middle,
+// caret should be at end, otherwise at start. This behavior matches Blink.
+//
+// TODO(emilio): Can we use this code path for other replaced elements other
+// than images? Or even all other frames? We only get there when we didn't find
+// selectable children... At least one XUL test fails if we make this apply to
+// XUL labels. Also, editable images need _not_ to use the frame edge, see
+// below.
 static bool UseFrameEdge(nsIFrame* aFrame) {
   if (aFrame->IsFlexOrGridContainer() || aFrame->IsTableFrame()) {
     return true;
   }
-  if (static_cast<nsImageFrame*>(do_QueryFrame(aFrame))) {
-    return !nsContentUtils::ContentIsDraggable(aFrame->GetContent()) &&
-           !aFrame->GetContent()->IsEditable();
+  const nsImageFrame* image = do_QueryFrame(aFrame);
+  if (image && !aFrame->GetContent()->IsEditable()) {
+    // Editable images are a special-case because editing relies on clicking on
+    // an editable image selecting it, for it to show resizers.
+    return true;
   }
   return false;
 }
 
-static FrameTarget LastResortFrameTargetForFrame(nsIFrame* aFrame) {
-  return {aFrame, UseFrameEdge(aFrame), false};
+static FrameTarget LastResortFrameTargetForFrame(nsIFrame* aFrame,
+                                                 const nsPoint& aPoint) {
+  if (!UseFrameEdge(aFrame)) {
+    return {aFrame, false, false};
+  }
+  const auto& rect = aFrame->GetRectRelativeToSelf();
+  nscoord reference;
+  nscoord middle;
+  if (aFrame->GetWritingMode().IsVertical()) {
+    reference = aPoint.y;
+    middle = rect.Height() / 2;
+  } else {
+    reference = aPoint.x;
+    middle = rect.Width() / 2;
+  }
+  const bool afterFrame = reference > middle;
+  return {aFrame, true, afterFrame};
 }
 
 // GetSelectionClosestFrame is the helper function that calculates the closest
@@ -5661,7 +5685,8 @@ static FrameTarget LastResortFrameTargetForFrame(nsIFrame* aFrame) {
 // restricted environments.
 // Cannot handle overlapping frames correctly, so it should receive the output
 // of GetFrameForPoint
-// Guaranteed to return a valid FrameTarget
+// Guaranteed to return a valid FrameTarget.
+// aPoint is relative to aFrame.
 static FrameTarget GetSelectionClosestFrame(nsIFrame* aFrame,
                                             const nsPoint& aPoint,
                                             uint32_t aFlags) {
@@ -5685,7 +5710,7 @@ static FrameTarget GetSelectionClosestFrame(nsIFrame* aFrame,
     }
   }
 
-  return LastResortFrameTargetForFrame(aFrame);
+  return LastResortFrameTargetForFrame(aFrame, aPoint);
 }
 
 static nsIFrame::ContentOffsets OffsetsForSingleFrame(nsIFrame* aFrame,
diff --git a/layout/generic/test/mochitest.ini b/layout/generic/test/mochitest.ini
index 08b1e08a81a20..c640340ce87c2 100644
--- a/layout/generic/test/mochitest.ini
+++ b/layout/generic/test/mochitest.ini
@@ -108,6 +108,7 @@ support-files =
   frame_visibility_in_iframe_child.html
 [test_image_selection.html]
 [test_image_selection_2.html]
+[test_image_selection_3.html]
 [test_image_selection_in_contenteditable.html]
 [test_intrinsic_size_on_loading.html]
 [test_movement_by_characters.html]
diff --git a/layout/generic/test/test_image_selection_3.html b/layout/generic/test/test_image_selection_3.html
new file mode 100644
index 0000000000000..32a49b54d0bec
--- /dev/null
+++ b/layout/generic/test/test_image_selection_3.html
@@ -0,0 +1,48 @@
+<!doctype html>
+<title>Test for bug 1754459</title>
+<script src="/tests/SimpleTest/SimpleTest.js"></script>
+<script src="/tests/SimpleTest/EventUtils.js"></script>
+<link rel="stylesheet" href="/tests/SimpleTest/test.css"/>
+<style>
+  /* This avoids the draggable image check that our code does to avoid handling the mousedown. */
+  img { pointer-events: none }
+</style>
+<div id="block">
+  Some text <img width="100" height="100" id="image" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAGQAAAAyAQMAAACQ%2B%2Bz9AAAAA1BMVEUA%2FwA0XsCoAAAAD0lEQVQoFWNgGAWjYGgCAAK8AAEb3eOQAAAAAElFTkSuQmCC"> and more.
+</div>
+<script>
+const image = document.getElementById("image");
+const block = document.getElementById("block");
+const selection = window.getSelection();
+
+function clickOnImage(x, y) {
+  synthesizeMouse(image, x, y, {});
+}
+
+add_task(async function test_click_pointer_events_none() {
+  await SimpleTest.promiseFocus(window);
+  clickOnImage(10, 10);
+  ok(selection.isCollapsed, "Should be collapsed");
+  is(selection.focusNode, block, "Should be at block");
+  is(selection.focusOffset, 1, "Should be at start of image");
+
+  clickOnImage(60, 10);
+  ok(selection.isCollapsed, "Should be collapsed");
+  is(selection.focusNode, block, "Should be at block");
+  is(selection.focusOffset, 2, "Should be at end of image");
+});
+
+add_task(async function test_click_pointer_events_none_vertical() {
+  block.style.writingMode = "vertical-lr";
+  block.getBoundingClientRect();
+  clickOnImage(10, 10);
+  ok(selection.isCollapsed, "Should be collapsed");
+  is(selection.focusNode, block, "Should be at start of image");
+  is(selection.focusOffset, 1, "Should be at start of image");
+
+  clickOnImage(10, 60);
+  ok(selection.isCollapsed, "Should be collapsed");
+  is(selection.focusNode, block, "Should be at block");
+  is(selection.focusOffset, 2, "Should be at end of image");
+});
+</script>

-- 
To stop receiving notification emails like this one, please contact
the administrator of this repository.