Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid to have to wait to zoom after scrolling #17724

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

calixteman
Copy link
Contributor

@calixteman calixteman commented Feb 24, 2024

Allow to zoom with the wheel once the scrolling is finished.
It's now possible to know that thanks to the scrollend event.

Fixes #17707.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Feb 25, 2024

Keep the previous way to work for browsers not supporting the scrollend event.

Do we actually have to care about such browsers, or could we perhaps take the opportunity to simplify the affected code?

Looking at the MDN compatibility data it seems that almost all modern browsers already support this. The only exception is unsurprisingly Safari, which has always been the "worst" browser to attempt to support. (Historically it's often been quite slow at implementing new web-platform features, and over time we've seen a large number issues that only affect Safari.)

@calixteman
Copy link
Contributor Author

Keep the previous way to work for browsers not supporting the scrollend event.

Do we actually have to care about such browsers, or could we perhaps take the opportunity to simplify the affected code?

Looking at the MDN compatibility data it seems that almost all modern browsers already support this. The only exception is unsurprisingly Safari, which has always been the "worst" browser to attempt to support. (Historically it's often been quite slow at implementing new web-platform features, and over time we've seen a large number issues that only affect Safari.)

I mostly added this extra code because of:
https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions#legacy-build
and Chrome 92+, when scrollend is available in Chrome 114+.
That said if you agree, I'm fully ok to remove this extra stuff.

@Snuffleupagus
Copy link
Collaborator

That said if you agree, I'm fully ok to remove this extra stuff.

My reasoning for removing the old code is:

  • We've never guaranteed that every feature will be available in older browsers, since some things cannot be polyfilled.
  • These changes don't apply to the PDF.js library, but only to the default viewer (which is developed for Firefox).
  • This was intended as nothing more than a work-around for an old Mac-only issue, where the user is pressing the zoom-modifier key during scrolling.

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we also need to ensure that the new event listeners are removed on "blur" for the mainContainer, and also reset this._isScrolling = false; then?

web/app.js Show resolved Hide resolved
@calixteman
Copy link
Contributor Author

Don't we also need to ensure that the new event listeners are removed on "blur" for the mainContainer, and also reset this._isScrolling = false; then?

Yes it's possible, but you've to alt+tab during scrolling: it's very unlikely to happen... That said I'll fix that.

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/5d021967e95770e/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/5d021967e95770e/output.txt

Total script time: 1.20 mins

Published

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Feb 25, 2024

Having tested the preview there's unfortunately cases where the "scrollend" event doesn't seem to fire as expected, or possibly there's an extra "scroll"-event at the end of scrolling, which means that mouse-wheel zooming breaks.

  • Case 1:

    1. Click on the scroll-wheel and move the mouse up or down, to trigger scrolling.
    2. Click again on the scroll-wheel, to stop scrolling.
    3. Hold Ctrl and scroll the mouse-wheel to zoom.
  • Case 2:

    1. Hold the scroll-wheel and move the mouse up or down, to trigger scrolling.
    2. Release the scroll-wheel, to stop scrolling.
    3. Hold Ctrl and scroll the mouse-wheel to zoom.

@calixteman
Copy link
Contributor Author

It's a Firefox bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1881974
That said I found a workaround.

@Snuffleupagus
Copy link
Collaborator

It's a Firefox bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1881974

Nice find, thanks for filing that!

That said I found a workaround.

Should we perhaps wait a couple of days, to see if there's a quick fix for the upstream bug, before landing a work-around?

web/app.js Outdated Show resolved Hide resolved
Allow to zoom with the wheel once the scrolling is finished.
It's now possible to know that thanks to the scrollend event.

Fixes mozilla#17707.
@calixteman
Copy link
Contributor Author

Should we perhaps wait a couple of days, to see if there's a quick fix for the upstream bug, before landing a work-around?

This bug is very likely there for a long time (I tested with a 2020 version) so I'm pretty sure that we won't have a fix soon (except maybe if it's easy enough to fix). So I think we should merge this PR (if everything is ok of course) and we can remove the workaround once it's fixed.

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/b6d7c722fb8f775/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/b6d7c722fb8f775/output.txt

Total script time: 1.33 mins

Published

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me, now it seems to work in all cases I've tested; thank you!

@calixteman calixteman merged commit e42b114 into mozilla:master Feb 26, 2024
7 checks passed
@calixteman calixteman deleted the issue17707 branch February 26, 2024 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

High value of WHEEL_ZOOM_DISABLED_TIMEOUT causes delay between scrolling and zooming
3 participants