[a11y][web] Selecting menu items triggers a back navigation #134842

Renzo-Olivares · 2023-09-15T20:39:40Z

Raised internally at b/299847356

Issue: Selecting a menu item with semantics enabled, for example through a pop up menu or dropdown menu, triggers a back navigation. This not only dismisses the menu, but also pops the next route from the navigator stack.

Expected: Selecting a menu item with semantics enabled dismisses the menu, but does not trigger another route to be popped from the navigator stack.

Bisected to engine roll: #131897
Bisected to engine commit: flutter/engine#43620

Minimal example for reproduction: https://dartpad.dev/?id=41045409a61245de472cb0083fc94742&channel=master

Screen.Recording.2023-09-15.at.1.44.04.PM.mov

Stack trace before change

dart-sdk/lib/_internal/js_dev_runtime/patch/core_patch.dart 941:28                get current
packages/flutter/src/material/popup_menu.dart 347:30                              handleTap
packages/flutter/src/material/ink_well.dart 1188:21                               handleTap
packages/flutter/src/gestures/recognizer.dart 275:24                              invokeCallback
packages/flutter/src/gestures/tap.dart 654:11                                     handleTapUp
packages/flutter/src/gestures/tap.dart 311:5                                      [_checkUp]
packages/flutter/src/gestures/tap.dart 244:7                                      handlePrimaryPointer
packages/flutter/src/gestures/recognizer.dart 630:9                               handleEvent
packages/flutter/src/gestures/pointer_router.dart 98:12                           [_dispatch]
packages/flutter/src/gestures/pointer_router.dart 143:9                           <fn>
dart-sdk/lib/_internal/js_dev_runtime/private/linked_hash_map.dart 21:13          forEach
packages/flutter/src/gestures/pointer_router.dart 141:17                          [_dispatchEventToRoutes]
packages/flutter/src/gestures/pointer_router.dart 127:7                           route
packages/flutter/src/gestures/binding.dart 488:19                                 handleEvent
packages/flutter/src/gestures/binding.dart 468:14                                 dispatchEvent
packages/flutter/src/rendering/binding.dart 439:11                                dispatchEvent
packages/flutter/src/gestures/binding.dart 413:7                                  [_handlePointerEventImmediately]
packages/flutter/src/gestures/binding.dart 376:5                                  handlePointerEvent
packages/flutter/src/gestures/binding.dart 323:7                                  [_flushPointerEventQueue]
packages/flutter/src/gestures/binding.dart 292:9                                  [_handlePointerDataPacket]
lib/_engine/engine/platform_dispatcher.dart 1295:13                               invoke1
lib/_engine/engine/platform_dispatcher.dart 273:5                                 invokeOnPointerDataPacket
lib/_engine/engine/pointer_binding.dart 168:39                                    [_onPointerData]
lib/_engine/engine/pointer_binding.dart 791:20                                    <fn>
lib/_engine/engine/pointer_binding.dart 720:14                                    <fn>
lib/_engine/engine/pointer_binding.dart 317:16                                    loggedHandler
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart 574:37  _checkAndCall
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart 579:39  dcall

Stack trace after change

dart-sdk/lib/_internal/js_dev_runtime/patch/core_patch.dart 941:28                get current
packages/flutter/src/widgets/navigator.dart 2525:30                               maybePop
packages/flutter/src/widgets/modal_barrier.dart 228:21                            handleDismiss
packages/flutter/src/rendering/proxy_box.dart 4492:24                             [_performTap]
packages/flutter/src/semantics/semantics.dart 3541:14                             <fn>
packages/flutter/src/semantics/semantics.dart 3358:14                             performAction
packages/flutter/src/rendering/binding.dart 447:64                                performSemanticsAction
packages/flutter/src/semantics/binding.dart 119:5                                 [_handleSemanticsActionEvent]
lib/_engine/engine/platform_dispatcher.dart 1295:13                               invoke1
lib/_engine/engine/platform_dispatcher.dart 1173:5                                invokeOnSemanticsAction
lib/_engine/engine/pointer_binding.dart 263:43                                    onClick
lib/_engine/engine/semantics/tappable.dart 34:22                                  <fn>
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart 574:37  _checkAndCall
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart 579:39  dcall

dart-sdk/lib/_internal/js_dev_runtime/patch/core_patch.dart 941:28                get current
packages/flutter/src/material/popup_menu.dart 347:30                              handleTap
packages/flutter/src/material/ink_well.dart 1188:21                               handleTap
packages/flutter/src/material/ink_well.dart 861:5                                 simulateTap
packages/flutter/src/rendering/proxy_box.dart 4492:24                             [_performTap]
packages/flutter/src/semantics/semantics.dart 3539:14                             <fn>
packages/flutter/src/semantics/semantics.dart 3357:14                             performAction
packages/flutter/src/rendering/binding.dart 447:64                                performSemanticsAction

From the stack trace it looks like a second semantics action is being dispatched and that is causing a second route to be popped from the navigator stack. The first pop that happens in PopUpMenu is expected

flutter/packages/flutter/lib/src/material/popup_menu.dart

Lines 341 to 346 in 367f9ea

 void handleTap() { 

 // Need to pop the navigator first in case onTap may push new route onto navigator. 

 Navigator.pop<T>(context, widget.value); 

 widget.onTap?.call(); 

 }

