From 56c1246a07906f707510452c9b91f7c1555895a7 Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Thu, 26 Oct 2023 13:29:49 -0700 Subject: [PATCH] reland click disambiguation with fix for nested tappables --- .../lib/src/engine/pointer_binding.dart | 20 ++++- .../lib/src/engine/semantics/semantics.dart | 7 +- .../lib/src/engine/semantics/tappable.dart | 6 +- lib/web_ui/test/common/matchers.dart | 24 ++--- .../test/engine/semantics/semantics_test.dart | 87 ++++++++++++++++++- .../engine/semantics/semantics_tester.dart | 4 +- 6 files changed, 123 insertions(+), 25 deletions(-) diff --git a/lib/web_ui/lib/src/engine/pointer_binding.dart b/lib/web_ui/lib/src/engine/pointer_binding.dart index 5d199b3f73a5e..85fca6d417605 100644 --- a/lib/web_ui/lib/src/engine/pointer_binding.dart +++ b/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 data) { assert( _state == null, diff --git a/lib/web_ui/lib/src/engine/semantics/semantics.dart b/lib/web_ui/lib/src/engine/semantics/semantics.dart index d700d58e5d7a2..b27c9233f10a2 100644 --- a/lib/web_ui/lib/src/engine/semantics/semantics.dart +++ b/lib/web_ui/lib/src/engine/semantics/semantics.dart @@ -426,6 +426,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(); @@ -438,9 +439,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; diff --git a/lib/web_ui/lib/src/engine/semantics/tappable.dart b/lib/web_ui/lib/src/engine/semantics/tappable.dart index 5adf4d0ec0807..d3612ae80348c 100644 --- a/lib/web_ui/lib/src/engine/semantics/tappable.dart +++ b/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'); } } diff --git a/lib/web_ui/test/common/matchers.dart b/lib/web_ui/test/common/matchers.dart index 169baaef8af95..51698843d66a8 100644 --- a/lib/web_ui/test/common/matchers.dart +++ b/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? ignoredAttributes, + bool throwOnUnusedStyleProperties = false, + List? 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); } diff --git a/lib/web_ui/test/engine/semantics/semantics_test.dart b/lib/web_ui/test/engine/semantics/semantics_test.dart index b5f1d96b11768..8534fe009b77c 100644 --- a/lib/web_ui/test/engine/semantics/semantics_test.dart +++ b/lib/web_ui/test/engine/semantics/semantics_test.dart @@ -433,7 +433,7 @@ void _testEngineSemanticsOwner() { expectSemanticsTree(''' - + '''); @@ -448,7 +448,7 @@ void _testEngineSemanticsOwner() { expectSemanticsTree(''' - + '''); expect(existingParent, tree[1]!.element.parent); @@ -2111,6 +2111,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 capturedActions = []; + 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: [ + 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(''' + + + + + +'''); + + // Tap on the outer element + { + final DomElement element = tester.getSemanticsObject(0).element; + final DomRect rect = element.getBoundingClientRect(); + + element.dispatchEvent(createDomMouseEvent('click', { + 'clientX': (rect.left + (rect.right - rect.left) / 2).floor(), + 'clientY': (rect.top + (rect.bottom - rect.top) / 2).floor(), + })); + + expect(capturedActions, [ + (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', { + '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, [ + (1, ui.SemanticsAction.tap, null), + ]); + } + + semantics().semanticsEnabled = false; + }); } void _testImage() { diff --git a/lib/web_ui/test/engine/semantics/semantics_tester.dart b/lib/web_ui/test/engine/semantics/semantics_tester.dart index f0c61e92ca541..8a40c918f6d42 100644 --- a/lib/web_ui/test/engine/semantics/semantics_tester.dart +++ b/lib/web_ui/test/engine/semantics/semantics_tester.dart @@ -353,9 +353,9 @@ class SemanticsTester { /// Verifies the HTML structure of the current semantics tree. void expectSemanticsTree(String semanticsHtml) { - const List ignoredAttributes = ['pointer-events']; + const List ignoredStyleProperties = ['pointer-events']; expect( - canonicalizeHtml(rootElement.querySelector('flt-semantics')!.outerHTML!, ignoredAttributes: ignoredAttributes), + canonicalizeHtml(rootElement.querySelector('flt-semantics')!.outerHTML!, ignoredStyleProperties: ignoredStyleProperties), canonicalizeHtml(semanticsHtml), ); }