From b42120bdb0915f453d220aaf2f8f01cb72f46edd Mon Sep 17 00:00:00 2001
From: Jonas Jenwald <jonas.jenwald@gmail.com>
Date: Mon, 21 Jun 2021 10:17:55 +0200
Subject: [PATCH] Take the position of the `selected` element into account when
 scrolling matches (issue 13596)

Note that as far as I can tell, this is *not* a regression but rather a bug which has existed since basically "forever".

**In order to reproduce this easily:**
 - Open the viewer.
 - Set the zoom level to `400%`,
 - Search for "expression".

The problem here is that when scrolling matches into view, we're scrolling to the start of the *containing* `textLayer` element rather than the start of the highlighted match itself.[1] When the entire width (or at least most) of the page is visible in the viewer, that doesn't really matter though which is likely why this bug has gone unnoticed for so long.[2]
Given that the highlighted match can be placed anywhere, e.g. even at the very end, within its `textLayer` element it's quite easy to see why the current implementation becomes a problem at higher zoom levels. All of this is then *further* exacerbated by `PDFFindController.scrollMatchIntoView` using a negative left offset, to ensure that the current match has some (visible) context available once scrolled into view.

In order to address this long-standing bug, we'll determine the (left) offset of the `selected` match and use that to modify the final position scrolled to in `PDFFindController.scrollMatchIntoView` such that the match is visible regardless of zoom level.

---
[1] Unfortunately we cannot directly scroll to the `selected` match, since it's not absolutely positioned and changing that would cause other bugs/regressions (note recent patches in that area).

[2] I did actually stumble upon this problem a little while ago, while working on PR 13482, but forgot to look into this again until I saw the new issue.
---
 web/pdf_find_controller.js |  9 +++++++--
 web/text_layer_builder.js  | 29 ++++++++++++++++-------------
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/web/pdf_find_controller.js b/web/pdf_find_controller.js
index 6da7b1f08..91302efc5 100644
--- a/web/pdf_find_controller.js
+++ b/web/pdf_find_controller.js
@@ -210,7 +210,12 @@ class PDFFindController {
     });
   }
 
-  scrollMatchIntoView({ element = null, pageIndex = -1, matchIndex = -1 }) {
+  scrollMatchIntoView({
+    element = null,
+    selectedLeft = 0,
+    pageIndex = -1,
+    matchIndex = -1,
+  }) {
     if (!this._scrollMatches || !element) {
       return;
     } else if (matchIndex === -1 || matchIndex !== this._selected.matchIdx) {
@@ -222,7 +227,7 @@ class PDFFindController {
 
     const spot = {
       top: MATCH_SCROLL_OFFSET_TOP,
-      left: MATCH_SCROLL_OFFSET_LEFT,
+      left: selectedLeft + MATCH_SCROLL_OFFSET_LEFT,
     };
     scrollIntoView(element, spot, /* scrollMatches = */ true);
   }
diff --git a/web/text_layer_builder.js b/web/text_layer_builder.js
index e5a17b9af..635574fae 100644
--- a/web/text_layer_builder.js
+++ b/web/text_layer_builder.js
@@ -227,7 +227,7 @@ class TextLayerBuilder {
     function beginText(begin, className) {
       const divIdx = begin.divIdx;
       textDivs[divIdx].textContent = "";
-      appendTextToDiv(divIdx, 0, begin.offset, className);
+      return appendTextToDiv(divIdx, 0, begin.offset, className);
     }
 
     function appendTextToDiv(divIdx, fromOffset, toOffset, className) {
@@ -242,9 +242,10 @@ class TextLayerBuilder {
         span.className = `${className} appended`;
         span.appendChild(node);
         div.appendChild(span);
-        return;
+        return className.includes("selected") ? span.offsetLeft : 0;
       }
       div.appendChild(node);
+      return 0;
     }
 
     let i0 = selectedMatchIdx,
@@ -263,15 +264,7 @@ class TextLayerBuilder {
       const end = match.end;
       const isSelected = isSelectedPage && i === selectedMatchIdx;
       const highlightSuffix = isSelected ? " selected" : "";
-
-      if (isSelected) {
-        // Attempt to scroll the selected match into view.
-        findController.scrollMatchIntoView({
-          element: textDivs[begin.divIdx],
-          pageIndex: pageIdx,
-          matchIndex: selectedMatchIdx,
-        });
-      }
+      let selectedLeft = 0;
 
       // Match inside new div.
       if (!prevEnd || begin.divIdx !== prevEnd.divIdx) {
@@ -286,14 +279,14 @@ class TextLayerBuilder {
       }
 
       if (begin.divIdx === end.divIdx) {
-        appendTextToDiv(
+        selectedLeft = appendTextToDiv(
           begin.divIdx,
           begin.offset,
           end.offset,
           "highlight" + highlightSuffix
         );
       } else {
-        appendTextToDiv(
+        selectedLeft = appendTextToDiv(
           begin.divIdx,
           begin.offset,
           infinity.offset,
@@ -305,6 +298,16 @@ class TextLayerBuilder {
         beginText(end, "highlight end" + highlightSuffix);
       }
       prevEnd = end;
+
+      if (isSelected) {
+        // Attempt to scroll the selected match into view.
+        findController.scrollMatchIntoView({
+          element: textDivs[begin.divIdx],
+          selectedLeft,
+          pageIndex: pageIdx,
+          matchIndex: selectedMatchIdx,
+        });
+      }
     }
 
     if (prevEnd) {