[web] fix clicks on merged semantic nodes (attempt #2)

[yjbanov]

This relands #43620 with a fix for nested tappable nodes. The first PR introduced this regression: flutter/flutter#134842.

This PR includes the original PR and a fix for the regression. The fix is to call stopPropagation on the "click" event so that it is not handled by the ancestor if the child has already decided to send a SemanticsAction.tap to the framework. This ensures that there cannot be more than one SemanticsAction.tap sent to the framework.

Fixes flutter/flutter#134842

[ditman]

LGTM.

Added a small note in case the EventTarget generating the click/tap events may have multiple listeners (not just bubble up)

[ditman]

Another way I've seen this handled (in jQuery, admittedly) is with stopImmediatePropagation which was a "flag for the framework" so other listeners attached to the same EventTarget wouldn't run (even without bubbling). (IIRC this was just a boolean flag written into the event object itself that other bits of the framework knew how to check).

We may need something like this in Flutter web, now that we're juggling potentially multiple sets of listeners into the same dom element? maybe?

[yjbanov]

Huh, TIL! Looks like it's a standard API, not just jQuery. In this case we don't have multiple listeners on the same element. All the elements in question are created by the engine and we are attaching just one click listener (unless there's a bug, but that's a different story). So we only need to stop propagation to ancestor elements. Do you think we should still do something like this in this case?

[auto-submit]

auto label is removed for flutter/engine/47360, due to - The status or check suite Linux linux_web_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

[auto-submit]

auto label is removed for flutter/engine/47360, due to - The status or check suite Linux linux_android_debug_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

[p-mazhnik]

I revisited this PR after encountering issues with semantics on the web. Although it’s 8 months old, the code is still relevant, so I want to document here what I’ve found

[p-mazhnik]

This actually leads to double clicks :)
Consider the following scenario:

<flt-semantics flt-tappable id="flt-semantic-node-1">
  <flt-semantics id="flt-semantic-node-2"> </flt-semantics>
</flt-semantics>

If pointer events are triggered on flt-semantic-node-2, we send them to framework in this condition (flt-tappable is not present), and isDebouncing will be false.
On the other hand, click event on the flt-semantic-node-2 propagates to flt-semantic-node-1 that has flt-tappable and is also sent to framework (isDebouncing is false).

(flutter/flutter#147050 (comment) and most likely issue flutter/flutter#150020)

[yjbanov]

Yeah, I fixed one variant of this in #53694. Just using the event.target is probably naive. It should start with event.target and then walk up to the nearest tappable ancestor.

[p-mazhnik]

I wonder if it is possible that isDebouncing is true but _state.target is not equal to click.target

[yjbanov]

Due to the problem you pointed out above (the sketchy target detection), it seems quite possible.

[p-mazhnik]

This is not working for long taps (issue flutter/flutter#147050).

In long tap case, it is possible that pointerup event is fired after 200 milliseconds since pointerdown, and as a result isDebounce will be false when click event is received, leading to duplication.

In the previous solution (that is still exists in parallel), timer was added after pointerup (and others) as well, so it is probably should fix the issue

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

Lines 2232 to 2250 in e5215e6

	const List<String> pointerEventTypes = <String>[
	'pointerdown',
	'pointermove',
	'pointerleave',
	'pointerup',
	'pointercancel',
	'touchstart',
	'touchend',
	'touchmove',
	'touchcancel',
	'mousedown',
	'mousemove',
	'mouseleave',
	'mouseup',
	];
	if (pointerEventTypes.contains(event.type)) {
	_temporarilyDisableBrowserGestureMode();
	}

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

Lines 2184 to 2195 in e5215e6

	/// Disables browser gestures temporarily because pointer events were detected.
	///
	/// This is used to deduplicate gestures detected by Flutter and gestures
	/// detected by the browser. Flutter-detected gestures have higher precedence.
	void _temporarilyDisableBrowserGestureMode() {
	const Duration kDebounceThreshold = Duration(milliseconds: 500);
	_getGestureModeClock()!.datetime = _now().add(kDebounceThreshold);
	if (_gestureMode != GestureMode.pointerEvents) {
	_gestureMode = GestureMode.pointerEvents;
	_notifyGestureModeListeners();
	}
	}

[yjbanov]

This is correct, and it's on my radar to fix. This method was enough for the screen reader case. However, we're trying to make it so semantics can be always enabled, and therefore it must also work for all interactions without the screen reader. This code needs a revision.