Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

For repeated 'findagain' operations, attempt to reset the search position if the user has e.g. scrolled in the document (issue 4141) #10217

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions web/base_viewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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] &&
Expand Down
5 changes: 5 additions & 0 deletions web/interfaces.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@ class IPDFLinkService {
* @param {Object} pageRef - reference to the page.
*/
cachePageRef(pageNum, pageRef) {}

/**
* @param {number} pageNumber
*/
isPageVisible(pageNumber) {}
}

/**
Expand Down
37 changes: 32 additions & 5 deletions web/pdf_find_controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -128,6 +128,8 @@ class PDFFindController {
}
this._extractText();

const findbarClosed = !this._highlightMatches;

if (this._findTimeout) {
clearTimeout(this._findTimeout);
this._findTimeout = null;
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
14 changes: 14 additions & 0 deletions web/pdf_link_service.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -483,6 +490,13 @@ class SimpleLinkService {
* @param {Object} pageRef - reference to the page.
*/
cachePageRef(pageNum, pageRef) {}

/**
* @param {number} pageNumber
*/
isPageVisible(pageNumber) {
return true;
}
}

export {
Expand Down