A significant portion of the code-base has now been converted to use `let`/`const`, rather than `var`, hence it should be possible to simply enable the ESLint `no-var` rule globally.
This way we can ensure that new code won't accidentally use `var`, and it also removes the need to manually enable the rule in various folders.
Obviously it makes sense to continue the efforts to replace `var`, but that should probably happen on a file and/or folder basis.
Please note that this patch excludes the following code:
- The `extensions/` folder, since that seemed easiest for now (and I don't know exactly what the support situation is for the Chromium-extension).
- The entire `external/` folder is ignored, since most of it's currently excluded from linting.
For the code that isn't imported from elsewhere (and should be ignored), we should probably (at some point) bring the code up to the same linting/formatting standard as the rest of the code-base.
- Various files in the `test/` folder are ignored, as necessary, since the way that a lot of this code is loaded will require some care (or perhaps larger re-factoring) when removing `var` usage.
*Please note:* Given the pre-existing issues raised in PR 13056, which seem to block immediate progress there, this patch extracts some *overall* improvements of the scripting/sandbox destruction in `PDFScriptingManager`.
As can be seen in `BaseViewer.setDocument`, it's currently necessary to *manually* delay the `PDFScriptingManager`-destruction in order for things to work correctly. This is, in hindsight, obviously an *extremely poor* design choice on my part; sorry about the churn here!
In order to improve things overall, the `PDFScriptingManager._destroyScripting`-method is re-factored to wait for the relevant events to be dispatched *before* sandbox-destruction occurs.
To avoid the scripting/sandbox-destruction hanging indefinitely, we utilize a timeout to force-destroy the sandbox after a short time (currently set to 1 second).
By moving this code from the `BaseViewer` and into `PDFScriptingManager`, all of the scripting initialization/handling code is now limited to just one file/class which help overall readability (in my opinion). Also, this patch is a *net reduction* in number of lines of code which can never hurt.
As part of these changes, the intermediary "pageopen"/"pageclose" events are now removed in favor of using the "regular" viewer events directly in `PDFScriptingManager`. Hence this removes some (strictly unnecessary) indirection in the current code, when handling PageOpen/PageClose events, which leads to overall fewer function calls in this part of the code.
The *main* purpose of this patch is to allow scripting to be used together with the viewer components, note the updated "simpleviewer"/"singlepageviewer" examples, in addition to the full default viewer.
Given how the scripting functionality is currently implemented in the default viewer, trying to re-use this with the standalone viewer components would be *very* hard and ideally you'd want it to work out-of-the-box.
For an initial implementation, in the default viewer, of the scripting functionality it probably made sense to simply dump all of the code in the `app.js` file, however that cannot be used with the viewer components.
To address this, the functionality is moved into a new `PDFScriptingManager` class which can thus be handled in the same way as all other viewer components (and e.g. be passed to the `BaseViewer`-implementations).
Obviously the scripting functionality needs quite a lot of data, during its initialization, and for the default viewer we want to maintain the current way of doing the lookups since that helps avoid a number of redundant API-calls.
To that end, the `PDFScriptingManager` implementation accepts (optional) factories/functions such that we can maintain the current behaviour for the default viewer. For the viewer components specifically, fallback code-paths are provided to ensure that scripting will "just work"[1].
Besides moving the viewer handling of the scripting code to its own file/class, this patch also takes the opportunity to re-factor the functionality into a number of helper methods to improve overall readability[2].
Note that it's definitely possible that the `PDFScriptingManager` class could be improved even further (e.g. for general re-use), since it's still heavily tailored to the default viewer use-case, however I believe that this patch is still a good step forward overall.
---
[1] Obviously *all* the relevant document properties might not be available in the viewer components use-case (e.g. the various URLs), but most things should work just fine.
[2] The old `PDFViewerApplication._initializeJavaScript` method, where everything was simply inlined, have over time (in my opinion) become quite large and somewhat difficult to *easily* reason about.
These changes will be necessary for the next patch, since we don't want to accidentally pull in the entire default viewer in the standalone viewer components.
Given that scripting is now enabled in Firefox Nightly (but only there), it seems weird to not have scripting enabled by default in `gulp server` mode.
Rather than having to spell out the English fallback strings at *every* single `IL10n.get` call-site throughout the viewer, we can simplify things by collecting them in *one* central spot.
This provides a much better overview of the fallback l10n strings used, which makes future changes easier and ensures that fallback strings occuring in multiple places cannot accidentally get out of sync.
Furthermore, by making the `fallback` parameter of the `IL10n.get` method *optional*[1] many of the call-sites (and their surrounding code) become a lot less verbose.
---
[1] It's obviously still possible to pass in a fallback string, it's just not required.
Note that this particular helper function is, with the exception of the `GENERIC` default viewer and the (unsupported) SVG-backend, mostly unused at this point in time. Hence we should be able to clean-up this helper function slightly.
Also, fixes a small inconsistency in the `SVGGraphics` initialization in the viewer, by passing in the `disableCreateObjectURL` compatibility-option. Given that the SVG-backend isn't officially supported/recommended this shouldn't have been an issue, but given that I spotted this it can't hurt to fix it.
For any viewer component not listed in `web/pdf_viewer.component.js`, it shouldn't be necessary to provide a default value for the `l10n`-parameters.
Note also that these *specific* components are heavily tailored towards the default viewer use-case, rather than for general usage.
Given that `PDFFindBar` is written *specifically* for the default viewer, rather than general usage (as opposed to the `PDFFindController`), we should be able to simply assume that the `findResultsCount` DOM-element is always present. Even more so, when we're purposely not doing any similar checks for other DOM-elements in this code.
Also, remove unnecessary `null` defaults for the various DOM-element options in the constructor, since the code simply assumes that all of the relevant DOM-elements are in fact available.
Note how the `PDFAttachmentViewer` handles PDF file attachments specially, by opening them in a new window/tab, rather than forcing them to be downloaded. This is done to improve the overall UX, since browsers in general are able to handle PDF files internally.
However, for file *annotations* we're currently not attempting to do the same thing and are instead just downloading them directly. In order to unify the behaviour, without having to duplicate a lot of code, the opening of PDF file attachments is thus moved into a new `DownloadManager.openOrDownloadData` method.
The only reason, as far as I can tell, for parsing the Metadata on the main-thread is how it was originally implemented. When Metadata support was first implemented, it utilized the [`DOMParser`](https://developer.mozilla.org/en-US/docs/Web/API/DOMParser) which isn't available in workers.
Today, with the custom XML-parser being used, that's no longer an issue and it seems reasonable to move the Metadata parsing to the worker-thread[1], since that's where all parsing should happen (for performance reasons).
Based on these changes, we'll be able to reduce the now unnecessary duplication of the XML-parser (and related code) in both of the *built* `pdf.js`/`pdf.worker.js` files.
Finally, this patch changes the `_repair` method to use "Array + join" rather than string concatenation.
---
[1] This needed the previous patch, to enable sending of `Map`s between threads with workers disabled.
I happened to look at this code, and I can't for the life of me figure out why I didn't just implement it like this patch in the first place (since the current format feels overly verbose).
*This is somewhat similar to PR 12931.*
For PDF documents where fonts are completely missing in the /Resources dictionaries, there's basically no "correct" way of rendering the document.
Hence it's very unlikely that another PDF viewer will do a better job than PDF.js in these cases, and consequently it seems highly questionable if the fallback bar really helps here.
Given that these event listeners should essentially never be needed, but are included simply to avoid breakage in edge-cases, it can't hurt to make this code slightly less verbose.
- Mark `BaseViewer.initializeScriptingEvents` as an `async` method, since that's actually how it's being used in the default viewer (see `PDFViewerApplication-_initializeJavaScript`).
- Change `BaseViewer._pageWidthScaleFactor` to access the *internal* scroll/spread-modes directly, rather than using the getters, since that's consistent with the rest of the code (and not just for these properties).
For reasons that I now can't for the life of me understand, I included handling of the `PresentationModeState.CHANGING`-case despite it not actually doing anything.
Given that these HTML elements are not being used at all in `MOZCENTRAL`-builds, note the preprocessor check in `PDFViewerApplication._otherError`, we obviously don't need the HTML code either.
Some of the localization strings (e.g. "loading_error") are repeated multiple times throughout the `web/app.js` file, which means that we need to duplicate the fallback strings as well. Furthermore, the signature of the `IL10n.get` method makes the call-sites quite verbose.
By adding a new helper method, in `PDFViewerApplication`, we're able to gather the localization fallback strings in one central spot in `web/app.js` and also make the lookup of the error/warning messages more compact.
This code is *very* old and it even predates the existence of arrow functions. Hence we can now reduce the overall verbosity by not having to explicitly spell out `PDFViewerApplication` everywhere.
This feature was Firefox-specific, and it's now been removed from the HTML specification and it's disabled by default starting with Firefox 85. Hence it seems completely unnecessary to keep this code in the default viewer.
Please refer to https://groups.google.com/g/mozilla.dev.platform/c/tc11BCenm2c and the resources that it links to.
Given that `getComputedStyle` only works on visible elements, the result of PR 12354 is that if the viewer is placed in a *hidden* `iframe` the viewer will now be broken. This obviously wasn't the intention of that PR, hence I believe that we should limit the `position: absolute;` check slightly to avoid this.
aria-controls state
In testing, screen readers such as JAWS have trouble understanding the expanded state of the buttons that expand hidden menus due to lacking aria-expanded attribute. Also, given that the buttons do not contain the controlled/shown element, they should also define the aria-controls attribute with associated element id per https://www.w3.org/TR/wai-aria-1.1/#aria-expanded
This fixes adds these requirements for the sidebar, find, and secondary toolbar buttons.
Currently it's not *immediately* clear from the code itself, unless you look at the definition of `this._pageLabels`, that the default value is `null`.[1]
We can improve this, and also reduce the amount of code, by using modern ECMAScript features such as optional chaining and nullish coalescing.
---
[1] Keep in mind that an *empty* string is actually a valid page label, according to the PDF specification.
Given that we don't focus the viewer *itself* (among other things) when the viewer is embedded, I suppose that it makes some sense to not focus the `PasswordPrompt` input-field either on load.
In order to improve the overall UX here, if an *incorrect* password was provided we'll still focus the input-field.
Fixes 12951 (assuming we care to do so, of course).
With PR 10539, we'll now always attempt to fallback to the PDF.js built-in font renderer for fonts that fail to load (i.e. are rejected by the sanitizer). Generally speaking, these errors are the result of insufficient validation in the PDF.js font code, however in almost all cases we've seen thus far our built-in font renderer manages just fine.
However, we still trigger the `onUnsupportedFeature` reporting, which in Firefox causes the fallback bar to be displayed. Given that, in a majority of cases[1], things look fine it seems unfortunate to bother the user with the fallback bar here.
Note that even though we no longer show the fallback bar in this case, we still report telemetry as before.
---
[1] The only *known* case where things aren't fine with the built-in font renderer is issue 10232, however that document is sufficiently broken that there's a couple of other things that will trigger the fallback bar.
- For wrapped scrolling, we unfortunately need to do a fair bit of parsing of the *current* page layout. Compared to e.g. the spread-modes, where we can easily tell how the pages are laid out, with wrapped scrolling we cannot tell without actually checking. In particular documents with varying page sizes require some care, since we need to check all pages on the "row" of the current page are visible and that there aren't any "holes" present. Otherwise, in the general case, there's a risk that we'd skip over pages if we'd simply always advance to the previous/next "row" in wrapped scrolling.
- For horizontal scrolling, this patch simply maintains the current behaviour of advancing *one* page at a time. The reason for this is to prevent inconsistent behaviour for the next and previous cases, since those cannot be handled identically. For the next-case, it'd obviously be simple to advance to the first not completely visible page. However for the previous-case, we'd only be able to go back *one* page since it's not possible to (easily) determine the page layout of non-visible pages (documents with varying page sizes being a particular issue).
- For vertical scrolling, this patch maintains the current behaviour by default. When spread-modes are being used, we'll now attempt to advance to the next *spread*, rather than just the next page, whenever possible. To prevent skipping over a page, this two-page advance will only apply when both pages of the current spread are visible (to avoid breaking documents with varying page sizes) and when the second page in the current spread is fully visible *horizontally* (to handle larger zoom values).
In order to reduce the performance impact of these changes, note that the previous/next-functionality will only call `getVisibleElements` for the scroll/spread-modes where that's necessary and that "normal" vertical scrolling is thus unaffected by these changes.
To support these changes, the `getVisibleElements` helper function will now also include the `widthPercent` in addition to the existing `percent` property.
The `PDFViewer._updateHelper` method is changed slightly w.r.t. updating the `currentPageNumber` for the non-vertical/spread modes, i.e. won't affect "normal" vertical scrolling, since that helped simplify the overall calculation of the page advance.
Finally, these new `BaseViewer` methods also allow (some) simplification of previous/next-page functionality in various viewer components.
*Please note:* There's one thing that this patch does not attempt to change, namely disabling of the previous/next toolbarButtons respectively the firstPage/lastPage secondaryToolbarButtons. The reason for this is that doing so would add quite a bit of complexity in general, and if for some reason `BaseViewer._getPageAdvance` would get things wrong we could end up incorrectly disabling the buttons. Hence it seemed overall safer to *not* touch this, and accept that the buttons won't be `disabled` despite in some edge-cases no further scrolling being possible.
