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

GUAC-1967: Remove duplicate wheel handler registrations. #996

Closed
wants to merge 1 commit into from

Conversation

neandrake
Copy link
Contributor

Modern-day browsers abide the standardized wheel event (typed WheelEvent), however to support older browsers the legacy DOMMouseScroll (FireFox) and mousewheel (all others) events are also being registered.

However modern browsers support both the wheel event as well as the legacy, which results in the wheel handler being registered multiple times. This results in the wheel handler firing twice instead of once, causing excessive scrolling.

This change separates the event registration such that either the modern or the legacy events are registered, and not both.

@neandrake
Copy link
Contributor Author

Oops my editor removed trailing spaces automatically. Let me know if that's a problem and I'll see to get that reverted.

Copy link
Contributor

@necouchman necouchman left a comment

Choose a reason for hiding this comment

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

Just a couple of style changes, and I think it's good.

guacamole-common-js/src/main/webapp/modules/Mouse.js Outdated Show resolved Hide resolved
guacamole-common-js/src/main/webapp/modules/Mouse.js Outdated Show resolved Hide resolved
Modern-day browsers abide the standardized wheel event (typed WheelEvent),
however to support older browsers the legacy DOMMouseScroll (FireFox) and
mousewheel (all others) events are also being registered.

However modern browsers support both the wheel event as well as the legacy,
which results in the wheel handler being registered multiple times. This
results in the wheel handler firing twice instead of once, causing excessive
scrolling.

This change separates the event registration such that either the modern or the
legacy events are registered, and not both.
Copy link
Contributor

@necouchman necouchman left a comment

Choose a reason for hiding this comment

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

@jmuehlner Any further concerns?

@jmuehlner
Copy link
Contributor

@jmuehlner Any further concerns?

Nope, LGTM.

@necouchman
Copy link
Contributor

@neandrake Actually, one more request - can you please rebase against the apache:patch branch (instead of main)?

@neandrake
Copy link
Contributor Author

@neandrake Actually, one more request - can you please rebase against the apache:patch branch (instead of main)?

I've no idea how to do that. I opened a new pull request on the patch branch, #997

@neandrake neandrake closed this Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants