This new event essentially mirrors the existing "pagesinit" event, and will allow e.g. a custom implementation of the viewer to be notified before the current PDF document is removed from the viewer.
By using this new event, we're thus able to dispatch a "pageclose" event for JavaScript actions when closing the existing document.
Having looked at the Acrobat JavaScript specification, see https://www.adobe.com/content/dam/acom/en/devnet/acrobat/pdfs/AcrobatDC_js_api_reference.pdf#G5.1963437, I suppose that introducing these two new events is probably the easiest solution overall.
However there's a number of things that, as far as I'm concerned, will help the overall implementation:
- Only dispatch these new events when `enableScripting = true` is set.
- Handle them *separately* from the existing "pagechanging" event dispatching, to avoid too much clutter.
- Don't dispatch either of the events if the page didn't actually change.
- When waiting for pages to render, don't dispatch "pageopen" if the page is no longer active when rendering finishes.
- Ensure that we only use *one* "pagerendered" event listener.
- Ensure that "pageopen" is actually dispatched when the document loads.
I suppose that we *could* avoid adding the "pageclose" event, and use the existing "pagechanging" event instead, however having a separate event might allow more flexibility in the future. (E.g. I don't know if we'll possibly want to dispatch "pageclose" on document close, as mentioned briefly in the specification.)
* move set/clear|Timeout/Interval and crackURL code in pdf.js
* remove the "backdoor" in the proxy (used to dispatch event) and so return the dispatch function in the initializer
* remove listeners if an error occured during sandbox initialization
* add support for alert and prompt in the sandbox
* add a function to eval in the global scope
Given that the GENERIC default viewer supports opening more than one document, and that a unique scripting-instance is now used for each document, the changes made in this patch seem appropriate.
While it's not entirely clear to me that it's ultimately desirable to use the `pdf.sandbox.js` in the Chromium-extension, given that the MOZCENTRAL-build uses `pdf.scripting.js` directly in a *custom* sandbox, the current state isn't that great since setting `enableScripting = true` with the Chromium-extension will currently fail completely.
Hence this patch, which should at least unbreak things for now.
Since the `close` method has become quite large, this small re-factoring shouldn't hurt (and may also be useful with future changes to the `_initializeJavaScript` method).
I completely missed this previously, but we obviously should remove the scriptElement as well to *really* clean-up everything properly.
Given that there's multiple existing usages of `loadScript` in the code-base, the safest/quickest solution seemed to be to have call-sites opt-in to remove the scriptElement using a new parameter.
This patch *attempts* to actually implement what's described for the `Count`-entry in the PDF specification, see https://www.adobe.com/content/dam/acom/en/devnet/acrobat/pdfs/PDF32000_2008.pdf#G11.2095911, which I mostly ignored back in PR 10890 since it seemed unnecessarily complicated[1].
Besides issue 12704, I've also tested a couple of other documents (e.g. the PDF specification) and these changes don't *seem* to break anything else; additional testing would be helpful though!
---
[1] At the time, all PDF documents that I tested worked even with a very simple approach and I thus hoped that it'd would suffice.
Similar to the previous patch, the GENERIC default viewer is capable of opening more than *one* PDF document and we should ensure that we handle that case correctly.
I was actually quite surprised to find that, despite the various `scripting`-getters implementing `destroySandbox` methods, there were no attempts at actually cleaning-up either the "sandbox" or removing the globally registered event listeners.
This patch also changes the method to skip *all* data fetching when "enableScripting" isn't active. Finally, simplifies some event-data accesses in the "updateFromSandbox" listener.
Another possible option here could be to use the `contentLength`, when it exists, and then using e.g. a custom event to always update the "filesize" in the sandbox "after the fact" with the result of the `getDownloadInfo`-call.
We can easily avoid unnecessary API-calls here, since most of the time the `metadata` will already be available here. In the *rare* case that it's not available, we can simply wait for the existing `getMetadata`-call to resolve.
This will be useful in the following patch, and note that there's also an old issue (see 5765) which asked for such an event. However, given that the use-case wasn't *clearly* specified, and that we didn't have an internal use for it at the time it wasn't implemented.
Also, ensure that all of the metadata-related properties are actually reset when the document is closed.
Compared to the, previously removed, `sandbox`/`watch-sandbox` gulp-tasks, these ones should work even when run against an non-existent/empty `build`-folder.
Also, to ensure that the development viewer actually works out-of-the-box, `gulp server` will now also include `gulp watch-dev-sandbox` to remove the need to *manually* invoke the build-tasks.
Finally, this patch also removes the `web/devcom.js` file since it shouldn't actually be needed, assuming that the "sandbox"-loading code in the `web/genericcom.js` file is actually *correctly* implemented.
Rather than having two slightly different ways of setting the pending/notFound appearance on the "findInput", we can simply use "data-status" in both cases since they're obviously mutually exclusive.
As mentioned in https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions#faq-support, PDF.js version `2.6.347` is the last release with IE 11/Edge support.
Hence we should now be able to reduce unnecessary duplication in the default viewer image resources, note the files in the `web/images/` folder with a `-dark` suffix, by using only *one* SVG-image for each icon and letting the `background-color` depend on the CSS theme instead.
For the `gulp mozcentral` build-target, the resulting `web/images/` folder is reduced from `43 997` to `28 566` bytes (~35 percent).
*Please note:* I don't really know if this implementation is necessarily the *best* solution, but it seems to work well enough in e.g. Firefox Nightly and Google Chrome Beta as far as my testing goes.
Given that we already include the "Content-Disposition"-header filename, when it exists, it shouldn't hurt to also include the information from the "Content-Length"-header.
For PDF documents opened via a URL, which should be a very common way for the PDF.js library to be used, this will[1] thus provide a way of getting the PDF filesize without having to wait for the `getDownloadInfo`-promise to resolve[2].
With these API improvements, we can also simplify the filesize handling in the `PDFDocumentProperties` class.
---
[1] Assuming that the server is correctly configured, of course.
[2] Since that's not *guaranteed* to happen in general, with e.g. `disableAutoFetch = true` set.
* quickjs-eval.js has been generated using https://github.com/mozilla/pdf.js.quickjs/
* lazy load of sandbox code
* Rewrite tests to use the sandbox
* Add a task `watch-sandbox` which update bundle pdf.sandbox.js on change in the sandbox code
Given that it's generally faster to call *one* function and have it loop through an object, rather than looping through an object and calling a function for every iteration, this patch will reduce the total time spent in `PDFViewerApplication._readPreferences` ever so slightly.
Also, over time we've been adding more and more preferences, rather than removing them, so using the new `AppOptions.setAll` method should be generally beneficial as well.
While the effect of these changes is quite small, it does reduces the time it takes for the preferences to be fully initialized. Given the amount of asynchronous code during viewer initialization, every bit of time that we can save should thus help.
Especially considering the recently added `viewerCssTheme` preference, which needs to be read very early to reduce the risk of the viewer UI "flashing" visibly as the theme changes, I figured that a couple of small patches reducing the time spend reading preferences cannot hurt.
Given that only two debugging hash parameters (i.e. `disableWorker` and `pdfBug`) will make this method asynchronous, we can avoid what's most of the time is an unnecessary `Promise.all` invocation.
While this does work pretty well in my quick testing, it's *very much* a hack since as far as I can tell there's no support in the CSS specification for using e.g. a CSS variable to override a `@media (prefers-color-scheme: dark) {...}` block.
The solution implemented here is thus to *edit* the viewer CSS, by either removing the entire `@media ...` block in light-mode or by ensuring that its rules become *unconditionally* applied in dark-mode.
To simplify the overall implementation, since all of this does seem like somewhat of an edge-case, the `viewerCssTheme` preference will *only* be read during viewer initialization. (Similar to many other existing preferences, a reload is thus required when changing it.)
Originally the default preferences were defined in a JSON-file checked into the repository, which was loaded using SystemJS in development mode.
Over the years a number of changes have been made to this code, most notably:
- The preferences JSON-file is now generated automatically, during building, from the `AppOptions` abstraction.
- All SystemJS usage has been removed from the development viewer.
Hence the default preferences are now available *synchronously* even in the development viewer, and it's thus no longer necessary to defer to the microtask queue (since `getDefaultPreferences` is async) just to get the default preferences.
While the effect of these changes is quite small, it *does* reduces the time it takes for the preferences to be fully initialized. Given the amount of asynchronous code during viewer initialization, every bit of time that we can save should thus help.
- Add support for logical assignment operators, i.e. `&&=`, `||=`, and `??=`, with a Babel-plugin. Given that these required incrementing the ECMAScript version in the ESLint and Acorn configurations, and that platform/browser support is still fairly limited, always transpiling them seems appropriate for now.
- Cache the `hasJSActions` promise in the API, similar to the existing `getAnnotations` caching. With this implemented, the lookup should now be cheap enough that it can be called unconditionally in the viewer.
- Slightly improve cleanup of resources when destroying the `WorkerTransport`.
- Remove the `annotationStorage`-property from the `PDFPageView` constructor, since it's not necessary and also brings it more inline with the `BaseViewer`.
- Update the `BaseViewer.createAnnotationLayerBuilder` method to actaually agree with the `IPDFAnnotationLayerFactory` interface.[1]
- Slightly tweak a couple of JSDoc comments.
---
[1] We probably ought to re-factor both the `IPDFTextLayerFactory` and `IPDFAnnotationLayerFactory` interfaces to take parameter objects instead, since especially the `IPDFAnnotationLayerFactory` one is becoming quite unwieldy. Given that that would likely be a breaking change for any custom viewer-components implementation, this probably requires careful deprecation.
*Note that I wasn't able to reproduce the issue in Firefox, but only in Chromium-browsers.*
The bug, and it's feels almost trivial once you've found it, is that we're not passing the `transform` parameter as intended to `PDFPageProxy.render` when drawing thumbnails on HiDPI displays. Instead the canvas context is, for reasons that I don't even pretent to understand, *manually* scaled in `PDFThumbnailView._getPageDrawContext`, which thus doesn't guarantee that the `baseTransform` property on the `CanvasGraphics`-instances becomes correct.
The solution is really simple though, just handle the `transform` the same way in `PDFThumbnailView.draw` as in `PDFPageView.paintOnCanvas` and things should just work.
*This is a pre-existing issue that I noticed while working on PR 12613, and fixing this also brings the thumbnail code inline with the page code.*
Given the intermittent nature of all of this, it's somewhat difficult to reproduce it consistently; however the following steps should at least provide an outline:
1. Open the sidebar, and the thumbnailView, and start scrolling around.
2. *Quickly* close the sidebar, so that all thumbnails won't have time to finish rendering.
3. Either wait for the cleanup-timeout to occur, or simply run `PDFViewerApplication.cleanup()` in the console.
What *intermittently* happens here is that `WorkerTransport.startCleanup` rejects, and consequently that cleanup doesn't complete as intended, since some of the thumbnails are left in a *pending* renderingState[1].
Fixing this is simple though, and only requires updating `PDFThumbnailViewer.cleanup` along the lines of `BaseViewer.cleanup`.
---
[1] Keep in mind that thumbnails will *only* render when the thumbnailView is visible, to reduce resource usage.
This patch will help reduce memory usage, especially for longer documents, when the user scrolls around in the thumbnailView (in the sidebar).
Note how the `PDFPageProxy.cleanup` method will, assuming it's safe to do so, release main-thread resources associated with the page. These include things such as e.g. image data (which can be arbitrarily large), and also the operatorList (which can also be quite large).
Hence when pages are evicted from the `PDFPageViewBuffer`, on the `BaseViewer`-instance, the `PDFPageView.destroy` method is invoked which will (among other things) call `PDFPageProxy.cleanup` in the API.
However, looking at the `PDFThumbnailViewer`/`PDFThumbnailView` classes you'll notice that there's no attempt to ever call `PDFPageProxy.cleanup`, which implies that in certain circumstances we'll essentially keep all resources allocated permanently on the `PDFPageProxy`-instances in the API.
In particular, this happens when the users opens the sidebar and starts scrolling around in the thumbnails. Generally speaking you obviously need to keep all thumbnail *images* around, since otherwise the thumbnailView is useless, but there's still room for improvement here.
Please note that the case where a *rendered page* is used to create the thumbnail is (obviously) completely unaffected by the issues described above, and this rather only applies to thumbnails being explicitly rendered by the `PDFThumbnailView.draw` method.
For the latter case, we can fix these issues simply by calling `PDFPageProxy.cleanup` once rendering has finished. To prevent *accidentally* pulling the rug out from under `PDFPageViewBuffer` in the viewer, which expects data to be available, this required adding a couple of new methods[1] to enable checking that it's indeed safe to call `PDFPageProxy.cleanup` from the `PDFThumbnailView.draw` method.
It's really quite fascinating that no one has noticed this issue before, since it's been around since basically "forever".
---
[1] While it should be *very* rare for `PDFThumbnailView.draw` to be called for a pageView that's also in the `PDFPageViewBuffer`, given that pages are rendered before thumbnails and that the *rendered page* is used to create the thumbnail, it can still happen since rendering is asynchronous.
Furthermore, it's also possible for `PDFThumbnailView.setImage` to be disabled, in which case checking the `PDFPageViewBuffer` for active pageViews *really* matters.
* When no actions then set it to null instead of empty object
* Even if a field has no actions, it needs to listen to events from the sandbox in order to be updated if an action changes something in it.
Note that a number of these cases are covered by existing unit-tests, and a few others only matter for the development/build scripts.
Furthermore, I've also tried to the best of my ability to test each case *manually* to hopefully further reduce the likelihood of this patch introducing any bugs.
Please find additional details about the ESLint rule at https://eslint.org/docs/rules/no-useless-escape
