With https://bugzilla.mozilla.org/show_bug.cgi?id=1505122 landing in Firefox 65, the native `ReadableStream` implementation is now enabled by default in Firefox.
Obviously it would be nice to simply stop bundling the polyfill in MOZCENTRAL builds altogether, however given that it's still possible to disable[1] `ReadableStream` this is probably not a good idea just yet.
Nonetheless, now that native support is available, it seems unnecessary (and wasteful) to keep bundling the polyfill twice[2] in MOZCENTRAL builds. Hence this patch, which contains a suggest approach for packing the polyfill in a *separate* file which is then *only* loaded if/when needed.
With this patch, the size of the `gulp mozcentral` build target is thus reduced accordingly:
| | `build/mozcentral`
|-------|-------------------
|master | 3 461 089
|patch | 3 340 268
Besides the PDF.js files taking up less space in Firefox this way, the additional benefit is that there's (by default) less code that needs to be loaded and parsed when the PDF Viewer is used which also cannot hurt.
---
[1] In `about:config`, by toggling the `javascript.options.streams` preference.
[2] Once in the `build/pdf.js` file, and once in the `build/pdf.worker.js` file.
Since most of the important rendering code is already (almost) identical between `PDFViewer.update` and `PDFSinglePageViewer.update`, it's possible to further reduce duplication by moving the code into `BaseViewer.update` instead.
Apparently in IE 11 when `history.{pushState, replaceState}` is called, there's actually a difference between not providing the *third* argument vs providing it set implicitly to `undefined`. It appears that in IE 11 it's actually being stringified, rather than ignored, which seems completely wrong (obviously other browsers aren't affected, so no surprises there).
This is yet another reason why I think the feature itself was a really bad idea, since it now requires extra/duplicated code just to prevent weird/incorrect URL behaviour in crappy browsers.
Previously a couple of different attempts at fixing this problem has been rejected, given how *crucial* this code is for the correct function of the viewer, since no one has thus far provided any evidence that the problem actually affects the default viewer[1] nor an example using the viewer components directly (without another library on top).
The fact that none of the prior patches contained even a *simple* unit-test probably contributed to the unwillingness of a reviewer to sign off on the suggested changes.
However, it turns out that it's possible to create a reduced test-case, using the default viewer, that demonstrates the error[2]. Since this utilizes a hidden `<iframe>`, please note that this error will thus affect Firefox as well.
Note that while errors are thrown when the hidden `<iframe>` loads, the default viewer doesn't break completely since rendering does start working once the `<iframe>` becomes visible (although the errors do break the initial Toolbar state).
Before making any changes here, I carefully read through not just the immediately relevant code but also the rendering code in the viewer (given it's dependence on `getVisibleElements`). After concluding that the changes should be safe in general, the default viewer was tested without any issues found. (The above being much easier with significant prior experience of working with the viewer code.)
Finally the patch also adds new unit-tests, one of which explicitly triggers the relevant code-path and will thus fail with the current `master` branch.
This patch also makes `PDFViewerApplication` slightly more robust against errors during document opening, to ensure that viewer/document initialization always completes as expected.
Please keep in mind that even though this patch prevents an error in `getVisibleElements`, it's still not possible to set the initial position/zoom level/sidebar view etc. when the viewer is hidden since rendering and scrolling is completely dependent[3] on being able to actually access the DOM elements.
---
[1] And hence the PDF Viewer that's built-in to Firefox.
[2] Copy the HTML code below and save it as `iframe.html`, and place the file in the `web/` folder. Then start the server, with `gulp server`, and navigate to http://localhost:8888/web/iframe.html
```html
<!DOCTYPE html>
<html>
<head>
<title>Iframe test</title>
<script>
window.onload = function() {
const button = document.getElementById('button1');
const frame = document.getElementById('frame1');
button.addEventListener('click', function(evt) {
frame.hidden = !frame.hidden;
});
};
</script>
</head>
<body>
<button id="button1">Toggle iframe</button>
<br>
<iframe id="frame1" width="800" height="600" src="http://localhost:8888/web/viewer.html" hidden="true"></iframe>
</body>
</html>
```
[3] This is an old, pre-exisiting, issue that's not relevant to this patch as such (and it's already being tracked elsewhere).
With modern JavaScript supporting block-scoped variables, it's no longer necessary to have large numbers of top-level variable definitions (especially when all variables are, implicitly, set to `undefined`).
Besides moving variable definintions, a number of them are also converted to `const` to help ensure that they cannot be *accidentally* modified in the code.
Finally, the `views.length` calculation is now cached *once* rather than being re-computed multiple times.
This is *really* the best that we can do here, since other proposed solutions would interfere with (and break) the painstakingly implemented browsing history that's present in the default viewer.
I'm still not convinced that this is a good idea in general, but this patch implements it in a way where it is possible to toggle[1] for users that wish to have this feature. In particular, there's a couple of reasons why I'm not finding this feature necessary/great:
- It's already possible to easily obtain the current hash, by simply clicking on the `viewBookmark` button at any time.
- Hash changes requires a bit of special handling[2], i.e. extra code, to prevent issues when the browser history is traversed (see `PDFHistory._popState`). Currently this is only necessary when the user has manually changed the hash, with this patch it will always be the case (assuming the feature is active).
- It's not always possible to change the URL when updating the browser history. For example: In the Firefox built-in viewer, the URL cannot be modified for local files (i.e. those using the `file://` protocol).
This leads to inconsistent behaviour, and may in some cases even result in errors being thrown and the history thus not updating, if the browser prevents changes to the URL during `pushState`/`replaceState` calls.
---
[1] Using the `historyUpdateUrl` viewer preference.
[2] This depends, to a great extent, on browsers always firing `popstate` events *before* `hashchange` events, which may or may not actually be guaranteed.
This should hopefully be sufficient to address issue 6847, and given the limited impact of the code changes I'm not completely sure if this would need to be controlled by a preference!?
Initially my intention was to try and provide some (slightly more detailed) implementation suggestions in the issue, but having looked briefly at doing that it would essentially have amounted to actually writing the code anyway. (Especially considering that the recent questions seemed to more-or-less ignore the information already provided in the first post.)
Finally, note that since `performance.navigation.type` is marked as deprecated, a slightly different approach was choosen instead.
If, as PR 10368 suggests, more parameters should be added to `getViewport` I think that it would be a mistake to not change the signature *first* to avoid needlessly unwieldy call-sites.
To not break any existing code and third-party use-cases, this is obviously implemented with a deprecation warning *and* with a working fallback[1] for the old method signature.
---
[1] This is limited to `GENERIC` builds, which should be sufficient.
Given that it's really not clear to me if this is actually desired functionality in the default viewer, and considering that it doesn't fit in *great* with the way that `PDFHistory` is initialized, this feature is currently off by default[1].
---
[1] It's controlled with the `disableOpenActionDestination` Preference/AppOption.
This patch re-factors, and extends, the already existing `zoomDisabledTimeout` used during mouse wheel zooming.
Unfortunately I haven't got the required hardware to actually test this patch, but there's a decent chance that it will fix, or at least reduce, the problems reported in https://bugzilla.mozilla.org/show_bug.cgi?id=1503412.
Given that a larger number of pages may now be visible at once, and importantly that their layout may be non-vertical, one of the conditions should be tweaked to not accidentally miss cases where a page is still visible.
Please note: This patch is based on code-inspection, and the only ill effect occurring without it would be a couple of (near) duplicate history entries in some *rare* edge-cases.
With the removal of the global `PDFJS` object, in PDF.js version `2.0`, the viewer options are no longer as easily accessible as they previously were (and issues have been filed about this).
In particular, since the viewer files aren't necessarily loaded *immediately*, this means that `PDFViewerApplication`/`PDFViewerApplicationOptions` aren't necessarily available directly. By dispatching an event once all viewer files are loaded but *before* the viewer initialization has run, setting `AppOptions` during load (in custom implementations of the default viewer) should hopefully become a little bit easier[1].
---
[1] In hindsight, this should probably have been implemented when the global `PDFJS` object was removed...
Rather than having a (somewhat) randomly choosen list of Preferences which `AppOptions` are allowed to override, it makes much more sense to simply add an AppOption to allow custom implementations to ignore Preferences altogether (it's also inline with the AppOption that allows the `ViewHistory` to be bypassed on load).
Rather than closing [bug 1505824](https://bugzilla.mozilla.org/show_bug.cgi?id=1505824) as WONTFIX (which is my preferred solution), given how *minor* this "problem" is, it's still possible to adjust the error messages a bit.
The main point here, which is relevant even if the changes in `BaseViewer` are ultimately rejected during review, is that we'll no longer attempt to call `BaseViewer.currentPageLabel` with an empty string from `webViewerPageNumberChanged` in `app.js`.
The other changes are:
- Stop printing an error in `BaseViewer._setCurrentPageNumber`, and have it return a boolean indicating if the page is within bounds.
- Have the `BaseViewer.{currentPageNumber, currentPageLabel}` setters print their own errors for invalid pages.
- Have the `BaseViewer.currentPageLabel` setter no longer depend, indirectly, on the `BaseViewer.currentPageNumber` setter.
- Improve a couple of other error messages.
When `highlightAll` is *not* set, there's only going to be a single match per page and unconditionally calling `PDFFindController.scrollMatchIntoView` doesn't really matter.
However, when `highlightAll` is set the current code may result in a large number of unnecessary `PDFFindController.scrollMatchIntoView` calls. Since `TextLayerBuilder._renderMatches` already checks if a particular match is the selected one, for highlighting purposes, it's simple enough to also skip scrolling completely for non-selected matches.
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)
Most of the code in `PDFFindController` assumes that a valid `state` always exits, hence it cannot hurt to add a simple check to avoid errors being thrown.
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.
Given that the `_updateScrollMode`/`_updateSpreadMode` methods are "private", there's no particular reason to not just directly call `_setCurrentPageNumber`.
Note that when e.g. presentation mode is active, we fail[1] to ensure that the `pageNumber` parameter is actually an integer before calling `_setCurrentPageNumber` (that method expects the argument be an integer).
Also changes the method signature, of `scrollPageIntoView`, to use object destructuring instead.
---
[1] Most likely, this is actually *my* oversight :-)
Currently we'll only attempt to start from the current page when a new search is done, however for 'findagain' operations we'll always continue from the last match position.
This could easily lead to confusing behaviour if the user has scrolled to a completely different part of the document. In an attempt to improve this somewhat, for repeated 'findagain' operations, we'll instead reset the position to the current page when it's *absolutely* certain that the user has scrolled.
Note that this required adding a new `BaseViewer` method, and exposing that through `PDFLinkService`, in order to check if a given page is visible.
In an attempt to avoid issues, in custom implementations of `PDFFindController`, the code checks for the existence of the `PDFLinkService.isPageVisible` method *before* using it.
Unfortunately the `PDFFindController.executeCommand` method has now become a bit more complicated than one would like, but hopefully this small change will improve the structure somewhat (especially for subsequent patches).
With only *one* event now being dispatched when the scale changes, in combination with there only being two call-sites, it doesn't seem necessary to keep the helper method for dispatching the 'scalechanging' event.
Currently `PDFFindController._calculateMatch` is (indirectly) dispatching an `updatetextlayermatches` event for every *single* page of the document. For short documents, such as the `tracemonkey` file, this probably doesn't matter too much, but for documents with a couple of thousand pages it seems unfortunate.
It shouldn't be necessary, in general, to dispatch `updatetextlayermatches` events here, since that's already being taken care of in `PDFFindController._updateMatch` which is always called when a match has been found.
However, when `highlightAll` is set we still need to ensure that pages which finished rendered *before* searching begun are updated correctly.
**STR:**
1. Open the default viewer, with the `tracemonkey` file.
2. Open the findbar, and search for "trace".
3. Enable the "Highlight All" option.
4. Close the findbar.
5. Re-open the findbar, and click on the "findNext" button.
6. Scroll down to the *second* page of the document.
**ER:**
Since "Highlight All" is active, all matches on the *second* page should be highlighted.
**AR:**
No matches are highlighted on the *second* page.
This is relevant for e.g. `PDFSinglePageViewer`, and `PDFViewer` with Presentation Mode active.
By moving this code to a helper method in `BaseViewer`, it's thus possible to reduce the amount of duplicate code that currently needed in `PDFViewer` and `PDFSinglePageViewer`.
For a short document, such as e.g. the `tracemonkey` file, this repeated normalization won't matter much, but for documents with a couple of thousand pages it seems completely unnecessary (and wasteful) to keep repeating the normalization whenever for every single page.
Currently the text-content is normalized every time that a new search operation is started, which seems completely useless considering that the "raw" text-content is never used for anything.
For a short document, such as e.g. the `tracemonkey` file, this repeated normalization won't matter much, but for documents with a couple of thousand pages it seems completely unnecessary (and wasteful) to keep repeating the normalization whenever e.g. a new search operation starts.
In the event that multiple instances of `PDFFindController` ever exists simultaneously, they will all be able to share just one `normalize` function in this way. Furthermore, the regular expression is now created lazily rather than at class construction time.
Despite all highlighted matches being removed in response to the 'findbarclose' event, there's a risk that a match could still be scrolled into view *after* the findbar has been closed[1].
Hence we need to ensure that long running searches, particularily those happening in large and/or slow loading documents[2], are ignored as well.
---
[1] The match is hidden, as expected, but the document could still scroll unexpectedly.
[2] Large documents loaded with `disableAutoFetch = true` and `disableStream = true` set are particularily susceptible to this issue.
Given that dispatching the 'updatetextlayermatches' event with `pageIndex = -1` set is now used to target the textLayers of *all* pages, there's no need to send individual events to every single page during `_nextMatch`. Since there can be an arbitrary number of pages in a document, this small/simple optimization seems too easy to ignore.
