For encrypted PDF documents without the required permissions set, this patch adds support for disabling of Annotation-editing. However, please note that it also requires that the `pdfjs.enablePermissions` preference is set to `true` (since PDF document permissions could be seen as user hostile).[1]
As I started looking at the issue, it soon became clear that *only* trying to fix the issue without slightly re-factor the surrounding code would be somewhat difficult.
The following is an overview of the changes in this patch; sorry about the size/scope of this!
- Use a new `AnnotationEditorUIManager`-instance *for each* PDF document opened in the GENERIC viewer, to prevent user-added Annotations from "leaking" from one document into the next.
- Re-factor the `BaseViewer.#initializePermissions`-method, to simplify handling of temporarily disabled modes (e.g. for both Annotation-rendering and Annotation-editing).
- When editing is enabled, let the Editor-buttons be `disabled` until the document has loaded. This way we avoid the buttons becoming clickable temporarily, for PDF documents that use permissions.
- Slightly re-factor how the Editor-buttons are shown/hidden in the viewer, and reset the toolbar-state when a new PDF document is opened.
- Flip the order of the Editor-buttons and the pre-exising toolbarButtons in the "toolbarViewerRight"-div. (To help reduce the size, a little bit, for the PR that adds new Editor-toolbars.)
- Enable editing by default in the development viewer, i.e. `gulp server`, since having to (repeatedly) do that manually becomes annoying after a while.
- Finally, support disabling of editing when `pdfjs.enablePermissions` is set; fixes issue 15049.
---
[1] Either manually with `about:config`, or using e.g. a [Group Policy](https://github.com/mozilla/policy-templates).
- each annotation has its coordinates/dimensions expressed in percentage,
hence it's correctly positioned whatever the scale factor is;
- the font sizes are expressed in percentage too and the main font size
is scaled thanks a css var (--scale-factor);
- the rotation is now applied on the div annotationLayer;
- this patch improve the rendering of some strings where the glyph spacing
was not correct (it's a Firefox bug);
- it helps to simplify the code and it should slightly improve the update of
page (on zoom or rotation).
We want to avoid adding regular `id`s to Annotation-elements, since that means that they become "linkable" through the URL hash in a way that's not supported/intended. This could end up clashing with "named destinations", and that could easily lead to bugs; see issue 11499 and PR 11503 for some context.
Rather than using `id`s, we'll instead use a *custom* `data-element-id` attribute such that it's still possible to access the Annotation-elements directly.
Unfortunately these changes required updating most of the integration-tests, and to reduce the amount of repeated code a couple of helper functions were added.
This option is not used, nor has it ever been used, in the *built-in* Firefox PDF Viewer. Hence we can define it only for the environments where it makes sense instead.
This should really have been done as part of PR 12470, since it's now possible to directly set the `defaultUrl`-option without having to fallback to `var`-usage.
- Right now, we must select the tool, then click to select a page and
click to start drawing and it's a bit painful;
- so just create a new ink editor when we're hovering a page without one.
Given that the SVG back-end is not defined anywhere except in GENERIC builds, we can remove a little bit of unnecessary code in e.g. the Firefox PDF Viewer.
Apparently the ESLint rule added in PR 15031 wasn't able to catch all cases that can be converted, which is probably not all that surprising given how some of these call-sites look.
- Use `Element.prepend()` to insert nodes before all other ones in the element, rather than using `firstChild` with `insertBefore`-calls; see https://developer.mozilla.org/en-US/docs/Web/API/Element/prepend
- Fix one *incorrect* `insertBefore` call, in the AnnotationLayer-code.
Initially the patch simply changed that to an `Element.before()`-call, however that broke one of the integration-tests. It turns out that the `index` may try to access a non-existent select-child, which triggers undefined behaviour; note the warning in https://developer.mozilla.org/en-US/docs/Web/API/Node/insertBefore#parameters
The update in https://bugzilla.mozilla.org/show_bug.cgi?id=1773794 failed, because the `editorNone` icon is identical to a pre-existing one. Given that all of the editor-icons are simply placeholders for now, we can just make a tiny change to the SVG-paths to prevent these kind of problems.
- Approximate the drawn curve by a set of Bezier curves in using
js code from https://github.com/soswow/fit-curves.
The code has been slightly modified in order to make the linter
happy.
Because of a capitalization error, the MIME type wasn't actually being set correctly. However, please note that downloading of font files still worked correctly which is probably why this has gone unnoticed since 2014.
Given the differences between XFA documents and "normal" PDF documents, we don't support editing of the former ones. Hence, when a XFA-document is opened, we temporarily disable the editor-buttons.
- Ensure that the modified-warning won't be displayed, when navigating away from the viewer, if the user has added custom Annotations and then *removed all* of them.
- Ensure that the *initial* editor-buttons state, i.e. the `toggled`-class, is correctly displayed in the toolbar when then viewer loads.
- Tweak the CSS-classes for the editor-buttons, such that they use the correct focus/hover-rules (similar to the sidebar-buttons).
- Remove a no longer accurate comment from the `BaseViewer.annotationEditorMode`-setter.
- Address a couple of *smaller* outstanding review comments, including some re-formatting changes, from PR 14976.
In PR 14710 we only included the JavaScript-part of the polyfill, however we probably need to include the CSS as well to reduce the risk of problems in older browsers.
With the recent CSS-related improvements in the `preprocess`-function we could probably have included this conditionally in the `viewer.css` file. However, considering that the `<dialog>` polyfill-code is only invoked when actually needed it seemed most appropriate/correct to lazy-load the polyfill-CSS as well.
- right now we're using the font size from the pdf itself but we use an other font
in the annotation layer. So this size doesn't really make sense and leads to bad
rendering (see pdf in #14928);
- use a sans-serif font for the fields containing text (fix issue #14736);
- remove useless padding in text-based fields (fix issue #14301);
- text fields allow/disallow scrolling bars (see bit 24 in Ff entry), so use this
value to hide/show scrollbars in annotation layer.
- it aims to fix https://bugzilla.mozilla.org/show_bug.cgi?id=1771477;
- hangul contains some syllables which are decomposed when using NFD, hence
the text must be correctly shifted in case it contains some of them.
Currently, when range-requests and/or streaming are not supported or for documents opened from `data`-URLs, we'll manually set the `contentDispositionFilename` (assuming it exists and is valid) from the `onOpenWithData`-callback in `PDFViewerApplication.initPassiveLoading`.
However, because of a small oversight in `PDFViewerApplication._initializeMetadata`, this *cached* `contentDispositionFilename` would be ignored and we'd only attempt to use the one returned by `PDFDocumentProxy.getMetadata` in the API (which in the cases outlined above will always be empty).
Also, to ensure that the document properties dialog always displays the *correct* fileName we'll now lookup it using the same exact method as in the viewer itself (via a new callback-function).
With the exception of `isThumbnailViewVisible`, these getters are completely unused. Generally speaking, using the `visibleView`-getter directly works just as well and seems (at least to me) to be overall preferable considering how our classes are usually implemented.
Currently, when non-standard `pageColors` are specified, the thumbnails will look inconsistent depending on how they're created.
The thumbnails that are created by downsizing the *page* canvases will obviously use the `pageColors` as intended, however the thumbnails which are rendered *directly* will always use the default colors.
Over time, as we've been introducing JavaScript code to modify CSS variables, we've been adding shorthand properties to various classes to reduce unnecessary repetition when accessing the document-styles.
Rather than repeating this in multiple places, it seems overall simpler to just introduce a constant and re-use that throughout the viewer instead.
After the changes in https://bugzilla.mozilla.org/show_bug.cgi?id=1757771, that simplified the MOZCENTRAL downloading code, the `ChromeActions.download`-method will no longer invoke the `sendResponse`-callback.
Hence it should no longer be necessary for the `DownloadManager`, in the MOZCENTRAL viewer, to use `FirefoxCom.requestAsync` since no response is ever provided.[1] For the allocated BlobURLs, they should (hopefully) be released when navigating away from the viewer.
---
[1] Note that that was *already* the case, for one of the previous code-paths in the `ChromeActions.download`-method.
After the changes in https://bugzilla.mozilla.org/show_bug.cgi?id=1757771, that simplified the MOZCENTRAL downloading code, the `sourceEventType` is now completely unused and should thus be removed (in my opinion).
Furthermore, with these changes, we also no longer need a *separate* internal "save"-event and can instead just use the older "download"-event everywhere.
The original `ProgressBar`-functionality is very old, and could thus do with some general clean-up.
In particular, while it currently accepts various options those have never really been used in either the default viewer or in any examples. The sort of "styling" that these options provided are *much better*, not to mention simpler, done directly with CSS rules.
As part of these changes, the "progress" is now updated using CSS variables rather than by directly modifying the `style` of DOM elements. This should hopefully simplify future changes to this code, see e.g. PR 14898.
Finally, this also fixes a couple of other small things in the "mobile viewer" example.
While zeroing the temporary `canvas` makes sense, manually clearing the canvas and its context doesn't really accomplish anything since those are tied to the scope of the method.
We need to wait for UI rendering to start *before* getting the CSS variable values, since otherwise the values will be `NaN`.
This is only an issue if the viewer is completely hidden during loading, e.g. in a `display: none` iframe-element.
*This patch can be tested, in the viewer, using the `annotation-fileattachment.pdf` document from the test-suite.*
It seems that the code to delay dispatching of the "attachmentsloaded"-event, when `attachmentsCount === 0`, is now effectively broken.[1]
Rather than only using an arbitrary timeout, the new code will instead wait for an "annotationlayerrendered"-event and only *fallback* to using a timeout.
---
[1] The timing of the annotationLayer-rendering changed slightly in PR 14247, and the old code in `PDFAttachmentViewer` wasn't good enough to handle that.
Given that the new API-option is an Object named `pageColors`, with `background`/`foreground` keys, it occurred to me that it'd be slightly more consistent if the options/preferences names fully reflected that.
An old shortcoming of the `preprocessCSS`-function is its complete lack of support for our "normal" defines, which makes it very difficult to have build-specific CSS rules. Recently we've started using specially crafted comments to remove CSS rules from the MOZCENTRAL build, but (ab)using the `preprocessCSS`-function in this way really doesn't feel great.
However, it turns out to be surprisingly simple to instead use the "regular" `preprocess`-function for the CSS files as well. The only special-handling that's still necessary is the helper-function for dealing with CSS-imports, but apart from that everything seems to just work.
One reason, as far as I can tell, for having a separate `preprocessCSS`-function was likely that we originally used *lots* of vendor-prefixed CSS rules in our CSS files. With improvements over the years, especially thanks to Autoprefixer and PostCSS, we've been able to remove *almost* all non-standard CSS rules and the need for special-casing the CSS parsing has mostly vanished.
*Please note:* As part of testing this patch I've diffed the output of `gulp generic`, `gulp mozcentral`, and `gulp chromium` against the `master`-branch to check that there was no obvious breakage.
These methods are completely unused in the Firefox PDF Viewer, and were only added to supplement the `PDFViewerApplication.{bindEvents, bindWindowEvents}`-methods since third-party implementations may apparently need to remove event listeners (see PR 8525).
However, in the MOZCENTRAL build that's just dead code and this patch reduces the size of the *built* `web/viewer.js`-file by `~3.5` kB.
*This patch can be tested, in the viewer, using the `annotation-fileattachment.pdf` document from the test-suite.*
Note how the `FileSpec`-implementation already uses `stringToPDFString` during the filename lookup, see cfac6fa511/src/core/file_spec.js (L70)
Hence there's no reason to repeat that again in the `FileAttachmentAnnotationElement`-constructor, and we can thus simplify the "fileattachmentannotation"-event handling a little bit.
This new CSS variable will allow us to simplify a couple of different viewer components, since we no longer need to use JavaScript-based hacks and can directly set the CSS rules instead. In particular:
- The `BaseViewer`-handling, used as part of the code that will center pages *vertically* in PresentationMode, can be simplified.
By using CSS to control the height of the "dummy"-page we avoid unnecessarily invalidating the scale-value, which can reduce *some* unneeded re-rendering while PresentationMode is active.
- The `SecondaryToolbar.#setMaxHeight`-method, and its associated parameters, are no longer necessary and can be completely removed.
Note that in order for things to work correctly in general, the new `--viewer-container-height` CSS variable must potentially be updated on any window-based "resize"-event (even when there's no zooming). While this is currently only done in the default viewer, that shouldn't be an issue since neither PresentationMode nor Toolbar-functionality is included in the "viewer components".
- Use Canvas & CanvasText color when they don't have their default value
as background and foreground colors.
- The colors used to draw (stroke/fill) in a pdf are replaced by the bg/fg
ones according to their luminance.
After recent changes, adding *basic* Spread mode support in PresentationMode has now become reasonably straightforward.
However, documents with *varying* page sizes are non-trivial to handle and would require re-writing (or at least re-factoring) a bunch of the zooming-code.
Hence this PR *purposely* only allow Spread modes to be used, in PresentationMode, for documents where all pages have exactly the same size. While this obviously isn't a fully complete solution, it will however cover the vast majority of all documents and should hopefully be deemed good enough for now.
In PR 14112 usage of this *internal* method was reduced, and it thus can't hurt to clean-up things a little bit more.
Note in particular that we can simplify the call-sites by directly passing in the already available `PDFPageView`-instance, since the `id`-property those contain can replace the previous `pageNumber`-parameter[1].
Given that the method name has always been prefixed with an underscore it was thus never intended to be "public", hence we can now enforce that with modern ECMAScript features.
---
[1] There's already a bunch of other spots, throughout the viewer-code, where we assume that the `PDFPageView.id`-property contains proper page *numbers* (and not e.g. indices); note how we initialize the `PDFPageView`-instances in the `BaseViewer.setDocument`-method.
Given that we're (ab)using spread-modes in order to ensure that pages are centered *vertically* in PresentationMode, this re-factoring simplifies the code slightly.
Furthermore, in the event that we *possibly* want to try and support spread-modes in PresentationMode[1] this re-factoring will also prevent future duplication.
---
[1] Note that I'm not particularly keen on doing that, since documents with varying page sizes will be annoying to handle.
There's no point in having these variables defined (implicitly) as `undefined` in e.g. the Firefox PDF Viewer.
By defining them with `var` and using ESList ignores, rather than `let`, we can move them into the relevant pre-processor block instead. Note that since the entire viewer-code is placed, by Webpack, in a top-level closure these variables will thus *not* become globally accessible.
Given that `webViewerOpenFileViaURL` only has a single call-site, and also isn't a particularly large/complex function, it doesn't seem necessary for this to be a separate function and hence it's simply inlined instead.
Also, changes the "no valid build-target was set"-case to throw unconditionally since the only way that it could ever be hit is if there are bugs in the `gulpfile`-code.
Currently we're using *both* ids and classes when specifying the button icons, which seems completely unnecessary and only serves to bloat the CSS code.
In general you always need to be careful about CSS specificity, but in these cases that should not actually be a problem.
Also, while not a button, this patch makes a similar simplification for the `pageNumber`-input.
Note how in the `viewer.html` file we're specifying class names for most buttons, despite that not really being necessary. First of all, in many cases those class names are *identical* to the element-ids. Secondly, looking through the CSS rules they are only ever used when specifying button icons.
All-in-all, we should be able to simplify the HTML-code and use the element-ids in the CSS rules instead. Obviously ids have a higher CSS specificity than classes, but given how the old classes were being used that shouldn't be a problem here.
Also, the patch generalizes the styling for buttons (e.g. `viewBookmark`) that are *actually* link-elements.
Finally, while slightly unrelated, this patch also removes a little bit of duplication when specifying the border for `toolbarField`s.
Searching through all of the files in the `web/`-folder has no *other* hits for the "textButton" string. Hence it's clear that there are no DOM elements actually using this class, and it's thus dead code.
This re-factors the various toolbar separators to *explicitly* specify both their dimensions and margins. Also, for the `horizontalToolbarSeparator`-class we can just set the `background-color` rather than using `border-top`.
Note that the `splitToolbarButtonSeparator`-class currently sets a number of unnecessary CSS rules, since as mentioned by the Firefox Devtools both the `display`- and `z-index`-properties are being ignored because `float` is used.
Finally, there's also no need to set a `z-index` for some of the `:hover`-rules. It's possible that this *was* necessary before the re-design, since the buttons had borders then.
Note how both of the openFile-buttons are always hidden during viewer initialization in the MOZCENTRAL build, i.e. the *built-in* Firefox PDF Viewer. Despite that we still include HTML, CSS, and JavaScript code for these buttons in the build.
This patch *reduces* the size of the `gulp mozcentral` output by `1679` bytes, which isn't a lot but still cannot hurt.
These rules became unnecessary with PR 7697, over five years ago, since printing is now done from a `printContainer`-element rather than "directly" using the viewer.
Note how the *entire* `outerContainer`, which contains all of the DOM elements that were being manually hidden, is now being hidden during printing. Furthermore, note also how the print-canvases/images and their containers are using custom CSS-classes[1] rather than re-using the ones from the viewer.
---
[1] See the `printedPage` respectively `xfaPrintedPage` classes.
Rather than modifying the leading/trailing `margin` on the actual toolbar buttons, to achieve appropriate spacing at the left/right edges of the toolbar(s), it seems much more appropriate (and simpler) to just specify an explicit `padding` for the relevant toolbar containers.
Also, for toolbar buttons placed in `splitToolbarButton`-classes we can reduce some complexity around setting the `margin` (since it should always be zero now).
With these changes, we're thus able to get rid of a couple more `!important`-rules.
- Remove a redundant `margin-top` rule for the `.dropdownToolbarButton`. After the (somewhat) recent UI-refresh all buttons now use `margin: 2px 1px;`, which renders the override unnecessary (and getting rid of an `!important`-rule can't hurt).
- Combine two `.toggled::before` rules, since they're identical.
Note that both the `errorWrapper` HTML and JavaScript code is being ignored in the MOZCENTRAL build, i.e. the *built-in* Firefox PDF Viewer, however the CSS rules are still being included.
That seems totally unnecessary, and while we currently don't have full build-target support in the CSS pre-processor we can actually improve things quite easily anyway. By (ab)using the existing CSS pre-processor, which will remove any non-Firefox CSS rules for the MOZCENTRAL build, it's possible to easily stop bundling any CSS rules by using comments that include a `-webkit`-string.
*Please note:* To easily test that this doesn't break the `errorWrapper` in GENERIC builds, try running e.g. `PDFViewerApplication._otherError("test");` in the web-console.
This special-case was added because the original PresentationMode-implementation used some CSS-tricks to hide everything except the current page. With the changes in PR 14112, which added a PAGE scroll-mode, many of the old PresentationMode-specific hacks could thus be removed from both the JS and CSS code.
This patch is yet another (small) clean-up step, to reduce the number of PresentationMode special-cases used throughout the `BaseViewer`. Note in particular that `BaseViewer.update` now works just fine in PresentationMode[1], and that we only need to ensure that the active page won't *accidentally* change because of the PresentationMode-specific zooming that occurs during page-switching.
---
[1] In the event that we ever want to try and support spread-modes in PresentationMode, which I'm really not keen on doing since documents with varying page sizes will be annoying to handle, these changes would be necessary as well.
The styling of the previous/next-buttons and the findInput, with the elements being "glued" together, was supposed to mimic the styling used in the Firefox *browser* findbar. However, after the most recent re-styling of the Firefox browser UI these elements are now visually separated.
Hence it makes sense, as far as I'm concerned, to try and follow this styling for the findbar used in the GENERIC viewer. One benefit of doing this is that we get more consistent styling, since the buttons now look/behave identically in both the main toolbar and the findbar. Additionally this also simplifies the CSS a bit, since a lot of the existing findbar-specific rules can be removed.
