As discussed in the issue, this is a small/simple patch that should help to prevent *outright* data loss in forms when a new document is opened in the GENERIC viewer.
While the implementation is perhaps a bit "simplistic", it does seem to work and should be fine given that this is an edge-case only relevant for the GENERIC viewer.
In the next patch we'll need to be able to actually wait for saving to complete, hence it's necessary to slightly re-factor the `save`-method.
As part of these changes, we can reduce some duplication in the `save`-method and slightly improve the overall code. For consistency, the `download`-method is updated similarily to improve the code (this functionality is *very* old, even pre-dating the introduction of Promises in the code-base).
As mentioned in the JSDoc comment, this should not be used unless you know what you're doing, since it will lead to increased memory usage. However, in some situations (e.g. SVG-rendering), we still want to be able to run general clean-up on both the main/worker-thread while keeping loaded fonts attached to the DOM.[1]
As part of these changes, `WorkerTransport.startCleanup` is converted to an async method and we'll also skip clean-up when destruction has started (since it's redundant).
---
[1] The SVG-rendering mode is obviously not officially supported, since it's both rather incomplete and inherently slower. However with recent changes, whereby we cache repeated images on the document rather than the page level, memory usage can be *a lot* worse than before if we never attempt to release e.g. cached image-data when the viewer is in SVG-rendering mode.
* JS - Handle correctly hierarchy of fields
- it aims to fix#13132;
- annotations can inherit their actions from the parent field;
- there are some fields which act as a container for other fields:
- they can be access through js so need to add them with an empty type (nothing in the spec about that but checked in Acrobat);
- calculation order list (CO) can reference them so need make them through this.getField;
- getArray method must return kids.
- field values are number, string, ... depending of their type but nothing in the spec on how to know what's the type:
- according to the comment for Canonical Format: https://www.adobe.com/content/dam/acom/en/devnet/pdf/pdfs/PDF32000_2008.pdf#page=461
- it seems that this "type" can be guessed from js action Format (when setting a type in Acrobat DC, the only affected thing is this action).
- util.scand with an empty string returns the current date.
Given that *all* data has been loaded on the main-thread, and then transferred to the worker-thread, ever since PR 8617 (almost four years ago) it should no longer be necessary to keep this special-case around.
Given that the `webViewerOpenFileViaURL` helper function is being defined in *all* builds anyway, the current pre-processor usage doesn't really improve readability in my opinion.
Currently `destRef === null`, which will only happen in documents with corrupt destinations, will (unsurprisingly) throw when trying to lookup the pageNumber. To avoid this, we can simply use the same format as in 1a2cdaffc5/web/pdf_link_service.js (L128)
The rotation handling that's currently living in `PDFViewerApplication` is *very* old, and pre-dates the introduction of the viewer components by years.
As can be seen in the `BaseViewer.pagesRotation` setter, we're not actually normalizing the rotation as intended and instead rely on the caller to handle that correctly. This is first of all inconsistent, given how other setters are implemented, and secondly it could also lead to the rotation being set to a value outside of the `[0, 360)`-range.
Finally, for improved consistency the rotation handling in `PageViewport` is updated similarly. Please note that this case, it's *not* changing the pre-existing logic.
This improves and simplifies #13102 in order to make printing of test-cases
like the one in bug 1698414 (where the real page is bigger than the target
page) much better, see incoming screenshots.
The reason why we need to stop setting .style.width / .style.height is to get
the right auto-sizing behavior in both axes. This shouldn't change behavior as
long as the print resolution is >= the CSS resolution, which seems like a
reasonable assumption.
If you try to print with a lower resolution than CSS, then instead of an
stretched canvas, you'd get a centered CSS-quality canvas, which seems
sensible. This could maybe be fixed with some CSS hackery (some combination of
min / max and viewport units perhaps?), but I think it's more trouble than it's
worth.
- implement few positioning properties: position, width, height, anchor;
- implement font element;
- implement fill element (used by font) and its children (linear, radial, ...);
- font property is inherited from ancestor container (see https://www.pdfa.org/wp-content/uploads/2020/07/XFA-3_3.pdf#page=43) so let CSS handles that stuff;
- in order to reduce the number of properties to set, only set non default properties and put the default in CSS;
- set a background to some containers to be able to see them (will be removed in a future commit).
The intention, in PR 12493, was that the page we're adding to the browser history should behave as if it were a "regular" internal destination (to properly convey user intent).
Unfortunately, since I didn't consider all the edge-cases correctly, it ended up behaving like a URL-hash instead which obviously wasn't intended. Note that currently this isn't a problem, however it can become an issue (in some cases) with upcoming re-factoring around `PDFHistory` and OpenAction support[1].
---
[1] I've started working on fixing the following TODO, which will require a couple of smaller tweaks here and there: 9d0ce6e79f/web/app.js (L1680-L1681)
In the `getAll`-method, we can have just one *explicit* loop rather than two indirect ones via the old `Object.assign`-call.
Also, changes the `get`-method to be slightly more compact (while keeping the logic intact).
Looking at this now, I cannot understand why we'd need to initialize `this.prefs` with all of the values from `this.defaults`.
Not only does this *indirectly* require one extra loop, via the `Object.assign`-call, but it also means that in GENERIC-builds changes to default-preference values might not be picked-up unless the the existing user-prefs are cleared (if the user had *manually* set prefs previously).
Given that the `enableXfa` parameter must to be passed to the API/Worker, and thus included in the `getDocument` call, it's not necessary to include it when initializing the `PDFViewer`-instance used in the default viewer. (Also, in `AppOptions`, the parameter is clearly marked with `OptionKind.API`.)
Furthermore, we probably don't want to display the fallback bar (in Firefox) for XFA documents when `enableXfa = true` is set.
While it's still not entirely clear if this would've prevented the issue as reported, given that the particular use-case reported apparently no longer applies, this small change really cannot hurt in general *and* it won't effect "regular" viewer builds in any way.
Given how the compatibility-values are being handled, it's not actually possible to override a *truthy* default-value with a *falsy* compatibility-value.
This is a simple oversight on my part, and with modern ECMAScript features this is very easy to support.
With the changes made in the previous patch, we can now list "disableTelemetry" in the `AppOptions` only for the `CHROME`-builds and thus remove the special-casing in the `checkChromePreferencesFile` helper function.
Originally the default preferences where simply placed in a JSON-file, checked into the repository, which over time became impractical, annoying, and error-prone to maintain; please see PR 10548.
While that improved the overall situation a fair bit, it however inherited one quite unfortunate property of the old JSON-based solution[1]: It's still not possible for *different* build targets to specify their *own* default preference values.
With some preferences, such as e.g. `enableScripting`, it's not inconceivable that you'd want to (at least) support build-specific default preference values. Currently that's not really possible, which is why this PR re-factors the default preferences generation to support this.
---
[1] This fact isn't really clear from the `AppOptions` implementation, unless you're familiar with the `gulpfile.js` code, which could lead to some confusion for those new to this part of the code-base.
A number of the currently supported *scripting* events only make sense in the "normal" viewer mode, and not when PresentationMode is active. For example:
- Changing the zoom-level will outright break rendering in PresentationMode, since it relies on "page-fit" being used.
- Focusing a particular (AcroForm) element won't work, and could break keyboard navigation, since forms should not be editable in PresentationMode (see issue 12232).
While this will perhaps not be perfect for *every* PDF document with mixed page orientation, based on the large number of bugs/issues seen over the years I'm however pretty convinced that it'll be an overall improvement in a majority of cases.
In order to improve things further, we'd probably need Firefox to support e.g. `@page` such that the viewer can provide better information to the print engine.
Currently, with `enablePrintAutoRotate = true` set, we're forced to loop through all the pages *twice* when checking for any landscape pages.
This seems completely unnecessary now, and using only *one* loop should be marginally more efficient in general.
Currently landscape pages are rotated *clockwise*, which for most documents feel wrong since holding the printed pages at their *left* edge causes the landscape pages to be viewed "upside down".
In general, since most documents are LTR ones, it feels more appropriate to instead rotate landscape pages *counterclockwise* for printing.
- add an option to enable XFA rendering if any;
- for now, let the canvas layer: it could be useful to implement XFAF forms (embedded pdf in xml stream for the background and xfa form for the foreground);
- ui elements in template DOM are pretty close to their html counterpart so we generate a fake html DOM from template one:
- it makes easier to translate template properties to html ones;
- it makes faster the creation of the html element in the main thread.
Given that https://bugzilla.mozilla.org/show_bug.cgi?id=1699219 has enabled scripting for all Firefox-channels, it seems reasonable to simply set `enableScripting = true` unconditionally in the viewer preferences/options.
For now, this patch leaves the standalone viewer-components alone (such as e.g. `BaseViewer`), and if those are used scripting will thus have to be manually enabled (see e.g. the "simpleviewer"/"singlepageviewer" examples).
It seems reasonable to place this alongside the *similar* `getFilenameFromUrl` helper function. This way, with the changes in the next patch, we also avoid having to expose the `isDataScheme` function in the API itself and we instead expose `getPdfFilenameFromUrl` in the API (which feels overall more appropriate).
The issue that this patch fixes is extremely unlikely, but still theoretically possible, and I really should've caught this earlier.
Note how `BaseViewer.pagesPromise` will only be defined when a document is active, see below, and that if a printing event (triggered from scripting) arrives while the document is been closed there's a small chance that the promise isn't defined.
eb92ed12f2/web/base_viewer.js (L426-L428)
This builds on top of #13100, but this changes printing behavior intentionally
so I thought it was worth discussing separately, to improve the rendering on
test-cases like the one in https://bugzil.la/1697778.
This matches what e.g. Evince does when you print the PDF in there on an A4
printer.
We use margins to center horizontally, and flex to center vertically. The
reasoning for this is that it should have better browser support (though maybe
pdf.js no longer supports browsers without flex support?) and it's just as
simple.
@supports() is not supposed to report support for page descriptors, this is
depending on a Chromium bug, which doesn't treat as invalid:
```
<div style="size: 1pt 1pt">
```
Even though it should. That is
https://bugs.chromium.org/p/chromium/issues/detail?id=1079214
There's no need to use @supports for this. If the descriptor is not accepted it
will just be ignored.
That way, when Firefox implements @page { size }, which is in progress, it will
get the right behavior.
First, there's just no need to do something like this, this is simpler and
closer to what the screen renderer does.
Second, this causes overflow, which Firefox tries to compensate for when
fitting to page width, and fails at it. That is tracked in:
https://bugzilla.mozilla.org/show_bug.cgi?id=1698136
But this bug works around it by not causing overflow.
For modern browsers, we could avoid the duplication setting the style attribute
by using something like width: min/max-content, but this is not a big deal I
think, let me know if you'd prefer that.
Also I had to add a max-height for Chromium not to create extra pages. This
is harmless in Firefox and workarounds the Chromium bug, so so be it.
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.
The whole purpose of showing a notification on the `sidebarToggle` button, when the sidebar is closed, was to give users *some* kind of indication that the PDF document contains outline/attachments/layers without having to manually open the sidebar to check.
However, in the implementation in PR 7959, I also added notifications for each view-buttons in the sidebar. Looking back at this, I've always questioned the value of the last part, since the view-buttons already have a `disabled`-state which shows if they're available or not. Hence we're actually, in a sense, duplicating notifications for the outline/attachments/layers-buttons without adding (in my opinion) all that much overall value.
All-in-all, I'm thus proposing that we only display the notification on the `sidebarToggle`-button itself, since that should really be sufficient here, which also allows us to simplify the relevant code a fair bit.
Note first of all how the `PDFDocumentProxy.getJSActions` method in the API caches the result, which makes repeated lookups cheap enough to not really be an issue.
Secondly, with the previous patch, we're now only dispatching "pageopen"/"pageclose"-events when there's actually a sandbox that listens for them.
All-in-all, with these changes we can thus simplify the default-viewer "pageopen"-event handler a fair bit.
This patch is a rebased *and* refactored version of PR 9448, such that it applies cleanly given that `PDFFindController` has changed since that PR was opened; obviously keeping the original author information intact.
This patch will thus ensure that e.g. fractions, and other things that we normalize before searching, will still be highlighted correctly in the textLayer.
Furthermore, this patch also adds basic unit-tests for this functionality.
*Note:* The `[api-minor]` tag is added, since third-party implementations of the `PDFFindController` must now always use the `pageMatchesLength` property to get accurate length information (see the `web/text_layer_builder.js` changes).
Co-authored-by: Ross Johnson <ross@mazira.com>
Co-authored-by: Jonas Jenwald <jonas.jenwald@gmail.com>
The "pageopen"/"pageclose"-events are only necessary if, and only if, there's actually a sandbox to dispatch the events in. Hence we shouldn't dispatch those events unconditionally, as soon as `enableScripting` is set, but rather initialize that functionality only when needed.
Furthermore, in `web/app.js`, there's currently a bug since we're attempting to *manually* simulate a "pageopen"-event for a page that may not actually have been rendered at the time. With the modified `BaseViewer.initializeScriptingEvents` method, we'll now dispatch a correct "pageopen"-event here.
Not only was long text in popups no longer wrapped correctly, the
alignment was also center instead of left (or right, depending on the
locale used) for both text in popups and the other parts within the
annotation's section, such as the icon.
Note that these changes were done automatically, using `gulp lint --fix`.
With this rule, we'll thus enforce a *consistent* formatting of zero-lengths in our CSS files.
Please find additional details about the Stylelint rule at https://stylelint.io/user-guide/rules/length-zero-no-unit
With the updated default viewer UI, some `dir`-dependent CSS rules are now redundant since *identical* rules are being specified for both LTR and RTL mode; after PR 12807 landed I've found even more of these cases.
Note in particular that the findbar-button rules can be simplified quite a bit, since there's a fair amount of unnecessary duplication in the CSS.
There's built-in ESLint rule, see `sort-imports`, to ensure that all `import`-statements are sorted alphabetically, since that often helps with readability.
Unfortunately there's no corresponding rule to sort `export`-statements alphabetically, however there's an ESLint plugin which does this; please see https://www.npmjs.com/package/eslint-plugin-sort-exports
The only downside here is that it's not automatically fixable, but the re-ordering is a one-time "cost" and the plugin will help maintain a *consistent* ordering of `export`-statements in the future.
*Note:* To reduce the possibility of introducing any errors here, the re-ordering was done by simply selecting the relevant lines and then using the built-in sort-functionality of my editor.
This implementation is inspired by the behaviour in (recent versions of) Adobe Reader, since it leads to reasonably simple and straightforward code as far as I'm concerned.
*Specifically:* We'll only consider *one* destination per page when finding/highlighting the current outline item, which is similar to e.g. Adobe Reader, and we choose the *first* outline item at the *lowest* level of the outline tree.
Given that this functionality requires not only parsing of the `outline`, but looking up *all* of the destinations in the document, this feature can when initialized have a non-trivial performance overhead for larger PDF documents.
In an attempt to reduce the performance impact, the following steps are taken here:
- The "find current outline item"-functionality will only be enabled once *one* page has rendered and *all* the pages have been loaded[1], to prevent it interfering with data regular fetching/parsing early on during document loading and viewer initialization.
- With the exception of a couple of small and simple `eventBus`-listeners, in `PDFOutlineViewer`, this new functionality is initialized *lazily* the first time that the user clicks on the `currentOutlineItem`-button.
- The entire "find current outline item"-functionality is disabled when `disableAutoFetch = true` is set, since it can easily lead to the setting becoming essentially pointless[2] by triggering *a lot* of data fetching from a relatively minor viewer-feature.
- Fetch the destinations *individually*, since that's generally more efficient than using `PDFDocumentProxy.getDestinations` to fetch them all at once. Despite making the overall parsing code *more* asynchronous, and leading to a lot more main/worker-thread message passing, in practice this seems faster for larger documents.
Finally, we'll now always highlight an outline item that the user manually clicked on, since only highlighting when the new "find current outline item"-functionality is used seemed inconsistent.
---
[1] Keep in mind that the `outline` itself already isn't fetched/parsed until at least *one* page has been rendered in the viewer.
[2] And also quite slow, since it can take a fair amount of time to fetch all of the necessary `destinations` data when `disableAutoFetch = true` is set.
With the code dispatching a "pageopen" event on the existing (general) `BaseViewer` event "pagesinit", in practice this means that the `Set` is always being created. Hence we can simplify the method overall, by always initializing the `this._pageOpenPendingSet` property.
Given that "pageopen" events are not guaranteed to occur, if the page becomes inactive *before* it finishes rendering, we should probably also avoid dispatching a "pageclose" event in that case to avoid confusing/inconsistent state in any event handlers.
Ensure that `PDFViewerApplication._contentLength` is always updated with the *correct* length, as returned by `PDFDocumentProxy.getDownloadInfo`, and only let the `PDFViewerApplication._initializeMetadata` method overwrite if it's not already been set.
Finally, in `PDFViewerApplication._initializeJavaScript`, the fallback `_contentLength` handling is now moved to just after the fallback `documentInfo` handling, such that all the fallback code is in one place within the method.
With the updated default viewer UI, a couple of `dir`-dependent CSS rules have now become redundant since *identical* rules are being specified for both LTR and RTL mode.
Furthermore, there's also some unnecessary re-defining of the `toolbarButton`/`secondaryToolbarButton`-icon related CSS rules.
Finally, for the toggle-buttons there's a particular styling applied to the `:hover:active` state, however the color wasn't defined with CSS variables.
With the updated default viewer UI, a couple of the toolbarButton icons are now *vertically* symmetrical; hence we can remove some now unneeded `transform: scaleX(-1);` rules from the viewer CSS.
Note how the `onerror` functionality is not being used in the GENERIC `DownloadManager`, since we have no way of knowing if downloading succeeded.
Hence this functionality is only *possibly* useful in MOZCENTRAL builds, however as outlined in the existing comments it's unlikely to be helpful in practice. Generally speaking, if downloading failed once in [`PdfStreamConverter.jsm`](https://searchfox.org/mozilla-central/rev/809ac3660845fef6faf18ec210232fdadc0f1ad9/toolkit/components/pdfjs/content/PdfStreamConverter.jsm#294-406) it seems very likely that it would fail again; all-in-all I'm thus suggesting that we just remove the `onerror` functionality altogether here.
Currently this code is duplicated no less than three times in the `web/app.js` file, and by introducing a helper method we can avoid unnecessary repetition.
There's a fair number of cases where `FirefoxCom.request`-calls are manually wrapped in a Promise to make it asynchronous. We can reduce the amount of boilerplate code in these cases by introducing a new `FirefoxCom.requestAsync` method instead.
Furthermore, a couple of `FirefoxCom.request`-calls in the `DownloadManager` are also changed to be asynchronous rather than using callback-functions.
With this patch, we're thus able to replace a lot of *direct* usages of `FirefoxCom.request` with the new `FirefoxCom.requestAsync` method instead.
*Please note:* It's highly recommended to ignore whitespace-only changes when looking at this patch.
Besides modernizing this code, by converting it to a standard class, the existing JSDoc comments are updated to actually agree better with the way that this functionality is used now. (The next patch will reduce usage of `FirefoxCom.request` significantly, hence the JSDocs for the optional `callback` is removed to not unnecessarily advertise that functionality.)
Finally, the unnecessary/unused `return` statement at the end of `FirefoxCom.request` is also removed.
This is the "modern" way of removing a node from the DOM, which has the benefit of being a lot shorter and more concise.
Also, this patch removes the `return` statement from the "pdf.js.response" event listener, since it's always `undefined`, given that none of the `callback`-functions used here ever return anything (and don't need to either). Generally speaking, returning a value from an event listener isn't normally necessary either.
This method currently accepts a callback-function, which does feel a bit old fashioned now. At the time that this code was introduced, native Promises didn't exist yet and there's a custom Promise-implementation used instead.
However, today with Promises and async/await being used *a lot* it seems reasonable to change `DefaultExternalServices.fallback` to an `async` method instead such that the callback-function can be removed.
Note how the end of the `{PDFOutlineViewer, PDFAttachmentViewer, PDFLayerViewer}.render` methods share *almost* identical code, hence we can reduce some duplication by introducing the new `BaseTreeViewer` helper method here.
Furthermore, setting `this._lastToggleIsShow` can be made ever so slightly more efficient, since we don't care about the number of ".treeItemsHidden"-classes but only want to know if at least one exists.
This follows the same principle as the `once` option that exists in the native `addEventListener` method, and will thus automatically remove an `EventBus` listener when it's invoked; see https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Parameters
Finally, this patch also tweaks some the existing `EventBus`-code to use modern features such as optional chaining and logical assignment operators.
Given that we already have a `PresentationModeState`-enumeration, we should use that with the "presentationmodechanged" event rather than including separate properties. Note that this new behaviour, of including an enumeration-value in the event, is consistent with lots of other existing viewer-events.
To hopefully avoid issues in custom implementations of the default viewer, any attempt to access the removed properties will now throw.
Similar to e.g. the "locale" option, this in *only* done for those build-targets where the "sandboxBundleSrc" is actually defined.
With these changes we can remove an `AppOptions` dependency from the `web/generic_scripting.js` file, thus limiting *direct* `AppOptions` usage in the default viewer files.
Given that the `dispatchEventInSandbox` method (on the scripting-classes) is asynchronous, there's a very real risk that the events won't be dispatched/handled until *after* their associated functionality has actually run (with the "Will..." events being particularily susceptible to this issue).
To reduce the likelihood of that happening, we can simply `await` the `dispatchEventInSandbox` calls as necessary. A couple of methods are now marked as `async` to support these changes, however that shouldn't be a problem as far as I can tell.
*Please note:* Given that the browser "beforeprint"/"afterprint" events are *synchronous*, we unfortunately cannot await the `WillPrint`/`DidPrint` event dispatching. To fix this properly the web-platform would need support for asynchronous printing, and we'll thus have to hope that things work correctly anyway.
Note that currently the `DidSave` event is not *guaranteed* to actually be dispatched if there's any errors during saving, which is easily fixed by simply moving it to occur in the `finally`-handler in `PDFViewerApplication.save` method.
For the `WillPrint`/`DidPrint` events, things are unfortunately more complicated. Currently these events will *only* be dispatched iff the printing request comes from within the viewer itself (e.g. by the user clicking on the "Print" toolbar button), however printing can be triggered in a few additional ways:
- In the GENERIC viewer:
- By the <kbd>Ctrl</kbd>+<kbd>P</kbd> keyboard shortcut.
- In the MOZCENTRAL viewer, i.e. the Firefox built-in viewer:
- By the <kbd>Ctrl</kbd>+<kbd>P</kbd> keyboard shortcut.
- By the "Print" item, as found in either the Firefox "Hamburger menu" or in the browser-window menu.
In either of the cases described above, no `WillPrint`/`DidPrint` events will be dispatched. In order to *guarantee* that things work in the general case, we thus have to move the `dispatchEventInSandbox` calls to the "beforeprint"/"afterprint" event handlers instead.
Rather than calling `getJavaScript` in the API and then ignoring the result, when "enableScripting" is set, it should be more efficient/faster to simply skip it altogether instead.
Finally, the `setTimeout` call at the end of `PDFViewerApplication._initializeAutoPrint` is removed, since it doesn't seem necessary any more as far as I can tell.[1]
Note that when this functionality was originally added, back in PR 2839, it seems that `pagesPromise` simply waited for the `getPage` calls of *all* pages to resolve. Today, on the other hand, the viewer fetches *and* renders the first page *before* doing the remaining `getPage` calls, and only afterwards is `pagesPromise` resolved. Hence it's not really clear why we now need to delay printing even further with a `setTimeout` call.
---
[1] The patch was tested with the following documents: https://github.com/mozilla/pdf.js/blob/master/test/pdfs/bug1001080.pdf and https://github.com/mozilla/pdf.js/blob/master/test/pdfs/issue6106.pdf
These callbacks should not be necessary *before* the document has been initialized. Furthermore, move the functionality to a new helper-method since `PDFViewerApplication.load` is already quite large.
Given that this relies on accessing properties on the `PDFDocumentProxy`-instance, it seems more appropriate for this code to live in `PDFViewerApplication`.
It seems that the timeout is way too short in practice, since this new integration-test failed *intermittently* already in PR 12702 (which is where the test was added).
The ideal solution here would be to simply await an event, dispatched by the viewer, however that unfortunately doesn't appear to be supported by Puppeteer.
Instead, the solution implemented here is to add a new method in `PDFViewerApplication` which Puppeteer can query to check if the scripting/sandbox has been fully initialized.
There's really no point, as far as I can tell, to attempt to dispatch an event in a non-existent sandbox. Generally speaking, even trying to do this *could* possibly even lead to errors in some cases.
Furthermore, utilize optional chaining to simplify some `dispatchEventInSandbox` calls throughout the viewer.
Finally, replace superfluous `return` statements with `break` in the switch-statement in the `updateFromSandbox` event-handler.
There's no really compelling reason, as far as I can tell, to introduce the `ENABLE_SCRIPTING` build-target, instead of simply re-using the existing `TESTING` build-target for the new `gulp integrationtest` task.
In general there should be no problem with just always enable scripting in TESTING-builds, and if I were to *guess* the reason that this didn't seem to work was most likely because the Preferences ended up over-writing the `AppOptions`.
As it turns out the GENERIC-viewer has already has built-in support for disabling of Preferences, via the `AppOptions`, and this can be utilized in TESTING-builds as well to ensure that whatever `AppOptions` are set they're always respected.
For DOM events all event names are lower-case, and the newly added PDF.js scripting-events thus "stick out" quite a bit. Even more so, considering that our internal `eventBus`-events follow the same naming convention.
Hence this patch, which changes the "updateFromSandbox"/"dispatchEventInSandbox" events to be lower-case instead.
Furthermore, using DOM events for communication *within* the PDF.js code itself (i.e. between code in `web/app.js` and `src/display/annotation_layer.js/`) feels *really* out of place.
That's exactly the reason that we have the `EventBus` abstraction, since it allowed us to remove prior use of DOM events, and this patch thus re-factors the code to make use of the `EventBus` instead for scripting-related events.
Obviously for events targeting a *specific element* using DOM events is still fine, but the "updatefromsandbox"/"dispatcheventinsandbox" ones should be using the `EventBus` internally.
*Drive-by change:* Use the `BaseViewer.currentScaleValue` setter unconditionally in `PDFViewerApplication._initializeJavaScript`, since it accepts either a string or a number.
- Actually remove the `isDown` property when destroying the scripting-instance.
- Mark all `mouseState` usage as "private" in the various classes.
- Ensure that the `AnnotationLayer` actually treats the parameter as properly *optional*, the same way that the viewer components do.
- For now remove the `mouseState` parameter from the `PDFPageView` class, and keep it only on the `BaseViewer`, since it's questionable if all of the scripting-functionality will work all that well without e.g. a full `BaseViewer`.
- Append the `mouseState` to the JSDoc for the `AnnotationElement` class, and just move its definition into the base-`AnnotationElement` class.
