[webkit-reviews] review granted: [Bug 198462] [Pointer Events] Add support for chorded button interactions : [Attachment 371147] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Antoine Quint <graouts at apple.com>'s
request for review:
Bug 198462: [Pointer Events] Add support for chorded button interactions
https://bugs.webkit.org/show_bug.cgi?id=198462

Attachment 371147: Patch

https://bugs.webkit.org/attachment.cgi?id=371147&action=review




--- Comment #17 from Dean Jackson <dino at apple.com> ---
Comment on attachment 371147
  --> https://bugs.webkit.org/attachment.cgi?id=371147
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=371147&action=review

> Source/WebCore/ChangeLog:28
> +	   Up until now, MouseEvent.button was an "unsigned short", as
specified up to and including DOM Level 2 Events. But the
> +	   UI Events spec says that property is a "short", and PointerEvent is
the only interface where a "-1" value is used. This
> +	   required some changes throughout our codebase since we used a "-1"
value to specify that no button was pressed when dealing
> +	   with NSEvent input and going through PlatformMouseEvent and
eventually MouseEvent. So now we change the various NoButton
> +	   enum values to be "-2" and use that value, which is not going to be
used for any mouse button, as the value reflected as
> +	   "0" through MouseEvent.button, as specified by UI Events.

You could have done this as a separate patch first.

> Source/WebCore/ChangeLog:33
> +	   Furthermore, we identified another issue: MouseEvent.buttons would
always return 0 in DRT and WKTR. We rely upon that
> +	   value in PointerCaptureController::pointerEventForMouseEvent() and
so we had to make that work for the relevant WPT test,
> +	  
web-platform-tests/pointerevents/pointerevent_mouse_capture_change_hover.html,
to pass and show a correct implementation
> +	   of chorded button interactions. The details of the work required for
this is in Tools/ChangeLog.

And this too.

> Source/WebCore/dom/MouseEvent.cpp:86
> +    , m_button(button == -2 ? 0 : button)

Please define a static in MouseEvent.h that is NoButtons = -2 and use that.

> Source/WebCore/page/PointerCaptureController.cpp:233
> +    short button = newButton == capturingData.previousMouseButton ? -1 :
newButton;

And similarly for -1.

> Source/WebCore/platform/PlatformMouseEvent.h:47
> +    enum MouseButton : int8_t { LeftButton = 0, MiddleButton, RightButton,
NoButton = -2 };

You don't need the = 0.

> Source/WebKitLegacy/mac/WebView/WebPDFView.mm:963
> -    const int noButton = -1;
> +    const int noButton = -2;

Make this refer to the value you specify in MouseEvent.h.

> LayoutTests/fast/events/constructors/mouse-event-constructor.html:-110
> -shouldBe("new MouseEvent('eventType', { button: 9007199254740991 }).button",
"0");

You should probably test a huge number like this.