[PDF Ink Signatures] Fix PdfInkModule invalidation areas

When CanonicalInkEnvelopeToInvalidationScreenRect() was initially added
it intentionally ignored the origin of the page content rectangle.
This was due to callers making an extra offset modification to the
returned result before actually doing the invalidation.  This omission
was a mistake, because the callers add an offset for the available area
but not for the page content origin.  This means that there could be a
shift in the resulting invalidation area that didn't account for that
origin.  This shift becomes much more noticeable once a user scrolls
the document.

Update the CanonicalPositionToScreenPosition() helper to account for
that offset, which now makes it a real inverse operation of the
EventPositionToCanonicalPosition() function.  This is because available
area offsets are also already accounted for in event positions.

This removes the need for the prior workaround function
CanonicalInkEnvelopeToExpandedInvalidationScreenRect().

Fixed: 375445386, 376301209
Change-Id: I6df4303e803ead43b1d54d503350b8ee11545ec7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6020758
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Alan Screen <awscreen@chromium.org>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1383301}

pdf/pdf_ink_module.cc

@@ -105,25 +105,6 @@ SkRect GetDrawPageClipRect(const gfx::Rect& content_rect,
   return gfx::RectFToSkRect(clip_rect);
 }
 
-gfx::Rect CanonicalInkEnvelopeToExpandedInvalidationScreenRect(
-    const ink::Envelope& envelope,
-    PageOrientation orientation,
-    const gfx::Rect& page_content_rect,
-    float scale_factor) {
-  gfx::Rect rect = CanonicalInkEnvelopeToInvalidationScreenRect(
-      envelope, orientation, page_content_rect, scale_factor);
-
-  // TODO(crbug.com/376301209): The bounds for `ink::Envelope` are smaller
-  // than the resulting PDF paths, which can lead to a clipping artifact during
-  // invalidation.  This workaround extends the invalidation area to ensure all
-  // of the stroke object is covered by the invalidation area.  This workaround
-  // should be removed once the reason for the discrepancy has been determined
-  // and resolved.
-  constexpr float kInkBoundsExpansion = 5.0f;
-  rect.Outset(kInkBoundsExpansion * scale_factor);
-  return rect;
-}
-
 }  // namespace
 
 PdfInkModule::PdfInkModule(PdfInkModuleClient& client)
@@ -543,7 +524,7 @@ bool PdfInkModule::FinishStroke(const gfx::PointF& position,
       CHECK(undo_redo_success);
     }
 
