Bug 178214

Created attachment 323549 [details] Patch

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.

Created attachment 323554 [details] Patch

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.

Created attachment 323556 [details] Patch

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

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

Created attachment 323596 [details] Patch

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

(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 on attachment 323596 [details] Patch Clearing flags on attachment: 323596 Committed r223264: <https://trac.webkit.org/changeset/223264>

All reviewed patches have been landed. Closing bug.

<rdar://problem/34967878>

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

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

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?

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

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

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

Mass move bugs into the DOM component.

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