By using CSS variables to set the width of the zoom dropdown, we can simplify both the relevant CSS and JS code. This will not only improve overall maintainability of this code, but should also make it (slightly) easier for third-party users that want to customize the width.
Note in particular that by having the code in `Toolbar._adjustScaleWidth` lookup the values of the CSS variables, we no longer need to worry about keeping hard-coded values up-to-date with the CSS rules.
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.)
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.
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.
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.
Please note that these changes were done automatically, using `gulp lint --fix`.
Given that the major version number was increased, there's a fair number of (primarily whitespace) changes; please see https://prettier.io/blog/2020/03/21/2.0.0.html
In order to reduce the size of these changes somewhat, this patch maintains the old "arrowParens" style for now (once mozilla-central updates Prettier we can simply choose the same formatting, assuming it will differ here).
Since the goal has always been, essentially since the `EventBus` abstraction was added, to remove all dispatching of DOM events[1] from the viewer components this patch tries to address one thing that came up when updating the examples:
The DOM events are always dispatched last, and it's thus guaranteed that all internal event listeners have been invoked first.
However, there's no such guarantees with the general `EventBus` functionality and the order in which event listeners are invoked is *not* specified. With the promotion of the `EventBus` in the examples, over DOM events, it seems like a good idea to at least *try* to keep this ordering invariant[2] intact.
Obviously this won't prevent anyone from manually calling the new *internal* viewer component methods on the `EventBus`, but hopefully that won't be too common since any existing third-party code would obviously use the `on`/`off` methods and that all of the examples shows the *correct* usage (which should be similarily documented on the "Third party viewer usage" Wiki-page).
---
[1] Looking at the various Firefox-tests, I'm not sure that it'll be possible to (easily) re-write all of them to not rely on DOM events (since getting access to `PDFViewerApplication` might be generally difficult/messy depending on scopes).
In any case, even if technically feasible, it would most likely add *a lot* of complication that may not be desireable in the various Firefox-tests. All-in-all, I'd be fine with keeping the DOM events only for the `MOZCENTRAL` target and gated on `Cu.isInAutomation` (or similar) rather than a preference.
[2] I wouldn't expect any *real* bugs in a custom implementation, simply based on event ordering, but it nonetheless seem like a good idea if any "external" events are still handled last.
This patch contains some *much* needed clean-up of, and improvements to, this old code thus addressing a number of issues:
- Set more reasonable *default* values for the widths, in `web/viewer.css`, since the current ones are actually too small even for the (default) `en-US` locale.
This obviously result in a slightly larger zoom dropdown width for many locales, but the more consistent UI does look good to me.
- Stop setting the `min-width`/`max-width` and just use `width` instead.
- Set an explicit `height` of the zoom dropdown, in an attempt to get Google Chrome to display it with the same height as the toolbar buttons.
- Remove additional `Element.setAttribute("style", ...)` usage.
- Actually check *all* of the predefined l10n strings, since the old implementation (implicitly) assumed that the currently selected one was the longest (note e.g. the `ja-JP` locale where one string is considerably longer than the rest).
- Stop invalidating the DOM multiple times when doing the measurements. This was achieved by using a temporary in-memory `canvas`, and we now only need to query the DOM once in order to get the current font properties of the zoom dropdown.
In order to eventually get rid of SystemJS and start using native `import`s instead, we'll need to provide "complete" file identifiers since otherwise there'll be MIME type errors when attempting to use `import`.
Please find additional details about the ESLint rule at https://eslint.org/docs/rules/prefer-const
Note that this patch is generated automatically, by using the ESLint `--fix` argument, and will thus require some additional clean-up (which is done separately).
This patch makes the follow changes:
- Remove no longer necessary inline `// eslint-disable-...` comments.
- Fix `// eslint-disable-...` comments that Prettier moved down, thus causing new linting errors.
- Concatenate strings which now fit on just one line.
- Fix comments that are now too long.
- Finally, and most importantly, adjust comments that Prettier moved down, since the new positions often is confusing or outright wrong.
Note that Prettier, purposely, has only limited [configuration options](https://prettier.io/docs/en/options.html). The configuration file is based on [the one in `mozilla central`](https://searchfox.org/mozilla-central/source/.prettierrc) with just a few additions (to avoid future breakage if the defaults ever changes).
Prettier is being used for a couple of reasons:
- To be consistent with `mozilla-central`, where Prettier is already in use across the tree.
- To ensure a *consistent* coding style everywhere, which is automatically enforced during linting (since Prettier is used as an ESLint plugin). This thus ends "all" formatting disussions once and for all, removing the need for review comments on most stylistic matters.
Many ESLint options are now redundant, and I've tried my best to remove all the now unnecessary options (but I may have missed some).
Note also that since Prettier considers the `printWidth` option as a guide, rather than a hard rule, this patch resorts to a small hack in the ESLint config to ensure that *comments* won't become too long.
*Please note:* This patch is generated automatically, by appending the `--fix` argument to the ESLint call used in the `gulp lint` task. It will thus require some additional clean-up, which will be done in a *separate* commit.
(On a more personal note, I'll readily admit that some of the changes Prettier makes are *extremely* ugly. However, in the name of consistency we'll probably have to live with that.)
Considering just how small/simple this code is, it doesn't seem necessary to have a separate method for it (even more so when there's only one call-site).
Currently the indicator may remain visible even after the document has been closed, which seems weird given that no page is either visible nor rendering :-)
Rather than having every invocation of `Toolbar._updateUIState` compute a valid `pageScaleValue`, it seems easier to simply ensure that it happens when the value is actually updated.
Looking at the history of this code, this parameter has never been used.
I'm guessing that most likely the code in `web/toolbar.js` began life as a copy of `web/secondary_toolbar.js`, which would probably explain why that parameter exists.
After PR 8510, we now always lookup the localized `page_scale_percent` string to prevent any possible ordering issues. Since the scaleSelect dropdown is updated asynchronous, there's really no point in having a helper function any more, hence this code can rather be placed inline in `Toolbar._updateUIState`.
Since the localization service is now asynchronous, depending on the load the browser is under, there's a small risk that the lookup of the 'page_scale_percent' string could be delayed slightly.
If the scale would change a couple of times in *very* quick succession, there's perhaps a *theoretical* possibility that the Zoom dropdown would display an incorrect value.
Consider the following, somewhat contrived, theoretical example of two zoom commands being executed *right* after one another:
```javascript
PDFViewerApplication.pdfViewer.currentScale = 1.23;
PDFViewerApplication.pdfViewer.currentScaleValue = 'page-width';
```
Only the `currentScale` call will currently trigger a l10n lookup in `selectScaleOption`. However, as far as I understand, there's no *guarantee* that the l10n string is resolved *before* `selectScaleOption` is called again as a result of the `currentScaleValue` call.
This thus has the possibility of putting the Zoom dropdown into an inconsistent state, since it's currently updated synchronously for one code-path and asynchronously for another.
To avoid these issues, I'm proposing that we *always* update the Zoom dropdown asynchronously, such that we can guarantee that the ordering is correct.
After PR 8394, at least in Firefox, the Zoom dropdown now frequently displays an old custom scale instead of the correct one. To see this behaviour, the following STR works for me:
1. Open https://mozilla.github.io/pdf.js/web/viewer.html.
2. Zoom in, by clicking on the corresponding button in the toolbar.
3. Run `PDFViewerApplication.pdfViewer.currentScaleValue` in the console.
4. Compare what's displayed in the Zoom dropdown with what's printed in the console. (If no difference can be observed, try repeating steps 2 through 4 a couple of times.)
I really don't understand why this happens, but it seems that waiting until a custom scale has been set *before* selecting it fixes things in Firefox (and works fine in e.g. Chrome as well).
Note that this patch thus makes this particular piece of the code consistent with the state prior to PR 8394.
Please see http://eslint.org/docs/rules/object-shorthand.
For the most part, these changes are of the search-and-replace kind, and the previously enabled `no-undef` rule should complement the tests in helping ensure that no stupid errors crept into to the patch.
Note that as discussed on IRC, this makes the viewer slightly slower to load *only* in `gulp server` mode, however the difference seem slight enough that I think it will be fine.