-    client_->Invalidate(CanonicalInkEnvelopeToExpandedInvalidationScreenRect(
+    client_->Invalidate(CanonicalInkEnvelopeToInvalidationScreenRect(
         invalidate_envelope, client_->GetOrientation(),
         client_->GetPageContentsRect(state.page_index), client_->GetZoom()));
   }
@@ -693,7 +674,7 @@ bool PdfInkModule::EraseHelper(const gfx::PointF& position, int page_index) {
   }
 
   // If `invalidate_envelope` isn't empty, then something got erased.
-  client_->Invalidate(CanonicalInkEnvelopeToExpandedInvalidationScreenRect(
+  client_->Invalidate(CanonicalInkEnvelopeToInvalidationScreenRect(
       invalidate_envelope, client_->GetOrientation(),
       client_->GetPageContentsRect(page_index), client_->GetZoom()));
   return true;
@@ -979,7 +960,7 @@ void PdfInkModule::ApplyUndoRedoCommandsHelper(
       stroke_ids.erase(id);
     }
 
-    client_->Invalidate(CanonicalInkEnvelopeToExpandedInvalidationScreenRect(
+    client_->Invalidate(CanonicalInkEnvelopeToInvalidationScreenRect(
         invalidate_envelope, client_->GetOrientation(),
         client_->GetPageContentsRect(page_index), client_->GetZoom()));
     page_indices_with_thumbnail_updates.insert(page_index);
@@ -1021,7 +1002,7 @@ void PdfInkModule::ApplyUndoRedoCommandsHelper(
       shape_ids.erase(id);
     }
 
-    client_->Invalidate(CanonicalInkEnvelopeToExpandedInvalidationScreenRect(
+    client_->Invalidate(CanonicalInkEnvelopeToInvalidationScreenRect(
         invalidate_envelope, client_->GetOrientation(),
         client_->GetPageContentsRect(page_index), client_->GetZoom()));
     page_indices_with_thumbnail_updates.insert(page_index);

pdf/pdf_ink_module_unittest.cc

@@ -992,7 +992,7 @@ TEST_F(PdfInkModuleStrokeTest, CanonicalAnnotationPoints) {
                                        kCanonicalMouseUpPosition}})));
 }
 
-TEST_F(PdfInkModuleStrokeTest, InvalidationsFromStroke) {
+TEST_F(PdfInkModuleStrokeTest, BasicLayoutInvalidationsFromStroke) {
   InitializeSimpleSinglePageBasicLayout();
   RunStrokeCheckTest(/*annotation_mode_enabled=*/true);
 
@@ -1003,7 +1003,36 @@ TEST_F(PdfInkModuleStrokeTest, InvalidationsFromStroke) {
                                              gfx::Size(14.0f, 14.0f));
   const gfx::Rect kInvalidationAreaMouseUp(gfx::Point(18.0f, 15.0f),
                                            gfx::Size(14.0f, 12.0f));
-  const gfx::Rect kInvalidationAreaFinishedStroke(3.0f, 8.0f, 35.0f, 17.0f);
+  const gfx::Rect kInvalidationAreaFinishedStroke(8.0f, 13.0f, 25.0f, 7.0f);
+  EXPECT_THAT(
+      client().invalidations(),
+      ElementsAre(kInvalidationAreaMouseDown, kInvalidationAreaMouseMove,
+                  kInvalidationAreaMouseUp, kInvalidationAreaFinishedStroke));
+}
+
+TEST_F(PdfInkModuleStrokeTest, TransformedLayoutInvalidationsFromStroke) {
+  // Setup to support examining the invalidation areas from page stroke points
+  // for a layout that is more complicated than what is provide by
+  // `InitializeSimpleSinglePageBasicLayout()`.  Include viewport offset,
+  // scroll, rotation, and zoom.
+  constexpr gfx::SizeF kPageSize(100.0f, 120.0f);
+  constexpr gfx::PointF kPageOrigin(5.0f, -15.0f);
+  constexpr gfx::RectF kPageLayout(kPageOrigin, kPageSize);
+  client().set_page_layouts(base::span_from_ref(kPageLayout));
+  client().set_page_visibility(0, true);
+  client().set_orientation(PageOrientation::kClockwise180);
+  client().set_zoom(2.0f);
+
+  RunStrokeCheckTest(/*annotation_mode_enabled=*/true);
+
+  // The default brush param size is 3.0.
+  const gfx::Rect kInvalidationAreaMouseDown(gfx::Point(8.0f, 13.0f),
+                                             gfx::Size(4.0f, 4.0f));
+  const gfx::Rect kInvalidationAreaMouseMove(gfx::Point(8.0f, 13.0f),
+                                             gfx::Size(14.0f, 14.0f));
+  const gfx::Rect kInvalidationAreaMouseUp(gfx::Point(18.0f, 15.0f),
+                                           gfx::Size(14.0f, 12.0f));
+  const gfx::Rect kInvalidationAreaFinishedStroke(7.0f, 12.0f, 27.0f, 9.0f);
   EXPECT_THAT(
       client().invalidations(),
       ElementsAre(kInvalidationAreaMouseDown, kInvalidationAreaMouseMove,
@@ -1493,8 +1522,8 @@ TEST_F(PdfInkModuleUndoRedoTest, UndoRedoInvalidationsBasic) {
   // This size is smaller than the area of the merged invalidation constants
   // above because InkStrokeModeler modeled the "V" shaped input into an input
   // with a much gentler line slope.
-  const gfx::Rect kInvalidationAreaEntireStroke(gfx::Point(3.0f, 8.0f),
-                                                gfx::Size(35.0f, 17.0f));
+  const gfx::Rect kInvalidationAreaEntireStroke(gfx::Point(8.0f, 13.0f),
+                                                gfx::Size(25.0f, 7.0f));
   EXPECT_THAT(
       client().invalidations(),
       ElementsAre(kInvalidationAreaMouseDown, kInvalidationAreaMouseMove,
@@ -1533,8 +1562,8 @@ TEST_F(PdfInkModuleUndoRedoTest, UndoRedoInvalidationsScaledRotated90) {
   // This size is smaller than the area of the merged invalidation constants
   // above because InkStrokeModeler modeled the "V" shaped input into an input
   // with a much gentler line slope.
-  const gfx::Rect kInvalidationAreaEntireStroke(gfx::Point(-3.0f, 2.0f),
-                                                gfx::Size(47.0f, 29.0f));
+  const gfx::Rect kInvalidationAreaEntireStroke(gfx::Point(7.0f, 12.0f),
+                                                gfx::Size(27.0f, 9.0f));
   EXPECT_THAT(
       client().invalidations(),
       ElementsAre(kInvalidationAreaMouseDown, kInvalidationAreaMouseMove,

pdf/pdf_ink_transform.cc

@@ -22,12 +22,11 @@ namespace chrome_pdf {
 
 namespace {
 
-// Performs what is mostly an inverse operation of
-// `EventPositionToCanonicalPosition()`, to convert from canonical coordinates
-// to screen coordinates.  This is not a strict inverse because it
-// intentionally does not adjust for `page_content_rect` offset from origin.
-// This is due to its usage for invalidation calculations, where clients account
-// for that offset separately.
+// Performs an inverse operation of `EventPositionToCanonicalPosition()`, to
+// convert from canonical coordinates to screen coordinates.
+// TODO(crbug.com/379003898): Change EventPositionToCanonicalPosition() to
+// return gfx::AxisTransform2d, so that callers can just use the inverse of
+// the transform instead of this helper.
 gfx::PointF CanonicalPositionToScreenPosition(
     const gfx::PointF& canonical_position,
     PageOrientation orientation,
@@ -57,6 +56,8 @@ gfx::PointF CanonicalPositionToScreenPosition(
           page_content_rect.height() - screen_position.x() - 1);
       break;
   }
+  // Account for scrolling, which is in the page content's origin.
+  screen_position += page_content_rect.origin().OffsetFromOrigin();
   return screen_position;
 }
 

pdf/pdf_ink_transform_unittest.cc

@@ -36,6 +36,9 @@ constexpr gfx::Size kPageSizeLandscape2x(kPageSizeLandscape.width() * 2,
 constexpr float kScaleFactor1x = 1.0f;
 constexpr float kScaleFactor2x = 2.0f;
 
+// Non-identity page scroll used in tests.
+constexpr gfx::Point kPageScrollOffset(15, 25);
+
 // Standard page content area for tests.
 constexpr gfx::Rect kPageContentAreaPortraitNoOffset(gfx::Point(),
                                                      kPageSizePortrait);
@@ -308,7 +311,7 @@ TEST(PdfInkTransformTest,
     ink::Envelope envelope(InkPointFromGfxPoint({20.0f, 35.0f}));
     envelope.Add(InkPointFromGfxPoint({40.0f, 45.0f}));
 
-    // Invalidation rectangle for `envelope` should result in save value as
+    // Invalidation rectangle for `envelope` should result in same value as
     // input.
     gfx::Rect screen_rect = CanonicalInkEnvelopeToInvalidationScreenRect(
         envelope, PageOrientation::kOriginal, kPageContentRect, kScaleFactor1x);
@@ -415,6 +418,72 @@ TEST(PdfInkTransformTest,
   }
 }
 
+TEST(PdfInkTransformTest,
+     CanonicalInkEnvelopeToInvalidationScreenRectScrolled) {
+  // Representation of page contents in screen coordinates without scale or
+  // rotation, but with a scroll.
+  constexpr gfx::Rect kPageContentRect(kPageScrollOffset, kPageSizePortrait);
+
+  {
+    // Envelope that covers the entire page contents, in canonical coordinates.
+    ink::Envelope envelope(InkPointFromGfxPoint(kCanonicalPositionTopLeft));
+    envelope.Add(InkPointFromGfxPoint(kCanonicalPositionBottomRight));
+
+    // Invalidation rectangle for `envelope` should result in the same value as
+    // the entire page contents.
+    gfx::Rect screen_rect = CanonicalInkEnvelopeToInvalidationScreenRect(
+        envelope, PageOrientation::kOriginal, kPageContentRect, kScaleFactor1x);
+    EXPECT_EQ(screen_rect, kPageContentRect);
+  }
+
+  {
+    // Envelope that covers a portion of page contents, in canonical
+    // coordinates.
+    ink::Envelope envelope(InkPointFromGfxPoint({20.0f, 35.0f}));
+    envelope.Add(InkPointFromGfxPoint({40.0f, 45.0f}));
+
+    // Invalidation rectangle for `envelope` should result in same value as
+    // input but shifted for the scroll amount.
+    gfx::Rect screen_rect = CanonicalInkEnvelopeToInvalidationScreenRect(
+        envelope, PageOrientation::kOriginal, kPageContentRect, kScaleFactor1x);
+    EXPECT_EQ(screen_rect, gfx::Rect(35.0f, 60.0f, 21.0f, 11.0f));
+  }
+}
+
+TEST(PdfInkTransformTest,
+     CanonicalInkEnvelopeToInvalidationScreenRectScrolledScaledAndRotated) {
+  // Scaled and rotated representation of page contents in screen coordinates.
+  constexpr gfx::Rect kPageContentRect(kPageScrollOffset, kPageSizeLandscape2x);
+
+  {
+    // Envelope that covers the entire page contents, in canonical coordinates.
+    ink::Envelope envelope(InkPointFromGfxPoint(kCanonicalPositionTopLeft));
+    envelope.Add(InkPointFromGfxPoint(kCanonicalPositionBottomRight +
+                                      kCanonicalPositionHalf));
+
+    // Invalidation rectangle for `envelope` should be scaled and rotated,
+    // resulting in the same value as the entire page contents.
+    gfx::Rect screen_rect = CanonicalInkEnvelopeToInvalidationScreenRect(
+        envelope, PageOrientation::kClockwise90, kPageContentRect,
+        kScaleFactor2x);
+    EXPECT_EQ(screen_rect, kPageContentRect);
+  }
+
+  {
+    // Envelope that covers a portion of page contents, in canonical
+    // coordinates.
+    ink::Envelope envelope(InkPointFromGfxPoint({20.0f, 35.0f}));
+    envelope.Add(InkPointFromGfxPoint({40.0f, 45.0f}));
+
+    // Invalidation rectangle for `envelope` should be scaled and rotated, and
+    // shifted for the scroll amount.
+    gfx::Rect screen_rect = CanonicalInkEnvelopeToInvalidationScreenRect(
+        envelope, PageOrientation::kClockwise90, kPageContentRect,
+        kScaleFactor2x);
+    EXPECT_EQ(screen_rect, gfx::Rect(44.0f, 65.0f, 21.0f, 41.0f));
+  }
+}
+
 TEST(PdfInkTransformTest, GetCanonicalToPdfTransform) {
   {
     gfx::AxisTransform2d tr = GetCanonicalToPdfTransform(/*page_height=*/0);