[macCatalyst] Mouse clicks dispatch duplicate pointerup and pointerdown events
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 23 Dec 2019 01:15:07 +0000 (01:15 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 23 Dec 2019 01:15:07 +0000 (01:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=205551
<rdar://problem/58058268>

Reviewed by Tim Horton.

Source/WebCore:

This began occuring after r251320, wherein some mouse event handling codepaths were enabled in macCatalyst.
For compatibility, gesture recognizers still fire in the macCatalyst platform. This includes the synthetic click
gesture, which will still synthesize and send mouseup and mousedown events to the page. After the change, this
results in pointer events being dispatched under the call to `shouldIgnoreMouseEvent()`. However, at the same
time, touch event handling codepaths have already dispatched "pointerup" and "pointerdown", so we end up with
redundant events.

To fix this macCatalyst-specific bug, simply avoid dispatching pointer events in the case where the synthetic
click type is some kind of tap gesture; in this case, pointer events have already been dispatched, so we don't
need to dispatch them again via mouse event handling code.

Test: pointerevents/ios/pointer-events-with-click-handler.html

* dom/Element.cpp:
(WebCore::dispatchPointerEventIfNeeded):

Also rename shouldIgnoreMouseEvent to dispatchPointerEventIfNeeded to better reflect that this function's
primary purposee is to dispatch pointer events in response to platform mouse events; then, change the return
value to an explicit enum class indicating whether the mouse event should be subsequently ignored (as a result
of the page preventing the dispatched pointer event).

(WebCore::Element::dispatchMouseEvent):
(WebCore::shouldIgnoreMouseEvent): Deleted.

LayoutTests:

* pointerevents/ios/pointer-events-with-click-handler-expected.txt: Added.
* pointerevents/ios/pointer-events-with-click-handler.html: Added.

Add a layout test to verify that the bug does not occur. While this is a macCatalyst fix, this test needs to be
in the `ios` directory for now because macCatalyst is still considered "iOS family". This test is also still
relevant to both platforms (on iOS, synthesizing a tap behaves as expected, and in macCatalyst, it simulates a
click at the same location).

* pointerevents/utils.js:
(EventTracker.prototype.assertMatchesEvents):
(EventTracker):

Drive-by fix: flip the order of arguments to `assert_equals`, so that when tests fail, the failure output
correctly shows how many events were expected, and how many were observed.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@253878 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/pointerevents/ios/pointer-events-with-click-handler-expected.txt [new file with mode: 0644]
LayoutTests/pointerevents/ios/pointer-events-with-click-handler.html [new file with mode: 0644]
LayoutTests/pointerevents/utils.js
Source/WebCore/ChangeLog
Source/WebCore/dom/Element.cpp

index 4c764f8..e2ff707 100644 (file)
@@ -1,3 +1,26 @@
+2019-12-22  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [macCatalyst] Mouse clicks dispatch duplicate pointerup and pointerdown events
+        https://bugs.webkit.org/show_bug.cgi?id=205551
+        <rdar://problem/58058268>
+
+        Reviewed by Tim Horton.
+
+        * pointerevents/ios/pointer-events-with-click-handler-expected.txt: Added.
+        * pointerevents/ios/pointer-events-with-click-handler.html: Added.
+
+        Add a layout test to verify that the bug does not occur. While this is a macCatalyst fix, this test needs to be
+        in the `ios` directory for now because macCatalyst is still considered "iOS family". This test is also still
+        relevant to both platforms (on iOS, synthesizing a tap behaves as expected, and in macCatalyst, it simulates a
+        click at the same location).
+
+        * pointerevents/utils.js:
+        (EventTracker.prototype.assertMatchesEvents):
+        (EventTracker):
+
+        Drive-by fix: flip the order of arguments to `assert_equals`, so that when tests fail, the failure output
+        correctly shows how many events were expected, and how many were observed.
+
 2019-12-22  Alexey Proskuryakov  <ap@apple.com>
 
         Add TextExpectations for flaky whlsl tests.
diff --git a/LayoutTests/pointerevents/ios/pointer-events-with-click-handler-expected.txt b/LayoutTests/pointerevents/ios/pointer-events-with-click-handler-expected.txt
new file mode 100644 (file)
index 0000000..b23c211
--- /dev/null
@@ -0,0 +1,3 @@
+
+PASS Verifies that a single pair of pointerup and pointerdown events are dispatched in an element with a click event handler. 
+
diff --git a/LayoutTests/pointerevents/ios/pointer-events-with-click-handler.html b/LayoutTests/pointerevents/ios/pointer-events-with-click-handler.html
new file mode 100644 (file)
index 0000000..8b1f389
--- /dev/null
@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <meta charset="utf-8">
+    <meta name="viewport" content="width=device-width, initial-scale=1">
+</head>
+<body>
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+<script src="../utils.js"></script>
+<script>
+'use strict';
+
+target_test((target, test) => {
+    const eventTracker = new EventTracker(target, ["pointerdown", "pointerup", "click"]);
+    ui.tap({ x: 50, y: 50 }).then(() => {
+        eventTracker.assertMatchesEvents([
+            { type: "pointerdown" },
+            { type: "pointerup" },
+            { type: "click" }
+        ]);
+        test.done();
+    });
+}, "Verifies that a single pair of pointerup and pointerdown events are dispatched in an element with a click event handler.");
+</script>
+</body>
+</html>
\ No newline at end of file
index e0c033d..35da10b 100644 (file)
@@ -98,12 +98,12 @@ class EventTracker
     assertMatchesEvents(expectedEvents)
     {
         assert_true(!!this.events.length, "Event tracker saw some events.");
-        assert_equals(expectedEvents.length, this.events.length, "Expected events and actual events have the same length.");
+        assert_equals(this.events.length, expectedEvents.length, "Expected events and actual events have the same length.");
         for (let i = 0; i < expectedEvents.length; ++i) {
             const expectedEvent = expectedEvents[i];
             const actualEvent = this.events[i];
             for (let property of Object.getOwnPropertyNames(expectedEvent))
-                assert_equals(expectedEvent[property], actualEvent[property], `Property ${property} matches for event at index ${i}.`);
+                assert_equals(actualEvent[property], expectedEvent[property], `Property ${property} matches for event at index ${i}.`);
         }
     }
 }
index 7fc3540..eeaf284 100644 (file)
@@ -1,3 +1,35 @@
+2019-12-22  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [macCatalyst] Mouse clicks dispatch duplicate pointerup and pointerdown events
+        https://bugs.webkit.org/show_bug.cgi?id=205551
+        <rdar://problem/58058268>
+
+        Reviewed by Tim Horton.
+
+        This began occuring after r251320, wherein some mouse event handling codepaths were enabled in macCatalyst.
+        For compatibility, gesture recognizers still fire in the macCatalyst platform. This includes the synthetic click
+        gesture, which will still synthesize and send mouseup and mousedown events to the page. After the change, this
+        results in pointer events being dispatched under the call to `shouldIgnoreMouseEvent()`. However, at the same
+        time, touch event handling codepaths have already dispatched "pointerup" and "pointerdown", so we end up with
+        redundant events.
+
+        To fix this macCatalyst-specific bug, simply avoid dispatching pointer events in the case where the synthetic
+        click type is some kind of tap gesture; in this case, pointer events have already been dispatched, so we don't
+        need to dispatch them again via mouse event handling code.
+
+        Test: pointerevents/ios/pointer-events-with-click-handler.html
+
+        * dom/Element.cpp:
+        (WebCore::dispatchPointerEventIfNeeded):
+
+        Also rename shouldIgnoreMouseEvent to dispatchPointerEventIfNeeded to better reflect that this function's
+        primary purposee is to dispatch pointer events in response to platform mouse events; then, change the return
+        value to an explicit enum class indicating whether the mouse event should be subsequently ignored (as a result
+        of the page preventing the dispatched pointer event).
+
+        (WebCore::Element::dispatchMouseEvent):
+        (WebCore::shouldIgnoreMouseEvent): Deleted.
+
 2019-12-22  Antti Koivisto  <antti@apple.com>
 
         Invalidate only affected elements after media query evaluation changes
index 70d9e1b..3796204 100644 (file)
@@ -79,6 +79,7 @@
 #include "MutationObserverInterestGroup.h"
 #include "MutationRecord.h"
 #include "NodeRenderStyle.h"
+#include "PlatformMouseEvent.h"
 #include "PlatformWheelEvent.h"
 #include "PointerCaptureController.h"
 #include "PointerEvent.h"
@@ -312,7 +313,8 @@ static bool isCompatibilityMouseEvent(const MouseEvent& mouseEvent)
 }
 #endif
 
