From 2971f522d43ee7173c4615bafa19f9b680736267 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 10 Jun 2017 13:10:23 +0200 Subject: [PATCH 1/2] Prevent the Zoom dropdown from intermittently displaying an incorrect custom scale in Firefox (PR 8394 follow-up) 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. --- web/toolbar.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/web/toolbar.js b/web/toolbar.js index e44ecc7b0..3e486edcf 100644 --- a/web/toolbar.js +++ b/web/toolbar.js @@ -194,11 +194,10 @@ var Toolbar = (function ToolbarClosure() { if (!predefinedValueFound) { var customScale = Math.round(scale * 10000) / 100; this.l10n.get('page_scale_percent', { scale: customScale, }, - '{{scale}}%'). - then((msg) => { + '{{scale}}%').then((msg) => { items.customScaleOption.textContent = msg; + items.customScaleOption.selected = true; }); - items.customScaleOption.selected = true; } }; From 75edb859ce4e281a21422d55798eee8397438dc8 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 10 Jun 2017 13:37:54 +0200 Subject: [PATCH 2/2] Refactor the `selectScaleOption` function, in `Toolbar._updateUIState`, to prevent any possible future display glitches 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. --- web/toolbar.js | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/web/toolbar.js b/web/toolbar.js index 3e486edcf..d30f168c8 100644 --- a/web/toolbar.js +++ b/web/toolbar.js @@ -180,25 +180,25 @@ var Toolbar = (function ToolbarClosure() { } let selectScaleOption = (value, scale) => { - var options = items.scaleSelect.options; - var predefinedValueFound = false; - for (var i = 0, ii = options.length; i < ii; i++) { - var option = options[i]; - if (option.value !== value) { - option.selected = false; - continue; + let customScale = Math.round(scale * 10000) / 100; + this.l10n.get('page_scale_percent', { scale: customScale, }, + '{{scale}}%').then((msg) => { + let options = items.scaleSelect.options; + let predefinedValueFound = false; + for (let i = 0, ii = options.length; i < ii; i++) { + let option = options[i]; + if (option.value !== value) { + option.selected = false; + continue; + } + option.selected = true; + predefinedValueFound = true; } - option.selected = true; - predefinedValueFound = true; - } - if (!predefinedValueFound) { - var customScale = Math.round(scale * 10000) / 100; - this.l10n.get('page_scale_percent', { scale: customScale, }, - '{{scale}}%').then((msg) => { + if (!predefinedValueFound) { items.customScaleOption.textContent = msg; items.customScaleOption.selected = true; - }); - } + } + }); }; var pageNumber = this.pageNumber;