- 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".)
