Correctly scroll the search result in the viewport with rotated pdfs (bug 2021392)

Author: calixteman

test/integration/find_spec.mjs

@@ -177,4 +177,56 @@ describe("find bar", () => {
       );
     });
   });
+
+  describe("Check that the search results are correctly visible in rotated PDFs (bug 2021392)", () => {
+    let pages;
+
+    beforeEach(async () => {
+      pages = await loadAndWait(
+        "hello_world_rotated.pdf",
+        ".textLayer",
+        "page-fit"
+      );
+    });
+
+    afterEach(async () => {
+      await closePages(pages);
+    });
+
+    it("must scroll each match into the viewport when navigating search results", async () => {
+      await Promise.all(
+        pages.map(async ([browserName, page]) => {
+          await page.click("#viewFindButton");
+          await page.waitForSelector("#findInput", { visible: true });
+          await page.type("#findInput", "hello");
+          await page.waitForSelector("#findInput[data-status='']");
+
+          for (let i = 0; i < 5; i++) {
+            if (i > 0) {
+              await page.click("#findNextButton");
+              await page.waitForSelector("#findInput[data-status='']");
+            }
+
+            // Verify we are on the expected match number.
+            const resultElement =
+              await page.waitForSelector("#findResultsCount");
+            const resultText = await resultElement.evaluate(
+              el => el.textContent
+            );
+            expect(resultText)
+              .withContext(`In ${browserName}, match ${i + 1}`)
+              .toEqual(`${FSI}${i + 1}${PDI} of ${FSI}5${PDI} matches`);
+
+            // The selected highlight must be visible in the viewport.
+            const selected = await page.waitForSelector(
+              ".textLayer .highlight.selected"
+            );
+            expect(await selected.isIntersectingViewport())
+              .withContext(`In ${browserName}, match ${i + 1}`)
+              .toBeTrue();
+          }
+        })
+      );
+    });
+  });
 });

test/pdfs/.gitignore

@@ -891,3 +891,4 @@
 !extractPages_null_in_array.pdf
 !issue20930.pdf
 !text_rise_eol_bug.pdf
+!hello_world_rotated.pdf

test/pdfs/hello_world_rotated.pdf

@@ -17,8 +17,8 @@
 /** @typedef {import("./event_utils").EventBus} EventBus */
 /** @typedef {import("./pdf_link_service.js").PDFLinkService} PDFLinkService */
 
-import { binarySearchFirstItem, scrollIntoView } from "./ui_utils.js";
 import { getCharacterType, getNormalizeWithNFKC } from "./pdf_find_utils.js";
