As part of the changes/improvement in PR 14092, we're no longer using the `addLinkAttributes` directly in e.g. the AnnotationLayer-code.
Given that the helper function is now *only* used in the viewer, hence it no longer seems necessary to expose it through the official API.
*Please note:* It seems somewhat unlikely that third-party users were relying *directly* on the helper function, which is why it's not being exported as part of the viewer components. (If necessary, we can always change this later on.)
This structure contains *almost* exclusively references to DOM elements (and a couple of simple strings), rather than complete classes/functions. Hence the `eventBus`-option sticks out a fair bit, and I'd guess that it's *mostly* unused in e.g. third-party implementations.
Given that we, in multiple places, mention that the default viewer shouldn't be used as-is I really don't think that we need to keep this special `eventBus`-option around. Furthermore, nowadays it's also a lot easier to (safely) access the existing `EventBus`-instance in the viewer; see https://github.com/mozilla/pdf.js/wiki/Third-party-viewer-usage#initialization-promise which shows how to listen for the default viewer being initialized (and its `eventBus` thus being available).
Besides converting `Catalog.getPageDict` to an `async` method, thus simplifying the code, this patch also allows us to pro-actively fix a existing issue.
Note how we're looking up References in such a way that `MissingDataException`s won't cause trouble, however it's *technically possible* that the entries (i.e. /Count, /Kids, and /Type) in a /Pages Dictionary could actually be indirect objects as well. In the existing code this could lead to *some*, or even all, pages failing to load/render as intended.
In practice that doesn't *appear* to happen in real-world PDF documents, but given all the weird things that PDF software do I'd prefer to fix this pro-actively (rather than waiting for a bug report).
With `Catalog.getPageDict` being `async` this is now really simple to address, however I didn't want to introduce a bunch more *unconditional* asynchronicity in this method if it could be avoided (since that could slow things down). Hence we'll *synchronously* lookup the *raw* data in a /Pages Dictionary, and only fallback to asynchronous data lookup when a Reference was encountered.
In addition to the above, this patch also makes the following notable changes:
- Let `Catalog.getPageDict` *consistently* reject with the actual error, regardless of what data we're fetching. Previously we'd "swallow" the actual errors except when looking up Dictionary entries, which is inconsistent and thus seem unfortunate. As can be seen from the updated unit-tests this change is API-observable, hence why the patch is tagged `[api-minor]`.
- Improve the consistency of the Dictionary /Type-checks in both the `Catalog.getPageDict` and `Catalog.getAllPageDicts` methods.
In `Catalog.getPageDict` there's a fallback code-path where we're *incorrectly* checking the /Page Dictionary for a /Contents-entry, which is wrong since a /Page Dictionary doesn't need to have a /Contents-entry in order to be valid.
For consistency the `Catalog.getAllPageDicts` method is also updated to handle errors in the /Type-lookup correctly.
- Reduce the `PagesCountLimit.PAUSE_EAGER_PAGE_INIT` viewer constant, to further improve loading/rendering performance of the *second* page during initialization of very long documents; PR 14359 follow-up.
*This addresses the following case missing from the previous patch:*
The viewer is loaded in an *active* window/tab, and enough time is allowed to pass in order to allow rendering to start. However, if the user then switches to another tab (or another program) *before* rendering has finished, the "load" event also needs to be unblocked.
Given that `requestAnimationFrame` is being used, see the `src/diplay/api.js` file, an inactive window/tab means that rendering will not run and we'll thus not fetch all pages. The latter is a requirement for the "load" event to be unblocked, in the MOZCENTRAL-version of, the default viewer.
This patch is a *partial* solution, since it only addresses the following situations:
- A *background* tab (containing the viewer) is reloaded, e.g. via the tab-bar context menu.
- The viewer is loaded in a active tab, but the user switches away from it (or switches to another program window) *before* rendering has started.
This provides a work-around for badly generated PDF documents that contain *negative* /FitH parameters (in the referenced issue the value `-32768` is used).
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.
The size of the `web/ui_utils.js` file has increased over time, as more code has been added to (or moved into) that file. To reduce its size slightly, this patch moves the event-related functionality into a separate file.
In PR 14114 this was only added to the default viewer, which means that in the viewer components the user would need to *manually* implement /Lang handling. This was (obviously) a bad choice, since the viewer components already support e.g. structTrees by default; sorry about overlooking this!
To avoid having to make *two* `getMetadata` API-calls[1] very early during initialization, in the default viewer, the API will now cache its result. This will also come in handy elsewhere in the default viewer, e.g. by reducing parsing when opening the "document properties" dialog.
---
[1] This not only includes a round-trip to the worker-thread, but also having to re-parse the /Metadata-entry when it exists.
By making this API-call *unconditionally*, we introduce a (slight) delay in the initialization of *all* documents.
That seems quite unfortunate, since `pdfjs.enablePermissions` is off by default, and it thus seem better only do the API-call when actually needed; sorry about this!
For encrypted PDF documents without the required permissions set, this patch adds support for disabling of form editing. However, please note that it also requires that the `pdfjs.enablePermissions` preference is set to `true`[1] (since PDF document permissions could be seen as user hostile).
Based on https://www.adobe.com/content/dam/acom/en/devnet/pdf/pdfs/PDF32000_2008.pdf#G6.1942134, this condition hopefully makes sense.
---
[1] Either manually with `about:config`, or using e.g. a [Group Policy](https://github.com/mozilla/policy-templates).
This patch is essentially *another* continuation of PR 11263, which tried to improve loading/initialization performance of *very* large/long documents.
For most documents, unless they're *very* long, we'll eagerly initialize all of the pages in the viewer. For shorter documents having all pages loaded/initialized early provides overall better performance/UX in the viewer, however there's cases where it can instead *hurt* performance.
For documents with a couple of thousand pages[1], the parsing and pre-rendering of the *second* page of the document can be delayed (quite a bit). The reason for this is that we trigger `PDFDocumentProxy.getPage` for *all pages* early during the viewer initialization, which causes the worker-thread to be swamped with handling (potentially) thousands of `getPage`-calls and leaving very little time for other parsing (such as e.g. of operatorLists).
To address this situation, this patch thus proposes temporarily "pausing" the eager `PDFDocumentProxy.getPage`-calls once a threshold has been reached, to give the worker-thread a change to handle other requests.[2]
Obviously this may *slightly* delay the "pagesloaded" event in longer documents, but considering that it's already the result of asynchronous parsing that'll hopefully not be seen as a blocker for these changes.[3]
---
[1] A particularly problematic example is https://github.com/mozilla/pdf.js/files/876321/kjv.pdf (16 MB large), which is a document with 2236 pages and a /Pages-tree that's only *one* level deep.
[2] Please note that I initially considered simply chaining the `PDFDocumentProxy.getPage`-calls, however that'd slowed things down for all documents which didn't seem appropriate.
[3] This patch will *hopefully* also make it possible to re-visit PR 11312, since it seems that changing `Catalog.getPageDict` to an `async` method wasn't the problem in itself. Rather it appears that it leads to slightly different timings, thus exacerbating the already existing issues with the worker-thread being overloaded by `getPage`-calls.
Having recently worked with that method, there's a couple of (very old) issues that I'd also like to address and having `Catalog.getPageDict` be `async` would simplify things a great deal.
These changes improves the consistency ever so slightly in the `PDFOutlineViewer._dispatchEvent` method, by making sure that we can tell the following two cases apart:
- The "pagesloaded" event has *not yet* been fired.
- The "pagesloaded" event has been fired, but no pages were available.
*This patch can be tested e.g. with the `poppler-85140-0.pdf` document from the test-suite.*
For some sufficiently corrupt documents the `getDocument` call will succeed, but fetching even the very first page fails. Currently we only print error messages (in the console) from the `{BaseViewer, PDFThumbnailViewer}.setDocument` methods, but don't actually provide these errors to allow the viewer to handle them properly.
In practice this means that the GENERIC viewer won't display the `errorWrapper`, and in the MOZCENTRAL viewer the *browser* loading indicator is never hidden (since we never unblock the "load" event).
This patch is essentially a continuation of PR 11263, which tried to improve loading/initialization performance of *very* large/long documents.
Note that browsers, in general, don't handle a huge amount of DOM-elements very well, with really poor (e.g. sluggish scrolling) performance once the number gets "large". Furthermore, at least in Firefox, it seems that DOM-elements towards the bottom of a HTML-page can effectively be ignored; for the PDF.js viewer that means that pages at the end of the document can become impossible to access.
Hence, in order to improve things for these *very* large/long documents, this patch will now enforce usage of the (recently added) PAGE-scrolling mode for these documents. As implemented, this will only happen once the number of pages *exceed* 15000 (which is hopefully rare in practice).
While this might feel a bit jarring to users being *forced* to use PAGE-scrolling, it seems all things considered like a better idea to ensure that the entire document actually remains accessible and with (hopefully) more acceptable performance.
Fixes [bug 1588435](https://bugzilla.mozilla.org/show_bug.cgi?id=1588435), to the extent that doing so is possible since the document contains 25560 pages (and is 197 MB large).
This was added on the assumption that the viewer would (eventually) start using the `PDFSinglePageViewer` for e.g. PAGE-scrolling mode and PresentationMode. However, having both a `PDFViewer` and a `PDFSinglePageViewer` side-by-side in the viewer would've been tricky to implement well, which is why PR 14112 implemented PAGE-scrolling for the general `BaseViewer` instead.
Given that the default viewer is no longer (potentially) going to use `PDFSinglePageViewer`, there's code in the `SecondaryToolbar` (and related CSS rules) which is now unnecessary.
*This patch can be tested e.g. with the `sizes.pdf` document in the test-suite.*
While this patch isn't necessarily the best solution, e.g. it might be possible to solve this with *only* CSS, it's what I was able to come up with to address an old issue.
The solution here re-uses the `spread`-class in PresentationMode, since that one already takes care of centering pages *vertically*, together with a dummy-page that takes up the entire height of the window.
Finally, some PresentationMode-related CSS-rules are also simplified slightly, since the changes in PR 14112 (using Page-scrolling) allows some clean-up here.
The issue that this patch fixes has existed ever since the viewer was first re-factored into components, however it only really affects the `disableAutoFetch = true` mode.
By default we're fetching all pages in `BaseViewer.setDocument`, and as part of the parsing/initialization we're also populating the `PDFLinkService`-pagesRefCache. The purpose of that cache is to make navigating to any internal destinations faster, by not having to (asynchronously) lookup the pageNumber via the API when handling the destination.
In comparison, when the `disableAutoFetch = true` mode is being used we're instead *lazily* initializing the pages in the `BaseViewer.#ensurePdfPageLoaded`-method. For some reason, that I can only assume is a simple oversight, we're not attempting to update the `PDFLinkService`-pagesRefCache in that case.
In the `BaseViewer` this cache is mostly relevant in the `disableAutoFetch = true` mode, since the pages are being initialized *lazily* in that case.
In the `PDFThumbnailViewer` this cache is mostly used for thumbnails that are actually being rendered, as opposed to those created directly from the "regular" pages.
Please note that I'm not suggesting that we remove these caches because they're only used in some situations, but rather because they're for all intents and purposes actually *redundant*. In the API itself, we're already caching both the page-promises and the actual pages themselves on the `WorkerTransport`-instance.
Hence these viewer-caches aren't really necessary in practice, and adds what to me mostly seems like an unnecessary level of indirection.[1]
Given that the viewer now relies on caching in the API itself, this patch also adds a new unit-test to ensure that page-caching works (and keep working) as expected.
---
[1] In the `WorkerTransport.getPage`-method the parameter is being validated on every call, but that's hardly enough code to warrant keeping the "duplicate" caches in the viewer in my opinion.
*Please note:* These changes will primarily benefit longer documents, somewhat at the expense of e.g. one-page documents.
The existing `PDFDocumentProxy.getStats` function, which in the default viewer is called for each rendered page, requires a round-trip to the worker-thread in order to obtain the current document stats. In the default viewer, we currently make one such API-call for *every rendered* page.
This patch proposes replacing that method with a *synchronous* `PDFDocumentProxy.stats` getter instead, combined with re-factoring the worker-thread code by adding a `DocStats`-class to track Stream/Font-types and *only send* them to the main-thread *the first time* that a type is encountered.
Note that in practice most PDF documents only use a fairly limited number of Stream/Font-types, which means that in longer documents most of the `PDFDocumentProxy.getStats`-calls will return the same data.[1]
This re-factoring will obviously benefit longer document the most[2], and could actually be seen as a regression for one-page documents, since in practice there'll usually be a couple of "DocStats" messages sent during the parsing of the first page. However, if the user zooms/rotates the document (which causes re-rendering), note that even a one-page document would start to benefit from these changes.
Another benefit of having the data available/cached in the API is that unless the document stats change during parsing, repeated `PDFDocumentProxy.stats`-calls will return *the same identical* object.
This is something that we can easily take advantage of in the default viewer, by now *only* reporting "documentStats" telemetry[3] when the data actually have changed rather than once per rendered page (again beneficial in longer documents).
---
[1] Furthermore, the maximium number of `StreamType`/`FontType` are `10` respectively `12`, which means that regardless of the complexity and page count in a PDF document there'll never be more than twenty-two "DocStats" messages sent; see 41ac3f0c07/src/shared/util.js (L206-L232)
[2] One example is the `pdf.pdf` document in the test-suite, where rendering all of its 1310 pages only result in a total of seven "DocStats" messages being sent from the worker-thread.
[3] Reporting telemetry, in Firefox, includes using `JSON.stringify` on the data and then sending an event to the `PdfStreamConverter.jsm`-code.
In that code the event is handled and `JSON.parse` is used to retrieve the data, and in the "documentStats"-case we'll then iterate through the data to avoid double-reporting telemetry; see https://searchfox.org/mozilla-central/rev/8f4c180b87e52f3345ef8a3432d6e54bd1eb18dc/toolkit/components/pdfjs/content/PdfStreamConverter.jsm#515-549
It shouldn't be necessary to iterate through *all* pages when using a non-default `spreadMode`, since we already know which page(s) should become visible.
This code is a left-over from the initial (local) implementation that resulted in PR 14112, however I forgot to clean-up some things such as e.g. this loop.
Also fixes an outdated comment, see PR 14204 which removed the mentioned data-structure.
This patch preserves the old behaviour of appending a `loadingIcon`-div to all pages that are not yet loaded/rendered. However, the actual `loadingIcon`-spinner (i.e. the `loading-icon.gif` image) will only be displayed on *visible* pages to improve performance.
To avoid having to iterate through all pages in the document, which doesn't seem like a good idea for a PDF document with thousands of pages, we use a combination of the currently visible *and* cached pages to toggle the `loadingIcon`-spinner.
This code is the last piece[1] of the viewer that's not using standard `class`es, and by converting this code we get rid of some now unneeded boilerplate code (slightly reducing the size of the *built* `web/viewer.js` file).
Note that while this code was originally imported from a separate repository, it was last sync-ed with upstream *five years* ago which is why this re-factoring should be OK as far as I'm concerned (and we've done some other clean-up since then as well).
---
[1] Technically the `web/debugger.js` file is left as well, however that code is first of all not bundled in the *built* `web/viewer.js` file and secondly it's not even loaded by default either.
- First step to fix https://bugzilla.mozilla.org/show_bug.cgi?id=1737260;
- several interactive pdfs use the possibility to hide/show buttons to show different icons;
- render pushbuttons on their own canvas and then insert it the annotation_layer;
- update test/driver.js in order to convert canvases for pushbuttons into images.
Reporting telemetry, in Firefox, includes using `JSON.stringify` on the data and then sending an event to the `PdfStreamConverter.jsm`-code.
In that code the event is handled and `JSON.parse` is used to retrieve the data, and in the "pageInfo"-case we'll then proceed to ignore everything except *the first* such event; see https://searchfox.org/mozilla-central/rev/24fac1ad31fb9c6e9c4c767c6a7ff45d226078f3/toolkit/components/pdfjs/content/PdfStreamConverter.jsm#509-514
All-in-all, sending the "pageInfo" telemetry for each rendered page is thus unnecessary and this patch makes the viewer send it only *once* instead.
*This is a tentative patch, since we unfortunately cannot easily test it (as far as I can tell).*
In Firefox this (obviously) works as-is, but in Google Chrome the `markedContent` spans are inserted within the regular text-content (in the DOM) and with non-zero heights.
*This is a tentative patch, since I don't have the necessary hardware to test it.*
See https://developer.mozilla.org/en-US/docs/Web/CSS/text-size-adjust, which is currently ignored in Firefox.
It seems overall safer, and more future-proof, to simply add this to the *entire* `textLayer` rather than its individual elements.
With the previous patch, this helper function is no longer used and keeping it around will simply increase the size of the builds.
This removal is purposely done *separately*, to make it easy to revert the patch in the future if this helper function would become useful again.
This relies on the fact that `Set`s preserve the insertion order[1], which means that we can utilize an iterator to access the *first* stored view.
Note that in the `resize`-method, we can now move the visible pages to the back of the buffer using a single loop (hence we don't need to use the `moveToEndOfArray` helper function any more).
---
[1] This applies to `Map`s as well, although that's not entirely relevant here.
The `PDFPageViewBuffer`-code is very important for the correct function of the viewer, but it's currently not tested at all.
While the `PDFPageViewBuffer` is obviously intended to be used with `PDFPageView`-instances, it only accesses a couple of `PDFPageView` properties/methods and consequently it's fairly easy to unit-test this code with dummy-data.
These unit-tests should help improve our confidence in this code, and will also come in handy with other changes that I'm working on (regarding modernizing and re-factoring the `PDFPageViewBuffer`-code).
The way that we're currently handling the last-`id` is very old, and there's no longer any good reason to special-case things when only one thumbnail is visible.
Furthermore, we can also modernize the loop slightly by using `for...of` instead of `Array.prototype.some()` when checking for fully visible thumbnails.
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.
I missed this one spot in PR 12870, when converting the other cases in the "keydown" event handler. However, given that it only matters in PresentationMode and/or when "page-fit" zooming is enabled, this oversight shouldn't have had any user-observable impact (but we should fix it nonetheless).
This is a follow-up to PR 10675, since there I completely overlooked that we also need to handle the case where a PDF document has *failed* to load when the "supportsRangedLoading" message is sent to the viewer.
There were some links not working in some XFA files,I realized that the anchor tag that contains the link has an inline display and couldn't receive any height, solved this by adding a "position: absolute". Tested with two different files in Firefox Nightly and Chrome and now all links are working perfectly fine. Added reftest to avoid future regressions
The only reason for using a `DocumentFragment` in the first place, originally added in PR 8724, was to prevent errors in the `PDFPageView`-constructor. However, we should be able to simply make its `container`-option *optional* instead, since it's not being used for anything else in the class.
Note that pre-rendering still works correctly in my testing, and given that the `BaseViewer` keeps references to all `PDFPageView`-instances (via its `_pages` Array) it also shouldn't be possible to "lose" any pages/canvases this way.
Unfortunately there exist PDF documents where all pageLabels are empty strings, see e.g. http://www.cs.cornell.edu/~ragarwal/pubs/blk-switch.pdf (taken from an old issue), which result in the pageNumber-input being completely blank. That doesn't seem very helpful, and this patch simply extends the approach used to ignore pageLabels that are identical to standard page numbering.
In the GENERIC viewer, e.g. when dragging-and-dropping a new PDF document which automatically opens the outline, there can now be breaking errors in the `{BaseViewer, PDFThumbnailViewer}.#getScrollAhead` methods since there's no visible pages/thumbs during loading; sorry about the breakage!
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.
This is a very old "issue", which has existed since essentially forever, and it affects all of the available scrollModes. However, in the recently added Page-mode it's particularily noticeable since we use a *simulated* scroll direction there.
When deciding what page(s) to pre-render, we only consider the current scroll direction. This works well in most cases, but can break down at the start/end of the document by trying to pre-render a page *outside* of the existing ones. To improve this, we'll thus *force* the scroll direction at the start/end of the document.
*Steps to reproduce:*
0. Open the viewer, e.g. https://mozilla.github.io/pdf.js/web/viewer.html
1. Enable vertical scrolling.
2. Press the <kbd>End</kbd> key.
3. Open the devtools and, using the DOM Inspector, notice how page 13 is *not* being pre-rendered.
*Please note:* This is a tentative patch, since I don't have the necessary a11y-software to actually test it.
To avoid having to add a new API-method just for a single string, I figured that adding the new property to the existing `documentInfo`-data (accessed via `PDFDocumentProxy.getMetadata` in the API) will hopefully be deemed acceptable.
Looking at the code, I do have to agree with the point made in issue 12731 about it being unexpected/unhelpful that the `PDFFindController.executeCommand`-method isn't directly usable with the "find"-event.
The reason for it being this way is, as so often, for historical reasons: The `executeCommand`-method was added (just) prior to the introduction of the `EventBus` in the viewer.
Obviously we cannot simply change the existing `PDFFindController.executeCommand`-method, since that'd be a breaking change in code which has existed for over five years.
Initially I figured that we could simply add a new method in `PDFFindController` that'd accept the state from the "find"-event, however after thinking about this and looking through the use-cases in the default viewer I settled on a slightly different approach: Let the `PDFFindController` just listen for the "find"-event (on the `EventBus`-instance) directly instead, which also removes one level of (unneeded) indirection during searching in the default viewer.
For GENERIC builds of the PDF.js library, the old `PDFFindController.executeCommand`-method is still available with a deprecation warning.
Many years ago now there were some `Promise` implementations that had issues resolving with an *implicitly* `undefined` value. That should no longer be the case, and we've not been using the `Promise.resolve(undefined)` format for a long time, hence this patch fixes the few remaining cases.
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.
With the previous commit, both of the `PDFViewer` and `PDFSinglePageViewer` clases are now small/simple enough that it no longer seems necessary to keep them in separate files.
This implements a new Page scrolling mode, essentially bringing (and extending) the functionality from `PDFSinglePageViewer` into the regular `PDFViewer`-class. Compared to `PDFSinglePageViewer`, which as its name suggests will only display one page at a time, in the `PDFViewer`-implementation this new Page scrolling mode also support spreadModes properly (somewhat similar to e.g. Adobe Reader).
Given the size and scope of these changes, I've tried to focus on implementing the basic functionality. Hence there's room for further clean-up and/or improvements, including e.g. simplifying the CSS/JS related to PresentationMode and implementing easier page-switching with the mouse-wheel/arrow-keys.
Note that PR 14049 removed this, since https://github.com/mozilla/pdf.js/pull/14049#discussion_r716245518 claimed that it's not necessary anymore. Unfortunately that, in my testing on Windows, actually re-introduced exactly the issue described in the comment; more specifically once the *last* character has been entered in the comb-field it's now again incorrectly scrolled (with the first character being invisible) until focus is lost.
This can be tested with e.g. `f1040.pdf`, see page 2, from the test-suite.
This patch helps reduce some duplication, given that we now have a few essentially identical `addLinkAttributes` call-sites in the code-base.
To prevent runtime errors in the Annotation/XFA-layer code, we'll warn if a custom/incomplete `PDFLinkService` is being used (limited to GENERIC builds).
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.
Note how both the annotationLayer and the document outline will apply various URL-related options when creating the link-elements.
For consistency the `xfaLayer`-rendering should obviously use the same options, to ensure that the existing options are indeed applied to all URLs regardless of where they originate.
- it aims to fix https://bugzilla.mozilla.org/show_bug.cgi?id=1716758;
- some buttons have a JS action with the pattern `app.launchURL(...)` (or similar) so extract when it's possible the url and generate a <a> element with the href equals to the found url;
- pdf.js already had some code to handle that so this patch slightly refactor that.
This replaces direct `document.getElementsByName` lookups with a helper method which:
- Lets the AnnotationLayer use the data returned by the `PDFDocumentProxy.getFieldObjects` API-method, such that we can directly lookup only the necessary DOM elements.
- Fallback to using `document.getElementsByName` as before, such that e.g. the standalone viewer components still work.
Finally, to fix the problems reported in issue 14003, regardless of the code-path we now also enforce that the DOM elements found were actually created by the AnnotationLayer code.
With these changes we'll thus be able to update form elements on all visible pages just as before, but we'll additionally update the AnnotationStorage for not-yet-rendered elements thus fixing a pre-existing bug.
Currently any AppOptions set using e.g. the "webviewerloaded" event listener can/will by default be overridden when the Preferences are read.
To avoid that happening the "disablePreferences"-option can be used, however unless it's been explicitly set all non-default AppOptions will be silently ignored. This patch thus attempts to improve the current situation somewhat, for third-party implementations, by logging a warning in the console when this happens.
Rather than re-computing this value in a number of different places throughout the code-base[1], we can expose this in the API via the existing `PixelsPerInch`-structure instead.
There's also been feature requests asking for the old `CSS_UNITS` viewer constant to be made accessible, such that it could be used in third-party implementations.
I suppose that it could be argued that it's somewhat confusing to place a unitless property in `PixelsPerInch`, however given that the `PDF_TO_CSS_UNITS`-property is defined strictly in terms of the existing properties this is hopefully deemed reasonable.
---
[1] These include:
- The viewer, with the `CSS_UNITS` name.
- The reference-tests.
- The display-layer, when rendering images; see PR 13991.
Given the simplicity of this functionality, we can move it from the default viewer and into the `BaseViewer` class instead. This way, it's possible to support more scripting functionality in the standalone viewer components; please see PR 14038.
Please note that I purposely went with `increaseScale`/`decreaseScale`-method names, rather than using "zoom", to better match the existing `currentScale`/`currentScaleValue` getters/setters that's being used in the `BaseViewer` class.
Rather than forcing the "regular" `EventBus` to check and handle `isInAutomation` for every `dispatch` call, we can take advantage of subclassing instead.
Hence this PR introduces a new `AutomationEventBus` class, which extends `EventBus`, and is used by the default viewer when `isInAutomation === true`.
Originally the library/viewer didn't support forms, and the hiding of the Download-buttons (when new documents are opened) didn't really matter all that much. Hence the simplest solution, at the time, was to hide the Download-buttons since we use the URL as a fallback when downloading data in the GENERIC `DownloadManager`.
Nowadays we obviously want to support saving of forms in the GENERIC viewer, regardless of how the document was opened, which could thus *potentially* lead to the fallback download-URL being wrong.
In order to be able to show the Download-buttons unconditionally, this patch slightly re-factors the viewer to track the download-URL *separately* to prevent any issues there.
*Please note:* As mentioned in the issue, the ViewBookmark-buttons are specific to the initial URL when the viewer is first opened. Hence they (still) don't make sense when a new document has been opened, since it's then impossible to obtain a usable link to the *currently* active document.
By using CSS variables to set the width of the zoom dropdown, we can simplify both the relevant CSS and JS code. This will not only improve overall maintainability of this code, but should also make it (slightly) easier for third-party users that want to customize the width.
Note in particular that by having the code in `Toolbar._adjustScaleWidth` lookup the values of the CSS variables, we no longer need to worry about keeping hard-coded values up-to-date with the CSS rules.
While some of the output looks worse to my eye, this behavior more
closely matches what I see when I open the PDFs in Adobe acrobat.
Fixes: #4706, #9713, #8245, #1344
Similar to other viewer components, e.g. the `PDFFindBar` and `PDFPresentationMode`, there's no need to create a `PDFHistory`-instance when it's not going to be used.
The old `update`-signature started to annoy me back when I added optional content support to the viewer, since we're (often) forced to pass in a bunch of arguments that we don't care about whenever these methods are called.
This is tagged `api-minor` since `PDFPageView` is being used in the `pageviewer` component example, and it's thus possible that these changes could affect some users; the next commit adds fallback handling for the old format.
While e.g. the `simpleviewer` and `singlepageviewer` examples work, since they're based on the `BaseViewer`-class, the standalone `pageviewer` example currently doesn't support either XFA- or StructTree-layers. This seems like an obvious oversight, which can be easily addressed simply by exporting the necessary functionality through `pdf_viewer.component.js`, similar to the existing Text/Annotation-layers.
While working on, and testing, these changes I happened to notice a number of smaller things that's also fixed in this patch:
- Ensure that `XfaLayerBuilder.render` always have a *consistent* return type, to prevent possible run-time failures in `PDFPageView`; PR 13908 follow-up.
- Change the order of the options in the `XfaLayerBuilder`-constructor to agree with the parameter order in the `DefaultXfaLayerFactory.createXfaLayerBuilder`-method.
- Add a missing `textHighlighterFactory`-option, in the JSDocs for the `PDFPageView`-class.
- A couple of small tweaks in the `TextLayerBuilder.render`-method: Re-use an existing Array rather than creating a new one, and replace an `if` with optional chaining instead.
*Please note:* For now XFA-support is currently disabled by default, similar to the regular viewer.
A lot of the code in this getter has existed ever since the initial PresentationMode-implementation was first added all the way back in PR 1938 (which is nine years ago now).
At this point in time however, there's now a simpler way detect if a browser supports the FullScreen API and we should thus be able to simplify this getter; please refer to https://developer.mozilla.org/en-US/docs/Web/API/Document/fullscreenEnabled#browser_compatibility
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
*/
```
- Use `Node.TEXT_NODE` rather than a magical constant, in `TextHighlighter._convertMatches`, to improve readability. According to MDN, this has been supported since "forever": https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeType#browser_compatibility
- Remove the `pageIdx`-property, on `TextLayerBuilder`-instances, since the re-factoring in PR 13908 meant that it's now unused.
- Remove the `matches`-property, on `TextLayerBuilder`-instances, since the re-factoring in PR 13908 meant that it's now unused.
Generalizing, and documenting, the `PDFHistory`-implementation as part of the web-interfaces doesn't seem entirely necessary and in hindsight I'm not entirely sure why we need it since:
- The `PDFHistory` implementation is/was written specifically for the default viewer use-case, which is why e.g. the `simpleviewer` component example isn't using it. (While it *could* be used there, it'd need to be manually created/initialized correctly.)
- There's only *one* `PDFHistory`-implementation present (and no other viewer-component will fail without it being available), as opposed to the other web-interfaces documented in this file.
- The `PDFHistory` implementation is not even usable with e.g. the `pageviewer` component example, since it (obviously) requires a complete viewer to work. (This is in contrast to e.g. `IPDFTextLayerFactory` and `IPDFAnnotationLayerFactory`.)
*This is a follow-up to PRs 13867 and 13899.*
This patch is tagged `api-minor` for the following reasons:
- It replaces the `renderInteractiveForms`/`includeAnnotationStorage`-options, in the `PDFPageProxy.render`-method, with the single `annotationMode`-option that controls which annotations are being rendered and how. Note that the old options were mutually exclusive, and setting both to `true` would result in undefined behaviour.
- For improved consistency in the API, the `annotationMode`-option will also work together with the `PDFPageProxy.getOperatorList`-method.
- It's now also possible to disable *all* annotation rendering in both the API and the Viewer, since the other changes meant that this could now be supported with a single added line on the worker-thread[1]; fixes 7282.
---
[1] Please note that in order to simplify the overall implementation, we'll purposely only support disabling of *all* annotations and that the option is being shared between the API and the Viewer. For any more "specialized" use-cases, where e.g. only some annotation-types are being rendered and/or the API and Viewer render different sets of annotations, that'll have to be handled in third-party implementations/forks of the PDF.js code-base.
Moves the logic out of TextLayerBuilder to handle
highlighting matches into a new separate class `TextHighlighter`
that can be used with regular PDFs and XFA PDFs.
To mimic the current find functionality in XFA, two arrays
from the XFA rendering are created to get the text content
and map those to DOM nodes.
Fixes#13878
The `loadAndEnablePDFBug` helper function, in `web/app.js`, can be simplified a little bit by making it `async`. Furthermore, given how `PDFBug` is being used, we can also (slightly) re-factor `PDFBug.init` such that the `PDFBug.enable`-call is done internally rather than having to handle that manually at the call-site.
(Finally, utilize `await` more in the `loadFakeWorker` helper function.)
The original idea behind including the class name, when logging errors, was to improve things in the *hypothetical case* where `PDFViewer`- and `PDFSinglePageViewer`-instances would be used side-by-side.
Given that all of the relevant methods are synchronous this seem unlikely to really be necessary, and furthermore it's probably best to avoid using `this.constructor.name` since that's not guaranteed to do what you intend (we've seen repeated issues with minifiers mangling function/class names).
This patch removes the only remaining closure in the `src/display/api.js` file, utilizing a similar approach as used in lots of other parts of the code-base, which results in a small decrease in the size of the *build* `pdf.js` file.
Given that `PDFWorker` is exposed through the *public* API, this complicates things somewhat since there's a couple of worker-related properties that really should stay *private*. Initially, while working on PR 13813, I believed that we'd need support for private (static) class fields in order to get rid of this closure, however I've managed to come up with what's hopefully deemed an acceptable work-around here.
Furthermore, some helper functions were simply moved into the `PDFWorker` class as static methods, thus simplifying the overall implementation (e.g. we don't need to manually cache the Promise in the `PDFWorker._setupFakeWorkerGlobal`-method).
Finally, as part of this re-factoring a number of missing JSDoc-comments were added which *together* with the removal of the closure significantly improves the `gulp jsdoc` output for the `PDFWorker` class.
*Please note:* This patch is tagged with `api-minor` since it deprecates `PDFWorker.getWorkerSrc()` in favor of the shorter `PDFWorker.workerSrc`, with the fallback limited to `GENERIC` builds.
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.
Even though the code as-is *should* be safe, given that we're using an Object with a `null` prototype, it cannot hurt to change this to a Map to prevent any issues (since we're parsing unknown and potentially unsafe data).
Overall I also think that these changes improve the `parseQueryString` call-sites, since we now have a proper way of checking for the existence of a particular key (and don't have to use `in` which stringifies the keys in the Object).
This patch also changes the default, when no `value` exists, from `null` to an empty string since the use of `decodeURIComponent` currently can modify the value in a somewhat surprising way (at least to me).
Note how `decodeURIComponent(null) === "null"` which is unlikely to be what you actually want, whereas `decodeURIComponent("") === ""` which seems much more helpful.
Prior to PR 13042, when scripting wasn't really possible to use outside of the full viewer, the `enableScripting` option made sense.
However, at this point in time having to both pass in a `PDFScriptingManager`-instance *and* set the `enableScripting`-boolean when creating a `BaseViewer`-instance feels redundant and (mostly) annoying. Hence this patch, which removes the *separate* boolean and always enables scripting when `scriptingManager` is provided.
The relevant "viewer component" examples are also updated (with a comment), but in such a way that scripting support won't just break when used with the current PDF.js releases.
- All the scale factors in for the substitution font were wrong because of different glyph positions between Liberation and the other ones:
- regenerate all the factors
- Text may have polish chars for example and in this case the glyph widths were wrong:
- treat substitution font as a composite one
- add a map glyphIndex to unicode for Liberation in order to generate width array for cid font
- In order to better compute text fields size, use line height with no gaps (and consequently guessed height for text are slightly better in general).
- Fix default background color in fields.
Given that we've over time been reducing the number of `compatibilityParams` in use, there's now few enough left that I think it makes sense to simply inline them directly in the `web/app_options.js` file.
Note that we recently inlined/removed the separate `src/display/api_compatibility.js` file, see PR 13525, and that it (in my opinion) thus makes sense to do the same in the `web/`-folder. This patch will also slightly reduce the size of *built* `web/viewer.js` file, which cannot hurt.
There's no good reason, as far as I can tell, to have `PDFPageView.reset` attempt to cancel `annotationLayer`/`xfaLayer` rendering in one special-case (this is mostly a leftover from older code). Previously cancelling was moved into the separate `PDFPageView.cancelRendering`-method, and by slightly tweaking the conditions there we're able to remove a bit of now unnecessary code from the `PDFPageView.reset`-method.
Originally the `xfaLayer` wasn't implemented in such a way that it supported being removed from the DOM when pages were evicted from the cache, however this limitation was lifted in PR 13427 and the `xfaLayer` should thus be handled similar to e.g. the `annotationLayer`.
In addition to removing the `xfaLayer` from the DOM, this patch *also* implements proper rendering/hiding-handling for it (mirroring the `annotationLayer`-code).
*Please note:* This patch is tagged API-minor just in case[1], since it changes the signatures of a couple of `PDFPageView`-methods to improve readability of the code.
---
[1] Although users are *hopefully* not directly accessing any of the affected methods, and are rather using e.g. `PDFViewer` in which case none of these changes will matter.
- a rectangle can be defined after a field and so from a z-index pov, it's on top of the field, which means that the rectangle avoid to have some mouse events in the fields;
- so this patch just disable pointer events for all elements under svg (included).
Given that Internet Explorer is, since some time now, no longer supported in the PDF.js library this `<meta>` tag can also be removed (added in PR 6374).
- font line height is taken into account by acrobat when it isn't with masterpdfeditor: I extracted a font from a pdf, modified some ascent/descent properties thanks to ttx and the reinjected the font in the pdf: only Acrobat is taken it into account. So in this patch, line heights for some substituted fonts are added.
- it seems that Acrobat is using a line height of 1.2 when the line height in the font is not enough (it's the only way I found to fix correctly bug 1718741).
- don't use flex in wrapper container (which was causing an horizontal overflow in the above bug).
- consequently, the above fixes introduced a lot of small regressions, so in order to see real improvements on reftests, I fixed the regressions in this patch:
- replace margin by padding in some case where padding is a part of a container dimensions;
- remove some flex display: some containers are wrongly sized when rendered;
- set letter-spacing to 0.01px: it helps to be sure that text is not broken because of not enough width in Firefox.
While I don't know if it's technically correct to even do this, it could provide a slightly better out-of-the-box behaviour in browsers that specify (from the PDF.js `l10n`-folder perspective) "incomplete" language codes.
Rather than immediately falling back to English, we'll use a white-list to try and re-write a "partial" language code to a (hopefully) suitable one that matches an existing `l10n`-folder. The disadvantage of this solution is that the list needs to be kept *manually* up-to-date with any changes in the `l10n`-folder, however new locales are added infrequently enough that this should be acceptable.
Fixes 13689 (assuming we actually want/care to do so, otherwise we should just WONTFIX the issue).
- Fix a typo in order to open the pdf in issue #13679
- After fixing the fill default color there wer some regressions because of z-index
and when fixing z-index there were some regressions because of borders
- So fix the borders rendering.
The PDF.js API has only ever supported accessing the original file ID, however the second one that (should) exist in *modified* documents have thus far been completely inaccessible through the API.
That seems like a simple oversight, caused e.g. by the viewer not needing it, since it really shouldn't hurt to provide API-users with the ability to check if a PDF document has been modified since its creation.[1]
Please refer to https://www.adobe.com/content/dam/acom/en/devnet/pdf/pdfs/PDF32000_2008.pdf#G13.2261661 for additional information.
For an example of how to update existing code to use the new API, please see the changes in the `web/app.js` file included in this patch.
*Please note:* While I'm not sure if we'll ever be able to remove the old `PDFDocumentProxy.fingerprint` getter, given that it's existed since "forever", that probably isn't a big deal given that it's now limited to only `GENERIC`-builds.
---
[1] Although this obviously depends on the PDF software following the specification, by updating the second file ID as intended.
Using `instanceof Object` is generally problematic, since it's not guaranteed to always do the right thing for all Objects.
(I stumbled upon this while working on another patch, when I noticed that the `outlineView` was broken with workers disabled.)
- support paragraph margins, line height, letter spacing, ...
- compute missing dimensions from fields based almost on the dimensions of caption contents.
- it aims to fix#13583;
- fix the switch to breakBefore target;
- force the layout of an unsplittable element on an empty page;
- don't fail when there is horizontal overflow (except in lr-tb);
- handle correctly overflow in the same content area (bug 1717805, bug 1717668);
- fix a typo in radial gradient first argument.
Given that this property is only used with password protected documents, and is consequently document-specific rather than viewer-specific, ensure that `IPDFLinkService.externalLinkEnabled` is actually being reset by `PDFViewerApplication.close`.
To make things less confusing/inconsistent, remove the *undocumented* `externalLinkEnabled` property from the `PDFLinkService` constructor and force it to always be manually set when needed.
- when the CSS line-height property is set to 'normal' then the value depends of the user agent. So use a line height based on the font itself and if for any reasons this value is not available use 1.2 as default.
- it's a partial fix for https://bugzilla.mozilla.org/show_bug.cgi?id=1717681.
This patch provides an overall simpler *and* more consistent way of handling the `viewport` parameter during printing of XFA forms, since it's now again guaranteed to always be an instance of `PageViewport`.
Furthermore, for anyone attempting to e.g. implement custom printing of XFA forms this probably cannot hurt either.
Given that the "print"-intent is special, and that we should always fallback to the "display"-intent, let's ensure that the code actually reflects that.
Also, ensure that the method always returns a `Promise` since that's what the documentation says.
The `web/ui_utils.js` file should be usable from basically anywhere in the `web/`-folder, hence it should ideally not have any dependecies on its own and particularily *not* onces that pull in entire (large) factories.
This extends the approach in PRs 12848 and 13606 to also apply to the `xfaLayer`, since otherwise XFA forms will be similarly broken in most non-default scroll/spread modes.
Note that as far as I can tell, this is *not* a regression but rather a bug which has existed since basically "forever".
**In order to reproduce this easily:**
- Open the viewer.
- Set the zoom level to `400%`,
- Search for "expression".
The problem here is that when scrolling matches into view, we're scrolling to the start of the *containing* `textLayer` element rather than the start of the highlighted match itself.[1] When the entire width (or at least most) of the page is visible in the viewer, that doesn't really matter though which is likely why this bug has gone unnoticed for so long.[2]
Given that the highlighted match can be placed anywhere, e.g. even at the very end, within its `textLayer` element it's quite easy to see why the current implementation becomes a problem at higher zoom levels. All of this is then *further* exacerbated by `PDFFindController.scrollMatchIntoView` using a negative left offset, to ensure that the current match has some (visible) context available once scrolled into view.
In order to address this long-standing bug, we'll determine the (left) offset of the `selected` match and use that to modify the final position scrolled to in `PDFFindController.scrollMatchIntoView` such that the match is visible regardless of zoom level.
---
[1] Unfortunately we cannot directly scroll to the `selected` match, since it's not absolutely positioned and changing that would cause other bugs/regressions (note recent patches in that area).
[2] I did actually stumble upon this problem a little while ago, while working on PR 13482, but forgot to look into this again until I saw the new issue.
With the introduction of `PDFScriptingManager._closeCapability` in PR 13074, the pre-existing `PDFScriptingManager._pageEventsReady` boolean essentially became redundant.
Given that you always want to avoid tracking closely related state *separately*, since it's easy to introduce subtle bugs that way, we should just remove `PDFScriptingManager._pageEventsReady` now.
Obviously I *should* have done this already back in PR 13074, sorry about the churn here!
- a checkbox or radio doesn't have to be rescaled when the container is large so give the extra space to the caption to avoid some word wrapping.
- when the caption is on the right, then put ui on the left as first element and so remove flex:row-reverse stuff.
- some elements weren't displayed because their rotation angle was not taken into account;
- fix box model (XFA concept):
- remove use of outline;
- position correctly border which isn't part of box dimensions;
- fix margins issues (see issue #13474).
- move border on button instead of having it on wrapping div;
This code was added in PR 3968, apparently in order to fix scrolling of search results in HiDPI-mode.
However, after PR 4570 nothing is setting these `dataset`-properties any more and this is thus dead code which should be removed. (If that change had broken scrolling of search results in HiDPI-mode, you'd really expect that it'd been reported and fixed a long time ago.)
I missed this during review, since some of the changes in `web/pdf_print_service.js` broke printing.
Also, as part of these changes the patch replaces what looks like unnecessary `setAttribute` usage with "regular" `className` assignment and finally updates a couple of the CSS-rules to be more consistent.
- I thought it was possible to rely on browser layout engine to handle layout stuff but it isn't possible
- mainly because when a contentArea overflows, we must continue to layout in the next contentArea
- when no more contentArea is available then we must go to the next page...
- we must handle breakBefore and breakAfter which allows to "break" the layout to go to the next container
- Sometimes some containers don't provide their dimensions so we must compute them in order to know where to put
them in their parents but to compute those dimensions we need to layout the container itself...
- See top of file layout.js for more explanations about layout.
- fix few bugs in other places I met during my work on layout.
This patch fixes the referenced bugs/issues, in a way that won't interfere with keyboard users, assuming that we actually want to fix these old bugs/issues. (If not, we should close them as WONTFIX.)
After PR 13117 it's now (finally) possible for *different* build targets to specify individual options/preferences, and we can utilize that to only expose the `renderer`-preference in builds where `SVGGraphics` is actually defined.
Note that for e.g. `MOZCENTRAL`-builds, trying to enable SVG-rendering will throw immediately and the preference thus doesn't make sense to include there.
Also, update the dummy `SVGGraphics` to use a class, tweak the `PDFJSDev`-check in `src/display/svg.js` to agree fully with the option/preference, and remove an unnecessary `eslint-disable`.
Reasons for the removal include:
- This functionality was always somewhat experimental and has never been enabled by default, partly because of worries about rendering bugs caused by e.g. bad/outdated graphics drivers.
- After the initial implementation, in PR 4286 (back in 2014), no additional functionality has been added to the WebGL implementation.
- The vast majority of all documents do not benefit from WebGL rendering, since only a couple of *specific* features are supported (e.g. some Soft Masks and Patterns).
- There is, and has always been, *zero* test-coverage for the WebGL implementation.
- Overall performance, in the PDF.js library, has improved since the experimental WebGL implementation was added.
Rather than shipping unused *and* untested code, it seems reasonable to simply remove the WebGL implementation for now; thanks to version control it's always possible to bring back the code should the need ever arise.
This functionality was originally implemented in PR 7029; however it's not, nor has it ever been, used as far as I can tell.[1]
Note in particular that the default viewer does not expose either a preference or even an option with which `disableCanvasToImageConversion` can be toggled, and source-code modification is thus required.
Furthermore, note also that we have multiple other instances of `canvas`-data accesses in both the `src/display/canvas.js` and `src/display/text_layer.js` files. If any of those are blocked, by e.g. browser settings, there will be outright rendering bugs and non-working thumbnails thus seem like a very small issue in the grand scheme of things; hence why I'm suggesting that we remove the unused `disableCanvasToImageConversion` functionality.
---
[1] For the Tor use-case mentioned in issue 7026, I *believe* that the solution was to white-list `canvas`-data accesses for its built-in PDF Viewer.
- app.alert and few other function can use an object as parameter ({cMsg: ...});
- support app.alert with a question and a yes/no answer;
- update field siblings when one is changed in an action;
- stop calculation if calculate is set to false in the middle of calculations;
- get a boolean for checkboxes when they've been set through annotationStorage instead of a string.
Also, removes a couple of unnecessary local variables in the `Stepper.breakIt` method.
Finally, this patch also disables the ESLint `no-var` rule, in preparation for the next patch, for a couple of data-structures that need to remain globally available.
Given that both the textLayer rendering *and* the structTree parsing is asynchronous, it's possible that we'll attempt to insert the structTree in a removed page. While there's thankfully no outright breakage caused by this, it will nonetheless lead to errors being printed in the console and we should obviously avoid this.
To reproduce this bug (without the patch), open http://localhost:8888/web/viewer.html?file=/test/pdfs/pdf.pdf#disableStream=true&disableAutoFetch=true and scroll *very quickly* through the document and notice the following error being (intermittently) printed in the console:
```
Uncaught (in promise) TypeError: can't access property "appendChild", this.canvas is undefined
```
Using `for...of` is a modern and generally much nicer pattern, since it gets rid of unnecessary callback-functions. (In a couple of spots, a "regular" `for` loop had to be used.)
According to a decision by UX and PM, please see https://bugzilla.mozilla.org/show_bug.cgi?id=1705060#c2 (and implemented in https://bugzilla.mozilla.org/show_bug.cgi?id=1705327), we no longer show the notification-bar in Firefox; hence the special `PDFViewerApplication._delayedFallback` functionality should no longer be necessary.
Furthermore, note that at this point in time *most* of the features which used the `PDFViewerApplication._delayedFallback` functionality is now enabled by default; hence that provides even less reason to keep this code around and existing calls are thus converted to "regular" `PDFViewerApplication.fallback` calls.
According to a decision by UX and PM, please see https://bugzilla.mozilla.org/show_bug.cgi?id=1705060#c2, in Firefox we should first of all *not* display the notification-bar for signatures. Secondly, as can also be seen there, we shouldn't display the notification-bar *at all* and it's thus disabled in https://bugzilla.mozilla.org/show_bug.cgi?id=1705327.
If we purposely don't display a notification, for documents with signatures, in the *built in* Firefox PDF Viewer then it cannot be necessary in the GENERIC viewer either.
To simplify the overall implementation, given that it only applies to the GENERIC-viewer, this patch purposely re-uses the existing `errorWrapper`-functionality to display the message.
While that one is mostly intended for actual *errors*, by re-using it here we considerably reduce the amount of code/complexity necessary for supporting this new warning. It's obviously possible to re-factor/improve this later on, but the patch should do just fine here since it'll indeed inform users (of the GENERIC-viewer) about unverified signatures.
Finally this patch also tweaks the background-color of the `errorWrapper`, making it 20 percent lighter respectively darker (depending on the theme) to make it "stand out" a little bit *less*.[1] While it may perhaps be useful to re-style/re-factor the `errorWrapper`, this patch probably isn't the right place for doing that.
---
[1] Note how in the MOZCENTRAL-viewer, which instead uses the browser notification-bar, we're purposely using a neutral colour to not draw too much attention to the notification-bar.
This is first of all consistent with existing API-methods, where we return `null` when the data in question doesn't exist. Secondly, it should also be (slightly) more efficient since there's less dummy-data that we need to transfer between threads.
Finally, this prevents us from adding an empty/unnecessary span to *every* single page even in documents without any structure tree data.
I (unsurprisingly) managed to forget about handling the case where a "pagesloaded" event arrives *before* the outline has been parsed, in which case we'd not actually enable the `currentOutlineButton` as intended.
Also, in the "pagesloaded" event handler, we should ensure that there's actually any pages loaded since otherwise the "find current outlineItem"-feature doesn't make any sense.
- but don't validate them for now;
- Firefox will display a bar to warn that the signature validation is not supported (see https://bugzilla.mozilla.org/show_bug.cgi?id=854315)
- almost all (all ?) pdf readers display signatures;
- validation is done in edge but for now it's behind a pref.
It's obviously better and more correct to handle the "pagesloaded" case within `PDFOutlineViewer` *itself*, rather than essentially splitting the logic in two parts and forcing `PDFSidebar` to deal with what should've been handled internally in `PDFOutlineViewer`.
This is what I *should* have done in PR 12777, but for some reason didn't figure out how to implement it well enough back then; sorry about the churn here!
*This patch fixes some technical debt in the viewer.*
Given that most API methods are (purposely) asynchronous, there's always a risk that the viewer could have been `close`d before the requested data arrives.
Lately we've started to check this case before using the data, to prevent errors and/or inconsistent state, however the outline/attachments/layers fetching and rendering is old enough that it pre-dates those checks.
When a PDF is "marked" we now generate a separate DOM that represents
the structure tree from the PDF. This DOM is inserted into the <canvas>
element and allows screen readers to walk the tree and have more
information about headings, images, links, etc. To link the structure
tree DOM (which is empty) to the text layer aria-owns is used. This
required modifying the text layer creation so that marked items are
now tracked.
Scripting, as implemented, requires access to a complete document/viewer in order to work. Hence it doesn't really make sense to keep the `enableScripting`-option on `PDFPageView`-instances.[1]
---
[1] Note that there's the `PDFSinglePageViewer`, which can be used in cases where you want access to all features/functionality of the viewer but only display *one* page at a time.
Note how we purposely don't expose the `AnnotationStorage`-class directly in the official API (see `src/pdf.js`), since trying to use *multiple* ones simultaneously doesn't really make sense (e.g. in the viewer).
Instead we lazily initialize, and cache, just *one* instance via `PDFDocumentProxy.annotationStorage` which should thus be available internally in the API itself without having to be manually passed to various methods.
To support these changes, the `AnnotationStorage`-instance initialization is moved into the `WorkerTransport`-class to allow both `PDFDocumentProxy` and `PDFPageProxy` to access it.
This patch implements the following simplifications:
- Remove the `annotationStorage`-parameter from `PDFDocumentProxy.saveDocument`, since it's already available internally.
Furthermore, while it's currently possible to call that method without an `AnnotationStorage`-instance, that really does *not* make any sense at all. In this case you're effectively reducing `PDFDocumentProxy.saveDocument` to a "regular" `PDFDocumentProxy.getData` call, but with *a lot* more overhead, which was obviously not the intention of the `PDFDocumentProxy.saveDocument`-method.
- Try to discourage third-party users from calling `PDFDocumentProxy.saveDocument` unconditionally, as a replacement for `PDFDocumentProxy.getData` (note the previous point).
- Replace the `annotationStorage`-parameter, in `PDFPageProxy.render`, with a boolean `includeAnnotationStorage`-parameter which simply indicates if the (internally available) `AnnotationStorage`-instance should be used during rendering (e.g. for printing).
- By removing the need to *manually* provide `annotationStorage`-parameters to various API-methods, using the API should become simpler (e.g. for third-parties) since you no longer need to worry about manually fetching and passing around this data.
The reason for the fairly large discrepancy, in the thumbnail quality, between the `draw`/`setImage`-methods is that in the former case we *directly* render the thumbnails at the final size that they'll appear at in the sidebar. In the latter case, we instead downsize the (generally) much larger "regular" pages.
To address this, I'm thus proposing that we let `PDFThumbnailView.draw` render thumbnails at *twice* their intended size and then downsize them to the final size.
Obviously this will increase *peak* memory usage during thumbnail rendering in `PDFThumbnailView.draw`, since doubling the width/height of a `canvas` will lead to its pixel-count increasing by a factor of `4`. Furthermore, since you need four components per pixel (given that it's RGBA-data), this will thus lead to the *temporary* thumbnail `canvas`-sizes increasing by a factor of `16` during rendering. Hence why rendering thumbnails at their "original" scale, i.e. using something like `PDFPageProxy.getViewport({ scale: 1 });`, would be an absolutely terrible idea!
To reduce the size and scope of these changes, I've tried to re-factor and re-use as much of the existing downsizing-implementation already present in `PDFThumbnailView` as possible.
While this will generally *not* make thumbnails rendered by `PDFThumbnailView.draw` look *identical* to those based on the rendered pages (via `PDFThumbnailView.setImage`), it's a considerable improvement as far as I'm concerned and enough to call the issue fixed.
*Please note:* This patch will not lead to *any* additional overhead, in either memory usage or parsing, for thumbnails which are based on the rendered pages.
A loop is less efficient than just overwriting the content, which is what we've generally been using (for years) in other parts of the code-base (see e.g. `BaseViewer` and `PDFThumbnailViewer`).
These properties are always updated/used together, and there's no other methods which depend on just one of them, hence they're changed into local variables instead.
Looking through the history of this code, it seems they were converted *from* local variables and to properties all the way back in PR 2914; however as far as I can tell from that diff it doesn't seem to have been necessary even back then!?
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.
* the goal is to execute actions like Open or OpenAction
* can be tested with issue6106.pdf (auto-print)
* once #12701 is merged, we can add page actions
This new event essentially mirrors the existing "pagesinit" event, and will allow e.g. a custom implementation of the viewer to be notified before the current PDF document is removed from the viewer.
By using this new event, we're thus able to dispatch a "pageclose" event for JavaScript actions when closing the existing document.
Having looked at the Acrobat JavaScript specification, see https://www.adobe.com/content/dam/acom/en/devnet/acrobat/pdfs/AcrobatDC_js_api_reference.pdf#G5.1963437, I suppose that introducing these two new events is probably the easiest solution overall.
However there's a number of things that, as far as I'm concerned, will help the overall implementation:
- Only dispatch these new events when `enableScripting = true` is set.
- Handle them *separately* from the existing "pagechanging" event dispatching, to avoid too much clutter.
- Don't dispatch either of the events if the page didn't actually change.
- When waiting for pages to render, don't dispatch "pageopen" if the page is no longer active when rendering finishes.
- Ensure that we only use *one* "pagerendered" event listener.
- Ensure that "pageopen" is actually dispatched when the document loads.
I suppose that we *could* avoid adding the "pageclose" event, and use the existing "pagechanging" event instead, however having a separate event might allow more flexibility in the future. (E.g. I don't know if we'll possibly want to dispatch "pageclose" on document close, as mentioned briefly in the specification.)
* move set/clear|Timeout/Interval and crackURL code in pdf.js
* remove the "backdoor" in the proxy (used to dispatch event) and so return the dispatch function in the initializer
* remove listeners if an error occured during sandbox initialization
* add support for alert and prompt in the sandbox
* add a function to eval in the global scope
Given that the GENERIC default viewer supports opening more than one document, and that a unique scripting-instance is now used for each document, the changes made in this patch seem appropriate.
While it's not entirely clear to me that it's ultimately desirable to use the `pdf.sandbox.js` in the Chromium-extension, given that the MOZCENTRAL-build uses `pdf.scripting.js` directly in a *custom* sandbox, the current state isn't that great since setting `enableScripting = true` with the Chromium-extension will currently fail completely.
Hence this patch, which should at least unbreak things for now.
Since the `close` method has become quite large, this small re-factoring shouldn't hurt (and may also be useful with future changes to the `_initializeJavaScript` method).
I completely missed this previously, but we obviously should remove the scriptElement as well to *really* clean-up everything properly.
Given that there's multiple existing usages of `loadScript` in the code-base, the safest/quickest solution seemed to be to have call-sites opt-in to remove the scriptElement using a new parameter.
This patch *attempts* to actually implement what's described for the `Count`-entry in the PDF specification, see https://www.adobe.com/content/dam/acom/en/devnet/acrobat/pdfs/PDF32000_2008.pdf#G11.2095911, which I mostly ignored back in PR 10890 since it seemed unnecessarily complicated[1].
Besides issue 12704, I've also tested a couple of other documents (e.g. the PDF specification) and these changes don't *seem* to break anything else; additional testing would be helpful though!
---
[1] At the time, all PDF documents that I tested worked even with a very simple approach and I thus hoped that it'd would suffice.
Similar to the previous patch, the GENERIC default viewer is capable of opening more than *one* PDF document and we should ensure that we handle that case correctly.
I was actually quite surprised to find that, despite the various `scripting`-getters implementing `destroySandbox` methods, there were no attempts at actually cleaning-up either the "sandbox" or removing the globally registered event listeners.
This patch also changes the method to skip *all* data fetching when "enableScripting" isn't active. Finally, simplifies some event-data accesses in the "updateFromSandbox" listener.
Another possible option here could be to use the `contentLength`, when it exists, and then using e.g. a custom event to always update the "filesize" in the sandbox "after the fact" with the result of the `getDownloadInfo`-call.
We can easily avoid unnecessary API-calls here, since most of the time the `metadata` will already be available here. In the *rare* case that it's not available, we can simply wait for the existing `getMetadata`-call to resolve.
This will be useful in the following patch, and note that there's also an old issue (see 5765) which asked for such an event. However, given that the use-case wasn't *clearly* specified, and that we didn't have an internal use for it at the time it wasn't implemented.
Also, ensure that all of the metadata-related properties are actually reset when the document is closed.
Compared to the, previously removed, `sandbox`/`watch-sandbox` gulp-tasks, these ones should work even when run against an non-existent/empty `build`-folder.
Also, to ensure that the development viewer actually works out-of-the-box, `gulp server` will now also include `gulp watch-dev-sandbox` to remove the need to *manually* invoke the build-tasks.
Finally, this patch also removes the `web/devcom.js` file since it shouldn't actually be needed, assuming that the "sandbox"-loading code in the `web/genericcom.js` file is actually *correctly* implemented.
Rather than having two slightly different ways of setting the pending/notFound appearance on the "findInput", we can simply use "data-status" in both cases since they're obviously mutually exclusive.
As mentioned in https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions#faq-support, PDF.js version `2.6.347` is the last release with IE 11/Edge support.
Hence we should now be able to reduce unnecessary duplication in the default viewer image resources, note the files in the `web/images/` folder with a `-dark` suffix, by using only *one* SVG-image for each icon and letting the `background-color` depend on the CSS theme instead.
For the `gulp mozcentral` build-target, the resulting `web/images/` folder is reduced from `43 997` to `28 566` bytes (~35 percent).
*Please note:* I don't really know if this implementation is necessarily the *best* solution, but it seems to work well enough in e.g. Firefox Nightly and Google Chrome Beta as far as my testing goes.
Given that we already include the "Content-Disposition"-header filename, when it exists, it shouldn't hurt to also include the information from the "Content-Length"-header.
For PDF documents opened via a URL, which should be a very common way for the PDF.js library to be used, this will[1] thus provide a way of getting the PDF filesize without having to wait for the `getDownloadInfo`-promise to resolve[2].
With these API improvements, we can also simplify the filesize handling in the `PDFDocumentProperties` class.
---
[1] Assuming that the server is correctly configured, of course.
[2] Since that's not *guaranteed* to happen in general, with e.g. `disableAutoFetch = true` set.
* quickjs-eval.js has been generated using https://github.com/mozilla/pdf.js.quickjs/
* lazy load of sandbox code
* Rewrite tests to use the sandbox
* Add a task `watch-sandbox` which update bundle pdf.sandbox.js on change in the sandbox code
Given that it's generally faster to call *one* function and have it loop through an object, rather than looping through an object and calling a function for every iteration, this patch will reduce the total time spent in `PDFViewerApplication._readPreferences` ever so slightly.
Also, over time we've been adding more and more preferences, rather than removing them, so using the new `AppOptions.setAll` method should be generally beneficial as well.
While the effect of these changes is quite small, it does reduces the time it takes for the preferences to be fully initialized. Given the amount of asynchronous code during viewer initialization, every bit of time that we can save should thus help.
Especially considering the recently added `viewerCssTheme` preference, which needs to be read very early to reduce the risk of the viewer UI "flashing" visibly as the theme changes, I figured that a couple of small patches reducing the time spend reading preferences cannot hurt.
Given that only two debugging hash parameters (i.e. `disableWorker` and `pdfBug`) will make this method asynchronous, we can avoid what's most of the time is an unnecessary `Promise.all` invocation.
While this does work pretty well in my quick testing, it's *very much* a hack since as far as I can tell there's no support in the CSS specification for using e.g. a CSS variable to override a `@media (prefers-color-scheme: dark) {...}` block.
The solution implemented here is thus to *edit* the viewer CSS, by either removing the entire `@media ...` block in light-mode or by ensuring that its rules become *unconditionally* applied in dark-mode.
To simplify the overall implementation, since all of this does seem like somewhat of an edge-case, the `viewerCssTheme` preference will *only* be read during viewer initialization. (Similar to many other existing preferences, a reload is thus required when changing it.)
Originally the default preferences were defined in a JSON-file checked into the repository, which was loaded using SystemJS in development mode.
Over the years a number of changes have been made to this code, most notably:
- The preferences JSON-file is now generated automatically, during building, from the `AppOptions` abstraction.
- All SystemJS usage has been removed from the development viewer.
Hence the default preferences are now available *synchronously* even in the development viewer, and it's thus no longer necessary to defer to the microtask queue (since `getDefaultPreferences` is async) just to get the default preferences.
While the effect of these changes is quite small, it *does* reduces the time it takes for the preferences to be fully initialized. Given the amount of asynchronous code during viewer initialization, every bit of time that we can save should thus help.
- Add support for logical assignment operators, i.e. `&&=`, `||=`, and `??=`, with a Babel-plugin. Given that these required incrementing the ECMAScript version in the ESLint and Acorn configurations, and that platform/browser support is still fairly limited, always transpiling them seems appropriate for now.
- Cache the `hasJSActions` promise in the API, similar to the existing `getAnnotations` caching. With this implemented, the lookup should now be cheap enough that it can be called unconditionally in the viewer.
- Slightly improve cleanup of resources when destroying the `WorkerTransport`.
- Remove the `annotationStorage`-property from the `PDFPageView` constructor, since it's not necessary and also brings it more inline with the `BaseViewer`.
- Update the `BaseViewer.createAnnotationLayerBuilder` method to actaually agree with the `IPDFAnnotationLayerFactory` interface.[1]
- Slightly tweak a couple of JSDoc comments.
---
[1] We probably ought to re-factor both the `IPDFTextLayerFactory` and `IPDFAnnotationLayerFactory` interfaces to take parameter objects instead, since especially the `IPDFAnnotationLayerFactory` one is becoming quite unwieldy. Given that that would likely be a breaking change for any custom viewer-components implementation, this probably requires careful deprecation.
*Note that I wasn't able to reproduce the issue in Firefox, but only in Chromium-browsers.*
The bug, and it's feels almost trivial once you've found it, is that we're not passing the `transform` parameter as intended to `PDFPageProxy.render` when drawing thumbnails on HiDPI displays. Instead the canvas context is, for reasons that I don't even pretent to understand, *manually* scaled in `PDFThumbnailView._getPageDrawContext`, which thus doesn't guarantee that the `baseTransform` property on the `CanvasGraphics`-instances becomes correct.
The solution is really simple though, just handle the `transform` the same way in `PDFThumbnailView.draw` as in `PDFPageView.paintOnCanvas` and things should just work.
*This is a pre-existing issue that I noticed while working on PR 12613, and fixing this also brings the thumbnail code inline with the page code.*
Given the intermittent nature of all of this, it's somewhat difficult to reproduce it consistently; however the following steps should at least provide an outline:
1. Open the sidebar, and the thumbnailView, and start scrolling around.
2. *Quickly* close the sidebar, so that all thumbnails won't have time to finish rendering.
3. Either wait for the cleanup-timeout to occur, or simply run `PDFViewerApplication.cleanup()` in the console.
What *intermittently* happens here is that `WorkerTransport.startCleanup` rejects, and consequently that cleanup doesn't complete as intended, since some of the thumbnails are left in a *pending* renderingState[1].
Fixing this is simple though, and only requires updating `PDFThumbnailViewer.cleanup` along the lines of `BaseViewer.cleanup`.
---
[1] Keep in mind that thumbnails will *only* render when the thumbnailView is visible, to reduce resource usage.
This patch will help reduce memory usage, especially for longer documents, when the user scrolls around in the thumbnailView (in the sidebar).
Note how the `PDFPageProxy.cleanup` method will, assuming it's safe to do so, release main-thread resources associated with the page. These include things such as e.g. image data (which can be arbitrarily large), and also the operatorList (which can also be quite large).
Hence when pages are evicted from the `PDFPageViewBuffer`, on the `BaseViewer`-instance, the `PDFPageView.destroy` method is invoked which will (among other things) call `PDFPageProxy.cleanup` in the API.
However, looking at the `PDFThumbnailViewer`/`PDFThumbnailView` classes you'll notice that there's no attempt to ever call `PDFPageProxy.cleanup`, which implies that in certain circumstances we'll essentially keep all resources allocated permanently on the `PDFPageProxy`-instances in the API.
In particular, this happens when the users opens the sidebar and starts scrolling around in the thumbnails. Generally speaking you obviously need to keep all thumbnail *images* around, since otherwise the thumbnailView is useless, but there's still room for improvement here.
Please note that the case where a *rendered page* is used to create the thumbnail is (obviously) completely unaffected by the issues described above, and this rather only applies to thumbnails being explicitly rendered by the `PDFThumbnailView.draw` method.
For the latter case, we can fix these issues simply by calling `PDFPageProxy.cleanup` once rendering has finished. To prevent *accidentally* pulling the rug out from under `PDFPageViewBuffer` in the viewer, which expects data to be available, this required adding a couple of new methods[1] to enable checking that it's indeed safe to call `PDFPageProxy.cleanup` from the `PDFThumbnailView.draw` method.
It's really quite fascinating that no one has noticed this issue before, since it's been around since basically "forever".
---
[1] While it should be *very* rare for `PDFThumbnailView.draw` to be called for a pageView that's also in the `PDFPageViewBuffer`, given that pages are rendered before thumbnails and that the *rendered page* is used to create the thumbnail, it can still happen since rendering is asynchronous.
Furthermore, it's also possible for `PDFThumbnailView.setImage` to be disabled, in which case checking the `PDFPageViewBuffer` for active pageViews *really* matters.
* When no actions then set it to null instead of empty object
* Even if a field has no actions, it needs to listen to events from the sandbox in order to be updated if an action changes something in it.
Note that a number of these cases are covered by existing unit-tests, and a few others only matter for the development/build scripts.
Furthermore, I've also tried to the best of my ability to test each case *manually* to hopefully further reduce the likelihood of this patch introducing any bugs.
Please find additional details about the ESLint rule at https://eslint.org/docs/rules/no-useless-escape
Given the number of parameters, and the fact that many of them are booleans, the call-sites are no longer particularly easy to read and understand. Furthermore, this slightly improves the formatting of the JSDoc-comment, since it needed updating as part of these changes anyway.
Finally, this removes an unnecessary `numViews === 0` check from `getVisibleElements`, since that should be *very* rare and more importantly that the `binarySearchFirstItem` function already has a fast-path for that particular case.
This patch addresses a review comment, which pointed out that we should *also* handle the pageNumber-input, from PR 12493.
Given that a user *manually* changing pages using the pageNumber-input, on the toolbar, could be regarded as a pretty strong indication of user-intent w.r.t. navigation in the document, hence I suppose that updating the browser history in this case as well probably won't hurt.
All of these methods will, in one way or another, cause e.g. scrolling or zooming to occur and consequently they don't really make sense unless there's an active PDF document. Especially since all of these methods end up calling into a `BaseViewer`-instance, which already contains similar early returns in essentially all of it's methods and setters.
This fixes only those warnings, as reported by https://lgtm.com/projects/g/mozilla/pdf.js?mode=list, that make sense (as far as I'm concerned).
Hence this patch leaves the following things unaddressed:
- The "recommendation"-category, since it only complains about unused variables. However, note that all of those cases are purposely included and that there's thus ESLint-disable comments added to explictly allow them.
- The "warning"-category, which still contains two complaints. However, as far as I can tell, they are both false positives.
Given first of all the false positives of the LGTM static analyzer, and secondly that we'd need to add (essentially duplicated) disable-comments for the unused variable cases, it's not entirely clear to me if we actually want to work towards including LGTM in the PDF.js project (e.g. running alongside Travis) or if we should just close issue 11965.
Given that we're now accessing certain API-functionality *directly* in this file, e.g. the AnnotationStorage and Optional Content configuration, ensuring that there's not a version mismatch definitely seem like a good idea to prevent any *subtle* future bugs.
Ensure that these tooltip-only Annotations are handled as "internalLink"s, to ensure that they behave as expected in PresentationMode (e.g. they should still use a `pointer`-cursor).
Ensure that `PDFLinkService.getDestinationHash` won't create links with empty hashes, since those don't really make a lot of sense in general (this improves things for tooltip-only Annotations).
This PDF file can be used for testing: http://mirrors.ctan.org/macros/latex/contrib/pdfcomment/doc/pdfcomment.pdf#page=14
- Return early in `PDFViewerApplication._initializeJavaScript` for PDF documents without any `fieldObjects`, which is the vast majority of all documents, to prevent errors when trying to parse a non-existent object.
- Similar to the other `PDFViewerApplication._initialize*` methods, ignore the `fieldObjects` if the document was closed before the data resolved.
- Fix the JSDoc comment for the `generateRandomStringForSandbox` helper function, since there's currently a bit too much copy-and-paste going on :-)
- Change `FirefoxScripting` to a class with static methods, which is consistent with the surrounding code in `web/firefoxcom.js`.
There's no compelling reason to update this property *manually* in multiple places, since that's error-prone with any future code changes, given that `_updateInternalState` is always called just before anyway.
While the referenced issue could very well be seen as an edge-case, this patch adds support for updating of the browser history when interacting with the thumbnails in the sidebar (assuming we want to do this).
The main reason for adding the history implementation in the first place, was to simplify navigating back to a previous position in the document when named/explicit destinations are used (e.g. when clicking on "links" or when using the outline in the sidebar).
As such, it never really crossed by mind to update the browser history when the thumbnails are used. However, a user clicking on thumbnails could be regarded as a pretty strong indication of user-intent w.r.t. navigation in the document, hence I suppose that updating the browser history in this particular case probably won't hurt.
This modernizes and improves the code, by using `async`/`await` and by extracting the helper function to its own method.
To hopefully avoid confusion, given the next patch, the method is also re-named to `goToDestination` to make is slightly clearer what it actually does.
Given that we're no longer using SystemJS to load the `web/` files, see PR 11919, there's nothing that prevents us from using standard `ìmport` statements in this file.
Obviously it's still necessary to load part of the code conditionally on the build type, however this still allows us to clean-up and simplify at least some of this file.
The `debugger`-statement would only, potentially, make sense during development and we thus want to prevent it from being accidentally included when landing code.
The `alert`, `confirm`, and `prompt` functions should generally be avoided, with the few intended cases manually allowed.
Please find additional details about the ESLint rules at:
- https://eslint.org/docs/rules/no-debugger
- https://eslint.org/docs/rules/no-alert
In the rest of the viewer code-base, we purposely don't treat `RenderingCancelledException`s as actual errors (since they aren't) and consequently we never log them.
Hence it makes sense, as far as I'm concerned, to simply treat `RenderingCancelledException`s the same way when printing in Firefox.
While I don't print a whole lot, I cannot remember seeing these "errors" logged when printing until *very* recently[1]. Given that the browser print functionality and UI, in Firefox, is under active development it's certainly possible that there's some recent changes to the related timings which make `RenderingCancelledException`s more likely now.
---
[1] Interestingly, only some PDF documents seem to be affected as well; I'm able to reproduce this pretty consistently by opening https://www.uni-muenster.de/imperia/md/content/ziv/pdf/printpay_flyer.pdf in Firefox and then repeating the following sequence:
Clicking on the PDF.js print button, and then cancelling printing.
This should be helpful to easily determine the *exact* version of the viewer itself, when looking at a *built* `web/viewer.js` file.
Note that we're already including this information in other built files, such as e.g. `pdf.js`, `pdf.worker.js`, `pdf_viewer.js`, and `pdf.image_decoders.js`.
This adds a new `PDFViewerApplication.triggerPrinting` method, which takes care of checking that printing is actually supported before calling `window.print`, to remove the need to duplicate that code in multiple places.
Also, removes the `PDFViewerApplication.printing` getter since it's not really necessary any more.
For now we need to use a Babel-plugin, since part of our build system doesn't support this fully (e.g. Babel-loader, Webpack 4.x, and SystemJS).
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/Optional_chaining
Given how those are used, there *should* not be any situation in which e.g. `undefined` is ever returned. However, actually checking that the pageView/thumbnail is defined cannot hurt.
Also, re-factor `webViewerPageRendered` slightly since the `pageView` is no longer unconditionally necessary after the previous patches; note in particular that the thumbnails will only be updated when the sidebar *and* the thumbnailView is visible.
Finally, fixes a bug in `webViewerPageChanging` whereby an empty string would not be treated as a valid pageLabel and instead be replaced by `null`.
Given that the default viewer only uses the "page stats" when debugging is enabled, it seems much simpler and more straightforward to simply query the API *directly* when this information is actually required. That way, there's a bit less information that needs to be stored/updated on each `PDFPageView`-instance.
Finally, since the `EventBus` now exists, we no longer need to handle the "page stats"-case in the regular listeners in `web/app.js`, but can instead add special "page stats"-listeners only when debugging is enabled.
The way that rendering errors are handled in `PDFPageView` is *very* old, and predates e.g. the introduction of the `EventBus` by several years.
Hence we should be able to simplify things a bit here, by including the Error (when it exists) in the "pagerendered" event and thus avoid having to reach into `PDFPageView` for it.
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.
For years the loadingBar and sidebarContainer has had a slightly annoying and unfortunate dependency, since the loadingBar width follows the main toolbar width[1].
To prevent the loadingBar from obscuring part of the sidebarContainer, especially the buttons, the sidebarContainer is thus moved down when the loadingBar is visible. This has always annoyed me[2], since it means that the buttons in the sidebar may thus move vertically which seems bad from a UX perspective.
Now that CSS variables are available in all supported browsers[3] however, fixing the loadingBar/sidebarContainer overlap issues are finally easy. The solution is simply to let the sidebarContainer, when visible, control the loadingBar left position (right in RTL locales) in the same way that the viewerContainer is handled. Hence the sidebarContainer can now have a *consistent* vertical postition, without the loadingBar overlapping it.
---
[1] Obviously the right position (left in RTL locales) of the loadingBar is, potentially, reduced to account for a scrollbar.
[2] I've tried to fix this a few times, but it always seemed like more trouble than it's worth.
[3] https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_custom_properties#Browser_compatibility
The `getVisibleElements` helper function currently requires the viewerContainer to be absolutely positioned; possibly fixing this is tracked in issue 11626.
Without `position: absolute;` set, in the CSS, there's a number of things that won't work correctly such as e.g.
- Determining which pages are currently visible, thus forcing all of them to render on load and increasing resource usage significantly; note https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions#allthepages
- Scrolling pages into view, by using the `BaseViewer.currentPageNumber` setter or similar.
Based on the number of opened issues over the years, the fact that `position: absolute;` is required has shown to be something that users can very easily overlook unless they follow e.g. the `simpleviewer` example to the letter.
Hence, to improve things until such a time that issue 11626 is fixed, we'll now refuse to initialize a `BaseViewer` instance unless the `container` has the required CSS set. (Forcibly setting `position: absolute;` on the viewerContainer element is bound to cause significantly more issues/confusion, hence the current approach of throwing an Error.)
This reverts commit 9e4552d792 for causing the sidebar to become too narrow when the entire viewer is resized.
**Steps to reproduce:**
1. Load the viewer.
2. Open the sidebar.
3. Resize the sidebar, making it wider.
4. Resize the entire viewer, i.e. the browser window, making it *narrower* than 400 pixels.
**Expected result:**
The sidebar width is clamped at 200 pixels.
**Actual result:**
The sidebar becomes too narrow.
The cause of this bug is, in hindsight, quite obvious since the `clamp` helper function implicitly assumes that the `min`/`max` arguments are correctly sorted. At viewer widths *below* 400 pixels, that assumption is broken which explains the bug.
Given that the outlineView/attachmentsView/layersView all share a common base-class and CSS rules, see PRs 12169 and 12170, the names of the CSS variables in question feels slightly strange now.
This patch purposely starts small, by removing IE-specific code from various JS/CSS files in the `web/` folder.
There's obviously lots of potential for additional clean-up, especially the removal of no longer necessary polyfills in `src/shared/compatibility.js`, however that will require some care considering that certain polyfills may also be necessary for e.g. Node.js or the Chromium-extension as well.
Generally speaking, once we start removing polyfills it's probably a good idea to consult the compatibility information on https://developer.mozilla.org/ and also https://caniuse.com/ first. (Deciding on the lowest supported Chromium version, for the extension, would also seem like a good idea.)
This reverts commit 2a0de0b66b.
I can no longer reproduce these issues locally, and if ad blockers are still interfering with this functionality we really ought to pursue a mozilla-central solution to the problem instead. (Also, I'm no longer getting an "Open with Firefox"-option in the "Open with"-dialog making the PDF attachments experience worse for all users.)
This should help prevent future issues, caused by the user omitting the `viewer` option and/or providing an incorrect `container` option, when initializing a `BaseViewer`-instance.
This fixes a set of issues described in Mozilla bug 1662426[1].
In particular, once the print callback fails once (because the printing
operation has been canceled in Gecko / replaced by a newer one, for example) it
can't be re-invoked.
This patch fixes it by properly cancelling the render task if it throws, or if
the print callback is called again while ongoing.
[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1662426
All of the core/display functionality needed to support this already exists, we simply need to handle these named actions in the viewer and the buttons will "just" work.
Unfortunately there's not really any good way of testing this, but given the size and scope of the patch that's hopefully OK.
This is *similar* to the existing linting for JavaScript files, but covers CSS files instead.
While there's a lot of rules that could potentially be used, the main advantage of using Stylelint is that it has Prettier integration which means that we can automatically enforce a *consistent* style for our CSS files as well.
As a proof of concept, this patch is purposely limited to:
- Adding a simple rule, here `block-no-empty` is chosen; see https://stylelint.io/user-guide/rules/block-no-empty
- Adding Prettier integration, to unify the style of our CSS files.
Please find additional information at https://stylelint.io/
*Besides, obviously, adding viewer support:* This patch attempts to improve the general API for Optional Content Groups slightly, by adding a couple of new methods for interacting with the (more complex) data structures of `OptionalContentConfig`-instances. (Thus allowing us to mark some of the data as "private", given that it probably shouldn't be manipulated directly.)
By utilizing not just the "raw" Optional Content Groups, but the data from the `/Order` array when available, we can thus display the Layers in a proper tree-structure with collapsible headings for PDF documents that utilizes that feature.
Note that it's possible to reset all Optional Content Groups to their default visibility state, simply by double-clicking on the Layers-button in the sidebar.
(Currently that's indicated in the Layers-button tooltip, which is obviously easy to overlook, however it's probably the best we can do for now without adding more buttons, or even a dropdown-toolbar, to the sidebar.)
Also, the current Layers-button icons are a little rough around the edges, quite literally, but given that the viewer will soon have its UI modernized anyway they hopefully suffice in the meantime.
To give users *full* control of the visibility of the various Optional Content Groups, even those which according to the `/Order` array should not (by default) be toggleable in the UI, this patch will place those under a *custom* heading which:
- Is collapsed by default, and placed at the bottom of the Layers-tree, to be a bit less obtrusive.
- Uses a slightly different formatting, compared to the "regular" headings.
- Is localizable.
Finally, note that the thumbnails are *purposely* always rendered with all Optional Content Groups at their default visibility state, since that seems the most useful and it's also consistent with other viewers.
To ensure that this works as intended, we'll thus disable the `PDFThumbnailView.setImage` functionality when the Optional Content Groups have been changed in the viewer. (This obviously means that we'll re-render thumbnails instead of using the rendered pages. However, this situation ought to be rare enough for this to not really be a problem.)
This patch:
- Removes the :hover effect from the `findMsg` element, since it's a simple span and clicking it *obviously* does nothing.
- Given the way that the checkboxes are visually hidden, with `opacity: 0;` and absolute positioning, they are unfortunately still focusable (fixed by adding `pointer-events: none;`). To reproduce this, in `master`: Place the mouse pointer over the upper left-hand corner of the "Highlight all"-option, and notice that the :hover effect vanishes and clicking toggles the "Match case"-option instead.
This special progressBar is only used in the (fortunately) rare case when a server doesn't provide a valid `Content-Length` header. Since this progressBar isn't normally seen, when testing the default viewer, it's certainly very easy to see why these CSS rules were missed during review.
Furthermore, this patch also makes a couple of *small* progressBar CSS tweaks not related to the colours.
With the changes in PR 11077, these panels are no longer aligned exactly with the *center* of the corresponding toolbar buttons. This is especially noticeable for the `findbar` at narrow viewer width.
Unfortunately the work-around implemented in PR 12286 didn't actually work in all cases, please refer to the previous commit messages.
To prevent opening of PDF attachments from being completely broken for some users, we'll simply force-download them for now in MOZCENTRAL-builds to unbreak things. (Given that the "Open with" dialog now features a "Open with Firefox"-option, this is less bad than it previously would've been.)
This should provide better filetype detection when downloading PDF attachments in the viewer.
Also, to avoid creating the "is PDF file" regular expression more than once it's extracted into a global constant instead.
This reverts commit 1e5d4b6a80, since it unfortunately doesn't work in all situations.
Please note that I did *successfully* test the patch in a local Firefox build, obviously with an ad blocker installed.
However, I've now tested the *latest* Nightly-build with my default profile, and unfortunately I can still reproduce the bug there!?
Unfortunately e.g. ad blockers can interfere with `window.open` calls, thus preventing PDF attachments from being opened/viewed. For the MOZCENTRAL-build, we can work-around this problem by using a (hidden) link instead.
Without these changes, clicking on the "Open With Different Viewer"-button on the Firefox fallback bar won't actually do anything and the following is printed in the web-console:
```
Uncaught TypeError: (destructured parameter) is undefined
download resource://pdf.js/web/viewer.js:956
response resource://pdf.js/web/viewer.js:1054
listener resource://pdf.js/web/viewer.js:11891
viewer.js:956:1
```
Furthermore, this patch also fixes `PDFViewerApplication.fallback` to pass in an explicit `sourceEventType` when triggering downloading. While this, on its own, would obviously have been sufficient to fix the bug described above, it seems wrong to outright break backwards compatibility of any older `PDFViewerApplication.download` calls.
Good form type detection is important to get reliable telemetry and to
only show the fallback bar if a form cannot be filled out by the user.
PDF.js only supports AcroForm data, so XFA data is explicitly unsupported
(tracked in issue #2373). However, the previous form type detection
couldn't separate AcroForm and XFA well enough, causing form type
telemetry to be incorrect sometimes and the fallback bar to be shown for
forms that could in fact be filled out by the user.
The solution in this commit is found by studying the specification and
the form documents that are available to us. In a nutshell the rules are:
- There is XFA data if the `XFA` entry is a non-empty array or stream.
- There is AcroForm data if the `Fields` entry is a non-empty array and
it doesn't consist of only document signatures.
The document signatures part was not handled in the old code, causing a
document with only XFA data to also be marked as having AcroForm data.
Moreover, the old code didn't check all the data types.
Now that AcroForm and XFA can be distinguished, the viewer is configured
to only show the fallback bar for documents that only have XFA data. If
a document also has AcroForm data, the viewer can use that to render the
form. We have not found documents where the XFA data was necessary in
that case.
Finally, we include unit tests to ensure that all cases are covered and
move the form type detection out of the `parse` function so that it's
only executed if the document information is actually requested
(potentially making initial parsing a tiny bit faster).
Currently there's enough leading padding that the `numPages` span feels somewhat "disconnected" from the `pageNumber` input, which seems unfortunate when they contain related state.
This solution is obviously *not* perfect, since printing being cancelled will thus remove the warning as well. However, a similar problem already exists for saving, since the user may cancel that one as well.
All-in-all, since way cannot really detect with absolute certainty that either saving or printing actually finished, this seems good enough for now.
Given that `renderInteractiveForms` is now enabled by default in "full" viewer, it seems reasonable to enable it by default in the viewer components as well.
Especially considering that it's simple to disable, when creating the affected components, for anyone implementing their own viewer.
Related to https://bugzilla.mozilla.org/show_bug.cgi?id=1659753
This allows Firefox trigger a "save" event from ctrl/cmd+s or the "Save
Page As" context menu, which in turn lets pdf.js generate a new PDF if
there is form data to save.
I also now use `sourceEventType` on downloads so Firefox can determine if
it should launch the "open with" dialog or "save as" dialog.
With the changes in PR 11077, the zoom dropdown now looks "squashed" in locales with longer than average zoom-strings[1]. The reason is that the zoom-value and the dropdown-icon are too close together, which doesn't look good in affected locales.
To fix this, the following changes are made:
- Increase the calculated dropdown width, in `Toolbar._adjustScaleWidth`, to account for the much wider icon (7 px -> 16 px) and the increased padding.
- Move the dropdown-icon *slightly* outwards, and also *slightly* reduce the left (right in RTL locales) padding of the dropdown-contents.
- Finally, remove the right (left in RTL locales) padding to reduce the chance of the *default* browser dropdown-icon being visible.
---
[1] This affects e.g. the `de` and `nl` locales, but there's probably other examples as well.
This is needed for some smoke tests in mozilla central for testing forms
in pdf.js.
Note: AnnotationLayerBuilder.render() doesn't really need to be async, but
we're talking of making the annotation's render functions async, so this
will make that switch easier.
Currently using a touchscreen with pdf.js doesn't work so well. In Firefox,
with apz.allow_zooming = false (default on current release/beta), it does a
reflow zoom which makes the UI elements bigger. And with apz.allow_zooming = true
(default on current Firefox nightly), or in Chrome, it does a smooth pinch-zoom
but that also scales up the entire UI. Neither of these is a particularly good
experience, so this patch just disables any multi-touch gestures. Touch-based
panning (which involves a single touch point) is left unaffected.
I obviously missed this during review, but currently `PDFViewerApplication._saveInProgress` is reset *synchronously* in `PDFViewerApplication.save`.
That was probably not intended, since it essentially renders the `PDFViewerApplication._saveInProgress` check pointless given that the actual saving is an *asynchronous* operation.
The original code would get a long sequence of miniscule "tick" values while
pinch-zooming, and each tick value would cause a 1.1x zoom. So even the smallest
pinch gesture on a trackpad would cause high amounts of zoom. This patch
accumulates the wheel deltas until they reach an integer threshold (with a
tweak of the scaling factor to make it feel more natural) at which point it
triggers the zoom based on the integer component of the accumulated delta. The
fractional part is retained in the accumulator.
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).
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
This was added in PR 4470, but doesn't appear to have been used since.
While it's certainly easy to understand how this was helpful during development of that PR, actually providing this hash parameter isn't going to work anymore given that the original CMap files were also removed from the repository.
I suppose that the hash parameter *could* be useful if you'd attempt to update the BCMap files, however that hasn't been attempted even once in over *five* years time. Furthermore, at this point using the `AppOptions` directly in that situation should also work fine.
All in all, this seems like a piece of old and unused code which we can simply remove now.
By using the same internal formatting here as in the `Ref.toString` method, in `src/core/primitives.js`, all cache-keys will become at least two bytes shorter (and most three bytes shorter).
Obviously this won't have a huge effect on memory since there's only one cache entry per page, but it nonetheless seems wasteful to use longer keys than strictly required.
