Use MouseEvent.buttons for button state tracking #1921
Merged
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
CendioOssman
requested changes
Dec 2, 2024
CendioHalim
force-pushed
the
mouse-buttonstate-remove
branch
from
278742b to
0925be1
Compare
|
I've updated my branch and addressed all comments |
CendioHalim
force-pushed
the
mouse-buttonstate-remove
branch
from
0925be1 to
19c2b83
Compare
CendioOssman
requested changes
Jan 8, 2025
CendioHalim
force-pushed
the
mouse-buttonstate-remove
branch
8 times, most recently
from
b14d32e to
173de1b
Compare
CendioOssman
requested changes
Jan 13, 2025
CendioHalim
force-pushed
the
mouse-buttonstate-remove
branch
3 times, most recently
from
471a456 to
5f2d0d7
Compare
CendioOssman
approved these changes
Jan 14, 2025
These functions can be used elsewhere in the tests. We want to use these in the dragging tests in the future instead of directly calling private methods.
CendioHalim
force-pushed
the
mouse-buttonstate-remove
branch
from
5f2d0d7 to
55409ab
Compare
This makes our tests reflect the real world better, as we now send real mouse events instead of calling private methods directly.
Previously, these unit tests did not check which events were sent to the server, only how many events were sent. This commit adds checks to see that the expected button events are sent.
This is needed if we want to test gestures with dragging.
There were no test for viewport dragging using gesture previously, so let's add some. Note that there currently are some viewport dragging behaviours that we don't want to have, so some tests have commented out what our desired behaviour should be.
Instead of keeping track of button states ourselves by looking at MouseEvent.button, we can use the MouseEvent.buttons which already contains the state of all buttons.
We don't want to send any mouse events to the server when dragging the viewport. Instead, we treat them as a client-only operation.
We want to flush pending mouse moves before we initiate viewport dragging. Before this commit, there were scenarios where the _mouseButtonMask would track a released button as being down.
CendioHalim
force-pushed
the
mouse-buttonstate-remove
branch
from
55409ab to
6383fa6
Compare
This was referenced Jan 14, 2025
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.
Instead of keeping track of button states ourselves by looking at
MouseEvent.button, we can use theMouseEvent.buttonswhich already contains the state of all buttons.Note that in
MouseEvent.button, button numbers are mapped as follows:But when looking at the
MouseEvent.buttonsproperty, the bits middle and right are switched, which is confusing:With this change, #1919 could be modified to work with Safari as well.
Unfortunately, the internal
_mouseButtonStateMaskstate is not removed in this PR as it would require a large rewrite of the general architecture.Tested on: