Bug 198462 – [Pointer Events] Add support for chorded button interactions

• Component: UI Events
• Status: RESOLVED FIXED

[Antoine Quint]

[Pointer Events] Add support for chorded button interactions

[Antoine Quint]

Created attachment 371126 [details]
Patch

[EWS Watchlist]

Attachment 371126 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/PlatformMouseEvent.h:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MouseEvent.cpp:86:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MouseEvent.cpp:88:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MouseEvent.cpp:89:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 4 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 371126 [details]
Patch

Attachment 371126 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12351633

New failing tests:
imported/w3c/web-platform-tests/pointerevents/pointerevent_mouse_capture_change_hover.html

[EWS Watchlist]

Created attachment 371132 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 371126 [details]
Patch

Attachment 371126 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12351661

New failing tests:
imported/w3c/web-platform-tests/pointerevents/pointerevent_mouse_capture_change_hover.html

[EWS Watchlist]

Created attachment 371133 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 371126 [details]
Patch

Attachment 371126 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12351613

New failing tests:
imported/w3c/web-platform-tests/pointerevents/pointerevent_mouse_capture_change_hover.html

[EWS Watchlist]

Created attachment 371134 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Antoine Quint]

Created attachment 371140 [details]
Patch

[EWS Watchlist]

Attachment 371140 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/PlatformMouseEvent.h:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MouseEvent.cpp:86:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MouseEvent.cpp:88:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MouseEvent.cpp:89:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 4 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antoine Quint]

Created attachment 371144 [details]
Patch

[EWS Watchlist]

Attachment 371144 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/PlatformMouseEvent.h:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MouseEvent.cpp:86:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MouseEvent.cpp:88:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MouseEvent.cpp:89:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 4 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antoine Quint]

Created attachment 371147 [details]
Patch

[EWS Watchlist]

Attachment 371147 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/PlatformMouseEvent.h:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MouseEvent.cpp:86:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MouseEvent.cpp:88:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MouseEvent.cpp:89:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 4 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 371147 [details]
Patch

Attachment 371147 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12355921

New failing tests:
jquery/offset.html

[EWS Watchlist]

Created attachment 371155 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Dean Jackson]

Comment on attachment 371147 [details]
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.

[Darin Adler]

Comment on attachment 371147 [details]
Patch

We should learn our lesson from this. Why do we have to have a magic number at all? Can we use Optional instead?

[Antoine Quint]

(In reply to Darin Adler from comment #18)
> Comment on attachment 371147 [details]
> Patch
> 
> We should learn our lesson from this. Why do we have to have a magic number
> at all? Can we use Optional instead?

Yes, I think so. I will make an updated patch that does this.

[Antoine Quint]

Committed r246103: <https://trac.webkit.org/changeset/246103>

[Radar WebKit Bug Importer]

<rdar://problem/51432754>

[Antoine Quint]

Given how big this patch already is, and after talking with the reviewer (Dean Jackson), I will remove the -2 values in favor of WTF::Optional types in a followup. I filed https://bugs.webkit.org/show_bug.cgi?id=198557.

[Truitt Savell]

Looks like this test is failing on Mojave now after r246103:
imported/w3c/web-platform-tests/pointerevents/pointerevent_mouse_capture_change_hover.html

History:
http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fpointerevents%2Fpointerevent_mouse_capture_change_hover.html

Diff:
https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r246104%20(4506)/imported/w3c/web-platform-tests/pointerevents/pointerevent_mouse_capture_change_hover-diff.txt

[Antoine Quint]

(In reply to Truitt Savell from comment #23)
> Looks like this test is failing on Mojave now after r246103:
> imported/w3c/web-platform-tests/pointerevents/
> pointerevent_mouse_capture_change_hover.html
> 
> History:
> http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-
> tests%2Fpointerevents%2Fpointerevent_mouse_capture_change_hover.html
> 
> Diff:
> https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/
> r246104%20(4506)/imported/w3c/web-platform-tests/pointerevents/
> pointerevent_mouse_capture_change_hover-diff.txt

I'll take a look at Mojave tomorrow.

[Ryan Haddad]

Also, fast/events/fire-mousedown-while-pressing-mouse-button.html is failing on Windows after this change.

--- /home/buildbot/worker/win10-release-tests/build/layout-test-results/fast/events/fire-mousedown-while-pressing-mouse-button-expected.txt
+++ /home/buildbot/worker/win10-release-tests/build/layout-test-results/fast/events/fire-mousedown-while-pressing-mouse-button-actual.txt
@@ -4,18 +4,12 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS received mousedown for the middle mouse button while pressing the left mouse button.
-PASS received mousedown for the right mouse button while pressing the left mouse button.
 
 When pressing and holding the "middle mouse button"
 
-PASS received mousedown for the left mouse button while pressing the middle mouse button.
-PASS received mousedown for the right mouse button while pressing the middle mouse button.
 
 When pressing and holding the "right mouse button"
 
-PASS received mousedown for the left mouse button while pressing the right mouse button.
-PASS received mousedown for the middle mouse button while pressing the right mouse button.
 
 TEST COMPLETE
 
https://build.webkit.org/results/Apple%20Win%2010%20Release%20(Tests)/r246105%20(2467)/results.html

[Antoine Quint]

Right, Mojave will need a rebaseline for imported/w3c/web-platform-tests/pointerevents/pointerevent_mouse_capture_change_hover.html, working on that now.

However, the Windows problem is not something I can reproduce, I don't have a Windows environment :(

[Antoine Quint]

WK1 results have been rebaselined in https://trac.webkit.org/changeset/246149/webkit.

[Shawn Roberts]

Also the following layout test is timing on Mojave WK1 Release and Debug after r246103

scrollbars/scrollbar-iframe-click-does-not-blur-content.html

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=scrollbars%2Fscrollbar-iframe-click-does-not-blur-content.html

[Antoine Quint]

(In reply to Shawn Roberts from comment #28)
> Also the following layout test is timing on Mojave WK1 Release and Debug
> after r246103
> 
> scrollbars/scrollbar-iframe-click-does-not-blur-content.html
> 
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=scrollbars%2Fscrollbar-iframe-click-does-not-
> blur-content.html

This was filed as https://bugs.webkit.org/show_bug.cgi?id=198800 and I have a patch out for it.

[Antoine Quint]

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