+import { binarySearchFirstItem } from "./ui_utils.js";
 
 const FindState = {
   FOUND: 0,
@@ -28,7 +28,6 @@ const FindState = {
 };
 
 const FIND_TIMEOUT = 250; // ms
-const MATCH_SCROLL_OFFSET_TOP = -50; // px
 
 const CHARACTERS_TO_NORMALIZE = {
   "\u2010": "-", // Hyphen
@@ -553,7 +552,6 @@ class PDFFindController {
   /**
    * @typedef {Object} PDFFindControllerScrollMatchIntoViewParams
    * @property {HTMLElement} element
-   * @property {number} selectedLeft
    * @property {number} pageIndex
    * @property {number} matchIndex
    */
@@ -562,12 +560,7 @@ class PDFFindController {
    * Scroll the current match into view.
    * @param {PDFFindControllerScrollMatchIntoViewParams}
    */
-  scrollMatchIntoView({
-    element = null,
-    selectedLeft = 0,
-    pageIndex = -1,
-    matchIndex = -1,
-  }) {
+  scrollMatchIntoView({ element = null, pageIndex = -1, matchIndex = -1 }) {
     if (!this._scrollMatches || !element) {
       return;
     } else if (matchIndex === -1 || matchIndex !== this._selected.matchIdx) {
@@ -576,11 +569,7 @@ class PDFFindController {
       return;
     }
     this._scrollMatches = false; // Ensure that scrolling only happens once.
-    const spot = {
-      top: MATCH_SCROLL_OFFSET_TOP,
-      left: selectedLeft,
-    };
-    scrollIntoView(element, spot, /* scrollMatches = */ true);
+    element.scrollIntoView({ block: "start", inline: "center" });
   }
 
   #reset() {

web/pdf_find_controller.js

@@ -195,11 +195,9 @@ class TextHighlighter {
         div.append(span);
 
         if (className.includes("selected")) {
-          const { left } = span.getClientRects()[0];
-          const parentLeft = div.getBoundingClientRect().left;
-          return left - parentLeft;
+          return span;
         }
-        return 0;
+        return null;
       }
 
       div.append(node);
@@ -233,7 +231,7 @@ class TextHighlighter {
       const end = match.end;
       const isSelected = isSelectedPage && i === selectedMatchIdx;
       const highlightSuffix = isSelected ? " selected" : "";
-      let selectedLeft = 0;
+      let selectedSpan = null;
 
       // Match inside new div.
       if (!prevEnd || begin.divIdx !== prevEnd.divIdx) {
@@ -248,14 +246,14 @@ class TextHighlighter {
       }
 
       if (begin.divIdx === end.divIdx) {
-        selectedLeft = appendTextToDiv(
+        selectedSpan = appendTextToDiv(
           begin.divIdx,
           begin.offset,
           end.offset,
           "highlight" + highlightSuffix
         );
       } else {
-        selectedLeft = appendTextToDiv(
+        selectedSpan = appendTextToDiv(
           begin.divIdx,
           begin.offset,
           infinity.offset,
@@ -271,8 +269,7 @@ class TextHighlighter {
       if (isSelected) {
         // Attempt to scroll the selected match into view.
         findController.scrollMatchIntoView({
-          element: textDivs[begin.divIdx],
-          selectedLeft,
+          element: selectedSpan,
           pageIndex: pageIdx,
           matchIndex: selectedMatchIdx,
         });

web/text_highlighter.js

@@ -110,6 +110,7 @@
     &.selected {
       background-color: var(--highlight-selected-bg-color);
       backdrop-filter: var(--highlight-selected-backdrop-filter);
+      scroll-margin-top: 50px;
     }
   }
 

web/text_layer_builder.css

@@ -13,8 +13,6 @@
  * limitations under the License.
  */
 
-import { MathClamp } from "pdfjs-lib";
-
 const DEFAULT_SCALE_VALUE = "auto";
 const DEFAULT_SCALE = 1.0;
 const DEFAULT_SCALE_DELTA = 1.1;
@@ -78,11 +76,8 @@ const AutoPrintRegExp = /\bprint\s*\(/;
  *   specifying the offset from the top left edge.
  * @param {number} [spot.left]
  * @param {number} [spot.top]
- * @param {boolean} [scrollMatches] - When scrolling search results into view,
- *   ignore elements that either: Contains marked content identifiers,
- *   or have the CSS-rule `overflow: hidden;` set. The default value is `false`.
  */
-function scrollIntoView(element, spot, scrollMatches = false) {
+function scrollIntoView(element, spot) {
   // Assuming offsetParent is available (it's not available when viewer is in
   // hidden iframe or object). We have to scroll: if the offsetParent is not set
   // producing the error. See also animationStarted.
@@ -94,11 +89,8 @@ function scrollIntoView(element, spot, scrollMatches = false) {
   let offsetY = element.offsetTop + element.clientTop;
   let offsetX = element.offsetLeft + element.clientLeft;
   while (
-    (parent.clientHeight === parent.scrollHeight &&
-      parent.clientWidth === parent.scrollWidth) ||
-    (scrollMatches &&
-      (parent.classList.contains("markedContent") ||
-        getComputedStyle(parent).overflow === "hidden"))
+    parent.clientHeight === parent.scrollHeight &&
+    parent.clientWidth === parent.scrollWidth
   ) {
     offsetY += parent.offsetTop;
     offsetX += parent.offsetLeft;
@@ -113,17 +105,7 @@ function scrollIntoView(element, spot, scrollMatches = false) {
       offsetY += spot.top;
     }
     if (spot.left !== undefined) {
-      if (scrollMatches) {
-        const elementWidth = element.getBoundingClientRect().width;
-        const padding = MathClamp(
-          (parent.clientWidth - elementWidth) / 2,
-          20,
-          400
-        );
-        offsetX += spot.left - padding;
-      } else {
-        offsetX += spot.left;
-      }
+      offsetX += spot.left;
       parent.scrollLeft = offsetX;
     }
   }