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.

Sign up for GitHub

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

Jump to bottom

Mouse wheel delta adjustments #7968

Merged
merged 4 commits into from
Feb 11, 2019
Merged

Mouse wheel delta adjustments #7968

merged 4 commits into from
Feb 11, 2019

Conversation

flostellbrink
Copy link
Contributor

Description

Right now the mouse wheel delta is not scaled properly. See #6283 for a discussion of this issue.

This pull request handles all event types and modes separately. All units are converted into steps, i.e. wheel notches.

Testing

Unfortunately I don't know how to automatically test this behavior.
I have manually tested this on Windows 10 with Chrome, Firefox, Edge, and IE11.

  • For Chrome the value depends on the system scaling (100% -> 1 step, 150% -> 1,5 steps).
  • All other browsers return exactly one step for mouse wheel notches, and look reasonable for touchpad scrolling.

You can use the following snippet to test this in isolation:

<html>
  <script src="index.js"></script>
</html>
window.addEventListener("wheel", testNormalize, true);
window.addEventListener("mousewheel", testNormalize, true);
window.addEventListener("DOMMouseScroll", testNormalize, true);

function testNormalize(event) {
  console.log(getMouseWheelDelta(event));
}

@juj
Copy link
Collaborator

juj commented Feb 1, 2019

For testing, one way is to create a test .cpp file that prints out the different scroll values, and create a test in tests/test_interactive.py that one can launch to assist to manually test the behavior. In such an interactive test, the test code provides a ready made application to do the test, but human then verifies that the values are as one expects.

Reading how the test_interactive.py script builds the test page also helps one with instructions to repro a build of the page manually, e.g. if one wants to test the behavior across different browsers/devices.

@flostellbrink
Copy link
Contributor Author

Interactive tests sound perfect for this! I'll look into it.

@flostellbrink
Copy link
Contributor Author

Okay, I have added a sanity check to the existing SDL mouse wheel test.

  • I chose 2 as an acceptable upper limit, because some variation is possible. I could only reach this upper limit by setting the listener directly on the window, scaling window 10 to 200%, and using Chrome. I don't think any worse can happen by accident.
  • The lower limit is already set to 1 by SDL.

This test also prints the received delta values, so testers should be able to tell if the values look reasonable. The test succeeded for both Chrome and Firefox.

@kripken
Copy link
Member

kripken commented Feb 9, 2019

ping @juj - this looks good to me, did you want to take another look?

Thanks @Owlinated!

@juj
Copy link
Collaborator

juj commented Feb 10, 2019

Looks good. It would be good to add a note in the Changelog, since this will disrupt everyone who is currently using SDL with scrolling. This does not preserve the scale in any scenario, but always rescales to a different coordinate scale than before, so everyone who is using SDL 1 will need to update their code to rescale to the new heuristics.

I am fine with this landing, though mostly since this is in library_browser.js, which is a bit of legacy/compatibility layer at this point, so I want to lean towards making development easier for anyone using it. For contrast, this would not be good behavior for HTML5 API mouse scrolling. SDL2 uses the HTML5 API, so it will be unaffected by this PR. lgtm!

@flostellbrink
Copy link
Contributor Author

Glad to hear it! I have added a note to the changelog. Hope that's what you had in mind.

Thanks for the help @juj and @kripken!

@juj
Copy link
Collaborator

juj commented Feb 10, 2019

Hmm, can you double check the edit to ChangeLog? Perhaps this was based on older revision of incoming, since this is now saying a conflict against ChangeLog.md. Rebase or merge against latest incoming?

@flostellbrink
Copy link
Contributor Author

@juj Done :)

@juj
Copy link
Collaborator

juj commented Feb 11, 2019

Thanks! Though sorry, looks like Alon landed something else to incoming just tonight, so the file is in conflict again. Apologies for that, can you resolve once again?

@flostellbrink
Copy link
Contributor Author

@juj Sure thing!

@juj
Copy link
Collaborator

juj commented Feb 11, 2019

Alright, thanks!

@juj juj merged commit 7f77c64 into emscripten-core:incoming Feb 11, 2019
@thomasballinger thomasballinger mentioned this pull request Aug 6, 2021
Closed
@pageldev pageldev mentioned this pull request Mar 12, 2022
Merged
juj pushed a commit that referenced this pull request Mar 13, 2022
@pageldev
Update library_glfw.js - use delta from getMouseWheelDelta(event) (#1…
f524cc8 
…6480)

* Update library_glfw.js

Use delta from `getMouseWheelDelta(event)` when using GLFW 3. The adjustments made in #7968 (scaling mouse wheel delta) do not apply when using GLFW 3. This fixes that.

* update ChangeLog.md - GLFW3 v-scroll amount changed
@Jonathhhan Jonathhhan mentioned this pull request Dec 13, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juj juj juj left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@flostellbrink @juj @kripken