Bug 178214 - [Mac] Add support for MouseEvent.buttons
Summary: [Mac] Add support for MouseEvent.buttons
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: https://www.w3.org/TR/uievents/#ref-f...
Keywords: InRadar, WebExposed
: 81855 (view as bug list)
Depends on: 191481
Blocks: 201445
  Show dependency treegraph
 
Reported: 2017-10-12 09:42 PDT by Chris Dumez
Modified: 2019-09-03 20:41 PDT (History)
14 users (show)

See Also:


Attachments
Patch (34.71 KB, patch)
2017-10-12 12:42 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (35.23 KB, patch)
2017-10-12 13:12 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (37.29 KB, patch)
2017-10-12 13:40 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
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 Details
Patch (40.65 KB, patch)
2017-10-12 16:26 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 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.
Comment 1 Chris Dumez 2017-10-12 12:42:57 PDT
Created attachment 323549 [details]
Patch
Comment 2 Build Bot 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.
Comment 3 Chris Dumez 2017-10-12 13:12:34 PDT
Created attachment 323554 [details]
Patch
Comment 4 Build Bot 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.
Comment 5 Chris Dumez 2017-10-12 13:40:53 PDT
Created attachment 323556 [details]
Patch
Comment 6 Build Bot 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.
Comment 7 Build Bot 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
Comment 8 Build Bot 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
Comment 9 Chris Dumez 2017-10-12 16:26:52 PDT
Created attachment 323596 [details]
Patch
Comment 10 Build Bot 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.
Comment 11 Ryosuke Niwa 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.
Comment 12 Chris Dumez 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, ... ).
"
Comment 13 WebKit Commit Bot 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>
Comment 14 WebKit Commit Bot 2017-10-12 17:24:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2017-10-12 17:25:29 PDT
<rdar://problem/34967878>
Comment 16 Darin Adler 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].
Comment 17 Chris Dumez 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
Comment 18 Chris Dumez 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]
Comment 19 Darin Adler 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.
Comment 20 Darin Adler 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.
Comment 21 Sam Weinig 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?
Comment 22 Chris Dumez 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.
Comment 23 Chris Dumez 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.
Comment 24 Beau Adkins 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
Comment 25 Lucas Forschler 2019-02-06 09:19:03 PST
Mass move bugs into the DOM component.
Comment 26 Fujii Hironori 2019-09-03 20:07:17 PDT
*** Bug 81855 has been marked as a duplicate of this bug. ***