[PDF Ink Signatures] Update cursor for text highlighting

Update the mouse cursor so that it shows an I-beam when hovering over
text with a highlighter and when selecting text.

Bug: 410761158
Change-Id: Ibd6888ab3ef697726c590a1756ae5778a9936554
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6459883
Commit-Queue: Andy Phan <andyphan@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1456729}

pdf/pdf_ink_module.cc

@@ -54,6 +54,7 @@
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/base/cursor/cursor.h"
+#include "ui/base/cursor/mojom/cursor_type.mojom.h"
 #include "ui/gfx/geometry/point.h"
 #include "ui/gfx/geometry/point_conversions.h"
 #include "ui/gfx/geometry/point_f.h"
@@ -370,6 +371,10 @@ bool PdfInkModule::OnMessage(const base::Value::Dict& message) {
 }
 
 void PdfInkModule::OnGeometryChanged() {
+  // If the highlighter tool is selected, and zooming moves the cursor onto
+  // text, the cursor should be an I-beam, but it will instead be the drawing
+  // cursor until a mousemove event occurs. There is not a way to get the new
+  // mouse position on geometry change.
   MaybeSetCursor();
 }
 
@@ -470,11 +475,11 @@ bool PdfInkModule::OnMouseUp(const blink::WebMouseEvent& event) {
     return false;
   }
 
+  gfx::PointF position = event.PositionInWidget();
   if (features::kPdfInk2TextHighlighting.Get() && is_text_highlighting()) {
-    return FinishTextHighlight();
+    return FinishTextHighlight(position);
   }
 
-  gfx::PointF position = event.PositionInWidget();
   return is_drawing_stroke()
              ? FinishStroke(position, event.TimeStamp(),
                             ink::StrokeInput::ToolType::kMouse)