, but the second pop that happens in ModalBarrier

flutter/packages/flutter/lib/src/widgets/modal_barrier.dart

Lines 223 to 233 in 367f9ea

 void handleDismiss() { 

 if (dismissible) { 

 if (onDismiss != null) { 

 onDismiss!(); 

 } else { 

 Navigator.maybePop(context); 

 } 

 } else { 

 SystemSound.play(SystemSoundType.alert); 

 } 

 }

is not expected.

The text was updated successfully, but these errors were encountered:

yjbanov · 2023-09-15T23:17:11Z

A few observations:

Root cause

The root cause is that semantics tree contains nested clickable/tappable nodes:

With such a DOM tree when a descendant receives a DOM click event, its ancestor also receives it. This leads to two SemanticsAction.tap events on the framework side, which results in two routes popped instead of one.

The bug is old

The bug is in the semantics, and it predates this manifestation probably by many stable releases. What happened recently was that what was previously delivered via gesture recognition is now delivered via semantics action, and since the bug is in the semantics it now manifests itself in a new situation.

The fix

This should be fixable on the web engine side, but clarification is needed from the framework team on what the expected behaviors are in the presence of nested clickable nodes. However, it's also possible that the bug is in the framework. I imagine we don't have nested clickable widgets in the widget tree. My guess is the semantics merging logic ends up merging nodes in this way, and that may not be a good thing to do.

HansMuller · 2023-09-15T23:21:02Z

@chunhtai - please take a look at this

chunhtai · 2023-09-18T16:47:58Z

I imagine we don't have nested clickable widgets in the widget tree

We do, but we handle it using gesture arena. For tap gesture the inner most one will win.

With such a DOM tree when a descendant receives a DOM click event, its ancestor also receives it

Should whichever receives the click that also has semantics tap prevent the event from bubbling?

jiahaog · 2023-09-18T23:54:58Z

Updating the priority to match the internal bug mapping

yjbanov · 2023-09-19T19:42:31Z

I'm reverting the fix for the merged clicks for now.

Should whichever receives the click that also has semantics tap prevent the event from bubbling?

I'm going to have to test this. One issue I've seen before (which is related to the PR that's being reverted) is that screen readers like to click in the middle of things. If we have overlapping nested clickables, I'm not sure if the screen reader would still end up clicking on the child anyway.

chunhtai · 2023-09-19T20:34:11Z

Yes that is something we should test, if they issue a really click instead of calling the onClick method on the div, this issue probably is not limited to flutter.

This reverts commit 0c1de9b. The commit caused flutter/flutter#134842. I'm going to try again, this time accounting for nested clickables/tappables.

yjbanov · 2023-09-25T19:01:05Z

Dropping to P1 as the PR that exacerbated the issue has been reverted.

This reverts commit 0c1de9b. The commit caused flutter/flutter#134842. I'm going to try again, this time accounting for nested clickables/tappables.

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

github-actions · 2023-11-22T05:00:35Z

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

Renzo-Olivares added the c: regression It was better in the past than it is now label Sep 15, 2023

jiahaog added P0 Critical issues such as a build break or regression and removed P1 High-priority issues at the top of the work list labels Sep 18, 2023

HansMuller assigned yjbanov Sep 19, 2023

yjbanov mentioned this issue Sep 19, 2023

Revert "[web] fix clicks on merged semantic nodes (#43620)" flutter/engine#46067

Merged

yjbanov added P1 High-priority issues at the top of the work list and removed P0 Critical issues such as a build break or regression labels Sep 25, 2023

yjbanov mentioned this issue Oct 26, 2023

[web] fix clicks on merged semantic nodes (attempt #2) flutter/engine#47360

Merged

auto-submit bot closed this as completed in flutter/engine#47360 Nov 8, 2023

github-actions bot locked as resolved and limited conversation to collaborators Nov 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[a11y][web] Selecting menu items triggers a back navigation #134842

[a11y][web] Selecting menu items triggers a back navigation #134842

Renzo-Olivares commented Sep 15, 2023 •

edited

Loading

yjbanov commented Sep 15, 2023

HansMuller commented Sep 15, 2023

chunhtai commented Sep 18, 2023

jiahaog commented Sep 18, 2023

yjbanov commented Sep 19, 2023

chunhtai commented Sep 19, 2023 •

edited

Loading

yjbanov commented Sep 25, 2023

github-actions bot commented Nov 22, 2023

[a11y][web] Selecting menu items triggers a back navigation #134842

[a11y][web] Selecting menu items triggers a back navigation #134842

Comments

Renzo-Olivares commented Sep 15, 2023 • edited Loading

yjbanov commented Sep 15, 2023

Root cause

The bug is old

The fix

HansMuller commented Sep 15, 2023

chunhtai commented Sep 18, 2023

jiahaog commented Sep 18, 2023

yjbanov commented Sep 19, 2023

chunhtai commented Sep 19, 2023 • edited Loading

yjbanov commented Sep 25, 2023

github-actions bot commented Nov 22, 2023

Renzo-Olivares commented Sep 15, 2023 •

edited

Loading

chunhtai commented Sep 19, 2023 •

edited

Loading