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).
The debugging hash parameters[1] are intended to facilitate access to various tools/settings in PRODUCTION builds, protected by the `pdfBugEnabled` preference.
At this point, the remaining debugging hash parameters are mainly intended to allow access to the `PDFBug` tools and/or to quickly toggle certain larger features.
The "useOnlyCssZoom" functionality doesn't really seem to fit in with the rest of these hash parameters, since:
- This is, comparatively speaking, a minor viewer-specific feature.
- The zooming implementation will (almost) always fallback to CSS-only zooming, for any document, once the canvases becomes large enough. Hence, the majority of the CSS zooming feature can still be tested *directly* in any build of the viewer.
- After the initial implementation, years ago, the CSS-only zooming code in question hasn't changed much (or even at all), i.e. it doesn't seem like an active development target.[2]
- If the "useOnlyCssZoom" functionality was added today, it's unlikely that a hash parameter would've been added.
- Last, but not least, there's also a `useOnlyCssZoom` preference hence toggling this functionality shouldn't be too difficult (e.g. if someone needs to hack on it).
All in all, I'm thus suggesting that we remove the "useOnlyCssZoom" hash parameter.
---
[1] Originally these hash parameters could be used directly in any build, which was bad since it would allow any link to potentially disable functionality and/or reduce performance.
[2] If it had seen active development over the years, I'd be *much* more inclined to keep the hash parameter.
For people running e.g. Firefox with the `pdfBugEnabled` preference set, to allow quick access to debugging tools, this method will obviously run for every opened PDF file. However, in most cases the URL hash is empty and we can thus skip most of the parsing and simply return early instead.
This removes a couple of, thanks to preceeding code, unnecessary `typeof PDFJSDev` checks, and also fixes a couple of incorrectly implemented (my fault) checks intended for `TESTING` builds.
After PR 9566, which removed all of the old Firefox extension code, the `FIREFOX` build flag is no longer used for anything.
It thus seems to me that it should be removed, for a couple of reasons:
- It's simply dead code now, which only serves to add confusion when looking at the `PDFJSDev` calls.
- It used to be that `MOZCENTRAL` and `FIREFOX` was *almost* always used together. However, ever since PR 9566 there's obviously been no effort put into keeping the `FIREFOX` build flags up to date.
- In the event that a new, Webextension based, Firefox addon is created in the future you'd still need to audit all `MOZCENTRAL` (and possibly `CHROME`) build flags to see what'd make sense for the addon.
While only the `MOZCENTRAL` builds will actually do anything meaningful with the telemetry data, none of the code in question actually runs *at all* in e.g. development mode.[1]
This seems bad since it essentially means that this code is completely untested, despite being quite important for the built-in Firefox PDF viewer, and this thus ought to be fixed.
In this case, the explanation for the current state of the code should be "for historical reasons". Before the viewer was split into the current components and before the pre-processor was improved, back when all code resided in the `web/viewer.js` file, the telemetry reporting was done with *direct* `FirefoxCom` calls. However, with the dummy `DefaultExternalServices.reportTelemetry` method there's nothing actually preventing attempting to report telemetry in any type of build.
NOTE: By running this code in GENERIC builds as well, in addition to just locally, the *viewer* part of telemetry reporting becomes tested e.g. in preview builds too which should help with reviewing.
---
[1] When fixing bug 1606566, I had to edit the relevant `PDFJSDev` checks to be able to actually test the changes locally.
With https://bugzilla.mozilla.org/show_bug.cgi?id=844349 now being fixed in Firefox, the textLayer will now actually stay hidden as intended regardless of the browser settings.
Hence it should no longer be necessary to display the fallback bar, nor print a warning in the console, for documents which contains a textLayer.
Besides removing the `supportsDocumentColors` methods in the default viewer, we can also remove a now unused l10n string.
This rule is already enabled in mozilla-central, and helps avoid some confusing formatting, see https://searchfox.org/mozilla-central/rev/9e45d74b956be046e5021a746b0c8912f1c27318/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js#209-210
With the recent introduction of Prettier some of the existing nested ternary statements became even more difficult to read, since any possibly helpful indentation was removed.
This particular ESLint rule wasn't entirely straightforward to enable, and I do recognize that there's a certain amount of subjectivity in the changes being made. Generally, the changes in this patch fall into three categories:
- Cases where a value is only clamped to a certain range (the easiest ones to update).
- Cases where the values involved are "simple", such as Numbers and Strings, which are re-factored to initialize the variable with the *default* value and only update it when necessary by using `if`/`else if` statements.
- Cases with more complex and/or larger values, such as TypedArrays, which are re-factored to let the variable be (implicitly) undefined and where all values are then set through `if`/`else if`/`else` statements.
Please find additional details about the ESLint rule at https://eslint.org/docs/rules/no-nested-ternary
As described in the issue, having a DOM element with `id=page2` (or any other number) will automatically cause that element to become linkable through the URL hash. That's currently leading to some confusing and outright wrong behaviour, since it obviously only works for pages that have been loaded and rendered.
For PDF documents the only officially supported way to reference a particular page through the URL hash is using the `#page=2` format, which also works for all pages regardless if they're loaded or not.
As far as I can tell there's nothing in the PDF.js default viewer that actually depends on the page/thumbnail `id` at this point in time, hence why I believe that this removal ought to be safe.
Just as a pre-caution this patch adds an `aria-label` to the page canvas, similar to the thumbnail canvas/image, to at least keep this information in the DOM.
In order to eventually get rid of SystemJS and start using native `import`s instead, we'll need to provide "complete" file identifiers since otherwise there'll be MIME type errors when attempting to use `import`.
When working on PR 11463 I couldn't help thinking that the `Array.prototype.some` callback function, used when determining the generator, was somewhat difficult to read with its partly unused and strangely named parameters.
This is not only useful to have one format for consistency, but also to
be able to quickly search for colors for e.g., finding duplicates or
when tweaking the CSS for custom deployments.
This covers the handful of cases that the `--fix` command couldn't deal with, and the changes aren't just fixing the linting errors but attempt to slightly improve the relevant code.
Please find additional details about the ESLint rule at https://eslint.org/docs/rules/prefer-const
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).
Rather than having a copy of this regular expression in the `test/unit/api_spec.js` file, with a comment about keeping it up-to-date with the code in the viewer (note the incorrect file reference as well), we can just import it instead to simplify all of this.
This patch makes the follow changes:
- Remove no longer necessary inline `// eslint-disable-...` comments.
- Fix `// eslint-disable-...` comments that Prettier moved down, thus causing new linting errors.
- Concatenate strings which now fit on just one line.
- Fix comments that are now too long.
- Finally, and most importantly, adjust comments that Prettier moved down, since the new positions often is confusing or outright wrong.
Note that Prettier, purposely, has only limited [configuration options](https://prettier.io/docs/en/options.html). The configuration file is based on [the one in `mozilla central`](https://searchfox.org/mozilla-central/source/.prettierrc) with just a few additions (to avoid future breakage if the defaults ever changes).
Prettier is being used for a couple of reasons:
- To be consistent with `mozilla-central`, where Prettier is already in use across the tree.
- To ensure a *consistent* coding style everywhere, which is automatically enforced during linting (since Prettier is used as an ESLint plugin). This thus ends "all" formatting disussions once and for all, removing the need for review comments on most stylistic matters.
Many ESLint options are now redundant, and I've tried my best to remove all the now unnecessary options (but I may have missed some).
Note also that since Prettier considers the `printWidth` option as a guide, rather than a hard rule, this patch resorts to a small hack in the ESLint config to ensure that *comments* won't become too long.
*Please note:* This patch is generated automatically, by appending the `--fix` argument to the ESLint call used in the `gulp lint` task. It will thus require some additional clean-up, which will be done in a *separate* commit.
(On a more personal note, I'll readily admit that some of the changes Prettier makes are *extremely* ugly. However, in the name of consistency we'll probably have to live with that.)
Apparently Ghostscript can, in some cases, generate/include `Metadata` with incorrectly encoded characters.[1] This results in the viewer title looking wrong, which we thus attempt to avoid by falling back to the `Info` entry instead.
*Please note:* Obviously it would be better if this was fixed in the `Metadata` parser instead, rather than using a viewer work-around, but I'm just not sure how or even *if* that could actually be done given that the `Metadata` stream contains no trace of the *original* character.
Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1605526
---
[1] The problematic characters are from the Specials Unicode block, see https://en.wikipedia.org/wiki/Specials_(Unicode_block)
With these changes we'll always set the `pdfTitle` to the `Info` dictionary entry *first* (assuming it exists and isn't empty), before attempting to override it with the `Metadata` stream entry (assuming it exists, is non-empty *and* valid).
There should be no functional changes with this patch, but it will simplify the following patch somewhat.
This patch reduces some duplication, by moving *all* fake worker loader code into the `setupFakeWorkerGlobal` function. Furthermore, the functions are simplified further by using `async`/`await` where appropriate.
Looking at the `parseCurrentHash` function again it's now difficult for me to understand *what* I was thinking, since having a helper function that needs to be manually passed a `linkService` reference just looks weird.
This patch addresses a couple of smaller issues with the `PDFHistory` class:
- Most, if not all, other viewer components can be reset in one way or another, and there's no good reason for the `PDFHistory` implementation to be different here.
- Currently it's (technically) possible to keep adding entries to the browser history, via the `PDFHistory` instance, even after the document has been closed. That obviously makes no sense, and is caused by the lack of a `reset` method.
- The internal `this._isPagesLoaded` property was never actually reset, which would lead to it being temporarily wrong when a new document was opened in the default viewer.
The `viewer` option was *only* used for checking that a document is loaded in `PDFPresentationMode.request`, however that's just as easy to do by simply utilizing `BaseViewer.pagesCount` instead and this way we can also avoid the DOM lookup.
This was a blatant oversight in PR 10217, since there's obviously no `this.pageNumber` property anywhere in the `BaseViewer`. Luckily this shouldn't have caused any bugs, since the only call-site is also validating the `pageNumber` (but correctly that time).
*This patch is simple enough that I almost feel like I'm overlooking some trivial reason why this would be a bad idea.*
Note how in `{BaseViewer, PDFThumbnailViewer}.setDocument` we're always getting the *first* `pdfPage` in order to initialize all pages/thumbnails.
However, once that's done the first `pdfPage` is simply ignored and rendering of the first page thus requires calling `PDFDocumentProxy.getPage` yet again. (And in the `BaseViewer` case, it's even done once more after `onePageRenderedCapability` is resolved.)
All in all, I cannot see why we cannot just immediately set the first `pdfPage` and thus avoid an early round-trip to the API in the `_ensurePdfPageLoaded` method before rendering can begin.
This code was originally added to support IE10 (and below), however with those browsers *explicitly* unsupported since PDF.js version `2.0` this code is now dead.
Obviously the `_pagesRequests` functionality is *mainly* used when `disableAutoFetch` is set, but it will also be used during ranged/streamed loading of documents.
However, the `_pagesRequests` property is currently an Array which seems a bit strange:
- Arrays are zero-indexed, but the first element will never actually be set in the code.
- The `_pagesRequests` Array is never cleared, unless a new document is loaded, and once the `PDFDocumentProxy.getPage` call has resolved/rejected the element is just replaced by `null`.
- Unless the document is browsed *in order* the resulting `_pagesRequests` Array can also be arbitrarily sparse.
All in all, I don't believe that an Array is an appropriate data structure to use for this purpose.
This patch simply restores the behaviour that existed prior to PR 7697, since I cannot imagine that that was changed other than by pure accident.
As mentioned by a comment in `BaseViewer.setDocument`: "Printing is semi-broken with auto fetch disabled.", and note that since triggering of printing is a synchronous operation there's generally no easy way to load the missing data.
https://github.com/mozilla/pdf.js/pull/7697/files#diff-529d1853ee1bba753a0fcb40ea778723L1114-L1118
Considering just how small/simple this code is, it doesn't seem necessary to have a separate method for it (even more so when there's only one call-site).
I've always disliked the solution in PR 10461, since it required changes to the `PDFHistory` code itself to deal with a bug in IE11.
Now that IE11 support is limited, it seems reasonable to remove these `pushState`/`replaceState` hacks from the main code-base and simply use polyfills instead.
The code in question is *only* relevant in non-`PRODUCTION` mode, i.e. the *development* version of the viewer run with `gulp server`, and has been completely unused at least since SystemJS was added.
I really cannot see any reason to keep this, since it's code which first of all isn't shipping and secondly isn't even being used in the development viewer.
For *very* long/large documents fetching all pages on load may cause quite bad performance, both memory and CPU wise. In order to at least slightly alleviate this, we can let the viewer treat these kind of documents[1] as if `disableAutoFetch` were set.
---
[1] One example of a really bad case is https://bugzilla.mozilla.org/show_bug.cgi?id=1588435, which this patch should at least help somewhat. In general, for these cases, we'd probably need to implement switching between `PDFViewer`/`PDFSinglePageViewer` (as already tracked on GitHub) and use the latter for these kind of long documents.
The issue has been open for years now, and has even been marked with `5-good-beginner-bug` for *months*, without any movement.
Considering just how simple the suggested solution is, I'm submitting this patch just to close out a long-standing issue.
Obviously userAgent checks aren't that great, since it's very easy to spoof, but it probably doesn't hurt to attempt to extend this (since it's limited to `GENERIC` builds).
Besides, anyone using the default viewer can always set the `maxCanvasPixels` option to a value of their choosing.
Sometimes we also used `@return`, but `@returns` is what the JSDoc
documentation recommends. Even though `@return` works as an alias, it's
good to use the recommended syntax and to be consistent within the
project.
Sometimes we also used `@return` or `@returns`, but `@type` is what
the JSDoc documentation recommends. This also improves the documentation
because before this commit the types were not shown and now they are.
I've absolutely no idea why I wrote the code that way originally, since the nested `if`s are not really helping readability one bit.
Hence this patch which changes things to use early `return`s instead, to help readability.
Rather than manually clamping the `width` here, we can just `export` an already existing helper function from `ui_utils.js` instead.
(Hopefully it will eventually be possible to replace this helper function with a native `Math.clamp` function, given that there exists a "Stage 1 Proposal" for adding such a thing to the ECMAScript specification.)
Besides avoiding errors during loading, this also ensures that the document will be correctly scrolled/zoomed into view once the viewer becomes visible.
This "new" behaviour was always intended, see PR 2613, however various re-factoring over the years seem to have broken this (and I'm probably at least somewhat responsible for that).
By transfering, rather than copying, `ArrayBuffer`s between the main- and worker-threads, you can avoid unnecessary allocations by only having *one* copy of the same data.
Hence manually setting `postMessageTransfers: false`, when calling `getDocument`, is a performance footgun[1] which will do nothing but waste memory.
Given that every reasonably modern browser supports `postMessage` transfers[2], I really don't see why it should be possible to force-disable this functionality.
Looking at the browser support, for `postMessage` transfers[2], it's highly unlikely that PDF.js is even usable in browsers without it. However, the feature testing of `postMessage` transfers is kept for the time being just to err on the safe side.
---
[1] This is somewhat similar to the, now removed, `disableWorker` parameter which also provided API users a much too simple way of reducing performance.
[2] See e.g. https://developer.mozilla.org/en-US/docs/Web/API/MessagePort/postMessage#Browser_compatibility and https://developer.mozilla.org/en-US/docs/Web/API/Transferable#Browser_compatibility
