When scrolling quickly, the constant re-rendering of the detail view
significantly affects rendering performance, causing Firefox to
not render even the _background canvas_, which is just a static canvas
not being re-drawn by JavaScript.
This commit changes the viewer to only render the detail view while
scrolling if its rendering hasn't just been cancelled. This means that:
- when the user is scrolling slowly, we have enough time to render the
detail view before that we need to change its area, so the user always
sees the full screen as high resolution.
- when the user is scrolling quickly, as soon as we have to cancel a
rendering we just give up, and the user will see the lower resolution
canvas. When then the user stops scrolling, we render the detail view
for the new visible area.
When rendering big PDF pages at high zoom levels, we currently fall back
to CSS zoom to avoid rendering canvases with too many pixels. This
causes zoomed in PDF to look blurry, and the text to be potentially
unreadable.
This commit adds support for rendering _part_ of a page (called
`PDFPageDetailView` in the code), so that we can render portion of a
page in a smaller canvas without hiting the maximun canvas size limit.
Specifically, we render an area of that page that is slightly larger
than the area that is visible on the screen (100% larger in each
direction, unless we have to limit it due to the maximum canvas size).
As the user scrolls around the page, we re-render a new area centered
around what is currently visible.
Converting errors to string drops their stack trace, making it more
difficult to debug their actual reason. We can instead pass the error
objects as-is to console.warn/error, so that Firefox/Chrome devtools
will show both the stack trace of the console.warn/error call, and the
original stack trace of the error.
This commit also enables the `unicorn/no-console-spaces` ESLint rule,
which avoids accidental extra spaces when passing multiple parameters to
`console.*` methods.
Given that this functionality is only relevant in third-party use-cases, for example the viewer-components, we can avoid needlessly including it in e.g. the MOZCENTRAL build.
This patch, first of all, removes circular dependencies in the TypeScript definitions. Secondly, it also moves `RenderingStates` into `web/ui_utils.js` to break another type-dependency and directly use the `XfaLayerBuilder` during XFA-printing.
Finally, note that this patch *slightly* reduces the size of the default viewer (e.g. in the `MOZCENTRAL` build) by not having to bundle code which is completely unused.
This patch circumvents the issues seen when trying to update TypeScript to version `4.5`, by "simply" fixing the broken/missing JSDocs and `typedef`s such that `gulp typestest` now passes.
As always, given that I don't really know anything about TypeScript, I cannot tell if this is a "correct" and/or proper way of doing things; we'll need TypeScript users to help out with testing!
*Please note:* I'm sorry about the size of this patch, but given how intertwined all of this unfortunately is it just didn't seem easy to split this into smaller parts.
However, one good thing about this TypeScript update is that it helped uncover a number of pre-existing bugs in our JSDocs comments.
Note how in `PDFPageViewBuffer.resize` we're manually iterating through the visible pages in order to build a Set of the visible page `id`s. By instead moving the building of this Set into the `getVisibleElements` helper function, as part of the existing parsing, this code becomes *ever so slightly* more efficient.
Furthermore, more direct access to the visible page `id`s also come in handy in other parts of the viewer as well.
In the `BaseViewer.isPageVisible` method we no longer need to loop through the visible pages, but can instead directly check if the pageNumber is visible.
In the `PDFRenderingQueue.getHighestPriority` method, when checking for "holes" in the page layout, we can also avoid some unnecessary look-ups this way.
*Sometimes I'll hopefully learn to optimize my code directly when writing it, rather than having to do multiple clean-up passes; sorry about the churn here!*
For most page layouts there won't be any "holes" in the visible pages (or thumbnails), and in those cases it'd obviously be preferable not having to repeat any checks of already rendered pages.
Rather than only checking the "distance" between the first/last pages, we can instead compare the theoretical number of pages (between first/last) with the actually visible number of pages instead. This way, we're able to better detect the "holes"-case and can skip unnecessary parsing in the common case.
This was a stupid oversight on my part, since the first/last visible pages have obviously already been rendered at the point when we're checking for any potential "holes" in the page layout.
While this will obviously not have any measurable effect on performance, we should nonetheless avoid doing completely unnecessary checks here.
Having recently worked with this code, in PR 14096 (and indirectly in PR 14112), I happened to notice a pre-existing issue with spreadModes at higher zoom levels.
The `PDFRenderingQueue` code was written back when the viewer only supported "normal" vertical scrolling, and some edge-cases related to spreadModes are thus not perfectly supported. Depending on the zoom level, it's possible that there are "holes" in the currently visible page layout, and those pages will not be pre-rendered as you'd expect.
*Steps to reproduce:*
0. Open the viewer, e.g. https://mozilla.github.io/pdf.js/web/viewer.html
1. Enable vertical scrolling.
2. Enable the ODD spreadMode.
3. Scroll down, such that both pages 1 and 3 are visible.
4. Zoom-in until *only* page 1 and 3 are visible.
5. Open the devtools and, using the DOM Inspector, notice how page 2 is *not* being pre-rendered despite all surrounding pages being rendered.
Please note that we (obviously) don't want to unconditionally pre-render more than one page all the time, since that could very easily lead to overall worse performance in some documents.[1]
However, when spreadModes are enabled it does make sense to attempt to pre-render both of the pages of the next/previous spread.
---
[1] Since it may cause pre-rendering to unnecessarily compete for parsing resources, on the worker-thread, with "regular" rendering.
The Viewer API definitions do not compile because of missing imports and
anonymous objects are typed as `Object`. These issues were not caught
during CI because the test project was not compiling anything from the
Viewer API.
As an example of the first problem:
```
/**
* @implements MyInterface
*/
export class MyClass {
...
}
```
will generate a broken definition that doesn’t import MyInterface:
```
/**
* @implements MyInterface
*/
export class MyClass implements MyInterface {
...
}
```
This can be fixed by adding a typedef jsdoc to specify the import:
```
/** @typedef {import("./otherFile").MyInterface} MyInterface */
```
See https://github.com/jsdoc/jsdoc/issues/1537 and
https://github.com/microsoft/TypeScript/issues/22160 for more details.
As an example of the second problem:
```
/**
* Gets the size of the specified page, converted from PDF units to inches.
* @param {Object} An Object containing the properties: {Array} `view`,
* {number} `userUnit`, and {number} `rotate`.
*/
function getPageSizeInches({ view, userUnit, rotate }) {
...
}
```
generates the broken definition:
```
function getPageSizeInches({ view, userUnit, rotate }: Object) {
...
}
```
The jsdoc should specify the type of each nested property:
```
/**
* Gets the size of the specified page, converted from PDF units to inches.
* @param {Object} options An object containing the properties: {Array} `view`,
* {number} `userUnit`, and {number} `rotate`.
* @param {number[]} options.view
* @param {number} options.userUnit
* @param {number} options.rotate
*/
```
Without this patch, when using `PDFPageView` directly[1] this CSS variable won't be updated and consequently things won't work as intended.
This is purposely implemented such that when a `PDFPageView`-instance is part of a viewer, we don't repeatedly set the CSS variable for every single page.
---
[1] See e.g. the "pageviewer" example in the `examples/components/` folder.
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.
Note that a `RenderingCancelledException` *should* never actually reach this method, but better safe than sorry I suppose, considering that both `PDFPageView` and `PDFThumbnailView` are already catching `RenderingCancelledException`s since those are *not* Errors in the normal sense of the word.
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).
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.)
Note that as discussed on IRC, this makes the viewer slightly slower to load *only* in `gulp server` mode, however the difference seem slight enough that I think it will be fine.
