Skip to content

[macOS] buttons property is incorrect for mouse events generated through the EventSender interface #39853

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
Merged
merged 1 commit into from
Feb 4, 2025
Merged

[macOS] buttons property is incorrect for mouse events generated through the EventSender interface #39853

merged 1 commit into from
Feb 4, 2025

Conversation

@aprotyas
Copy link
Member

@aprotyas aprotyas commented Feb 1, 2025 •
edited by webkit-early-warning-system
Loading

7fe5188

[macOS] `buttons` property is incorrect for mouse events generated through the EventSender interface
https://bugs.webkit.org/show_bug.cgi?id=267801
rdar://121296080

Reviewed by Wenson Hsieh.

Currently, `eventSender.mouseDown(2)` produces a `mousedown` event with
a `buttons` value of 4, rather than 2. This is because the button number
ordering as prescribed by the DOM spec (Left=0, Middle=1, Right=2) does
not exactly map to the bit order for button presence in the `buttons`
value where, instead, Left=Bit0, Right=Bit1, and Middle=Bit2, c.f.
https://w3c.github.io/uievents/#dom-mouseevent-buttons.

Our `buttons` value was wrong because we were simply shifting left based
on the former order. This patch addresses this error by accounting for
the change in order for the `buttons` value, inspired by a similar bug
fix for webdriver in 283790@main. We do so by tracking active button
presses in a map keyed by (known) WebCore::MouseButton values, and then
safely building up the appropriate `buttons` value by following the
latter order. To facilitate this new tracking, we introduce the
`buttonAsShort()` helper on `MouseButton`. When we get a button number
from EventSender, we can convert this number to a known MouseButton case,
instead of having to define and use yet another local `MouseButton`
enumeration.

* LayoutTests/fast/events/fire-mousedown-while-pressing-mouse-button.html:

Correct the same (wrong) assumption we made in WKTR.

* LayoutTests/fast/events/mouse-event-buttons-expected.txt: Added.
* LayoutTests/fast/events/mouse-event-buttons.html: Added.

New test to sanity check the `buttons` values when doing mouse
interactions through EventSender.

* LayoutTests/imported/w3c/web-platform-tests/pointerevents/pointerevent_attributes_mouse-right-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/pointerevents/pointerevent_attributes_mouse-right-nonstandard-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/pointerevents/pointerevent_attributes_pen-right-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/pointerevents/pointerevent_attributes_pen-right-nonstandard-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/uievents/mouse/synthetic-mouse-enter-leave-over-out-button-state-after-target-removed.tentative_buttonType=MIDDLE&button=1&buttons=4-expected.txt:

Adjust test results because we now reflect the correct `buttons` value.

* LayoutTests/platform/ios/TestExpectations:
* LayoutTests/platform/mac-wk1/fast/events/mouse-event-buttons-expected.txt: Added.
* LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/uievents/mouse/synthetic-mouse-enter-leave-over-out-button-state-after-target-removed.tentative_buttonType=MIDDLE&button=1&buttons=4-expected.txt:
* LayoutTests/platform/win/TestExpectations:
* LayoutTests/platform/wpe/TestExpectations:
* Source/WebCore/dom/MouseEvent.cpp:
(WebCore::MouseEvent::buttonFromShort):
(WebCore::MouseEvent::button const):
* Source/WebCore/dom/MouseEvent.h:
* Tools/DumpRenderTree/mac/EventSendingController.mm:
(eventTypeForMouseButtonAndAction):
(swizzledEventPressedMouseButtons):
(-[EventSendingController mouseDown:withModifiers:]):
(-[EventSendingController mouseUp:withModifiers:]):

Drive-by cleanup: Make `MouseAction` an enum class.

* Tools/WebKitTestRunner/EventSenderProxy.h:

Keep the data type for m_mouseButtonsCurrentlyDown unchanged for
non-Cocoa ports, since said ports compute the `buttons` value
differently.

(WTR::EventSenderProxy::mouseButtonsCurrentlyDown const): Deleted.
* Tools/WebKitTestRunner/InjectedBundle/ios/EventSenderProxyIOS.mm:
* Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:
* Tools/WebKitTestRunner/cocoa/EventSenderProxyCocoa.mm: Added.
(WTR::EventSenderProxy::mouseButtonsCurrentlyDown const):

