Merge pull request #10217 from Snuffleupagus/findagain-position-reset

For repeated 'findagain' operations, attempt to reset the search position if the user has e.g. scrolled in the document (issue 4141)
mozilla · Nov 3, 2018 · 45758dd · 45758dd
2 parents ec76aa5 + d805d79
commit 45758dd
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 5 deletions.
diff --git a/web/base_viewer.js b/web/base_viewer.js
@@ -878,6 +878,23 @@ class BaseViewer {
     throw new Error('Not implemented: _getVisiblePages');
   }
 
+  /**
+   * @param {number} pageNumber
+   */
+  isPageVisible(pageNumber) {
+    if (!this.pdfDocument) {
+      return false;
+    }
+    if (this.pageNumber < 1 || pageNumber > this.pagesCount) {
+      console.error(
+        `${this._name}.isPageVisible: "${pageNumber}" is out of bounds.`);
+      return false;
+    }
+    return this._getVisiblePages().views.some(function(view) {
+      return (view.id === pageNumber);
+    });
+  }
+
   cleanup() {
     for (let i = 0, ii = this._pages.length; i < ii; i++) {
       if (this._pages[i] &&

diff --git a/web/interfaces.js b/web/interfaces.js
@@ -77,6 +77,11 @@ class IPDFLinkService {
    * @param {Object} pageRef - reference to the page.
    */
   cachePageRef(pageNum, pageRef) {}
+
+  /**
+   * @param {number} pageNumber
+   */
+  isPageVisible(pageNumber) {}
 }
 
 /**

diff --git a/web/pdf_find_controller.js b/web/pdf_find_controller.js
@@ -113,7 +113,7 @@ class PDFFindController {
   executeCommand(cmd, state) {
     const pdfDocument = this._pdfDocument;
 
-    if (this._state === null || cmd !== 'findagain') {
+    if (this._state === null || this._shouldDirtyMatch(cmd)) {
       this._dirtyMatch = true;
     }
     this._state = state;
@@ -128,6 +128,8 @@ class PDFFindController {
       }
       this._extractText();
 
+      const findbarClosed = !this._highlightMatches;
+
       if (this._findTimeout) {
         clearTimeout(this._findTimeout);
         this._findTimeout = null;
@@ -139,14 +141,16 @@ class PDFFindController {
           this._nextMatch();
           this._findTimeout = null;
         }, FIND_TIMEOUT);
-      } else if (cmd === 'findagain' && !this._dirtyMatch) {
-        const updateHighlightAll = (!this._highlightMatches &&
-                                    this._state.highlightAll);
+      } else if (this._dirtyMatch) {
+        // Immediately trigger searching for non-'find' operations, when the
+        // current state needs to be reset and matches re-calculated.
+        this._nextMatch();
+      } else if (cmd === 'findagain') {
         this._nextMatch();
 
         // When the findbar was previously closed, and `highlightAll` is set,
         // ensure that the matches on all active pages are highlighted again.
-        if (updateHighlightAll) {
+        if (findbarClosed && this._state.highlightAll) {
           this._updateAllPages();
         }
       } else {
@@ -194,6 +198,29 @@ class PDFFindController {
     return this._normalizedQuery;
   }
 
+  _shouldDirtyMatch(cmd) {
+    switch (cmd) {
+      case 'findagain':
+        const pageNumber = this._selected.pageIdx + 1;
+        const linkService = this._linkService;
+        // Only treat a 'findagain' event as a new search operation when it's
+        // *absolutely* certain that the currently selected match is no longer
+        // visible, e.g. as a result of the user scrolling in the document.
+        //
+        // NOTE: If only a simple `this._linkService.page` check was used here,
+        // there's a risk that consecutive 'findagain' operations could "skip"
+        // over matches at the top/bottom of pages thus making them completely
+        // inaccessible when there's multiple pages visible in the viewer.
+        if (pageNumber >= 1 && pageNumber <= linkService.pagesCount &&
+            linkService.page !== pageNumber && linkService.isPageVisible &&
+            !linkService.isPageVisible(pageNumber)) {
+          break;
+        }
+        return false;
+    }
+    return true;
+  }
+
   /**
    * Helper for multi-term search that fills the `matchesWithLength` array
    * and handles cases where one search term includes another search term (for

diff --git a/web/pdf_link_service.js b/web/pdf_link_service.js
@@ -352,6 +352,13 @@ class PDFLinkService {
     let refStr = pageRef.num + ' ' + pageRef.gen + ' R';
     return (this._pagesRefCache && this._pagesRefCache[refStr]) || null;
   }
+
+  /**
+   * @param {number} pageNumber
+   */
+  isPageVisible(pageNumber) {
+    return this.pdfViewer.isPageVisible(pageNumber);
+  }
 }
 
 function isValidExplicitDestination(dest) {
@@ -483,6 +490,13 @@ class SimpleLinkService {
    * @param {Object} pageRef - reference to the page.
    */
   cachePageRef(pageNum, pageRef) {}
+
+  /**
+   * @param {number} pageNumber
+   */
+  isPageVisible(pageNumber) {
+    return true;
+  }
 }
 
 export {