reland click disambiguation with fix for nested tappables

Author: yjbanov

lib/web_ui/lib/src/engine/pointer_binding.dart

@@ -272,8 +272,7 @@ class ClickDebouncer {
       // recently and if the node is currently listening to event, forward to
       // the framework.
       if (isListening && _shouldSendClickEventToFramework(click)) {
-        EnginePlatformDispatcher.instance.invokeOnSemanticsAction(
-            semanticsNodeId, ui.SemanticsAction.tap, null);
+        _sendSemanticsTapToFramework(click, semanticsNodeId);
       }
       return;
     }
@@ -285,8 +284,7 @@ class ClickDebouncer {
       final DebounceState state = _state!;
       _state = null;
       state.timer.cancel();
-      EnginePlatformDispatcher.instance.invokeOnSemanticsAction(
-          semanticsNodeId, ui.SemanticsAction.tap, null);
+      _sendSemanticsTapToFramework(click, semanticsNodeId);
     } else {
       // The semantic node is not listening to taps. Flush the pointer events
       // for the framework to figure out what to do with them. It's possible
@@ -295,6 +293,20 @@ class ClickDebouncer {
     }
   }
 
+  void _sendSemanticsTapToFramework(DomEvent click, int semanticsNodeId) {
+    // Tappable nodes can be nested inside other tappable nodes. If a click
+    // lands on an inner element and is allowed to propagate, it will also
+    // land on the ancestor tappable, leading to both the descendant and the
+    // ancestor sending SemanticsAction.tap to the framework, creating a double
+    // tap/click, which is wrong. More details:
+    //
+    // https://github.com/flutter/flutter/issues/134842
+    click.stopPropagation();
+
+    EnginePlatformDispatcher.instance.invokeOnSemanticsAction(
+        semanticsNodeId, ui.SemanticsAction.tap, null);
+  }
+
   void _startDebouncing(DomEvent event, List<ui.PointerData> data) {
     assert(
       _state == null,

lib/web_ui/lib/src/engine/semantics/semantics.dart

@@ -427,6 +427,7 @@ abstract class PrimaryRoleManager {
   /// Initializes a role for a [semanticsObject] that includes basic
   /// functionality for focus, labels, live regions, and route names.
   PrimaryRoleManager.withBasics(this.role, this.semanticsObject) {
+    element = _initElement(createElement(), semanticsObject);
     addFocusManagement();
     addLiveRegion();
     addRouteName();
@@ -439,9 +440,11 @@ abstract class PrimaryRoleManager {
   /// Use this constructor for highly specialized cases where
   /// [RoleManager.withBasics] does not work, for example when the default focus
   /// management intereferes with the widget's functionality.
-  PrimaryRoleManager.blank(this.role, this.semanticsObject);
+  PrimaryRoleManager.blank(this.role, this.semanticsObject) {
+    element = _initElement(createElement(), semanticsObject);
+  }
 
-  late final DomElement element = _initElement(createElement(), semanticsObject);
+  late final DomElement element;
 
   /// The primary role identifier.
   final PrimaryRole role;

lib/web_ui/lib/src/engine/semantics/tappable.dart

@@ -39,7 +39,7 @@ class Tappable extends RoleManager {
         _isListening,
       );
     });
-    semanticsObject.element.addEventListener('click', _clickListener);
+    owner.element.addEventListener('click', _clickListener);
   }
 
   DomEventListener? _clickListener;
@@ -60,9 +60,9 @@ class Tappable extends RoleManager {
     // contract is that the element that has this attribute is also the element
     // that receives pointer and "click" events.
     if (_isListening) {
-      semanticsObject.element.setAttribute('flt-tappable', '');
+      owner.element.setAttribute('flt-tappable', '');
     } else {
-      semanticsObject.element.removeAttribute('flt-tappable');
+      owner.element.removeAttribute('flt-tappable');
     }
   }
 

lib/web_ui/test/common/matchers.dart

@@ -227,23 +227,23 @@ enum HtmlComparisonMode {
 
 /// Rewrites [htmlContent] by removing irrelevant style attributes.
 ///
-/// If [throwOnUnusedAttributes] is `true`, throws instead of rewriting. Set
-/// [throwOnUnusedAttributes] to `true` to check that expected HTML strings do
+/// If [throwOnUnusedStyleProperties] is `true`, throws instead of rewriting. Set
+/// [throwOnUnusedStyleProperties] to `true` to check that expected HTML strings do
 /// not contain irrelevant attributes. It is ok for actual HTML to contain all
 /// kinds of attributes. They only need to be filtered out before testing.
 String canonicalizeHtml(
   String htmlContent, {
   HtmlComparisonMode mode = HtmlComparisonMode.nonLayoutOnly,
-  bool throwOnUnusedAttributes = false,
-  List<String>? ignoredAttributes,
+  bool throwOnUnusedStyleProperties = false,
+  List<String>? ignoredStyleProperties,
 }) {
   if (htmlContent.trim().isEmpty) {
     return '';
   }
 
-  String? unusedAttribute(String name) {
-    if (throwOnUnusedAttributes) {
-      fail('Provided HTML contains style attribute "$name" which '
+  String? unusedStyleProperty(String name) {
+    if (throwOnUnusedStyleProperties) {
+      fail('Provided HTML contains style property "$name" which '
           'is not used for comparison in the test. The HTML was:\n\n$htmlContent');
     }
 
@@ -329,7 +329,7 @@ String canonicalizeHtml(
                 if (parts.length == 2) {
                   final String name = parts.first;
 
-                  if (ignoredAttributes != null && ignoredAttributes.contains(name)) {
+                  if (ignoredStyleProperties != null && ignoredStyleProperties.contains(name)) {
                     return null;
                   }
 
@@ -343,7 +343,7 @@ String canonicalizeHtml(
                   ].contains(name);
 
                   if (isStaticAttribute) {
-                    return unusedAttribute(name);
+                    return unusedStyleProperty(name);
                   }
 
                   // Whether the attribute is set by the layout system.
@@ -363,7 +363,7 @@ String canonicalizeHtml(
 
                   if (forLayout && !isLayoutAttribute ||
                       !forLayout && isLayoutAttribute) {
-                    return unusedAttribute(name);
+                    return unusedStyleProperty(name);
                   }
                 }
               }
@@ -378,7 +378,7 @@ String canonicalizeHtml(
           replacement.attributes['style'] = processedAttributes;
         }
       }
-    } else if (throwOnUnusedAttributes && original.attributes.isNotEmpty) {
+    } else if (throwOnUnusedStyleProperties && original.attributes.isNotEmpty) {
       fail('Provided HTML contains attributes. However, the comparison mode '
           'is $mode. The HTML was:\n\n$htmlContent');
     }
@@ -414,7 +414,7 @@ String canonicalizeHtml(
 void expectHtml(DomElement element, String expectedHtml,
     {HtmlComparisonMode mode = HtmlComparisonMode.nonLayoutOnly}) {
   expectedHtml =
-      canonicalizeHtml(expectedHtml, mode: mode, throwOnUnusedAttributes: true);
+      canonicalizeHtml(expectedHtml, mode: mode, throwOnUnusedStyleProperties: true);
   final String actualHtml = canonicalizeHtml(element.outerHTML!, mode: mode);
   expect(actualHtml, expectedHtml);
 }

lib/web_ui/test/engine/semantics/semantics_test.dart

@@ -428,7 +428,7 @@ void _testEngineSemanticsOwner() {
     expectSemanticsTree('''
 <sem style="$rootSemanticStyle">
   <sem-c>
-    <sem aria-label="Hello"></sem>
+    <sem aria-label="Hello" role="text"></sem>
   </sem-c>
 </sem>''');
 
@@ -443,7 +443,7 @@ void _testEngineSemanticsOwner() {
     expectSemanticsTree('''
 <sem style="$rootSemanticStyle">
   <sem-c>
-    <a aria-label="Hello" role="button" style="display: block;"></a>
+    <a aria-label="Hello" style="display: block;"></a>
   </sem-c>
 </sem>''');
     expect(existingParent, tree[1]!.element.parent);
@@ -2106,6 +2106,89 @@ void _testTappable() {
 
     semantics().semanticsEnabled = false;
   });
+
+  // Regression test for: https://github.com/flutter/flutter/issues/134842
+  //
+  // If the click event is allowed to propagate through the hierarchy, then both
+  // the descendant and the parent will generate a SemanticsAction.tap, causing
+  // a double-tap to happen on the framework side.
+  test('inner tappable overrides ancestor tappable', () async {
+    semantics()
+      ..debugOverrideTimestampFunction(() => _testTime)
+      ..semanticsEnabled = true;
+
+    final List<CapturedAction> capturedActions = <CapturedAction>[];
+    EnginePlatformDispatcher.instance.onSemanticsActionEvent = (ui.SemanticsActionEvent event) {
+      capturedActions.add((event.nodeId, event.type, event.arguments));
+    };
+
+    final SemanticsTester tester = SemanticsTester(semantics());
+    tester.updateNode(
+      id: 0,
+      isFocusable: true,
+      hasTap: true,
+      hasEnabledState: true,
+      isEnabled: true,
+      isButton: true,
+      rect: const ui.Rect.fromLTRB(0, 0, 100, 50),
+      children: <SemanticsNodeUpdate>[
+        tester.updateNode(
+          id: 1,
+          isFocusable: true,
+          hasTap: true,
+          hasEnabledState: true,
+          isEnabled: true,
+          isButton: true,
+          rect: const ui.Rect.fromLTRB(0, 0, 100, 50),
+        ),
+      ],
+    );
+    tester.apply();
+
+    expectSemanticsTree('''
+<sem flt-tappable role="button" style="$rootSemanticStyle">
+  <sem-c>
+    <sem flt-tappable role="button"></sem>
+  </sem-c>
+</sem>
+''');
+
+    // Tap on the outer element
+    {
+      final DomElement element = tester.getSemanticsObject(0).element;
+      final DomRect rect = element.getBoundingClientRect();
+
+      element.dispatchEvent(createDomMouseEvent('click', <Object?, Object?>{
+        'clientX': (rect.left + (rect.right - rect.left) / 2).floor(),
+        'clientY': (rect.top + (rect.bottom - rect.top) / 2).floor(),
+      }));
+
+      expect(capturedActions, <CapturedAction>[
+        (0, ui.SemanticsAction.tap, null),
+      ]);
+    }
+
+    // Tap on the inner element
+    {
+      capturedActions.clear();
+      final DomElement element = tester.getSemanticsObject(1).element;
+      final DomRect rect = element.getBoundingClientRect();
+
+      element.dispatchEvent(createDomMouseEvent('click', <Object?, Object?>{
+        'bubbles': true,
+        'clientX': (rect.left + (rect.right - rect.left) / 2).floor(),
+        'clientY': (rect.top + (rect.bottom - rect.top) / 2).floor(),
+      }));
+
+      // The click on the inner element should not propagate to the parent to
+      // avoid sending a second SemanticsAction.tap action to the framework.
+      expect(capturedActions, <CapturedAction>[
+        (1, ui.SemanticsAction.tap, null),
+      ]);
+    }
+
+    semantics().semanticsEnabled = false;
+  });
 }
 
 void _testImage() {

lib/web_ui/test/engine/semantics/semantics_tester.dart

@@ -352,9 +352,9 @@ class SemanticsTester {
 
 /// Verifies the HTML structure of the current semantics tree.
 void expectSemanticsTree(String semanticsHtml) {
-  const List<String> ignoredAttributes = <String>['pointer-events'];
+  const List<String> ignoredStyleProperties = <String>['pointer-events'];
   expect(
-    canonicalizeHtml(appHostNode.querySelector('flt-semantics')!.outerHTML!, ignoredAttributes: ignoredAttributes),
+    canonicalizeHtml(appHostNode.querySelector('flt-semantics')!.outerHTML!, ignoredStyleProperties: ignoredStyleProperties),
     canonicalizeHtml(semanticsHtml),
   );
 }