Sakurai/pdf.js
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Only scroll search results into view as a result of an actual find operation, and not when the user scrolls/zooms/rotates the document (bug 1237076, issue 6746)

Browse Source 
Currently searching, and particularily highlighting of search results, may interfere with subsequent user-interactions such as scrolling/zooming/rotating which can result in a somewhat jarring UX where the document suddenly "jumps" to a previous position.
This is especially annoying in cases where the highlighted search result isn't even visible when a user initiated scrolling/zooming/rotating happens, and there exists a couple of bugs/issues about this behaviour.

It seems reasonable, as far as I'm concerned, to treat searching as one operation and any subsequent non-search user interactions with the viewer as separate and thus not scroll the current search result into view *unless* the user is actually doing another search.
This also seems consistent with general searching in e.g. Firefox and Adobe Reader:
 - Compare with "regular" searching of e.g. HTML files in Firefox, where the user scrolling and/or zooming the document will not force a currently highlighted search result to become re-scrolled into view.
 - Compare also with Adobe Reader, where the user scrolling, zooming, and/or rotating the document will not force the currently highlighted search result to become re-scrolled into view.

The question is then why search highlighting was implemented this way in PDF.js to begin with. It might be that this wasn't really intended behaviour, but more a consequence of the asynchronous nature of the API. Considering that most operations, such as fetching the page, rendering it and extracting its text-content are all asynchronous; searching and highlighting of matches thus becomes asynchronous too.
However, it should be possible to track when search results have been scrolled into view and highlighted, and thus prevent these wierd "jumps" when the user interacts with the document.

*Please note:* Unfortunately this required moving the scrolling of matches back into `PDFFindController`, since I simply couldn't see any other (reasonable) way of implementing the functionality without tracking the `_shouldScroll` property in only *one* spot.
However, given that the new `PDFFindController.scrollMatchIntoView` method follows a similar pattern as `BaseViewer.scrollPageIntoView` and `PDFThumbnailViewer.scrollThumbnailIntoView`, this is hopefully deemed OK.
This commit is contained in:
Jonas Jenwald 2018-10-30 11:08:09 +01:00
parent 2194aef03e
commit fd87f13521
2 changed files with 33 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

30
web/pdf_find_controller.js
View File

@ -13,9 +13,9 @@
* limitations under the License.
*/
import { getGlobalEventBus, scrollIntoView } from './ui_utils';
import { createPromiseCapability } from 'pdfjs-lib';
import { getCharacterType } from './pdf_find_utils';
import { getGlobalEventBus } from './ui_utils';
const FindState = {
FOUND: 0,
@ -25,6 +25,8 @@ const FindState = {
};
const FIND_TIMEOUT = 250; // ms
const MATCH_SCROLL_OFFSET_TOP = -50; // px
const MATCH_SCROLL_OFFSET_LEFT = -400; // px
const CHARACTERS_TO_NORMALIZE = {
'\u2018': '\'', // Left single quotation mark
@ -159,8 +161,26 @@ class PDFFindController {
});
}
scrollMatchIntoView({ element = null, pageIndex = -1, matchIndex = -1, }) {
if (!this._scrollMatches || !element) {
return;
} else if (matchIndex === -1 || matchIndex !== this._selected.matchIdx) {
return;
} else if (pageIndex === -1 || pageIndex !== this._selected.pageIdx) {
return;
}
this._scrollMatches = false; // Ensure that scrolling only happens once.
const spot = {
top: MATCH_SCROLL_OFFSET_TOP,
left: MATCH_SCROLL_OFFSET_LEFT,
};
scrollIntoView(element, spot, /* skipOverflowHiddenElements = */ true);
}
_reset() {
this._highlightMatches = false;
this._scrollMatches = false;
this._pdfDocument = null;
this._pageMatches = [];
this._pageMatchesLength = [];
@ -428,10 +448,10 @@ class PDFFindController {
}
_updatePage(index) {
if (this._selected.pageIdx === index) {
if (this._scrollMatches && this._selected.pageIdx === index) {
// If the page is selected, scroll the page into view, which triggers
// rendering the page, which adds the text layer. Once the text layer
// is built, it will scroll to the selected match.
// is built, it will attempt to scroll the selected match into view.
this._linkService.page = index + 1;
}
@ -487,7 +507,6 @@ class PDFFindController {
this._updateUIState(FindState.FOUND);
return;
}
// If we're waiting on a page, we return since we can't do anything else.
if (this._resumePageIdx) {
return;
@ -595,6 +614,9 @@ class PDFFindController {
this._updateUIState(state, this._state.findPrevious);
if (this._selected.pageIdx !== -1) {
// Ensure that the match will be scrolled into view.
this._scrollMatches = true;
this._updatePage(this._selected.pageIdx);
}
}

20
web/text_layer_builder.js
View File

@ -13,12 +13,10 @@
* limitations under the License.
*/
import { getGlobalEventBus, scrollIntoView } from './ui_utils';
import { getGlobalEventBus } from './ui_utils';
import { renderTextLayer } from 'pdfjs-lib';
const EXPAND_DIVS_TIMEOUT = 300; // ms
const MATCH_SCROLL_OFFSET_TOP = -50;
const MATCH_SCROLL_OFFSET_LEFT = -400;
/**
* @typedef {Object} TextLayerBuilderOptions
@ -243,16 +241,12 @@ class TextLayerBuilder {
let isSelected = (isSelectedPage && i === selectedMatchIdx);
let highlightSuffix = (isSelected ? ' selected' : '');
// Scroll the selected match into view.
if (findController.selected.matchIdx === i &&
findController.selected.pageIdx === pageIdx) {
const spot = {
top: MATCH_SCROLL_OFFSET_TOP,
left: MATCH_SCROLL_OFFSET_LEFT,
};
scrollIntoView(textDivs[begin.divIdx], spot,
/* skipOverflowHiddenElements = */ true);
}
// Attempt to scroll the selected match into view.
findController.scrollMatchIntoView({
element: textDivs[begin.divIdx],
pageIndex: pageIdx,
matchIndex: i,
});
// Match inside new div.
if (!prevEnd || begin.divIdx !== prevEnd.divIdx) {