Prior to PR 11601, the `disableCreateObjectURL` option was present on `getDocument` in the API, since it was (potentially) used when decoding JPEG images natively in the browser. Hence setting this option, which was done automatically using compatibility-code, were in some browsers necessary in order for e.g. JPEG images to be correctly rendered.
The downside of the `disableCreateObjectURL` option is that memory usage increases significantly, since we're forced to build and use `data:` URIs (rather than `blob:` URLs).
However, at this point in time the `disableCreateObjectURL` option is only necessary for *some* (non-essential) functionality in the default viewer; in particular:
- The openfile functionality, used only when manually opening a new file in the default viewer.
- The download functionality, used when downloading either the PDF document itself or its attached files (if such exists).
- The print functionality, in the generic `PDFPrintService` implementation.
Hence neither the general PDF.js library, nor the *basic* functionality of the default viewer, depends on the `disableCreateObjectURL` option any more; which is why I'm thus proposing that we remove the option since using it is a performance footgun.
*Please note:* To not outright break currently "supported" browsers, which lack proper `URL.createObjectURL` support, this patch purposely keeps the compatibility-code to explicitly disable `URL.createObjectURL` usage *only* for browsers which are known to not work correctly.[1]
While it's certainly possible that there's additional, likely older, browsers with broken `URL.createObjectURL` support, the last time that these types of problems were reported was over *three* years ago.[2]
Hence in the *very* unlikely event that additional problems occur, as a result of these changes, we can either add a new case in the compatibility-code or simply declare the affected browser as unsupported.
---
[1] Which are IE11 (see issue 3977), and Google Chrome on iOS (see PR 8081).
[2] Given that `URL.createObjectURL` is used by default, you'd really expect more reports if these problems were widespread.
With recent changes, these event handlers are now essentially identical. Hence a new helper function is added, to reduce unnecessary duplication (will also be helpful with upcoming changes).
These two classes are unsurprisingly quite similar, and with upcoming changes[1] the amount of (essentially) duplicated code will increase even further.
Notable changes:
- Collect shared functionality in the `BaseTreeViewer` class, reducing both current and future code-duplication.
- Reduce unnecessary duplication in the CSS rules, which will be particularly useful with upcoming changes.
- Tweak the attachmentsView to use links, rather than buttons, to simplify (primarily) the CSS rules.
---
[1] Once API support for "Optional Content" lands, I've got more-or-less finished patches to add viewer support as well.
While the parameter name (clearly) suggests that an `AnnotationStorage`-instance is expected, looking at the only call-sites that include the parameter (i.e. the `PDFPrintServiceFactory` instances) it actually contains just a normal Object.
Hence it seems much more reasonable to actually pass a valid `AnnotationStorage`-instance, as the name suggests, and simply have `PDFPageProxy.render` do the `annotationStorage.getAll()` call. (Since we cannot send an `AnnotationStorage`-instance as-is to the worker-thread, given the "structured clone algorithm".)
Since the attachment fetching/parsing is already asynchronous, possibly delaying the dispatching of an "attachmentsloaded" event should thus not be a problem in general.
Note that in some cases, i.e. PDF documents with no "regular" attachments and only FileAttachment annotations, we'll thus no longer dispatch an "attachmentsloaded" event when `attachmentsCount === 0` and instead wait for the FileAttachment parsing to finish. (The use of a timeout still guarantees that an "attachmentsloaded" event is *eventually* dispatched though.)
This patch *considerably* simplifies the "attachmentsloaded" event handler, in `PDFSidebar`, since the details are now abstracted away from the consumer.
Finally, add a check in `_appendAttachment` to ensure (indirectly) that the FileAttachment annotation is actually relevant for the active document.
This approach is already used in other parts of the code-base, see e.g. `PDFOutlineViewer`, and has the advantage of only invalidating the DOM once rather than for every attachment item.
This reverts commit 50f73092e1. This
causes an inconsistency with the integrated find bar that should be
discussed more before moving on with this (refer to PR #12141).
*The [api-minor] label probably ought to have been added to the original PR, given the changes to the `createAnnotationLayerBuilder` signature (if nothing else).*
This patch fixes the following things:
- Let the `AnnotationLayer.render` method create an `AnnotationStorage`-instance if none was provided, thus making the parameter *properly* optional. This not only fixes the reference tests, it also prevents issues when the viewer components are used.
- Stop exporting `AnnotationStorage` in the official API, i.e. the `src/pdf.js` file, since it's no longer necessary given the change above. Generally speaking, unless absolutely necessary we probably shouldn't export unused things in the API.
- Fix a number of JSDocs `typedef`s, in `src/display/` and `web/` code, to actually account for the new `annotationStorage` parameter.
- Update `web/interfaces.js` to account for the changes in `createAnnotationLayerBuilder`.
- Initialize the storage, in `AnnotationStorage`, using `Object.create(null)` rather than `{}` (which is the PDF.js default).
These changes improves the existing search functionality triggered when
the URL contains a `#search` hash.
In addition to performing the actual search immediately like before,
this will ensure the search text field inside the find bar gets
populated with the text currently being searched for.
- Given the `DefaultExternalServices` implementation, the `PDFViewerApplication.supportsDocumentFonts` getter is guaranteed to be defined and we can thus remove some (now) unnecessary `PDFJSDev` checks from the `webViewerInitialized` function.
- By slightly tweaking the "pdfBugEnabled" definition in `web/app_options`, similar to the existing ones for "workerSrc" and "cMapUrl", we can remove some `PDFJSDev` checks from the `PDFViewerApplication._parseHashParameters` method.
The current behavior for `getPagesOverview` assumes we want to only
auto-rotate if:
- `enablePrintAutoRotate` is `true`
- `isFirstPagePortrait !== isPortraitOrientation(size)`
This second check is what is breaking #9297. The two PDFs linked have a
landscape orientation first page, as well as subsequent pages. Since
`false === false`, we print portrait.
Let's drop the comparison with `isFirstPagePortrait`, and print
landscape if `!isPortraitOrientation(size)`.
Fixes#9297.
Given the dummy-methods on `DefaultExternalServices`, there's no longer any compelling reason not to (attempt to) report telemetry unconditionally.
The only larger change consists of moving the `KNOWN_VERSIONS` and `KNOWN_GENERATORS` arrays ouf of the `PDFViewerApplication._initializeMetadata` method.
*Please note:* Most of this patch consists of whitespace-only changes.
There's a few things that could be improved in the current implementation, such as:
- It's currently necessary to *both* manually track the `featureId`s which should trigger delayed fallback, as well as manually report telemetry in affected cases.
Obviously there's only two call-sites as of now (forms and javaScript), but it still feels somewhat error-prone especially if more cases were to be added in the future. To address this, this patch adds a new (private) method which abstracts away these details from the call-sites.
- Generally, it also seems nice to reduce *and* simplify the amount of state we need to store on `PDFViewerApplication` in order to support the "delayedFallback" functionality.
Also, having to *manually* work with the "delayedFallback"-array in multiple places feels cumbersome and makes e.g. the `PDFViewerApplication.fallback` method less clear as to its behaviour.
- Having code *outside* of `PDFViewerApplication`, i.e. in the event handlers, directly access properties which are marked as "private" via a leading underscore doesn't seem that great in general.
Furthermore, having the event handlers directly deal with that should be internal state also seem unfortunate. To address this, the patch will instead make use of a new `PDFViewerApplication.triggerDelayedFallback` callback.
- There's at least one code-path in the viewer, see `PDFViewerApplication.error`, where `fallback` can be called without an argument.
It's currently possible (although maybe somewhat unlikely) that such a call *could* be overridden by the `featureId` of a pending "delayedFallback" call, thus not reporting the *correct* fallback reason.
- The "delayedFallback"-state weren't being reset on document close (which shouldn't affect Firefox, but nonetheless it ought to be fixed).
For now we need to use a Babel-plugin, since Webpack 4.x doesn't seem to support it yet. (Most likely we'll have to update to Webpack 5, once that becomes available, in order for this to be directly supported. This is thus also blocked on removing the `webpack-stream` package.)
While the `??` operator will thus always be transpiled by Babel, even in modern builds, simply supporting it for development purposes seems like a step in the right direction.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing_operator
With the changes in previous patches, the `disableCreateObjectURL` option/functionality is no longer used for anything in the API and/or in the Worker code.
Note however that there's some functionality, mainly related to file loading/downloading, in the GENERIC version of the default viewer which still depends on this option.
Hence the `disableCreateObjectURL` option (and related compatibility code) is moved into the viewer, see e.g. `web/app_options.js`, such that it's still available in the default viewer.
With these changes SystemJS is now only used, during development, on the worker-thread and in the unit/font-tests, since Firefox is currently missing support for worker modules; please see https://bugzilla.mozilla.org/show_bug.cgi?id=1247687
Hence all the JavaScript files in the `web/` and `src/display/` folders are now loaded *natively* by the browser (during development) using standard `import` statements/calls, thanks to a nice `import-maps` polyfill.
*Please note:* As soon as https://bugzilla.mozilla.org/show_bug.cgi?id=1247687 is fixed in Firefox, we should be able to remove all traces of SystemJS and thus finally be able to use every possible modern JavaScript feature.
Given that the `getPDFFileNameFromURL` helper function has a specific code-path for handling non-string inputs, this empty string fallback really isn't necessary at the call-site in `web/pdf_document_properties.js`.
Apparently the old link format used in MOZCENTRAL-builds, with the blob URL separated from the filename with a `?` character violates the specification; see https://bugzilla.mozilla.org/show_bug.cgi?id=1632644#c5
Obviously just removing the `?`-part of the URL would have worked, but that would also have meant that we'd no longer be able to provide the correct filename when the user attempts to download the opened PDF attachment.
To fix this we'll instead append the filename in the hash-part of the URL, which however required using a *custom* hash-parameter to avoid triggering the fallback "named destination" code-paths in the viewer.
Note that only changing the `web/pdf_attachment_viewer.js` file wasn't sufficient to fix the bug, and we also need to tweak the `webViewerInitialized` function in `web/app.js` since MOZCENTRAL-builds used to ignore *everything* in the URL hash.
This particular code is very old, but changing it *should* be completely safe given that the `PDFViewerApplication.setTitleUsingUrl` method since some time now stores both the original URL (in `this.url`) as well as one without the hash (in `this.baseUrl`). The latter one is already used everywhere where it matters, so this change seem fine to me.
This patch thus restores the original behaviour for PDF attachments in the MOZCENTRAL-build, by once again allowing them to be opened *directly* in the browser without downloading. (The fallback added in PR 11845 is obviously kept, since it seems generally useful to have.)
Originally the `default_preferences.json` file was checked into the repository, and we thus needed to load it in non-PRODUCTION mode (which was originally done asynchronously using `XMLHttpRequest`). Over the years a lot has changed and the `default_preferences.json` file is now built, by the `gulp default_preferences` task, from the `web/app_options.js` file. Hence it's no longer necessary, in non-PRODUCTION mode, to use SystemJS here since we can simply use a standard `import` statement instead.
Note how e.g. `web/app.js` already imports from `web/app_options.js` in the same exact way that `web/preferences.js` now does, hence this patch will *not* result in any significant changes in the built/bundled viewer file.
This is another (small) part in trying to reduce usage of SystemJS, with the goal of hopefully getting rid of it completely. (I've started working on this, and doing so has identified a number of problem areas; this patch addresses one of them.)
This replaces some additional `require`/`exports` usage with standard `import`/`export` statements instead.
Hence another, small, part in the effort to reduce the reliance on SystemJS-specific functionality in the development viewer.
Given that the `PDFLinkService.setHash` method itself if completely synchronous, moving the handling of "nameddest" to occur last *shouldn't* cause any problems (famous last words).
This way the destination will still override any previous parameter, such as e.g. the "page", as expected. Furthermore, given that the `PDFLinkService.navigateTo` method is asynchronous that should provide additional guarantees that the "nameddest" parameter is always respected.
As sort-of expected, this fairly innocent looking change also required some tweaks in the `PDFHistory` to prevent dummy history entires upon document load (only an issue when both "page" *and* "nameddest" parameters are provided in the hash).
This rule complements the existing `accessor-pairs` nicely, and ensures that a getter/setter pair is always consistently ordered.
Please find additional details about this rule at https://eslint.org/docs/rules/grouped-accessor-pairs
By preserving the exception type, more fine-grained error handling can be performed via client-side logic (e.g. redirect to a search page if a PDF is not found, or to a ticket system in case of invalid PDF files).
The original exception is now re-thrown.
Fixes#11658
This is a simple work-around for https://bugzilla.mozilla.org/show_bug.cgi?id=1632644 which was caused by platform changes in Firefox. Ideally the Firefox bug should still be fixed, but these PDF.js changes seem generally useful to prevent both current and future issues here.
There's no particular reason for using the PDF.js helper function `createObjectURL` here, given that the relevant code-path is already guarded by multiple "disableCreateObjectURL" option checks.
Given that `URL.createObjectURL` is assumed to always be available in MOZCENTRAL builds, note the existing usage in the file, there's no reason to depend on the PDF.js helper function `createObjectURL` at all here.
Furthermore this patch also changes `DownloadManager.downloadData` to actually revoke the `blobUrl` after downloading has completed, which is similar to the existing code in `DownloadManager.download`.
This is necessary in order to support cases where the default viewer is embedded in a *dynamically* created <iframe> element.
In order to also support a use-case where there's *multiple* <iframe> elements (containing default viewers) on the same page, the "webviewerloaded" event now includes a `source` detail parameter such that it's possible to associate an event with the relevant DOM element.
Somewhat surprisingly, despite the GENERIC viewer implementing "openfile" support, there's never been a keyboard shortcut available. Similar to the previous patch, this utilizes the `EventBus` for consistency with the `Toolbar`/`SecondaryToolbar` buttons.
*Please note:* This patch should NOT be construed as carte blanche to simply convert all of the code in `webViewerKeyDown`, or elsewhere, to make use of the `EventBus` instead of direct function calls.
Any further changes, along the lines in this patch, would need to be evaluated on a case-by-case basis to determine if they are actually wanted, given that many/most existing cases in `webViewerKeyDown` should already be *indirectly* observable through the `EventBus` instance.
This improves the consistency of the "download" handling, in the default viewer, such that the `Toolbar`/`SecondaryToolbar` buttons *and* the keyboard shortcut are now handled in the same way (using the `EventBus`).
Given that the "download" keyboard shortcut handling is limited to GENERIC/CHROME builds and that the issue does raise a valid point about only being able to observe *some* downloads, these changes seem acceptable in this particular case.
Finally the pre-processor condition is adjusted to *explicitly*, rather than implicitly, list the affected build targets.
*Please note:* This patch should NOT be construed as carte blanche to simply convert all of the code in `webViewerKeyDown`, or elsewhere, to make use of the `EventBus` instead of direct function calls.
Any further changes, along the lines in this patch, would need to be evaluated on a case-by-case basis to determine if they are actually wanted, given that many/most existing cases in `webViewerKeyDown` should already be *indirectly* observable through the `EventBus` instance.
These two `AppOptions` are only defined in GENERIC builds, hence it's completely unnecessary to check them in the extension builds (e.g. MOZCENTRAL and CHROME).
Also, simply let the "printResolution" option be defined in all builds since it's being accessed in `web/firefox_print_service.js` as well.
*Please note:* These changes were done automatically, using the `gulp lint --fix` command.
This rule is already enabled in mozilla-central, see https://searchfox.org/mozilla-central/rev/567b68b8ff4b6d607ba34a6f1926873d21a7b4d7/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js#103-104
The main advantage, besides improved consistency, of this rule is that it reduces the size of the code (by 3 bytes for each case). In the PDF.js code-base there's close to 8000 instances being fixed by the `dot-notation` ESLint rule, which end up reducing the size of even the *built* files significantly; the total size of the `gulp mozcentral` build target changes from `3 247 456` to `3 224 278` bytes, which is a *reduction* of `23 178` bytes (or ~0.7%) for a completely mechanical change.
A large number of these changes affect the (large) lookup tables used on the worker-thread, but given that they are still initialized lazily I don't *think* that the new formatting this patch introduces should undo any of the improvements from PR 6915.
Please find additional details about the ESLint rule at https://eslint.org/docs/rules/dot-notation
What the user did:
Open the PDF Viewer in Chrome;
Mouse click into the “Page number” input field;
What they saw:
A pop-up list with seemingly random numbers
What you were expecting to see:
Nothing
What they see is the Chrome “Autofill” feature at work – that is suggesting values that you have previously entered into number fields in forms, as possible values you may want to enter into this field. The list has nothing to do with the PDF currently open but the user does not know this.
Please note that these changes were done automatically, using `gulp lint --fix`.
Given that the major version number was increased, there's a fair number of (primarily whitespace) changes; please see https://prettier.io/blog/2020/03/21/2.0.0.html
In order to reduce the size of these changes somewhat, this patch maintains the old "arrowParens" style for now (once mozilla-central updates Prettier we can simply choose the same formatting, assuming it will differ here).
*Please note:* Most of the necessary API work was done in PR 10033, and the only remaining thing to do here was to implement it in the viewer.
The new preference should thus allow e.g. enterprise users to disable copying in the viewer, for PDF documents whose permissions specify that.
In order to simplify things the "copy"-permission was implemented using CSS, as suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=792816#c55, which should hopefully suffice.[1]
The advantage of this approach, as opposed to e.g. disabling the `textLayer` completely, is first of all that it ensures that searching still works correctly even in copy-protected documents. Secondly this also greatly simplifies the overall implementation, since it doesn't require a lot of code for something that's disabled by default.
---
[1] As the discussion in the bug shows, this kind of copy-protection is not very strong and is also generally easy to remove/circumvent in various ways. Hence a simple solution, targeting "regular"-users rather than "power"-users is hopefully deemed acceptable here.
For years now, the `Font.exportData` method has (because of its previous implementation) been exporting many properties despite them being completely unused on the main-thread and/or in the API.
This is unfortunate, since among those properties there's a number of potentially very large data-structures, containing e.g. Arrays and Objects, which thus have to be first structured cloned and then stored on the main-thread.
With the changes in this patch, we'll thus by default save memory for *every* `Font` instance created (there can be a lot in longer documents). The memory savings obviously depends a lot on the actual font data, but some approximate figures are: For non-embedded fonts it can save a couple of kilobytes, for simple embedded fonts a handful of kilobytes, and for composite fonts the size of this auxiliary can even be larger than the actual font program itself.
All-in-all, there's no good reason to keep exporting these properties by default when they're unused. However, since we cannot be sure that every property is unused in custom implementations of the PDF.js library, this patch adds a new `getDocument` option (named `fontExtraProperties`) that still allows access to the following properties:
- "cMap": An internal data structure, only used with composite fonts and never really intended to be exposed on the main-thread and/or in the API.
Note also that the `CMap`/`IdentityCMap` classes are a lot more complex than simple Objects, but only their "internal" properties survive the structured cloning used to send data to the main-thread. Given that CMaps can often be *very* large, not exporting them can also save a fair bit of memory.
- "defaultEncoding": An internal property used with simple fonts, and used when building the glyph mapping on the worker-thread. Considering how complex that topic is, and given that not all font types are handled identically, exposing this on the main-thread and/or in the API most likely isn't useful.
- "differences": An internal property used with simple fonts, and used when building the glyph mapping on the worker-thread. Considering how complex that topic is, and given that not all font types are handled identically, exposing this on the main-thread and/or in the API most likely isn't useful.
- "isSymbolicFont": An internal property, used during font parsing and building of the glyph mapping on the worker-thread.
- "seacMap": An internal map, only potentially used with *some* Type1/CFF fonts and never intended to be exposed in the API. The existing `Font.{charToGlyph, charToGlyphs}` functionality already takes this data into account when handling text.
- "toFontChar": The glyph map, necessary for mapping characters to glyphs in the font, which is built upon the various encoding information contained in the font dictionary and/or font program. This is not directly used on the main-thread and/or in the API.
- "toUnicode": The unicode map, necessary for text-extraction to work correctly, which is built upon the ToUnicode/CMap information contained in the font dictionary, but not directly used on the main-thread and/or in the API.
- "vmetrics": An array of width data used with fonts which are composite *and* vertical, but not directly used on the main-thread and/or in the API.
- "widths": An array of width data used with most fonts, but not directly used on the main-thread and/or in the API.
- Use template strings when printing document/viewer information in `_initializeMetadata`, since the old format feels overly verbose.
Also, get the WebGL state from the `BaseViewer` instance[1] rather than the `AppOptions`. Since the `AppOptions` value could theoretically have been changed (by the user) after the viewer components were initialized, it seems much more useful to print the *actual* value that'll be used during rendering.
- Change `_initializePdfHistory` to actually do the "is embedded"-check first, in accordance with the comment and given that the "disableHistory" option usually shouldn't be set.
---
[1] Admittedly reaching into the `BaseViewer` instance and just grabbing the value perhaps isn't a great approach overall, but given that the WebGL-backend isn't even on by default this probably doesn't matter too much.
Over time, with more and more API-functionality added, the `PDFViewerApplication.load` method has become quite large and complex. In an attempt to improve the current situation somewhat, this patch moves the fetching and initialization of "auto print" out into its own (private) helper method instead.
Over time, with more and more API-functionality added, the `PDFViewerApplication.load` method has become quite large and complex. In an attempt to improve the current situation somewhat, this patch moves the fetching and initialization of "metadata" out into its own (private) helper method instead.
Over time, with more and more API-functionality added, the `PDFViewerApplication.load` method has become quite large and complex. In an attempt to improve the current situation somewhat, this patch moves the fetching and initialization of "page labels" out into its own (private) helper method instead.
This avoids unnecessary duplication of many images, thus reducing the size of PDF.js image resources slightly.
Note that since the images should only be flipped horizontally, this required specifying the horizontal/vertical scaling separately for the hiDPI-images.
This functionality was only added to the default viewer for backwards compatibility and to support the various PDF viewer tests in mozilla-central, with the intention to eventually remove it completely.
While the different mozilla-central tests cannot be *easily* converted from DOM events, it's however possible to limit that functionality to only MOZCENTRAL builds *and* when tests are running.
Rather than depending of the re-dispatching of internal events to the DOM, the default viewer can instead be used in e.g. the following way:
```javascript
document.addEventListener("webviewerloaded", function() {
PDFViewerApplication.initializedPromise.then(function() {
// The viewer has now been initialized, and its properties can be accessed.
PDFViewerApplication.eventBus.on("pagerendered", function(event) {
console.log("Has rendered page number: " + event.pageNumber);
});
});
});
```
The correct/intended way of working with the "viewer components" is by providing an `EventBus` instance upon initialization, and the `getGlobalEventBus` was only added for backwards compatibility.
Note, for example, that using `getGlobalEventBus` doesn't really work at all well with a use-case where there's *multiple* `PDFViewer` instances on a one page, since it may then be difficult/impossible to tell which viewer a particular event originated from.
All of the "viewer components" examples have been previously updated, such that there's no longer any code/examples which relies on the now removed `getGlobalEventBus` functionality.
Given the way that "classes" were previously implemented in PDF.js, using regular functions and closures, there's a fair number of false positives when the `no-shadow` ESLint rule was enabled.
Note that while *some* of these `eslint-disable` statements can be removed if/when the relevant code is converted to proper `class`es, we'll probably never be able to get rid of all of them given our naming/coding conventions (however I don't really see this being a problem).
Given that none of the relevant options are marked as optional in the code/JSDocs, and that the `PDFDocumentProperties` class is specific to the default viewer (and not exposed as part of the viewer components), there's no good reason as far as I can tell for these checks.
The `BaseViewer.setDocument` method in particular is necessary for rendering to start when the viewer loads, hence you obviously want that to happen as soon as possible and without any unnecessary delays.
Unfortunately *some* API calls need to be done before that, note existing comments, however the `ViewHistory` initialization (and subsequent fetching of data) in particular can be moved slightly without any adverse effects.
As part of testing I've used logging with `performance.now()` inserted in various parts of this code, and there's *obviously* no discernible changes between `master` and this patch for e.g. rendering starting in the viewer.
*Note:* The vast majority of this patch is simple indentation changes, which were forced by Prettier (and done automatically with `gulp lint --fix`).
- Ensure that `database.files` actually contains an Array, rather than some arbitrary data.
- Only try to lookup an existing entry when the `database` existed on load, since there's obviously nothing to find when `database.files = []` was set (this case is very common in the MOZCENTRAL build since `sessionStorage` is being used there).
This property has never been documented and/or *intentionally* exposed through the API, instead the `PDFPageProxy.pageNumber` property is the documented/intended API to use here.
Hence pageIndex is changed to a "private" property on `PDFPageProxy` instances, and internal API functionality is also updated to *consistently* use `this._pageIndex` rather than a mix of formats.
The viewer doesn't currently support executing of any JavaScript, as found in some PDF documents, for security reasons. (Although there's a small hack to at least provide basic support for automatic printing on document load, without running scripts.)
However, in the event that the browser doesn't support printing we're not run *any* of this code. In particular that means that we're also not displaying the "Warning: JavaScript is not supported" message, which seems strange since JavaScript found in a PDF document can really contain *anything* (and not only printing instructions).
It thus seem reasonable, as far as I'm concerned, to always display the JavaScript-warning *even* if printing happens to be unsupported.
*This is part of a series of patches that will try to split PR 11566 into smaller chunks, to make reviewing more feasible.*
Once all the code has been fixed, we'll be able to eventually enable the ESLint `no-shadow` rule; see https://eslint.org/docs/rules/no-shadow
While Firefox originally used transition effects for browser UI toolbar buttons, that was removed years ago in https://bugzilla.mozilla.org/show_bug.cgi?id=1393057
Since the PDF.js viewer toolbar transitions were likely based on the Firefox ones, it seems reasonable that these transition effects are removed from PDF.js as well. Besides removing a bunch of CSS, this also makes the toolbar feel ever so slightly more "snappy" without these delays on mouse interaction.
(In order to make it more feasible to modernize/improve the viewer UI, trying to clean-up/simplify existing rules iteratively seems like the most reasonble way to make any progress here w.r.t. being able to test/review things.)
For reasons that I don't even pretend to understand, the `textLayerFactory` property is determined for *every single* page in the PDF document.
Given that the `TextLayerMode` should be consistent for *all* pages in a document, we obviously could/should define `textLayerFactory` just once instead.
There's a couple of issues with this functionality:
- The respective `PromiseCapability` instances are not being reset, in `BaseViewer._resetView`, when the document is closed which is inconsistent with all other state.
- While the default viewer depends on these promises, and they thus ought to be considered part of e.g. the `PDFViewer` API-surface, they're not really defined in a particularily user-visible way (being that they're attached to the `BaseViewer` instance *inline* in `BaseViewer.setDocument`).
- There's some internal `BaseViewer` state, e.g. `BaseViewer._pageViewsReady`, which is tracked manually and could instead be tracked indirectly via the relevant `PromiseCapability`, thus reducing the need to track state *twice* since that's always best to avoid.
*Please note:* In the existing implementation, these promises are not defined *until* the `BaseViewer.setDocument` method has been called.
While it would've been simple to lift that restriction in this patch, I'm purposely choosing *not* to do so since this ensures that any Promise handlers added inside of `BaseViewer.setDocument` are always invoked *before* any external ones (and keeping that behaviour seems generally reasonable).
This patch deprecates the existing `getOpenActionDestination` API method, in favor of a better and more general `getOpenAction` method instead. (For now JavaScript actions, related to printing, are still handled as before.)
By clearly separating "regular" Print actions from the JavaScript handling, it's thus possible to get rid of the somewhat annoying and strictly incorrect warning when the viewer loads.
It occured to me that similar to the `getGlobalEventBus` function, it's probably a good idea to *also* notify users of the fact that `eventBusDispatchToDOM` is now deprecated.
Rather than depending of the re-dispatching of internal events to the DOM, the default viewer can instead be used in e.g. the following way:
```javascript
document.addEventListener("webviewerloaded", function() {
PDFViewerApplication.initializedPromise.then(function() {
// The viewer has now been initialized, and its properties can be accessed.
PDFViewerApplication.eventBus.on("pagerendered", function(event) {
console.log("Has rendered page number: " + event.pageNumber);
});
});
});
```
I overlooked this in PR 11631; sorry about that!
Also, ensure that `EventBus` instances *always* track "external" events using a boolean regardless of the actual option value.
This changes the dropdown icon from being set using the `background` CSS property, to being set with `::after` which is *similar* to all the other toolbar button icons (which use `::before`).
Also tweaks the dropdown `background-color` on `:hover` slightly, since the other changes made it too light.
Since the goal has always been, essentially since the `EventBus` abstraction was added, to remove all dispatching of DOM events[1] from the viewer components this patch tries to address one thing that came up when updating the examples:
The DOM events are always dispatched last, and it's thus guaranteed that all internal event listeners have been invoked first.
However, there's no such guarantees with the general `EventBus` functionality and the order in which event listeners are invoked is *not* specified. With the promotion of the `EventBus` in the examples, over DOM events, it seems like a good idea to at least *try* to keep this ordering invariant[2] intact.
Obviously this won't prevent anyone from manually calling the new *internal* viewer component methods on the `EventBus`, but hopefully that won't be too common since any existing third-party code would obviously use the `on`/`off` methods and that all of the examples shows the *correct* usage (which should be similarily documented on the "Third party viewer usage" Wiki-page).
---
[1] Looking at the various Firefox-tests, I'm not sure that it'll be possible to (easily) re-write all of them to not rely on DOM events (since getting access to `PDFViewerApplication` might be generally difficult/messy depending on scopes).
In any case, even if technically feasible, it would most likely add *a lot* of complication that may not be desireable in the various Firefox-tests. All-in-all, I'd be fine with keeping the DOM events only for the `MOZCENTRAL` target and gated on `Cu.isInAutomation` (or similar) rather than a preference.
[2] I wouldn't expect any *real* bugs in a custom implementation, simply based on event ordering, but it nonetheless seem like a good idea if any "external" events are still handled last.
To avoid outright breaking third-party usages of the "viewer components" the `getGlobalEventBus` functionality is left intact, but a deprecation message is printed if the function is invoked.
The various examples are updated to *explicitly* initialize an `EventBus` instance, and provide that when initializing the relevant viewer components.
This complements the existing `PDFViewerApplication.initialized` boolean property, and may be helpful for custom implementations of the default viewer. This will thus provide users of the default viewer an alternative to setting the preference to dispatch events to the DOM (and listen for the "localized" event), since they can instead use:
```javascript
document.addEventListener("webviewerloaded", function() {
PDFViewerApplication.initializedPromise.then(function() {
// The viewer has now been initialized.
})
});
```
Note that in order to avoid manually tracking the initialization state *twice*, this implementation purposely uses the `PromiseCapability` functionality to handle both `PDFViewerApplication.initialized` and `PDFViewerApplication.initializedPromise` internally.
This patch contains some *much* needed clean-up of, and improvements to, this old code thus addressing a number of issues:
- Set more reasonable *default* values for the widths, in `web/viewer.css`, since the current ones are actually too small even for the (default) `en-US` locale.
This obviously result in a slightly larger zoom dropdown width for many locales, but the more consistent UI does look good to me.
- Stop setting the `min-width`/`max-width` and just use `width` instead.
- Set an explicit `height` of the zoom dropdown, in an attempt to get Google Chrome to display it with the same height as the toolbar buttons.
- Remove additional `Element.setAttribute("style", ...)` usage.
- Actually check *all* of the predefined l10n strings, since the old implementation (implicitly) assumed that the currently selected one was the longest (note e.g. the `ja-JP` locale where one string is considerably longer than the rest).
- Stop invalidating the DOM multiple times when doing the measurements. This was achieved by using a temporary in-memory `canvas`, and we now only need to query the DOM once in order to get the current font properties of the zoom dropdown.
This should hopefully be useful in environments where restrictive CSPs are in effect.
In most cases the replacement is entirely straighforward, and there's only a couple of special cases:
- For the `src/display/font_loader.js` and `web/pdf_outline_viewer.js `cases, since the elements aren't appended to the document yet, it shouldn't matter if the style properties are set one-by-one rather than all at once.
- For the `web/debugger.js` case, there's really no need to set the `padding` inline at all and the definition was simply moved to `web/viewer.css` instead.
*Please note:* There's still *a single* case left, in `web/toolbar.js` for setting the width of the zoom dropdown, which is left intact for now.
The reasons are that this particular case shouldn't matter for users of the general PDF.js library, and that it'd make a lot more sense to just try and re-factor that very old code anyway (thus fixing the `setAttribute` usage in the process).
Having thought *briefly* about using `css-vars-ponyfill`, I'm no longer convinced that it'd be a good idea. The reason is that if we actually want to properly support CSS variables, then that functionality should be available in *all* of our CSS files.
Note in particular the `pdf_viewer.css` file that's built as part of the `COMPONENTS` target, in which case I really cannot see how a rewrite-at-the-client solution would ever be guaranteed to always work correctly and without accidentally touching other CSS in the surrounding application.
All-in-all, simply re-writing the CSS variables at build-time seems much easier and is thus the approach taken in this patch; courtesy of https://github.com/MadLittleMods/postcss-css-variables
By using its `preserve` option, the built files will thus include *both* a fallback and a modern `var(...)` format[1]. As a proof-of-concept this patch removes a couple of manually added fallback values, and converts an additional sidebar related property to use a CSS variable.
---
[1] Comparing the `master` branch with this patch, when using `gulp generic`, produces the following diff for the built `web/viewer.css` file:
```diff
@@ -408,6 +408,7 @@
:root {
--sidebar-width: 200px;
+ --sidebar-transition-duration: 200ms;
}
* {
@@ -550,27 +551,28 @@
position: absolute;
top: 32px;
bottom: 0;
- width: 200px; /* Here, and elsewhere below, keep the constant value for compatibility
- with older browsers that lack support for CSS variables. */
+ width: 200px;
width: var(--sidebar-width);
visibility: hidden;
z-index: 100;
border-top: 1px solid rgba(51, 51, 51, 1);
-webkit-transition-duration: 200ms;
transition-duration: 200ms;
+ -webkit-transition-duration: var(--sidebar-transition-duration);
+ transition-duration: var(--sidebar-transition-duration);
-webkit-transition-timing-function: ease;
transition-timing-function: ease;
}
html[dir='ltr'] #sidebarContainer {
-webkit-transition-property: left;
transition-property: left;
- left: -200px;
+ left: calc(-1 * 200px);
left: calc(-1 * var(--sidebar-width));
}
html[dir='rtl'] #sidebarContainer {
-webkit-transition-property: right;
transition-property: right;
- right: -200px;
+ right: calc(-1 * 200px);
right: calc(-1 * var(--sidebar-width));
}
@@ -640,6 +642,8 @@
#viewerContainer:not(.pdfPresentationMode) {
-webkit-transition-duration: 200ms;
transition-duration: 200ms;
+ -webkit-transition-duration: var(--sidebar-transition-duration);
+ transition-duration: var(--sidebar-transition-duration);
-webkit-transition-timing-function: ease;
transition-timing-function: ease;
}
```
Since this rule is now enforced in the entire `/web` folder, there's no good reason for it to remain disabled in this file. Most likely, it's added to reduce the number of linting errors when we started enforcing block-scoped variables.
There's no particular reason for using the PDF.js helper function `createObjectURL`[1] in the debugger, instead of the native `URL.createObjectURL` directly, for a couple of reasons:
- The relevant code-path only applies to fonts loaded with the Font Loading API, and this isn't supported in IE anyway.
- The debugger can, since quite some time, not even be loaded in IE any more.
- General support for IE is now limited, and there's no guaratee that everything actually works.
---
[1] It provides a fallback for browsers with broken `Blob` support, which as usual means Internet Explorer :-P
Please find additional details about the ESLint rule at https://eslint.org/docs/rules/prefer-const
With the recent introduction of Prettier this sort of mass enabling of ESLint rules becomes a lot easier, since the code will be automatically reformatted as necessary to account for e.g. changed line lengths.
Note that this patch is generated automatically, by using the ESLint `--fix` argument, and will thus require some additional clean-up (which is done separately).