File containing Cocoa-specific EventSenderProxy code. Currently only
populated by ::mouseButtonsCurrentlyDown(), but we should move more code
in here.

* Tools/WebKitTestRunner/mac/EventSenderProxy.mm:
(WTR::eventTypeForMouseButtonAndAction):
(WTR::EventSenderProxy::EventSenderProxy):
(WTR::EventSenderProxy::mouseDown):
(WTR::EventSenderProxy::mouseUp):
(): Deleted.

Drive-by cleanup: Make `MouseAction` an enum class.
Canonical link: https://commits.webkit.org/289800@main

0c543b8

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@aprotyas aprotyas self-assigned this Feb 1, 2025
@aprotyas aprotyas added the Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases label Feb 1, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Feb 1, 2025 •
edited
Loading

EWS run on previous version of this PR (hash a0a5dd8)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ❌ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2   🧪 win-tests
✅ 🧪 webkitperl   🧪 ios-wk2 ✅ 🧪 api-mac ❌ 🧪 api-wpe
❌ 🧪 ios-wk2-wpt   🧪 mac-wk1   🛠 wpe-cairo
✅ 🧪 api-ios   🧪 mac-wk2 ❌ 🛠 gtk
✅ 🛠 vision   🧪 mac-AS-debug-wk2 ❌ 🧪 gtk-wk2
✅ 🛠 vision-sim ❌ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🧪 vision-wk2   🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 1, 2025
@aprotyas aprotyas removed the merging-blocked Applied to prevent a change from being merged label Feb 1, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Feb 1, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 7eb6e7e)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ❌ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ❌ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ❌ 🛠 wpe-cairo
✅ 🧪 api-ios ❌ 🧪 mac-wk2 ❌ 🛠 gtk
✅ 🛠 vision ❌ 🧪 mac-AS-debug-wk2 ❌ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🧪 vision-wk2 ❌ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 1, 2025
@aprotyas aprotyas removed the merging-blocked Applied to prevent a change from being merged label Feb 2, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Feb 2, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 8f72f4d)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ❌ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ❌ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ❌ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ❌ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ❌ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 2, 2025
@aprotyas aprotyas removed the merging-blocked Applied to prevent a change from being merged label Feb 3, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Feb 3, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 7998e17)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2 ❌ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 3, 2025
@aprotyas aprotyas removed the merging-blocked Applied to prevent a change from being merged label Feb 3, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Feb 3, 2025 •
edited
Loading

EWS run on previous version of this PR (hash bd9f351)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 3, 2025
@aprotyas aprotyas removed the merging-blocked Applied to prevent a change from being merged label Feb 3, 2025
@aprotyas aprotyas changed the title [WKTR] buttons property is incorrect for mouse events generated through the EventSender interface [macOS] buttons property is incorrect for mouse events generated through the EventSender interface Feb 3, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Feb 3, 2025 •
edited
Loading

EWS run on current version of this PR (hash 0c543b8)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@aprotyas aprotyas marked this pull request as ready for review February 4, 2025 08:33
@aprotyas aprotyas requested review from pxlcoder and whsieh February 4, 2025 08:34
@aprotyas
Copy link
Member Author

aprotyas commented Feb 4, 2025

Thanks for the review!

@aprotyas aprotyas added the merge-queue Applied to send a pull request to merge-queue label Feb 4, 2025
@aprotyas
[macOS] buttons property is incorrect for mouse events generated th…
7fe5188 
…rough the EventSender interface

https://bugs.webkit.org/show_bug.cgi?id=267801
rdar://121296080

Reviewed by Wenson Hsieh.

Currently, `eventSender.mouseDown(2)` produces a `mousedown` event with
a `buttons` value of 4, rather than 2. This is because the button number
ordering as prescribed by the DOM spec (Left=0, Middle=1, Right=2) does
not exactly map to the bit order for button presence in the `buttons`
value where, instead, Left=Bit0, Right=Bit1, and Middle=Bit2, c.f.
https://w3c.github.io/uievents/#dom-mouseevent-buttons.

Our `buttons` value was wrong because we were simply shifting left based
on the former order. This patch addresses this error by accounting for
the change in order for the `buttons` value, inspired by a similar bug
fix for webdriver in 283790@main. We do so by tracking active button
presses in a map keyed by (known) WebCore::MouseButton values, and then
safely building up the appropriate `buttons` value by following the
latter order. To facilitate this new tracking, we introduce the
`buttonAsShort()` helper on `MouseButton`. When we get a button number
from EventSender, we can convert this number to a known MouseButton case,
instead of having to define and use yet another local `MouseButton`
enumeration.

