*Please note:* This patch contains a couple of micro-optimizations, hence I understand if it's deemed unnecessary.
Move the `AppOptions` initialization into the `Preferences` constructor, since that allows us to remove a couple of function calls, a bit of asynchronicity and one loop that's currently happening in the early stages of the default viewer initialization.
Finally, move the `Preferences` initialization to occur a *tiny* bit earlier since that cannot hurt given that the entire viewer initialization depends on it being available.
Note that CSS-features such as e.g. `flex` didn't exist, or had poor cross-browser support, back when the JavaScript-based solution was initially implemented.
- For the generic viewer we use @fluent/dom and @fluent/bundle
- For the builtin pdf viewer in Firefox, we set a localization url
and then we rely on document.l10n which is a DOMLocalization object.
Given that we only use standard `import`/`export` statements now, after recent PRs, the "exports" global is unused.
Instead we add "__non_webpack_import__" to the `globals` to avoid having to sprinkle disable statements throughout the code.
Finally, the way that `globals` are defined has changed in ESLint and we should thus explicitly specify them as "readonly"; please find additional details at https://eslint.org/docs/latest/use/configure/language-options#specifying-globals
When pdfBug is true, the substitution font is used in the text layer in order
to be able to know what is the font really used thanks to the devtools.
And to be sure that fonts are loaded, the font cache isn't cleaned up when
the debugger is active.
It's been loaded as a JavaScript module for a long time, and given that the file is bundled as-is (without building) it seems reasonable to just change the file extension now.
At this point in time all browsers, and also Node.js, support standard `import`/`export` statements and we can now finally consider outputting modern JavaScript modules in the builds.[1]
In order for this to work we can *only* use proper `import`/`export` statements throughout the main code-base, and (as expected) our Node.js support made this much more complicated since both the official builds and the GitHub Actions-based tests must keep working.[2]
One remaining issue is that the `pdf.scripting.js` file cannot be built as a JavaScript module, since doing so breaks PDF scripting.
Note that my initial goal was to try and split these changes into a couple of commits, however that unfortunately didn't really work since it turned out to be difficult for smaller patches to work correctly and pass (all) tests that way.[3]
This is a classic case of every change requiring a couple of other changes, with each of those changes requiring further changes in turn and the size/scope quickly increasing as a result.
One possible "issue" with these changes is that we'll now only output JavaScript modules in the builds, which could perhaps be a problem with older tools. However it unfortunately seems far too complicated/time-consuming for us to attempt to support both the old and modern module formats, hence the alternative would be to do "nothing" here and just keep our "old" builds.[4]
---
[1] The final blocker was module support in workers in Firefox, which was implemented in Firefox 114; please see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import#browser_compatibility
[2] It's probably possible to further improve/simplify especially the Node.js-specific code, but it does appear to work as-is.
[3] Having partially "broken" patches, that fail tests, as part of the commit history is *really not* a good idea in general.
[4] Outputting JavaScript modules was first requested almost five years ago, see issue 10317, and nowadays there *should* be much better support for JavaScript modules in various tools.
The minified default viewer has never been distributed in either official releases or through pdfjs-dist, which means that it's most likely unused, and it's has never been tested nor actively maintained.
Setting the alpha-value explicitly to `1` in `rgb` colors is unnecessary, since that's the default value, and this way we ever so slightly reduce the size of our CSS files.
Unfortunately I've not found a Stylelint rule to enforce this automatically, and the patch was generated using search and replace.
When an editing button is disabled, focused and the user press Enter (or space), an
editor is automatically added at the center of the current page.
Next creations can be done in using the same keys within the focused page.
This has been deprecated since version `2.15.349`, which is a year ago.
Removing this will also simplify some upcoming changes, specifically outputting of JavaScript modules in the builds.
This removes the only remaining old and non-standard handling of exports in the `web/`-folder, since some initial attempts at outputting JavaScript modules in the builds have identified this file as a potential problem.
While this uses a hard-coded list, for overall simplicity, I don't believe that that's a big problem since:
- Generating this file automatically would require a bunch more parsing *every single time* that the library is built.
- The official API-surface doesn't change often enough for this to really impede development in any significant way.
- The added unit-test helps ensure that this list cannot accidentally become outdated.
Given that this is accessed multiple times per page in the viewer, that leads to a number of (strictly speaking unneeded) function calls and allocated Objects for each invocation. By converting `layerProperties` to a, lazily initialized, Object we can avoid this.
When PR 17015 removed the `disabled` handling for the "Save"-button it left a bunch of now unused CSS rules behind, which seems like a simply oversight.
Rather than shipping "dead" CSS rules, let's remove those until such a time that they're actually needed.
*For many non-English locales the translated strings will be longer, which is easy to forget about during development/review.*
Note how for some locales (e.g. Swedish) the altText-button end up looking horizontally "cramped", hence it seems reasonable to add a bit of inline padding to improve this.
but keep it for the text area.
Disable pointerdown on the alt-text button to disable dragging the editor
when the button is clicked (especially when slightly moving the mouse
between the down and the up).
The dialog element handles closing with <kbd>Esc</kbd> automatically, however we're not reporting telemetry in that case.
In order to fix that the easiest solution, as far as I'm concerned, seem to be moving the telemetry reporting into the dialog-close handler since it's always invoked.
This patch addresses an edge-case that'll probably never happen, but it nonetheless seems like something that we want to fix.
Note how we're using the `#currentEditor`-field to prevent opening the dialog when it's already active, and it being reset once the dialog has been closed.
By also resetting the `#currentEditor`-field during destruction, instead of waiting until the dialog has actually closed (assuming it's currently open), there's a *tiny* window of time[1] during which we could theoretically allow to (incorrectly) re-open the dialog and thus getting out-of-sync state in the viewer-component.
---
[1] Since the "close" event, on a dialog-element, is dispatched asynchronously by the browser.
When the user edit an existing alt-text and remove it, we want to be able
to save this state and consequently remove the done state from the
alt-text button.
Remove the button from its parent when the editor is removed: it should
help to save few Kb of memory.
Radio-buttons can also be toggled by clicking on their associated `label`-elements, and not only the `input`-elements itself, however it seems that "pointerdown" event listeners don't cover that case.
Hence it's possible that telemetry could miss certain cases of a mouse being used, and the easiest solution seem to be to instead use "click" event listeners and just ignore keyboard-based events.
Given the limitations of the old pre-processor that's used for CSS/HTML files, this unfortunately isn't as "easy" to implement as it is for JavaScript code.
Since this is the first case where we've wanted to do conditional CSS imports, rather than trying to completely re-write the pre-processor, this patch settles for handling it explicitly in the `expandCssImports` function.
Looking at the save-telemetry values they're all boolean *except* for "alt_text_edit" in one instance, since `this.#previousAltText` may be an empty string (looking at the `editAltText` method) and this value may thus become an empty string as well.
When closing a document in the viewer, e.g. by running `PDFViewerApplication.close()` in the console, the `AltTextManager.#finish` method currently throws *unless* the `altText` dialog is actually open.
Similar to e.g. the PasswordPrompt, we should thus only attempt to close the `altText` dialog when it's open.
It's a part of the UX specifications. There's a drawing issue in Firefox
(see bug https://bugzilla.mozilla.org/1853288) but setting the
background-clip property to content-box seems to be a good workaround.
The goal is to always have something which is focusable to let the user select
it with the keyboard.
It fixes the mentioned bug because, the annotation layer will now have a container
to attach the canvas for annotations having their own canvas.
`.grab-to-pan-grab:active` is `#viewerContainer` when the mouse is
pressed down. It is supposed to have a `cursor: grabbing` appearance
immediately on mousedown,
`.grab-to-pan-grabbing` is the overlay that is supposed to cover
everything, and also has the `cursor: grabbing` appearance. The "cover
everything" result is achieved through `position:fixed`, `inset:0`, etc.
The block with these CSS properties for "cover everything" is currently
shared by `.grab-to-pan-grab:active` and `.grab-to-pan-grabbing`, but
only "cursor" need to be shared. The original JS and CSS code at
https://github.com/Rob--W/grab-to-pan.js shows that these were supposed
to be associated with the overlay only.
The PR that added this to PDF.js also shows that the "cover everything"
CSS properties were supposed to be limited to the overlay only:
https://github.com/mozilla/pdf.js/pull/4209#discussion-diff-9285917
But the final version of the PR mistakenly merged them together.
This patch rectifies that mistake.
Using `removeNullCharacters` on the URL should be completely redundant, given the kind of data that we're passing to the `addLinkAttributes` helper function. Note that whenever we're handling a URL, originating in the worker-thread, in the viewer that helper function is always being used.
Furthermore, on the worker-thread all URLs are parsed with the `createValidAbsoluteUrl` helper function, which uses `new URL()` to ensure that a valid URL is obtained. Note that the `URL` constructor will either throw, or in some cases just ignore them, when encountering `\u0000`-characters during parsing.
Hence it should be *impossible* for a valid URL to contain `\u0000`-characters and we can thus simplify the viewer-code a tiny bit. The use of `removeNullCharacters` is most likely a left-over from back when `new URL()` wasn't generally available in browsers.
Testing the `tagged_stamp.pdf` document locally in the viewer, I noticed that e.g. the /Alt entry for the StampAnnotation contains "Secondary text for stamp\u0000".
Elsewhere in the viewer we're skipping null-chars and it's easy enough to do that in the `StructTreeLayerBuilder` class as well. (Note that we generally let the API itself return the data as-is.)
This fixes invalid type references (either due to invalid paths for the
import or missing imports) in the JS doc, as well as some missing or
invalid parameter names for @param annotations.
The main stamp button will be used to just enter in a add/edit image mode:
- the user can add a new image in using the new button.
- the user can edit an image in resizing, moving it.
In image mode, when the user clicks outside on the page but not on an editor,
then all the selected editors will be unselected.
After the `src/core/`-changes in PR 16779 the `PDFDocumentProxy.getJSActions` method should no longer be able to return *empty* entries, which means that we can simplify the "JavaScript support is not enabled"-warning in the viewer.
Furthermore, improve the auto-printing hack used when scripting is disabled.
There are 2 rotation we've to deal with: the viewer one and the editor one.
The previous implementation was a bit complex and having to deal with these
rotation would have potentially increase it.
So this patch aims to simplify the implementation and deal with all the possible
cases.
The main idea is to transform the mouse deltas according to the rotations and then
apply the resizing in the page coordinates system.
When resizing an editor we're currently using unidirectional cursors, please refer to https://developer.mozilla.org/en-US/docs/Web/CSS/cursor
Given that editors can (generally) be resized to become either smaller or larger, it seems overall more appropriate to use bidirectional cursors to make this clearer to the user.
Note that as mentioned in the MDN article some environments, which seems to apply to e.g. Windows 11, doesn't differentiate between the two cursor formats and simply use bidirectional ones unconditionally.
One additional benefit of these changes is that the relevant CSS rules become slightly more compact.
This method is very old, however with the exception of the auto-print hack (when scripting is disabled) in the viewer it's never actually been used.
Most likely the idea with `PDFDocumentProxy.getJavaScript` was that it'd be useful if scripting support was added, however it turned out that it was a bit too simplistic and instead a number of new methods were added for the scripting use-cases.
Without this patch the password dialog is pretty difficult to use in the GeckoView-viewer, because of a number of missing CSS variables.
*Please note:* This patch makes no effort at actually styling the dialog to better suite the overall look of the GeckoView-viewer, but focuses solely on making it actually usable (since password protected PDF documents are somewhat rare).
If the current PDF document is closed while the password dialog is open, e.g. manually by calling `PDFViewerApplication.close()` from the console, the password dialog wouldn't be closed as intended.
*Please note:* This could only affect the GENERIC viewer, although it's very unlikely to ever happen, since that's the only one that supports opening more than one PDF document.
*Please note:* This situation should never happen in practice, but it nonetheless cannot hurt to fix this.
If the `PasswordPrompt.open` method would ever be called synchronously back-to-back *and* if opening of the dialog fails the first time, then the second invocation would remain pending indefinitely since we just clear out the capability.
Given that the `useOnlyCssZoom` option is essentially just a special-case of the `maxCanvasPixels` functionality, we can combine the two options in order to simplify the overall implementation.
Note that the `useOnlyCssZoom` functionality was only ever used, by default, in the PDF Viewer for the B2G/FirefoxOS project (which was abandoned years ago).
This is quite old code, however the error-handling no longer seems necessary for a couple of reasons:
- The `PDFViewerApplication.open` method is asynchronous, which means that it cannot throw a "raw" `Error` and the try-catch is not needed in that case.
- None of the other affected methods should throw, and if they do that'd rather indicate an *implementation* error in the code.
- Finally, and most importantly, with the `PDFViewerApplication.run` method now being asynchronous an (unlikely) `Error` thrown within it will lead to a rejected `Promise` and not affect execution of other code.
We can use modern JavaScript features, in this case optional chaining, to (ever so slightly) simplify how `ViewHistory` errors are handled.
Also, use arrow functions when handling a few other (very rare) errors during loading since that's a tiny bit shorter.
Given that the `debugger` is loaded as a module we can use "top level await" in development mode to access the necessary API-functionality, which removes the need to manually pass in the required properties.
- it'll improve the way to resize images: diagonally (in keeping ratio between dimensions)
or horizontally/vertically.
- the resizer was almost invisible in HCM.
- make a resize undoable/redoable.
In order to reproduce the original issue:
- switch to freetext mode
- add a text somewhere
- double click outside and add some text
- repeat the previous step several times
no text is selected during the edition.
*Please note:* This only removes the preference itself, however both the viewer-option and the actual implementation is still available.
The `useOnlyCssZoom` functionality was only ever used, by default, in the PDF Viewer for the B2G/FirefoxOS project (which was abandoned years ago). Given that CSS-only zooming can easily make the document look blurry even at low zoom levels, this functionality was only intended for low-powered mobile devices.
Hence it seems reasonable to remove the `useOnlyCssZoom` preference now, since neither the default viewer nor the GeckoView-specific viewer uses this functionality.
By leveraging import maps we can get rid of *most* of the remaining `require`-calls in the `src/display/`-folder, since we should strive to use modern `import`-statements wherever possible.
The only remaining cases are Node.js-specific dependencies, since those seem very difficult to convert unless we start producing a bundle *specifically* for Node.js environments.
Localization of this button broke in PR 16340, which I assume was completely accidental, since the download-button now tries to access a l10n-id that was removed some time ago (see PR 15617).
Note how loading even the development viewer, i.e. http://localhost:8888/web/viewer-geckoview.html#locale=en-US, currently logs l10n-warnings on the `master` branch.
Having a `require` in this file has never made sense in e.g. the Firefox PDF Viewer and shouldn't really be necessary.
Possibly the idea was to facilitate some kind of third-party bundling, however the *built* `pdf.js` file has always exposed the API-contents globally.
Currently this class contains a few "special" code-paths for the COMPONENTS build-target, which normally wouldn't be a problem. However, in this particular case that means accessing code that we don't want to include unconditionally in all builds.
This is currently implemented using build-time `require`-calls which we nowadays want to avoid, and we should strive to remove all such cases from the code-base. (Generally speaking `import` is the future, and build-tools may not always play well with a mix of both formats.)
We can easily improve things here by using sub-classing for the COMPONENTS build-target, and then use the ability to re-name when exporting (to avoid breaking existing code).
There's no good reason for getting this option multiple times in the same method. Also, we can slightly re-factor how the `editorStampButton` is made visible.
This regressed in PR 16659, when the signature of the `PDFViewer.annotationEditorMode`-setter was changed, and it currently leads to an Error being thrown when exiting PresentationMode.
`PDFViewerApplication` reads from `location.hash` to initialize
`initialBookmark`. But when extensions/chromium/pdfHandler.js prepares
the redirect URL, the reference fragment is encoded instead of bare.
`rewriteUrlClosure` in `chromecom.js` is responsible for decoding the
URL, but that currently runs too late.
To fix this, update `initialBookmark` after rewriting the URL.
This was not a problem in the past because `rewriteUrlClosure` in
`chromecom.js` executed before the initialization of `initialBookmark`.
These options are completely unused in the PDF.js viewer, and given that the last update of the `GrabToPan`-code from upstream was in 2016 it shouldn't hurt to remove them.
This is something that I completely overlooked during review of PR 16593, since the idea is (obviously) that the viewer-components should be usable as-is without the user needing to manually pass in any *additional* parameters.
To support this we can very easily expose the current `FilterFactory`-instance on the `PDFPageProxy`-class[1], and if needed initialize the highlight-filters when initializing the page (again limited to the viewer-components).
- Modify the text and background colors in popup to fit a11y requirements
- Add a backdrop filter on clickable areas in using a svg filter mapping
canvas colors to Highlight and HighlightText ones.
It occurred to me that we can actually run this unit-test in Node.js environments by making use of the preprocessor to stub out the browser globals there.
Until now we've not actually had *any* tests that ensure that the *official* PDF.js-viewer API exposes the intended functionality, which means that things can easily break accidentally.
*Please note:* This unit-test cannot (easily) be run in Node.js-environments, since the `external/webL10n/l10n.js` file contains various browser-specific functionality.
- Change (most) fields/methods into private ones, since that's now supported.
- Tweak the constructor-parameters, and simplify the sandbox initialization w.r.t. the viewer components.
- Remove some unused function/method parameters.
- Slightly simplify the "updatefromsandbox"-handler by using local variables and inverting some conditions.
Rather than sprinkling pre-processor statements throughout the viewer-code, simply "disable" the relevant `PDFViewer` setters instead.
Also, given that the GeckoView-specific viewer doesn't have a sidebar we don't actually need to explicitly ignore a `pageMode` during loading.
This helper function was added almost two years ago, in PR 13696, and it still has only a single call-site. Furthermore, with the changes made in PR 16572 it also cannot hurt to reduce the size of the `web/l10n_utils.js` file slightly.
Note how the [`ChromeActions.getPreferences` method](https://searchfox.org/mozilla-central/rev/4e8f62a231e71dc53eb50b6d74afca21d6b254e9/toolkit/components/pdfjs/content/PdfStreamConverter.sys.mjs#497-530) returns the preferences as a string, which we then have to convert back into an Object in the viewer.
Back when that code was originally written it wasn't possible to send Objects from the platform-code, however that's no longer the case and we should be able to (eventually) remove this unnecessary string-parsing now.
*Please note that in order to prevent breakage we'll need to land these changes in stages:*
- Land this patch in mozilla-central, as part of regular the PDF.js updates.
- Change the return type in the `ChromeActions.getPreferences` method, in a mozilla-central patch.
- Remove the string-handling from the `FirefoxPreferences._readFromStorage` method.
Please note that we've never had any functionality in the viewer itself that *set* preferences, and we've thus only ever read them.
For the GENERIC viewer it obviously makes sense for the user to be able to modify preferences, e.g. via the console, but that doesn't really apply to the *built-in* Firefox PDF Viewer since preferences are already accessible via `about:config` there. Hence it does seems somewhat strange to expose, a limited part of, the Firefox preference system in this way when we're not even using it.
Note that the unused preference setting-code also include a fair amount of *additional* validation on the platform-side, such as limiting any possible preference changes to the `pdfjs.`-branch and also an explicit white-list of preference names[1], to make sure that this is safe; please see:
- https://searchfox.org/mozilla-central/rev/4e8f62a231e71dc53eb50b6d74afca21d6b254e9/toolkit/components/pdfjs/content/PdfStreamConverter.sys.mjs#458-495
- https://searchfox.org/mozilla-central/rev/4e8f62a231e71dc53eb50b6d74afca21d6b254e9/toolkit/modules/AsyncPrefs.sys.mjs#21-48
Assuming that this patch lands, I'll follow-up with a mozilla-central patch to remove the code mentioned above.
---
[1] This hard-coded list contains preferences that no longer exist, and also at least one (fairly obvious) typo.
This method was added only for consistency with the `register`-method, however it's never actually been used. To avoid including dead code in the builds, let's just remove the `unregister`-method for now.
*Please note:* If this method ever becomes useful, it'll be trivial to revert this commit.
With the changes in PR 16552 we can now move general translation into the `AnnotationLayer` itself, which should improve things ever so slightly in third-party implementations where the default viewer isn't used.
*This is something that I completely overlooked during review of PR 16552, despite leaving a l10n-related comment.*
The new l10n-handling of PopupAnnotations assume that the `AnnotationLayer` is always initialized with a l10n-instance, which might not actually be the case in third-party implementations where the default viewer isn't used.
To work-around that we'll now bundle, and fallback on, the existing `NullL10n`-implementation in GENERIC builds of the PDF.js library. This will only result in a slight file-size increase for the *built* `pdf.js` file, again limited to GENERIC builds, since the `web/l10n_utils.js` file has no dependencies.
Also, tweaks a couple of TESTING pre-processor checks to *only* include that code when running the reference tests.
- it'll help to be able to move popups on screen to let the user read the text
- popups won't inherit some properties from their parent:
- the popup can be misrendered if for example the parent has a clip-path property.
- add an outline to the popup when the parent is focused.
- hide a popup when it's clicked.
While it's slightly difficult to trigger in practice, unless the `defaultZoomDelay`-value is increased, it's currently possible to generate thumbnails from *partially* rendered pages when doing *temporary* CSS-only zooming.
We shouldn't dispatch a "pagerendered"-event when doing *temporary* CSS-only zooming, but simply wait until the actual rendering is done.
While I don't believe that this regression has caused any actual bugs, dispatching *duplicate* events is nonetheless inconsistent and should be fixed.
Given that this functionality is only relevant in third-party use-cases, for example the viewer-components, we can avoid needlessly including it in e.g. the MOZCENTRAL build.
This patch does two things:
- Moves the updating of thumbnails into `web/app.js`, via a new `PDFSidebar` callback-function, to avoid having to include otherwise unnecessary parameters when initializing a `PDFSidebar`-instance.
- Only attempt to generate thumbnail-images from pages that are *cached* in the viewer. Note that only pages that exist in the `PDFPageViewBuffer`-instance can be rendered, hence it's not actually meaningful to check every single page when updating the thumbnails.
For large documents, with thousands of pages, this should be a tiny bit more efficient when e.g. opening the sidebar since we no longer need to check pages that we know have not been rendered.
The way that the cleanup was implemented in PR 12613 has always bothered me slightly, since the `isPageCached`-method that I introduced there always felt quite out-of-place in the `IPDFLinkService`-implementations.
By introducing a new "thumbnailrendered" event, similar to the existing "pagerendered" one, we're able to move the cleanup handling into the `PDFViewer`-class instead.
The way that this was implemented in PR 10217 has always bothered me slightly, since the `isPageVisible`-method that I introduced there always felt quite out-of-place in the `IPDFLinkService`-implementations.
Hence this is instead replaced by a callback-function in `PDFFindController`, to handle the page-visibility checks. Note that since the `PDFViewer`-constructor always sets this callback-function, e.g. the viewer-component examples still work as-is.
- Remove the dependency on fit-curve;
- Improve the way to draw the current line in using a Path2D and
in clearing only the last part of the curve instead of clearing
all the canvas;
- Smooth the curve when drawing to avoid to have some changes after
the drawing ends;
- Make the smoothing a bit less agressive.
Looking at the behaviour in Adobe Reader it doesn't appear that attachments are sorted alphabetically, hence it doesn't seem necessary for us to do so either in the viewer.
An additional benefit of *not* sorting the attachments is that any "actual" attachments are now always placed at the top of the list in the sidebar, and if any `FileAttachment`-annotations exist in the document they will now be appended at the end.
