Prevent the Zoom dropdown from intermittently displaying an incorrect custom scale in Firefox (PR 8394 follow-up) #8510
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… 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.
…`, 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.
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 1 Live output at: http://54.67.70.0:8877/199e56d8d076178/output.txt |
Snuffleupagus
changed the title
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/199e56d8d076178/output.txt Total script time: 2.18 mins Published |
|
Thank you for the patch. |
|
@rvandermeulen Is there any chance that we could squeeze in one more PDF.js update on mozilla-central before the next branch date, and if so would you have time to help with this? I completely understand if you don't have time, or if we're simply too late here (in which case we'll just have to uplift this PR to beta after the merge). |
|
Sure thing!
…On Sun, Jun 11, 2017 at 7:27 AM, Jonas Jenwald ***@***.***> wrote:
@rvandermeulen <https://github.com/rvandermeulen> Is there any chance
that we could squeeze in *one* more PDF.js update on mozilla-central
before the next branch date, and if so would you have time to help with
this?
I completely understand if you don't have time, or if we're simply too
late here (in which case we'll just have to uplift this PR to beta after
the merge).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8510 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADfdv6HlcwWYEa1qjFinsFnwyGAmMM0Sks5sC88EgaJpZM4N2GH9>
.
|
|
@rvandermeulen Thanks a lot for your help here :-) |
movsb
pushed a commit
to movsb/pdf.js
that referenced
this pull request
Jul 14, 2018
…ches Prevent the Zoom dropdown from intermittently displaying an incorrect custom scale in Firefox (PR 8394 follow-up)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please note: The first commit fixes an unfortunate regression from PR #8394, and we may need to uplift at least that one (since we're right before the next Firefox branch date).
Edit: Slightly smaller diff with https://github.com/mozilla/pdf.js/pull/8510/files?w=1.
Prevent the Zoom dropdown from intermittently displaying an incorrect custom scale in Firefox (PR 8394 follow-up)
After PR Wraps mozL10n to async calls; splits firefox and generic l10n libs. #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:
PDFViewerApplication.pdfViewer.currentScaleValuein the console.Refactor the
selectScaleOptionfunction, inToolbar._updateUIState, to prevent any possible future display glitchesSince 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:
Only the
currentScalecall will currently trigger a l10n lookup inselectScaleOption. However, as far as I understand, there's no guarantee that the l10n string is resolved beforeselectScaleOptionis called again as a result of thecurrentScaleValuecall.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.