@@ -484,10 +489,8 @@ bool PdfInkModule::OnMouseUp(const blink::WebMouseEvent& event) {
 bool PdfInkModule::OnMouseMove(const blink::WebMouseEvent& event) {
   CHECK(enabled());
 
-  // TODO(crbug.com/342445982): Set the cursor for hovering over text with the
-  // highlighter brush while not drawing.
-
   gfx::PointF position = event.PositionInWidget();
+
   bool still_interacting_with_ink =
       event.GetModifiers() & blink::WebInputEvent::kLeftButtonDown;
   if (still_interacting_with_ink) {
@@ -507,6 +510,7 @@ bool PdfInkModule::OnMouseMove(const blink::WebMouseEvent& event) {
   // that now, and compensate by synthesizing a mouse-up input event at the
   // last known input position.  Intentionally do not use `position`.
   if (is_drawing_stroke()) {
+    MaybeSetCursorOnMouseMove(position);
     DrawingStrokeState& state = drawing_stroke_state();
     if (!state.input_last_event.has_value()) {
       // Ignore when not drawing.
@@ -772,7 +776,12 @@ bool PdfInkModule::FinishStroke(const gfx::PointF& position,
   state.page_index = -1;
   state.input_last_event.reset();
 
-  if (MaybeSetDrawingBrush()) {
+  bool set_drawing_brush = MaybeSetDrawingBrush();
+  if (features::kPdfInk2TextHighlighting.Get() &&
+      state.brush_type == PdfInkBrush::Type::kHighlighter &&
+      client_->IsSelectableTextOrLinkArea(position)) {
+    client_->UpdateInkCursor(ui::mojom::CursorType::kIBeam);
+  } else if (set_drawing_brush) {
     MaybeSetCursor();
   }
 
@@ -984,7 +993,7 @@ bool PdfInkModule::ContinueTextHighlight(const gfx::PointF& position) {
   return true;
 }
 
-bool PdfInkModule::FinishTextHighlight() {
+bool PdfInkModule::FinishTextHighlight(const gfx::PointF& position) {
   CHECK(is_text_highlighting());
 
   auto& highlight_strokes = text_highlight_state().highlight_strokes;
@@ -1018,6 +1027,10 @@ bool PdfInkModule::FinishTextHighlight() {
   drawing_stroke_state().brush_type = PdfInkBrush::Type::kHighlighter;
 
   client_->ClearSelection();
+
+  if (!client_->IsSelectableTextOrLinkArea(position)) {
+    MaybeSetCursor();
+  }
   return true;
 }
 
@@ -1609,7 +1622,6 @@ void PdfInkModule::MaybeSetCursor() {
   }
 
   if (features::kPdfInk2TextHighlighting.Get() && is_text_highlighting()) {
-    // TODO(crbug.com/342445982): Set the cursor for text highlighting.
     return;
   }
 
@@ -1633,6 +1645,25 @@ void PdfInkModule::MaybeSetCursor() {
       ui::Cursor::NewCustom(std::move(bitmap), std::move(hotspot)));
 }
 
+void PdfInkModule::MaybeSetCursorOnMouseMove(const gfx::PointF& position) {
+  if (!features::kPdfInk2TextHighlighting.Get()) {
+    return;
+  }
+
+  CHECK(is_drawing_stroke());
+  if (drawing_stroke_state().brush_type != PdfInkBrush::Type::kHighlighter ||
+      !client_->IsSelectableTextOrLinkArea(position)) {
+    if (client_->GetCursor().type() == ui::mojom::CursorType::kIBeam) {
+      MaybeSetCursor();
+    }
+    return;
+  }
+
+  if (client_->GetCursor().type() != ui::mojom::CursorType::kIBeam) {
+    client_->UpdateInkCursor(ui::mojom::CursorType::kIBeam);
+  }
+}
+
 PdfInkModule::DrawingStrokeState::DrawingStrokeState() = default;
 
 PdfInkModule::DrawingStrokeState::~DrawingStrokeState() = default;

pdf/pdf_ink_module.h

@@ -356,7 +356,7 @@ class PdfInkModule {
                           int click_count,
                           base::TimeTicks timestamp);
   bool ContinueTextHighlight(const gfx::PointF& position);
-  bool FinishTextHighlight();
+  bool FinishTextHighlight(const gfx::PointF& position);
 
   // Returns a highlighter stroke that matches the position and size of
   // `selection_rect`. `selection_rect` must be in screen coordinates.
@@ -455,8 +455,14 @@ class PdfInkModule {
   void ApplyUndoRedoDiscards(
       const PdfInkUndoRedoModel::DiscardedDrawCommands& discards);
 
+  // Sets the cursor to a drawing/erasing brush cursor when necessary.
   void MaybeSetCursor();
 
+  // Handles setting the cursor only for mousemove events at `position`. This
+  // differs from `MaybeSetCursor()` in that it may also set the cursor to an
+  // I-beam for text highlighting.
+  void MaybeSetCursorOnMouseMove(const gfx::PointF& position);
+
   // Returns whether the drawing brush was set or not.
   bool MaybeSetDrawingBrush();
 

pdf/pdf_ink_module_client.h

@@ -12,6 +12,7 @@
 #include "pdf/pdf_ink_ids.h"
 #include "pdf/ui/thumbnail.h"
 #include "third_party/ink/src/ink/geometry/partitioned_mesh.h"
+#include "ui/base/cursor/cursor.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/vector2d.h"
 
@@ -55,6 +56,9 @@ class PdfInkModuleClient {
   // `point`. `point` must be in device coordinates.
   virtual void ExtendSelectionByPoint(const gfx::PointF& point) {}
 
+  // Returns the current cursor.
+  virtual ui::Cursor GetCursor() = 0;
+
   // Gets the current page orientation.
   virtual PageOrientation GetOrientation() const = 0;
 

pdf/pdf_ink_module_unittest.cc

@@ -199,6 +199,13 @@ std::map<int, std::vector<raw_ref<const ink::Stroke>>> CollectVisibleStrokes(
   return visible_stroke_shapes;
 }
 
+blink::WebMouseEvent CreateMouseMoveEventAtPoint(const gfx::PointF& point) {
+  return MouseEventBuilder()
+      .SetType(blink::WebInputEvent::Type::kMouseMove)
+      .SetPosition(point)
+      .Build();
+}
+
 blink::WebMouseEvent CreateMouseMoveWithLeftButtonEventAtPoint(
     const gfx::PointF& point) {
   return MouseEventBuilder()
@@ -261,6 +268,8 @@ class FakeClient : public PdfInkModuleClient {
               (const gfx::PointF& point),
               (override));
 
+  MOCK_METHOD(ui::Cursor, GetCursor, (), (override));
+
   PageOrientation GetOrientation() const override { return orientation_; }
 
   MOCK_METHOD(std::vector<gfx::Rect>, GetSelectionRects, (), (override));
@@ -3098,6 +3107,8 @@ class PdfInkModuleTextHighlightTest : public PdfInkModuleStrokeTest {
         .WillRepeatedly(Return(selection_rects));
     EXPECT_CALL(client(), IsSelectableTextOrLinkArea(kStartPointInsidePage0))
         .WillRepeatedly(Return(true));
+    EXPECT_CALL(client(), IsSelectableTextOrLinkArea(kEndPointInsidePage0))
+        .WillRepeatedly(Return(true));
 
     EXPECT_CALL(client(), OnTextOrLinkAreaClick(kStartPointInsidePage0,
                                                 /*click_count=*/1));
@@ -3330,10 +3341,12 @@ TEST_P(PdfInkModuleTextHighlightTest, MultipleSelection) {
       .WillRepeatedly(Return(selection_rects));
   EXPECT_CALL(client(), IsSelectableTextOrLinkArea(kStartPointInsidePage0))
       .WillRepeatedly(Return(true));
+  constexpr gfx::PointF kEndPoint2InsidePage0{25.0, 30.0};
+  EXPECT_CALL(client(), IsSelectableTextOrLinkArea(kEndPoint2InsidePage0))
+      .WillRepeatedly(Return(true));
 
   EXPECT_CALL(client(), OnTextOrLinkAreaClick(kStartPointInsidePage0,
                                               /*click_count=*/1));
-  constexpr gfx::PointF kEndPoint2InsidePage0{25.0, 30.0};
   EXPECT_CALL(client(), ExtendSelectionByPoint(kEndPoint2InsidePage0));
 
   // Apply a text highlight stroke at the given points.
@@ -3663,7 +3676,7 @@ TEST_P(PdfInkModuleTextHighlightTest,
   std::vector<gfx::Rect> selection_rects{gfx::Rect(9, 14, 5, 10)};
   EXPECT_CALL(client(), GetSelectionRects())
       .WillRepeatedly(Return(selection_rects));
-  EXPECT_CALL(client(), IsSelectableTextOrLinkArea(kMouseDownPoint))
+  EXPECT_CALL(client(), IsSelectableTextOrLinkArea(_))
       .WillRepeatedly(Return(true));
 
   // There should not be any text selection extension after the miss, as the
@@ -3673,6 +3686,173 @@ TEST_P(PdfInkModuleTextHighlightTest,
   RunStrokeMissedEndEventThenMouseMoveTest();
 }
 
+TEST_P(PdfInkModuleTextHighlightTest, CursorOnMouseMove) {
+  EnableAnnotationMode();
+  InitializeSimpleSinglePageBasicLayout();
+
+  // Select the pen tool with a "Light Red" color. The cursor should be the
+  // custom pen cursor.
+  EXPECT_CALL(client(),
+              UpdateInkCursor(CursorBitmapImageSizeEq(SkISize(8, 8))));
+
+  TestAnnotationBrushMessageParams params = {/*color_r=*/0xF2, /*color_g=*/0x8B,
+                                             /*color_b=*/0x82, /*size=*/6.0f};
+  SelectBrushTool(PdfInkBrush::Type::kPen, params);
+
+  // `kStartPointInsidePage0` will be the selectable text area position, while
+  // all other positions will be non-text areas.
+  EXPECT_CALL(client(), IsSelectableTextOrLinkArea(_))
+      .WillRepeatedly(Return(false));
+  EXPECT_CALL(client(), IsSelectableTextOrLinkArea(kStartPointInsidePage0))
+      .WillRepeatedly(Return(true));
+
+  // Move to a text position. The cursor should remain as the custom pen cursor.
+  blink::WebMouseEvent mouse_move_event =
+      CreateMouseMoveEventAtPoint(kEndPointInsidePage0);
+  // The event will be considered not handled, but the cursor will still update.
+  EXPECT_FALSE(ink_module().HandleInputEvent(mouse_move_event));
+
+  VerifyAndClearExpectations();
+
+  // Select the highlighter tool. The cursor should be the custom highlighter
+  // cursor.
+  EXPECT_CALL(client(),
+              UpdateInkCursor(CursorBitmapImageSizeEq(SkISize(10, 10))));
+  params.size = 8.0f;
+  SelectBrushTool(PdfInkBrush::Type::kHighlighter, params);
+
+  VerifyAndClearExpectations();
+
+  // Move to a text position. The cursor should be an I-beam.
+  EXPECT_CALL(client(),
+              UpdateInkCursor(ui::Cursor(ui::mojom::CursorType::kIBeam)));
+  mouse_move_event = CreateMouseMoveEventAtPoint(kStartPointInsidePage0);
+  EXPECT_FALSE(ink_module().HandleInputEvent(mouse_move_event));
+
+  VerifyAndClearExpectations();
+
+  // Move to a non-text position. The cursor should restore to the custom
+  // highlighter cursor.
+  {
+    InSequence seq;
+    EXPECT_CALL(client(), GetCursor())
+        .WillOnce(Return(ui::mojom::CursorType::kIBeam));
+    EXPECT_CALL(client(),
+                UpdateInkCursor(CursorBitmapImageSizeEq(SkISize(10, 10))));
+  }
+  mouse_move_event = CreateMouseMoveEventAtPoint(kEndPointInsidePage0);
+  EXPECT_FALSE(ink_module().HandleInputEvent(mouse_move_event));
+}
+
+TEST_P(PdfInkModuleTextHighlightTest, CursorOnMouseMoveWhileTextSelecting) {
+  EnableAnnotationMode();
+  InitializeSimpleSinglePageBasicLayout();
+
+  // Select the highlighter tool. The cursor should be the custom highlighter
+  // cursor.
+  EXPECT_CALL(client(),
+              UpdateInkCursor(CursorBitmapImageSizeEq(SkISize(10, 10))));
+  TestAnnotationBrushMessageParams params = {/*color_r=*/0xF2, /*color_g=*/0x8B,
+                                             /*color_b=*/0x82, /*size=*/8.0f};
+  SelectBrushTool(PdfInkBrush::Type::kHighlighter, params);
+
+  VerifyAndClearExpectations();
+
+  // `kStartPointInsidePage0` will be the selectable text area position, while
+  // all other positions will be non-text areas.
+  EXPECT_CALL(client(), IsSelectableTextOrLinkArea(_))
+      .WillRepeatedly(Return(false));
+  EXPECT_CALL(client(), IsSelectableTextOrLinkArea(kStartPointInsidePage0))
+      .WillRepeatedly(Return(true));
+
+  // Move to a text position. The cursor should be an I-beam.
+  EXPECT_CALL(client(),
+              UpdateInkCursor(ui::Cursor(ui::mojom::CursorType::kIBeam)));
+  blink::WebMouseEvent mouse_move_event =
+      CreateMouseMoveEventAtPoint(kStartPointInsidePage0);
+  EXPECT_FALSE(ink_module().HandleInputEvent(mouse_move_event));
+
+  VerifyAndClearExpectations();
+
+  // Start text highlighting and move to a non-text position. The cursor should
+  // remain as an I-beam.
+  EXPECT_CALL(client(), UpdateInkCursor(_)).Times(0);
+  blink::WebMouseEvent mouse_down_event =
+      MouseEventBuilder()
+          .CreateLeftClickAtPosition(kStartPointInsidePage0)
+          .Build();
+  EXPECT_TRUE(ink_module().HandleInputEvent(mouse_down_event));
+  mouse_move_event =
+      CreateMouseMoveWithLeftButtonEventAtPoint(kEndPointInsidePage0);
+  EXPECT_TRUE(ink_module().HandleInputEvent(mouse_move_event));
+
+  VerifyAndClearExpectations();
+
+  // End text highlighting. The cursor should restore to the custom highlighter
+  // cursor.
+  EXPECT_CALL(client(),
+              UpdateInkCursor(CursorBitmapImageSizeEq(SkISize(10, 10))));
+  blink::WebMouseEvent mouse_up_event =
+      MouseEventBuilder()
+          .CreateLeftMouseUpAtPosition(kEndPointInsidePage0)
+          .Build();
+  EXPECT_TRUE(ink_module().HandleInputEvent(mouse_up_event));
+}
+
+TEST_P(PdfInkModuleTextHighlightTest, CursorOnMouseMoveWhileBrushDrawing) {
+  EnableAnnotationMode();
+  InitializeSimpleSinglePageBasicLayout();
+
+  // Select the highlighter tool. The cursor should be the custom highlighter
+  // cursor.
+  EXPECT_CALL(client(),
+              UpdateInkCursor(CursorBitmapImageSizeEq(SkISize(10, 10))));
+  TestAnnotationBrushMessageParams params = {/*color_r=*/0xF2, /*color_g=*/0x8B,
+                                             /*color_b=*/0x82, /*size=*/8.0f};
+  SelectBrushTool(PdfInkBrush::Type::kHighlighter, params);
+
+  VerifyAndClearExpectations();
+
+  // `kEndPointInsidePage0` will be the selectable text area position, while
+  // all other positions will be non-text areas.
+  EXPECT_CALL(client(), IsSelectableTextOrLinkArea(_))
+      .WillRepeatedly(Return(false));
+  EXPECT_CALL(client(), IsSelectableTextOrLinkArea(kEndPointInsidePage0))
+      .WillRepeatedly(Return(true));
+
+  // Move to a non-text position. The cursor should remain as the custom
+  // highlighter cursor.
+  EXPECT_CALL(client(), UpdateInkCursor(_)).Times(0);
+  blink::WebMouseEvent mouse_move_event =
+      CreateMouseMoveEventAtPoint(kStartPointInsidePage0);
+  EXPECT_FALSE(ink_module().HandleInputEvent(mouse_move_event));
+
+  VerifyAndClearExpectations();
+
+  // Start drawing and move to a text position. The cursor should remain as the
+  // custom highlighter cursor.
+  EXPECT_CALL(client(), UpdateInkCursor(_)).Times(0);
+  blink::WebMouseEvent mouse_down_event =
+      MouseEventBuilder()
+          .CreateLeftClickAtPosition(kStartPointInsidePage0)
+          .Build();
+  EXPECT_TRUE(ink_module().HandleInputEvent(mouse_down_event));
+  mouse_move_event =
+      CreateMouseMoveWithLeftButtonEventAtPoint(kEndPointInsidePage0);
+  EXPECT_TRUE(ink_module().HandleInputEvent(mouse_move_event));
+
+  VerifyAndClearExpectations();
+
+  // End drawing. The cursor should be an I-beam.
+  EXPECT_CALL(client(),
+              UpdateInkCursor(ui::Cursor(ui::mojom::CursorType::kIBeam)));
+  blink::WebMouseEvent mouse_up_event =
+      MouseEventBuilder()
+          .CreateLeftMouseUpAtPosition(kEndPointInsidePage0)
+          .Build();
+  EXPECT_TRUE(ink_module().HandleInputEvent(mouse_up_event));
+}
+
 INSTANTIATE_TEST_SUITE_P(All,
                          PdfInkModuleTest,
                          testing::ValuesIn(GetAllInkTestVariations()));

pdf/pdf_view_web_plugin.cc

@@ -300,6 +300,8 @@ class PdfViewWebPlugin::PdfInkModuleClientImpl : public PdfInkModuleClient {
     plugin_->engine_->ExtendSelectionByPoint(point);
   }
 
+  ui::Cursor GetCursor() override { return plugin_->cursor_; }
+
   PageOrientation GetOrientation() const override {
     return plugin_->engine_->GetCurrentOrientation();
   }

pdf/pdf_view_web_plugin_unittest.cc

@@ -3049,6 +3049,16 @@ TEST_P(PdfViewWebPluginInkTest, SendThumbnailWithNoStrokes) {
   SendThumbnail(/*message_id=*/"foo", /*page_size=*/{50, 25});
 }
 
+TEST_P(PdfViewWebPluginInkTest, GetCursor) {
+  plugin_->set_cursor_type_for_testing(ui::mojom::CursorType::kPointer);
+  EXPECT_EQ(ui::mojom::CursorType::kPointer,
+            plugin_->ink_module_client_for_testing()->GetCursor().type());
+
+  plugin_->set_cursor_type_for_testing(ui::mojom::CursorType::kIBeam);
+  EXPECT_EQ(ui::mojom::CursorType::kIBeam,
+            plugin_->ink_module_client_for_testing()->GetCursor().type());
+}
+
 TEST_P(PdfViewWebPluginInkTest, UpdateCursor) {
   UpdatePluginGeometryWithoutWaiting(2.0f, {0, 0, 20, 20});