Bug 178214 – [Mac] Add support for MouseEvent.buttons

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 323549 [details]
Patch

[Build Bot]

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]

Created attachment 323554 [details]
Patch

[Build Bot]

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]

Created attachment 323556 [details]
Patch

[Build Bot]

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 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]

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]

Created attachment 323596 [details]
Patch

[Build Bot]

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 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]

(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 on attachment 323596 [details]
Patch

Clearing flags on attachment: 323596

Committed r223264: <https://trac.webkit.org/changeset/223264>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34967878>

[Darin Adler]

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]

(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 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 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 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]

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]

(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]

(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]

This revision has caused a drag/drop regression. Details here: https://bugs.webkit.org/show_bug.cgi?id=191481

[Lucas Forschler]

Mass move bugs into the DOM component.

[Fujii Hironori]

*** Bug 81855 has been marked as a duplicate of this bug. ***