Repository navigation
Revert: fix(widgets): dismiss open RawTooltip on Escape key (WCAG 1.4.13) - #193183
Conversation
….13) (flutter#192966)" This reverts commit f8c8551.
There was a problem hiding this comment.
Code Review
This pull request removes the global HardwareKeyboard event handler and the _handleKeyEvent method from RawTooltipState, which were used to dismiss tooltips when the Escape key was pressed. Additionally, the corresponding test case verifying this Escape key dismissal behavior has been removed. There are no review comments, so I have no feedback to provide.
|
Detected the If you add the The "Merge" button is also unlocked. To bypass presubmits as well as the tree status, press the GitHub "Add to Merge Queue". |
kevmoo
left a comment
There was a problem hiding this comment.
thanks. also investigating so we can avoid this in the future
|
auto label is removed for flutter/flutter/193183, Failed to enqueue flutter/flutter/193183 with HTTP 400: Pull request Required status check "Check Code Freeze" is queued.. |
|
auto label is removed for flutter/flutter/193183, Failed to enqueue flutter/flutter/193183 with HTTP 400: Pull request Required status check "Check Code Freeze" is queued.. |
….13)" (flutter#193192) Relands flutter#192966 (reverted in flutter#193183) with `OverlayEntry` disposal in `raw_tooltip_test.dart` and `FocusManager` early key-event interception in `raw_tooltip.dart`. ### What changed since flutter#192966 (`f8c85519a060`) 1. **Test `OverlayEntry` Leak Fix (`raw_tooltip_test.dart`)**: - In `raw_tooltip_test.dart` (`Escape key dismisses hovered RawTooltip even when primaryFocus is on a sibling consuming Escape`), `flutter#192966` passed an inline `OverlayEntry(...)` to `Overlay(initialEntries: <OverlayEntry>[...])` without calling `entry.remove()` and `entry.dispose()` in `addTearDown`. Because `Overlay` does not dispose `initialEntries` on unmount, the `OverlayEntry`'s internal `ValueNotifier<_OverlayEntryWidgetState?>` was flagged as `notDisposed` by `leak_tracker` in CI (`Linux framework_tests_widgets_leak_tracking`). - This reland captures `OverlayEntry? entry` and registers `addTearDown(() { entry?.remove(); entry?.dispose(); })`. Verified locally with `flutter test --dart-define=LEAK_TRACKING=true test/widgets/raw_tooltip_test.dart`. 2. **Stop `Escape` Propagation Before Focus-Tree Shortcuts (`raw_tooltip.dart`)**: - In `flutter#192966`, `RawTooltipState` registered `HardwareKeyboard.instance.addHandler(_handleKeyEvent)` alone. Because `KeyEventManager` runs `HardwareKeyboard` handlers *before* `FocusManager.handleKeyMessage` without stopping the focus-tree walk when returning `true`, emptying `RawTooltip._openedTooltips` in `HardwareKeyboard` caused `Escape` to still reach focused siblings (`siblingReceivedEscape`) or `WidgetsApp`'s `Shortcuts` (`DismissIntent` on enclosing `ModalRoute`s such as `DatePickerDialog`). - This reland registers a single static pair (`FocusManager.instance.addEarlyKeyEventHandler(_handleEarlyKeyEvent)` plus `HardwareKeyboard.instance.addHandler(_handleHardwareKeyEvent)` fallback when `primaryFocus == null`) when `RawTooltip._openedTooltips` transitions from empty to non-empty, and removes them when the last open tooltip closes or disposes. Returning `KeyEventResult.handled` from `_handleEarlyKeyEvent` stops `FocusManager` before walking the focus tree, and a second test (`Escape key dismisses open RawTooltip inside WidgetsApp without invoking ancestor DismissIntent`) verifies both cases. ### Original description Fixes http://b/463905826 WCAG 1.4.13 (Content on Hover or Focus) requires a mechanism to dismiss additional content triggered by pointer hover or keyboard focus (`Escape` key) without moving pointer hover or keyboard focus, and without unintended side effects such as dismissing an enclosing modal dialog (`DatePickerDialog`) when a tooltip is open. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] All existing and new tests are passing.
Reverts: fix(widgets): dismiss open RawTooltip on Escape key (WCAG 1.4.13)
Initiated by: @b-luk
Reason for reverting: Breaks
Windows framework_tests_widgets_leak_tracking. https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8669930694849114497/+/u/run_test.dart_for_framework_tests_shard_and_subshard_widgets/stdoutOriginal PR Author: @kevmoo
Reviewed By: @navaronbracke
The original PR description is provided below:
Description
Improves
RawTooltipkeyboard accessibility (WCAG 1.4.13 Dismissible, http://b/463905826):RawTooltipEscapekey dismissal: Registers aHardwareKeyboardhandler inRawTooltipStatethat dismisses open tooltips onLogicalKeyboardKey.escape(KeyDownEvent) and consumes the key event when a tooltip is open, preventingEscapefrom prematurely closing parent modal dialogs (such asDatePickerDialog) while a tooltip is displayed.Autocomplete,DrawerHeader, andTooltipThemeData.ignorePointerwill be submitted separately topackages/material_uiinflutter/packagesper #188444).Fixes http://b/463905826
Pre-launch Checklist
///).