[Pointer Events] Add support for chorded button interactions
Created attachment 371126 [details] Patch
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.
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
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
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
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
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
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
Created attachment 371140 [details] Patch
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.
Created attachment 371144 [details] Patch
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.
Created attachment 371147 [details] Patch
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.
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
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
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.
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?
(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.
Committed r246103: <https://trac.webkit.org/changeset/246103>
<rdar://problem/51432754>
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.
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
(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.
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
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 :(
WK1 results have been rebaselined in https://trac.webkit.org/changeset/246149/webkit.
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
(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.
*** Bug 198965 has been marked as a duplicate of this bug. ***