* LayoutTests/fast/events/fire-mousedown-while-pressing-mouse-button.html:

Correct the same (wrong) assumption we made in WKTR.

* LayoutTests/fast/events/mouse-event-buttons-expected.txt: Added.
* LayoutTests/fast/events/mouse-event-buttons.html: Added.

New test to sanity check the `buttons` values when doing mouse
interactions through EventSender.

* LayoutTests/imported/w3c/web-platform-tests/pointerevents/pointerevent_attributes_mouse-right-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/pointerevents/pointerevent_attributes_mouse-right-nonstandard-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/pointerevents/pointerevent_attributes_pen-right-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/pointerevents/pointerevent_attributes_pen-right-nonstandard-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/uievents/mouse/synthetic-mouse-enter-leave-over-out-button-state-after-target-removed.tentative_buttonType=MIDDLE&button=1&buttons=4-expected.txt:

Adjust test results because we now reflect the correct `buttons` value.

* LayoutTests/platform/ios/TestExpectations:
* LayoutTests/platform/mac-wk1/fast/events/mouse-event-buttons-expected.txt: Added.
* LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/uievents/mouse/synthetic-mouse-enter-leave-over-out-button-state-after-target-removed.tentative_buttonType=MIDDLE&button=1&buttons=4-expected.txt:
* LayoutTests/platform/win/TestExpectations:
* LayoutTests/platform/wpe/TestExpectations:
* Source/WebCore/dom/MouseEvent.cpp:
(WebCore::MouseEvent::buttonFromShort):
(WebCore::MouseEvent::button const):
* Source/WebCore/dom/MouseEvent.h:
* Tools/DumpRenderTree/mac/EventSendingController.mm:
(eventTypeForMouseButtonAndAction):
(swizzledEventPressedMouseButtons):
(-[EventSendingController mouseDown:withModifiers:]):
(-[EventSendingController mouseUp:withModifiers:]):

Drive-by cleanup: Make `MouseAction` an enum class.

* Tools/WebKitTestRunner/EventSenderProxy.h:

Keep the data type for m_mouseButtonsCurrentlyDown unchanged for
non-Cocoa ports, since said ports compute the `buttons` value
differently.

(WTR::EventSenderProxy::mouseButtonsCurrentlyDown const): Deleted.
* Tools/WebKitTestRunner/InjectedBundle/ios/EventSenderProxyIOS.mm:
* Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:
* Tools/WebKitTestRunner/cocoa/EventSenderProxyCocoa.mm: Added.
(WTR::EventSenderProxy::mouseButtonsCurrentlyDown const):

File containing Cocoa-specific EventSenderProxy code. Currently only
populated by ::mouseButtonsCurrentlyDown(), but we should move more code
in here.

* Tools/WebKitTestRunner/mac/EventSenderProxy.mm:
(WTR::eventTypeForMouseButtonAndAction):
(WTR::EventSenderProxy::EventSenderProxy):
(WTR::EventSenderProxy::mouseDown):
(WTR::EventSenderProxy::mouseUp):
(): Deleted.

Drive-by cleanup: Make `MouseAction` an enum class.
Canonical link: https://commits.webkit.org/289800@main
@webkit-commit-queue
Copy link
Collaborator

webkit-commit-queue commented Feb 4, 2025

Committed 289800@main (7fe5188): https://commits.webkit.org/289800@main

Reviewed commits have been landed. Closing PR #39853 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 7fe5188 into WebKit:main Feb 4, 2025
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 4, 2025
@aprotyas aprotyas deleted the eng/267801 branch February 4, 2025 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@whsieh whsieh whsieh approved these changes

@rniwa rniwa Awaiting requested review from rniwa rniwa is a code owner

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

@JonWBedard JonWBedard Awaiting requested review from JonWBedard JonWBedard is a code owner

@pxlcoder pxlcoder Awaiting requested review from pxlcoder

@rr-codes rr-codes Awaiting requested review from rr-codes

@megangardner megangardner Awaiting requested review from megangardner

@hortont424 hortont424 Awaiting requested review from hortont424

Assignees

@aprotyas aprotyas

Labels

Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@aprotyas @webkit-early-warning-system @webkit-commit-queue @whsieh @webkit-ews-buildbot