Reduce the amount scaled when using mouse-wheel zooming in Chromium-browsers (issue 16325) #16596
Conversation
Abishek-Jayan
changed the title
There was a problem hiding this comment.
Sorry, but I still think this feels risky and it could easily break some other configuration (and thus require follow-up patches); hence I'm unfortunately not comfortable approving this.
The PR title mentions scrolling, which seems quite confusing since the issue is regarding mouse-wheel zooming. (Also, the title should use the words (issue 16325) and not ( bug #16325 ) since the original issue was filed in GitHub and not Bugzilla.)
The commit message should have the same title as the PR, and then also contain all of the contents from #16596 (comment).
Finally, please note https://github.com/mozilla/pdf.js/wiki/Squashing-Commits since you've got an unrelated commit included.
Abishek-Jayan
changed the title
Abishek-Jayan
changed the title
Snuffleupagus
changed the title
|
@Snuffleupagus hmm there is one thing i dont understand... in 2782 the variable PIXELS_PER_LINE_SCALE is set as a constant 30? why? If im correct in assuming that that variable represents the number of pixels in one line, couldn't it change based on the size of the device or something? |
Abishek-Jayan
changed the title
Abishek-Jayan
force-pushed
the
bugfix-zoom-chrome
branch
from
1de3fa4
to
afd41e9
Compare
|
@Snuffleupagus { Whereas in firefox, logging that property returns undefined. I had searched to see if there were ways that the window.chrome property could be faked (since the navigator.userAgent could be faked) but I couldnt find anything regarding that. Whats more, Edge, Opera, Brave and Chrome all support this property as per https://stackoverflow.com/a/13348618 So the current fix just adds the windows.chrome check to the if condition at line 2762. I had tested this functionality myself on all these browsers and they work as intended in both desktop and mobile versions. Hopefully I got the commit message and title right this time as well |
…rowsers (issue 16325)
So I have found that rather than messing with the delta values, the simplest way to solve the problem would be to find a clean way to
detect if the browser was Chromium or not. I found that chromium browsers have a property called windows.chrome which on console logging, looks something like this:
{
app: {
isInstalled: false,
InstallState: {
DISABLED: disabled,
INSTALLED: installed,
NOT_INSTALLED: not_installed
},
RunningState: {
CANNOT_RUN: cannot_run,
READY_TO_RUN: ready_to_run,
RUNNING: running
}
},
appPinningPrivate: {}
}
Whereas in firefox, logging that property returns undefined. I had searched to see if there were ways that the window.chrome property could be faked (since the navigator.userAgent could be faked) but I couldnt find anything regarding that. Whats more, Edge, Opera, Brave and Chrome all support this property as per https://stackoverflow.com/a/13348618
So the current fix just adds the windows.chrome check to the if condition at line 2762.
I had tested this functionality myself on all these browsers and they work as intended in both desktop and mobile versions.
Update: Added PDFJSDev checks to ensure no unnecessary code ends up in
the built-in Firefox PDF Viewer
Abishek-Jayan
force-pushed
the
bugfix-zoom-chrome
branch
from
afd41e9
to
89a1615
Compare
|
@Snuffleupagus now the current commit should have everything in order |
|
@Snuffleupagus hope everything is correct |
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 3 Live output at: http://54.241.84.105:8877/2390355f2565b5b/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/2390355f2565b5b/output.txt Total script time: 1.59 mins Published |
|
I'm still a little bit worried about this potentially causing issues in some edge-case, or for some users, given that this Chrome-only issue has (as far as I can tell) only been reported once. |
I don't have my windows machine for the moment, but iirc, I already met this issue with chrome on Windows. |
|
@Snuffleupagus @timvandermeij @calixteman hi, so...is this fix ok? |
|
Closing this for now, since as mentioned in #16596 (comment) we want to understand how/why Chrome behaves differently before making changes here. |
The deltaMode value apparently defaults to a different value depending on the browser OS combo (as per w3c/uievents#181 (comment) ) . In our case I've noticed that for Chromium browsers the delta value always comes out to a value greater than the PIXELS_PER_LINE_SCALE variable. This is what causes the error. So to fix, I just put a check there and set the correct tick value accordingly. Ive tested in both Edge and Brave browsers and it seems to be working fine both in desktop view and mobile view.
This is a fix for #16325