Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Commit 56c1246

Browse files
yjbanovyjbanov
committed
reland click disambiguation with fix for nested tappables
1 parent b4ea656 commit 56c1246

File tree

6 files changed

+123
-25
lines changed

6 files changed

+123
-25
lines changed

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

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -272,8 +272,7 @@ class ClickDebouncer {
272272
// recently and if the node is currently listening to event, forward to
273273
// the framework.
274274
if (isListening && _shouldSendClickEventToFramework(click)) {
275-
EnginePlatformDispatcher.instance.invokeOnSemanticsAction(
276-
semanticsNodeId, ui.SemanticsAction.tap, null);
275+
_sendSemanticsTapToFramework(click, semanticsNodeId);
277276
}
278277
return;
279278
}
@@ -285,8 +284,7 @@ class ClickDebouncer {
285284
final DebounceState state = _state!;
286285
_state = null;
287286
state.timer.cancel();
288-
EnginePlatformDispatcher.instance.invokeOnSemanticsAction(
289-
semanticsNodeId, ui.SemanticsAction.tap, null);
287+
_sendSemanticsTapToFramework(click, semanticsNodeId);
290288
} else {
291289
// The semantic node is not listening to taps. Flush the pointer events
292290
// for the framework to figure out what to do with them. It's possible
@@ -295,6 +293,20 @@ class ClickDebouncer {
295293
}
296294
}
297295

