Some PDF files contain JavaScript actions that consist of nothing more that one, or possibly several, empty string(s). At least to me, printing a warning/showing the fallback seems completely unnecessary in that case.
Furthermore, this patch also makes use of an early `return`, so that we no longer will attempt to check for printing instructions when no JavaScript is present in the PDF file.
*Note:* It would perhaps make sense to change the API/core code, such that we ignore empty entries there instead. However, that would probably be considered a breaking changing with respect to backwards compatibility, hence this simple viewer only solution.
Fixes 5767.
I don't know if this is a regression, but I noticed earlier today that depending on the initial scale *and* sidebar state, the `annotationLayer` of the first rendered page may end up duplicated; please see screen-shot below.
[screen-shot]
I can reproduce this reliable with e.g. https://arxiv.org/pdf/1112.0542v1.pdf#zoom=page-width&pagemode=bookmarks.
When the document loads, rendering of the first page begins immediately. When the sidebar is then opened, that forces re-rendering which thus aborts rendering of the first page.
Note that calling `PDFPageView.draw()` will always, provided an `AnnotationLayerFactory` instance exists, call `AnnotationLayerBuilder.render()`. Hence the events described above will result in *two* such calls, where the actual annotation rendering/updating happens asynchronously.
For reasons that I don't (at all) understand, when multiple `pdfPage.getAnnotations()` promises are handled back-to-back (in `AnnotationLayerBuilder.render()`), the `this.div` property seems to not update in time for the subsequent calls.
This thus, at least in Firefox, result in double rendering of all annotations on the first page.
Obviously it'd be good to find out why it breaks, since it *really* shouldn't, but this patch at least provides a (hopefully) acceptable work-around by ignoring `getAnnotations()` calls for `AnnotationLayerBuilder` instances that we're destroying (in `PDFPageView.reset()`).
*It appears that this accidentally broken in PR 8775.*
Note that `PDFHistory.forward` is only used with certain named actions, and these aren't that commonly used, which ought to explain why this error managed to sneak in.
Steps to reproduce the issue (and verify the fix):
1. Navigate to e.g. http://mirrors.ctan.org/info/lshort/english/lshort.pdf
2. Click on a couple of links, or outline items, such that the history is populated with a few entries.
3. In the console, execute `PDFViewerApplication.pdfHistory.back()` one or more times, thus navigating back to a previous viewer position.
4. In the console, execute `PDFViewerApplication.pdfHistory.forward() one or more times.
At the last step above, no (forward) navigation happens with the current `master`; now compare with this patch.
The new `PDFSinglePageViewer` class extends the previously created abstract `BaseViewer` class.
There's *a lot* of existing functionality in `PDFViewer` that depends on all the pages being loaded and synchronously available, once the `setDocument` method has been called.
Given that initializing `PDFPageView` instances requires passing a DOM element to which the page is attached, the simplest solution I could come up with is to append all pages to a (hidden) document fragment and just swap them (one at a time) into the viewer when page switching occurs.
This patch introduces an abstract `BaseViewer` class, that the existing `PDFViewer` then extends. *Please note:* This lays the necessary foundation for the next patch.
Rather that registering a 'change' event listener on the `window`, which will thus (unnecessarily) fire in *a number* of other situations such as e.g. when the user changes the pageNumber or the current search term, we could/should just register it directly on the dynamically created `fileInput` DOM element instead.
I can see no really compelling reason why we actually need to listen for `file` changes on the `window` itself, and this way we're also able to keep the `fileInput` related code confined to one part of the code which should aid readability.
Furthermore, in custom deployments, there's less risk that we're going to interfere with "outside" code this way.
Finally, preprocessor guards were added to the `webViewerOpenFile` function, since that code doesn't make sense in e.g. the extension builds.
Rather than (basically) duplicating the `SimpleLinkService` in `test/driver.js`, with potential test failuires if you forget to update the test mock, it seems much nicer to just re-use the viewer component.
Note that `SimpleLinkService` is already bundled into the `build/components/pdf_viewer.js` file. Hence we only need to expose it similar to the other viewer components in that file, and make sure that the `gulp components` command runs as part of the test-setup.
reference tests
This patch allows us to use the common styles as used by the viewer as a
baseline for the annotation layer reference tests. They are extended
with a small set of overrides to ensure that all elements are visible
during the test.
The overrides file now only contains the absolutely necessary rules to
make all elements visible and is therefore no longer an almost verbatim
copy of the common styles.
This changes both `PDFViewer` and `PDFThumbnailViewer` to return early in the `pagesRotation` setters if the rotation doesn't change.
It also fixes an existing issue, in `PDFViewer`, that would cause errors if the rotation changes *before* the scale has been set to a non-default value.
Finally, in preparation for subsequent patches, it also refactors the rotation code in `web/app.js` to update the thumbnails and trigger rendering with the new `rotationchanging` event.
When testing the new `PDFHistory` implementation in practice, I felt that the current value of `UPDATE_VIEWAREA_TIMEOUT` is too large to be truly useful.
The purpose of the timeout is to attempt to address (the PDF.js part of) https://bugzilla.mozilla.org/show_bug.cgi?id=1153393, and it's currently fairly easy for the user e.g. close the browser before the timeout had a change to finish.
Obviously, the timeout is a best-effort solution, but with the current value of `UPDATE_VIEWAREA_TIMEOUT` it's not as useful as one would want.
Please note that lowering it shouldn't be a problem, since it still prevents the browser history from updating at *every* 'updateviewarea' event or during (quick) scrolling, which is all that's really needed to not impact the UX negatively.
---
Furthermore, with this lower timeout, we can also simplify the part of the 'popstate' event handler that attempted to update the browser history with the current position before moving back. In most cases, the current position will now already exist in the history, and this *greatly* decreases the complexity of this code path.
The main impetus for this change though, is that I unfortunately found that given the asynchronous nature of updating the browser history, there is some *edge* cases where that code could cause history corruption.
In practice, the user could thus get "stuck" at a particular history entry and not be able to move back. I haven't got any reliable STR for this, since it's so difficult to trigger, but it involved navigating around in a document such that a number of destinations are added to the browser history and then changing the rotation before going back/forward in the history.
Rather that attempting to patch this code, and making it even more difficult to understand than it already is or adding more asynchronous behaviour, by far the easiest solution is to remove it and simply rely on the (lowered) `UPDATE_VIEWAREA_TIMEOUT` instead.
Since e.g. zooming can occur when navigating to a new destionation, ensure that a resulting 'updateviewarea' event doesn't trigger adding of a *temporary* position to the browser history at a bad time.
By using the (heuristic) `POSITION_UPDATED_THRESHOLD` constant, we can ensure that the current document position will be added to the browser history when a sufficiently "large" number of `updateviewarea` events have been dispatched.
This patch attempts to address an issue in the old `PDFHistory` implementation, where the current position wouldn't be correctly saved when the browser was closed.
In theory this *should* already be working, however as the discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=1153393 showed, it seems that both `pagehide` and `beforeunload` arrive to late to successfully update the history during closing.
Hence a timeout is used to *temporarily* add the current position to the browser history when the viewer is idle.
Note that we need to take care not to update the browser history too often, since that would render the viewer more or unusable. Furthermore, if the timeout is *too* long it may end up effectively disable this whole functionality.
The `UPDATE_VIEWAREA_TIMEOUT` constant is thus a heuristic value, which we may need to tweak taking the above into account.
This patch completely re-implements `PDFHistory` to get rid of various bugs currently present, and to hopefully make maintenance slightly easier. Most of the interface is similar to the existing one, but it should be somewhat simplified.
The new implementation should be more robust against failure, compared to the old one. Previously, it was too easy to end up in a state which basically caused the browser history to lock-up, preventing the user from navigating back/forward. (In the new implementation, the browser history should not be updated rather than breaking if things go wrong.)
Given that the code has to deal with various edge-cases, it's still not as simple as I would have liked, but it should now be somewhat easier to deal with.
The main source of complication in the code is actually that we allow the user to change the hash of a already loaded document (we'll no longer try to navigate back-and-forth in this case, since the next commit contains a workaround).
In the new code, there's also *a lot* more comments (perhaps too many?) to attempt to explain the logic. This is something that the old implementation was serverly lacking, which is a one of the reasons why it was so difficult to maintain.
One particular thing to note is that the new code uses the `pagehide` event rather than `beforeunload`, since the latter seems to be a bad idea based on https://bugzilla.mozilla.org/show_bug.cgi?id=1336763.
The current implementation of `PDFHistory` contains a number of smaller bugs, which are *very* difficult to address without breaking other parts of its code.
Possibly the main issue with the current implementation, is that I wrote it quite some time ago, and at the time my understanding of the various edge-cases the code has to deal with was quite limited.
Currently `PDFHistory` may, despite most of those cases being fixed, in certain edge-cases lock-up the browser history, essentially preventing the user from navigating back/forward.
Hence rather than trying to iterate on `PDFHistory` to make it better, the only viable approach is unfortunately rip it out in its entirety and re-write it from scratch.
*It appears that this accidentally broke with PR 8394.*
Currently, the following will be printed in the console:
```
An error occurred while loading the PDF.
[object Promise],[object Promise]
```
With this patch we'll again get proper output, e.g. something with this format:
```
An error occurred while loading the PDF.
PDF.js v? (build: ?)
Message: unknown encryption method
```
Since the very early days of the viewer, it's been possible to pass in a `scale` when opening a PDF file. However, most of the time it was/is actually being ignored, which limits its usefulness considerably.
In older versions of the viewer, if a document hash was present (i.e. `PDFViewerApplication.initialBookmark` being set) or if the document existed in the `ViewHistory`, the `scale` passed to `PDFViewerApplication.open` would thus always be ignored.
In addition to the above, in the current viewer there's even more cases where the `scale` parameter will be ignored: if a (valid) browser history entry exists on document load, or if the `defaultZoomValue` preference is set to a non-default value.
Hence the result is that in most situation, a `scale` passed to `PDFViewerApplication.open` will be completely ignored.
A much better, not to mention supported, way of setting the initial scale is by using the `defaultZoomLevel` preference. In comparision, this also has the advantage of being used in situations where the `scale` would be ignored.
All in all this leads to the current situation where we have code which is essentially dead, since no part of the viewer (by default) relies on it.
To clean up this code, and to avoid having to pass (basically) unused parameters around, I'd thus like to remove the ability to pass a `scale` to `PDFViewerApplication.open`.
The method signature was improved in PR 7440, which has now been present in a number of releases (starting with `v1.6.210`).
Hence we should be able to remove this now, and just print an error message if the old format is used.
This was added in PR 7793, which has now been present in a number of PDF.js releases (from version `v1.7.225`).
Hence we should be able to remove it now, considering that the migration code was only intended as a best effort solution to avoid wiping out all existing user data at once. Also, keep in mind that `ViewHistory` is already limited with regards to the number of documents it will simultaneous store data for.
