RESOLVED FIXED 178214
[Mac] Add support for MouseEvent.buttons
https://bugs.webkit.org/show_bug.cgi?id=178214
Summary [Mac] Add support for MouseEvent.buttons
Chris Dumez
Reported 2017-10-12 09:42:36 PDT
Add support for MouseEvent.buttons on Mac: https://www.w3.org/TR/uievents/#ref-for-dom-mouseevent-buttons-1 This is supported by Firefox and Chrome already.
Attachments
Patch (34.71 KB, patch)
2017-10-12 12:42 PDT, Chris Dumez
no flags
Patch (35.23 KB, patch)
2017-10-12 13:12 PDT, Chris Dumez
no flags
Patch (37.29 KB, patch)
2017-10-12 13:40 PDT, Chris Dumez
no flags
Archive of layout-test-results from ews121 for ios-simulator-wk2 (961.25 KB, application/zip)
2017-10-12 15:13 PDT, Build Bot
no flags
Patch (40.65 KB, patch)
2017-10-12 16:26 PDT, Chris Dumez
no flags
Chris Dumez
Comment 1 2017-10-12 12:42:57 PDT
Build Bot
Comment 2 2017-10-12 12:43:52 PDT
Attachment 323549 [details] did not pass style-queue: ERROR: Source/WebCore/dom/MouseEvent.cpp:96: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Chris Dumez
Comment 3 2017-10-12 13:12:34 PDT
Build Bot
Comment 4 2017-10-12 13:14:13 PDT
Attachment 323554 [details] did not pass style-queue: ERROR: Source/WebCore/dom/MouseEvent.cpp:96: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Chris Dumez
Comment 5 2017-10-12 13:40:53 PDT
Build Bot
Comment 6 2017-10-12 13:43:14 PDT
Attachment 323556 [details] did not pass style-queue: ERROR: Source/WebCore/dom/MouseEvent.cpp:96: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 1 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
Build Bot
Comment 7 2017-10-12 15:13:45 PDT
Comment on attachment 323556 [details] Patch Attachment 323556 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4836748 New failing tests: imported/w3c/web-platform-tests/dom/events/Event-subclasses-constructors.html
Build Bot
Comment 8 2017-10-12 15:13:46 PDT
Created attachment 323576 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Chris Dumez
Comment 9 2017-10-12 16:26:52 PDT
Build Bot
Comment 10 2017-10-12 16:29:27 PDT
Attachment 323596 [details] did not pass style-queue: ERROR: Source/WebCore/dom/MouseEvent.cpp:96: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 1 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
Ryosuke Niwa
Comment 11 2017-10-12 16:38:16 PDT
Comment on attachment 323596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323596&action=review > Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:134 > + return static_cast<unsigned short>([NSEvent pressedMouseButtons]); According to https://developer.apple.com/documentation/appkit/nsevent/1527943-pressedmousebuttons?language=objc pressedMouseButtons could contain values with 1 >> n where n is greater than 2 but not necessarily 3 so we have to make sure to coerce back those values to 4.
Chris Dumez
Comment 12 2017-10-12 16:44:05 PDT
(In reply to Ryosuke Niwa from comment #11) > Comment on attachment 323596 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=323596&action=review > > > Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:134 > > + return static_cast<unsigned short>([NSEvent pressedMouseButtons]); > > According to > https://developer.apple.com/documentation/appkit/nsevent/1527943- > pressedmousebuttons?language=objc > pressedMouseButtons could contain values with 1 >> n where n is greater than > 2 but not necessarily 3 > so we have to make sure to coerce back those values to 4. I noticed that but the specification allows for this: https://www.w3.org/TR/uievents/#ref-for-dom-mouseevent-buttons-1 " Some pointing devices provide or simulate more buttons. To represent such buttons, the value MUST be doubled for each successive button (in the binary series 8, 16, 32, ... ). "
WebKit Commit Bot
Comment 13 2017-10-12 17:24:26 PDT
Comment on attachment 323596 [details] Patch Clearing flags on attachment: 323596 Committed r223264: <https://trac.webkit.org/changeset/223264>
WebKit Commit Bot
Comment 14 2017-10-12 17:24:28 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 15 2017-10-12 17:25:29 PDT
Darin Adler
Comment 16 2017-10-14 19:47:17 PDT
Comment on attachment 323596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323596&action=review > Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:748 > + m_buttons = currentlyPressedMouseButtons(); This isn’t quite correct. This gives us the buttons pressed at the *time* the PlatformEvent object is created, but what we want are the buttons that were pressed at the time the event was generated, which is not the same thing. A correct implementation needs to use -[NSEvent buttonMask], not +[NSEvent pressedMouseButtons].
Chris Dumez
Comment 17 2017-10-15 11:43:08 PDT
(In reply to Darin Adler from comment #16) > Comment on attachment 323596 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=323596&action=review > > > Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:748 > > + m_buttons = currentlyPressedMouseButtons(); > > This isn’t quite correct. This gives us the buttons pressed at the *time* > the PlatformEvent object is created, but what we want are the buttons that > were pressed at the time the event was generated, which is not the same > thing. A correct implementation needs to use -[NSEvent buttonMask], not > +[NSEvent pressedMouseButtons]. The buttonMask documentation says it is for *tablet* events? https://developer.apple.com/documentation/appkit/nsevent/1535428-buttonmask?language=objc
Chris Dumez
Comment 18 2017-10-15 11:49:23 PDT
Comment on attachment 323596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323596&action=review >>> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:748 >>> + m_buttons = currentlyPressedMouseButtons(); >> >> This isn’t quite correct. This gives us the buttons pressed at the *time* the PlatformEvent object is created, but what we want are the buttons that were pressed at the time the event was generated, which is not the same thing. A correct implementation needs to use -[NSEvent buttonMask], not +[NSEvent pressedMouseButtons]. > > The buttonMask documentation says it is for *tablet* events? > https://developer.apple.com/documentation/appkit/nsevent/1535428-buttonmask?language=objc I agree that relying on [NSEvent pressedMouseButtons] is not ideal but I could not find a better API. In practice, the WebMouseEvent (in WK2 case) and PlatformMouseEvent (in WK1 case) should get constructed at the point when the event is generated, no? WK2: User clicks mouse > NSEvent received by UIProcess > WebKeyboardEvent constructed from NSEvent and using [NSEvent pressedMouseButtons] > IPC to WebProcess > create PlatformKeyboardEvent from WebKeyboardEvent. WK1: User clicks mouse > NSEvent received by process > PlatformKeyboardEvent constructed from NSEvent and using [NSEvent pressedMouseButtons]
Darin Adler
Comment 19 2017-10-15 12:11:22 PDT
Comment on attachment 323596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323596&action=review >>>> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:748 >>>> + m_buttons = currentlyPressedMouseButtons(); >>> >>> This isn’t quite correct. This gives us the buttons pressed at the *time* the PlatformEvent object is created, but what we want are the buttons that were pressed at the time the event was generated, which is not the same thing. A correct implementation needs to use -[NSEvent buttonMask], not +[NSEvent pressedMouseButtons]. >> >> The buttonMask documentation says it is for *tablet* events? >> https://developer.apple.com/documentation/appkit/nsevent/1535428-buttonmask?language=objc > > I agree that relying on [NSEvent pressedMouseButtons] is not ideal but I could not find a better API. In practice, the WebMouseEvent (in WK2 case) and PlatformMouseEvent (in WK1 case) should get constructed at the point when the event is generated, no? > > WK2: User clicks mouse > NSEvent received by UIProcess > WebKeyboardEvent constructed from NSEvent and using [NSEvent pressedMouseButtons] > IPC to WebProcess > create PlatformKeyboardEvent from WebKeyboardEvent. > WK1: User clicks mouse > NSEvent received by process > PlatformKeyboardEvent constructed from NSEvent and using [NSEvent pressedMouseButtons] By the time a process receives an NSEvent, it can already be a bit old. I understand that WebKit will do this work as soon as it can, but that might not be soon enough. An NSEvent is a record of something that happen in the recent past. I’m sorry to hear that NSEvent does not have a suitable method.
Darin Adler
Comment 20 2017-10-15 12:21:17 PDT
Comment on attachment 323596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323596&action=review >>>>> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:748 >>>>> + m_buttons = currentlyPressedMouseButtons(); >>>> >>>> This isn’t quite correct. This gives us the buttons pressed at the *time* the PlatformEvent object is created, but what we want are the buttons that were pressed at the time the event was generated, which is not the same thing. A correct implementation needs to use -[NSEvent buttonMask], not +[NSEvent pressedMouseButtons]. >>> >>> The buttonMask documentation says it is for *tablet* events? >>> https://developer.apple.com/documentation/appkit/nsevent/1535428-buttonmask?language=objc >> >> I agree that relying on [NSEvent pressedMouseButtons] is not ideal but I could not find a better API. In practice, the WebMouseEvent (in WK2 case) and PlatformMouseEvent (in WK1 case) should get constructed at the point when the event is generated, no? >> >> WK2: User clicks mouse > NSEvent received by UIProcess > WebKeyboardEvent constructed from NSEvent and using [NSEvent pressedMouseButtons] > IPC to WebProcess > create PlatformKeyboardEvent from WebKeyboardEvent. >> WK1: User clicks mouse > NSEvent received by process > PlatformKeyboardEvent constructed from NSEvent and using [NSEvent pressedMouseButtons] > > By the time a process receives an NSEvent, it can already be a bit old. I understand that WebKit will do this work as soon as it can, but that might not be soon enough. An NSEvent is a record of something that happen in the recent past. > > I’m sorry to hear that NSEvent does not have a suitable method. Since you and I work for Apple, I recommend contacting the AppKit team about this and asking for advice. Consider these other pairs of methods: -[NSEvent locationInWindow] and +[NSEvent mouseLocation] -[NSEvent modifierFlags] and +[NSEvent modifierFlags] In both cases the documentation emphasizes that the instance method gives information about the "event stream" and the class method gives information that is "independent of which events have been delivered via the event stream". The comment on the pressedMouseButtons method saying "this method is not suitable for tracking" is alluding to this very problem, I think. This will only get it wrong when event processing is really slow. Like on a heavily loaded system. But in those cases this mask could even have a 0 bit for the button that generated the event if it has already been released by the time this is called.
Sam Weinig
Comment 21 2017-10-15 14:32:56 PDT
I have not tested what it does for multiple mouse buttons down (I don't have a mouse with buttons at the moment), but does NSEvent buttonNumber https://developer.apple.com/documentation/appkit/nsevent/1527828-buttonnumber do what you are looking for?
Chris Dumez
Comment 22 2017-10-16 09:35:26 PDT
(In reply to Sam Weinig from comment #21) > I have not tested what it does for multiple mouse buttons down (I don't have > a mouse with buttons at the moment), but does NSEvent buttonNumber > https://developer.apple.com/documentation/appkit/nsevent/1527828- > buttonnumber do what you are looking for? I will check but the documentation does not indicate it is a mask that would cover having several buttons pressed. Chances are it only provides a single button.
Chris Dumez
Comment 23 2017-10-16 10:17:14 PDT
(In reply to Chris Dumez from comment #22) > (In reply to Sam Weinig from comment #21) > > I have not tested what it does for multiple mouse buttons down (I don't have > > a mouse with buttons at the moment), but does NSEvent buttonNumber > > https://developer.apple.com/documentation/appkit/nsevent/1527828- > > buttonnumber do what you are looking for? > > I will check but the documentation does not indicate it is a mask that would > cover having several buttons pressed. Chances are it only provides a single > button. I have confirmed that buttonNumber only provides a single button so it is not suitable here.
Beau Adkins
Comment 24 2018-11-10 13:02:16 PST
This revision has caused a drag/drop regression. Details here: https://bugs.webkit.org/show_bug.cgi?id=191481
Lucas Forschler
Comment 25 2019-02-06 09:19:03 PST
Mass move bugs into the DOM component.
Fujii Hironori
Comment 26 2019-09-03 20:07:17 PDT
*** Bug 81855 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.