296+
void _sendSemanticsTapToFramework(DomEvent click, int semanticsNodeId) {
297+
// Tappable nodes can be nested inside other tappable nodes. If a click
298+
// lands on an inner element and is allowed to propagate, it will also
299+
// land on the ancestor tappable, leading to both the descendant and the
300+
// ancestor sending SemanticsAction.tap to the framework, creating a double
301+
// tap/click, which is wrong. More details:
302+
//
303+
// https://github.com/flutter/flutter/issues/134842
304+
click.stopPropagation();
305+
306+
EnginePlatformDispatcher.instance.invokeOnSemanticsAction(
307+
semanticsNodeId, ui.SemanticsAction.tap, null);
308+
}
309+
298310
void _startDebouncing(DomEvent event, List<ui.PointerData> data) {
299311
assert(
300312
_state == null,

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -426,6 +426,7 @@ abstract class PrimaryRoleManager {
426426
/// Initializes a role for a [semanticsObject] that includes basic
427427
/// functionality for focus, labels, live regions, and route names.
428428
PrimaryRoleManager.withBasics(this.role, this.semanticsObject) {
429+
element = _initElement(createElement(), semanticsObject);
429430
addFocusManagement();
430431
addLiveRegion();
431432
addRouteName();
@@ -438,9 +439,11 @@ abstract class PrimaryRoleManager {
438439
/// Use this constructor for highly specialized cases where
439440
/// [RoleManager.withBasics] does not work, for example when the default focus
440441
/// management intereferes with the widget's functionality.
441-
PrimaryRoleManager.blank(this.role, this.semanticsObject);
442+
PrimaryRoleManager.blank(this.role, this.semanticsObject) {
443+
element = _initElement(createElement(), semanticsObject);
444+
}
442445

443-
late final DomElement element = _initElement(createElement(), semanticsObject);
446+
late final DomElement element;
444447

445448
/// The primary role identifier.
446449
final PrimaryRole role;

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ class Tappable extends RoleManager {
3939
_isListening,
4040
);
4141
});
42-
semanticsObject.element.addEventListener('click', _clickListener);
42+
owner.element.addEventListener('click', _clickListener);
4343
}
4444

4545
DomEventListener? _clickListener;
@@ -60,9 +60,9 @@ class Tappable extends RoleManager {
6060
// contract is that the element that has this attribute is also the element
6161
// that receives pointer and "click" events.
6262
if (_isListening) {
63-
semanticsObject.element.setAttribute('flt-tappable', '');
63+
owner.element.setAttribute('flt-tappable', '');
6464
} else {
65-
semanticsObject.element.removeAttribute('flt-tappable');
65+
owner.element.removeAttribute('flt-tappable');
6666
}
6767
}
6868

‎lib/web_ui/test/common/matchers.dart

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -227,23 +227,23 @@ enum HtmlComparisonMode {
227227

228228
/// Rewrites [htmlContent] by removing irrelevant style attributes.
229229
///
230-
/// If [throwOnUnusedAttributes] is `true`, throws instead of rewriting. Set
231-
/// [throwOnUnusedAttributes] to `true` to check that expected HTML strings do
230+
/// If [throwOnUnusedStyleProperties] is `true`, throws instead of rewriting. Set
231+
/// [throwOnUnusedStyleProperties] to `true` to check that expected HTML strings do
232232
/// not contain irrelevant attributes. It is ok for actual HTML to contain all
233233
/// kinds of attributes. They only need to be filtered out before testing.
234234
String canonicalizeHtml(
235235
String htmlContent, {
236236
HtmlComparisonMode mode = HtmlComparisonMode.nonLayoutOnly,
237-
bool throwOnUnusedAttributes = false,
238-
List<String>? ignoredAttributes,
237+
bool throwOnUnusedStyleProperties = false,
238+
List<String>? ignoredStyleProperties,
239239
}) {
240240
if (htmlContent.trim().isEmpty) {
241241
return '';
242242
}
243243

244-
String? unusedAttribute(String name) {
245-
if (throwOnUnusedAttributes) {
246-
fail('Provided HTML contains style attribute "$name" which '
244+
String? unusedStyleProperty(String name) {
245+
if (throwOnUnusedStyleProperties) {
246+
fail('Provided HTML contains style property "$name" which '
247247
'is not used for comparison in the test. The HTML was:\n\n$htmlContent');
248248
}
249249

@@ -329,7 +329,7 @@ String canonicalizeHtml(
329329
if (parts.length == 2) {
330330
final String name = parts.first;
331331

332-
if (ignoredAttributes != null && ignoredAttributes.contains(name)) {
332+
if (ignoredStyleProperties != null && ignoredStyleProperties.contains(name)) {
333333
return null;
334334
}
335335

@@ -343,7 +343,7 @@ String canonicalizeHtml(
343343
].contains(name);
344344

345345
if (isStaticAttribute) {
346-
return unusedAttribute(name);
346+
return unusedStyleProperty(name);
347347
}
348348

349349
// Whether the attribute is set by the layout system.
@@ -363,7 +363,7 @@ String canonicalizeHtml(
363363

364364
if (forLayout && !isLayoutAttribute ||
365365
!forLayout && isLayoutAttribute) {
366-
return unusedAttribute(name);
366+
return unusedStyleProperty(name);
367367
}
368368
}
369369
}
@@ -378,7 +378,7 @@ String canonicalizeHtml(
378378
replacement.attributes['style'] = processedAttributes;
379379
}
380380
}
381-
} else if (throwOnUnusedAttributes && original.attributes.isNotEmpty) {
381+
} else if (throwOnUnusedStyleProperties && original.attributes.isNotEmpty) {
382382
fail('Provided HTML contains attributes. However, the comparison mode '
383383
'is $mode. The HTML was:\n\n$htmlContent');
384384
}
@@ -414,7 +414,7 @@ String canonicalizeHtml(
414414
void expectHtml(DomElement element, String expectedHtml,
415415
{HtmlComparisonMode mode = HtmlComparisonMode.nonLayoutOnly}) {
416416
expectedHtml =
417-
canonicalizeHtml(expectedHtml, mode: mode, throwOnUnusedAttributes: true);
417+
canonicalizeHtml(expectedHtml, mode: mode, throwOnUnusedStyleProperties: true);
418418
final String actualHtml = canonicalizeHtml(element.outerHTML!, mode: mode);
419419
expect(actualHtml, expectedHtml);
420420
}

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

Lines changed: 85 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -433,7 +433,7 @@ void _testEngineSemanticsOwner() {
433433
expectSemanticsTree('''
434434
<sem style="$rootSemanticStyle">
435435
<sem-c>
436-
<sem aria-label="Hello"></sem>
436+
<sem aria-label="Hello" role="text"></sem>
437437
</sem-c>
438438
</sem>''');
439439

@@ -448,7 +448,7 @@ void _testEngineSemanticsOwner() {
448448
expectSemanticsTree('''
449449
<sem style="$rootSemanticStyle">
450450
<sem-c>
451-
<a aria-label="Hello" role="button" style="display: block;"></a>
451+
<a aria-label="Hello" style="display: block;"></a>
452452
</sem-c>
453453
</sem>''');
454454
expect(existingParent, tree[1]!.element.parent);
@@ -2111,6 +2111,89 @@ void _testTappable() {
21112111

21122112
semantics().semanticsEnabled = false;
21132113
});
2114+
2115+
// Regression test for: https://github.com/flutter/flutter/issues/134842
2116+
//
2117+
// If the click event is allowed to propagate through the hierarchy, then both
2118+
// the descendant and the parent will generate a SemanticsAction.tap, causing
2119+
// a double-tap to happen on the framework side.
2120+
test('inner tappable overrides ancestor tappable', () async {
2121+
semantics()
2122+
..debugOverrideTimestampFunction(() => _testTime)
2123+
..semanticsEnabled = true;
2124+
2125+
final List<CapturedAction> capturedActions = <CapturedAction>[];
2126+
EnginePlatformDispatcher.instance.onSemanticsActionEvent = (ui.SemanticsActionEvent event) {
2127+
capturedActions.add((event.nodeId, event.type, event.arguments));
2128+
};
2129+
2130+
final SemanticsTester tester = SemanticsTester(semantics());
2131+
tester.updateNode(
2132+
id: 0,
2133+
isFocusable: true,
2134+
hasTap: true,
2135+
hasEnabledState: true,
2136+
isEnabled: true,
2137+
isButton: true,
2138+
rect: const ui.Rect.fromLTRB(0, 0, 100, 50),
2139+
children: <SemanticsNodeUpdate>[
2140+
tester.updateNode(
2141+
id: 1,
2142+
isFocusable: true,
2143+
hasTap: true,
2144+
hasEnabledState: true,
2145+
isEnabled: true,
2146+
isButton: true,
2147+
rect: const ui.Rect.fromLTRB(0, 0, 100, 50),
2148+
),
2149+
],
2150+
);
2151+
tester.apply();
2152+
2153+
expectSemanticsTree('''
2154+
<sem flt-tappable role="button" style="$rootSemanticStyle">
2155+
<sem-c>
2156+
<sem flt-tappable role="button"></sem>
2157+
</sem-c>
2158+
</sem>
2159+
''');
2160+
2161+
// Tap on the outer element
2162+
{
2163+
final DomElement element = tester.getSemanticsObject(0).element;
2164+
final DomRect rect = element.getBoundingClientRect();
2165+
2166+
element.dispatchEvent(createDomMouseEvent('click', <Object?, Object?>{
2167+
'clientX': (rect.left + (rect.right - rect.left) / 2).floor(),
2168+
'clientY': (rect.top + (rect.bottom - rect.top) / 2).floor(),
2169+
}));
2170+
2171+
expect(capturedActions, <CapturedAction>[
2172+
(0, ui.SemanticsAction.tap, null),
2173+
]);
2174+
}
2175+
2176+
// Tap on the inner element
2177+
{
2178+
capturedActions.clear();
2179+
final DomElement element = tester.getSemanticsObject(1).element;
2180+
final DomRect rect = element.getBoundingClientRect();
2181+
2182+
element.dispatchEvent(createDomMouseEvent('click', <Object?, Object?>{
2183+
'bubbles': true,
2184+
'clientX': (rect.left + (rect.right - rect.left) / 2).floor(),
2185+
'clientY': (rect.top + (rect.bottom - rect.top) / 2).floor(),
2186+
}));
2187+
2188+
// The click on the inner element should not propagate to the parent to
2189+
// avoid sending a second SemanticsAction.tap action to the framework.
2190+
expect(capturedActions, <CapturedAction>[
2191+
(1, ui.SemanticsAction.tap, null),
2192+
]);
2193+
}
2194+
2195+
semantics().semanticsEnabled = false;
2196+
});
21142197
}
21152198

21162199
void _testImage() {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -353,9 +353,9 @@ class SemanticsTester {
353353

354354
/// Verifies the HTML structure of the current semantics tree.
355355
void expectSemanticsTree(String semanticsHtml) {
356-
const List<String> ignoredAttributes = <String>['pointer-events'];
356+
const List<String> ignoredStyleProperties = <String>['pointer-events'];
357357
expect(
358-
canonicalizeHtml(rootElement.querySelector('flt-semantics')!.outerHTML!, ignoredAttributes: ignoredAttributes),
358+
canonicalizeHtml(rootElement.querySelector('flt-semantics')!.outerHTML!, ignoredStyleProperties: ignoredStyleProperties),
359359
canonicalizeHtml(semanticsHtml),
360360
);
361361
}

0 commit comments

Comments
 (0)