-static bool shouldIgnoreMouseEvent(Element& element, const MouseEvent& mouseEvent, const PlatformMouseEvent& platformEvent, bool& didNotSwallowEvent)
+enum class ShouldIgnoreMouseEvent : bool { No, Yes };
+static ShouldIgnoreMouseEvent dispatchPointerEventIfNeeded(Element& element, const MouseEvent& mouseEvent, const PlatformMouseEvent& platformEvent, bool& didNotSwallowEvent)
 {
 #if ENABLE(POINTER_EVENTS)
     if (RuntimeEnabledFeatures::sharedFeatures().pointerEventsEnabled()) {
@@ -320,18 +322,21 @@ static bool shouldIgnoreMouseEvent(Element& element, const MouseEvent& mouseEven
             auto& pointerCaptureController = page->pointerCaptureController();
 #if ENABLE(TOUCH_EVENTS)
             if (platformEvent.pointerId() != mousePointerID && mouseEvent.type() != eventNames().clickEvent && pointerCaptureController.preventsCompatibilityMouseEventsForIdentifier(platformEvent.pointerId()))
-                return true;
+                return ShouldIgnoreMouseEvent::Yes;
 #else
             UNUSED_PARAM(platformEvent);
 #endif
+            if (platformEvent.syntheticClickType() != NoTap)
+                return ShouldIgnoreMouseEvent::No;
+
             if (auto pointerEvent = pointerCaptureController.pointerEventForMouseEvent(mouseEvent)) {
                 pointerCaptureController.dispatchEvent(*pointerEvent, &element);
                 if (isCompatibilityMouseEvent(mouseEvent) && pointerCaptureController.preventsCompatibilityMouseEventsForIdentifier(pointerEvent->pointerId()))
-                    return true;
+                    return ShouldIgnoreMouseEvent::Yes;
                 if (pointerEvent->defaultPrevented() || pointerEvent->defaultHandled()) {
                     didNotSwallowEvent = false;
                     if (pointerEvent->type() == eventNames().pointerdownEvent)
-                        return true;
+                        return ShouldIgnoreMouseEvent::Yes;
                 }
             }
         }
@@ -343,7 +348,7 @@ static bool shouldIgnoreMouseEvent(Element& element, const MouseEvent& mouseEven
     UNUSED_PARAM(didNotSwallowEvent);
 #endif
 
-    return false;
+    return ShouldIgnoreMouseEvent::No;
 }
 
 bool Element::dispatchMouseEvent(const PlatformMouseEvent& platformEvent, const AtomString& eventType, int detail, Element* relatedTarget)
@@ -361,7 +366,7 @@ bool Element::dispatchMouseEvent(const PlatformMouseEvent& platformEvent, const
 
     bool didNotSwallowEvent = true;
 
-    if (shouldIgnoreMouseEvent(*this, mouseEvent.get(), platformEvent, didNotSwallowEvent))
+    if (dispatchPointerEventIfNeeded(*this, mouseEvent.get(), platformEvent, didNotSwallowEvent) == ShouldIgnoreMouseEvent::Yes)
         return false;
 
     ASSERT(!mouseEvent->target() || mouseEvent->target() != relatedTarget);