Conversation
… is false TooltipVisibility(visible: false) is meant to suppress only the visual tooltip overlay (e.g. so it doesn't distract screen reader users), while still exposing the message as a semantics label. Previously, the entire RawTooltip subtree -- including its Semantics(tooltip:) node -- was skipped whenever the tooltip was not visible, so widgets like IconButton lost their accessibility label entirely instead of falling back to it. Fixes flutter/flutter#189062
…edback Replaces the custom _semanticsNodeWithLabel tree-walk helper with the built-in tester.getSemantics(finder).tooltip, per QuncCccccc's review suggestion. Works cleanly here because the Tooltip itself is the nearest semantics boundary above the labelled child once wrapped in Semantics.
github-actions
Bot
added
p: material_ui
triage-design
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request ensures that Tooltip preserves its semantics label when wrapped in TooltipVisibility(visible: false) by adding a fallback Semantics widget when tooltip rendering is suppressed. Corresponding tests and a changelog entry have been added. Feedback suggests simplifying the condition in tooltip.dart by removing the redundant _tooltipMessage.isNotEmpty check, as it is already handled earlier in the build method.
| ignorePointer: widget.ignorePointer ?? widget.message != null, | ||
| child: effectiveChild, | ||
| ); | ||
| } else if (!excludeFromSemantics && _tooltipMessage.isNotEmpty) { |
There was a problem hiding this comment.
The check _tooltipMessage.isNotEmpty is redundant here because _tooltipMessage.isEmpty is already checked at the beginning of the build method (line 502), which results in an early return. We can safely simplify this condition to just !excludeFromSemantics.
} else if (!excludeFromSemantics) {
QuncCccccc
left a comment
QuncCccccc
left a comment
There was a problem hiding this comment.
Thank you for your contribution! Because of the volume of PRs we receive, we require that new contributors use our checklist to guide them through critical steps in creating a Flutter PR. This PR's description is missing that checklist, so it is being marked as a Draft.
Please edit the PR description to add the checklist, then ensure that you have completed all of the steps. Once you've done that, please mark the PR as ready for review.
If you need help, consider asking for advice on the #hackers-new channel on Discord.
There was a problem hiding this comment.
Code Review
This pull request ensures that Tooltip preserves its semantics label when wrapped in TooltipVisibility with visible set to false, adding corresponding tests and a changelog entry. Feedback on the changes suggests removing a redundant _tooltipMessage.isNotEmpty check and caching _tooltipMessage in a local variable to optimize performance by avoiding redundant text span tree traversals.
| ignorePointer: widget.ignorePointer ?? widget.message != null, | ||
| child: effectiveChild, | ||
| ); | ||
| } else if (!excludeFromSemantics && _tooltipMessage.isNotEmpty) { |
There was a problem hiding this comment.
The check _tooltipMessage.isNotEmpty is redundant here because there is already an early return at the beginning of the build method (line 502) if _tooltipMessage.isEmpty is true. Additionally, as a performance improvement, consider caching _tooltipMessage in a local variable at the start of the build method. Since _tooltipMessage is a getter that calls toPlainText() on widget.richMessage (which traverses the text span tree), caching it avoids redundant traversals across multiple checks and widget instantiations within the same build cycle.
} else if (!excludeFromSemantics) {
|
Hello, please edit the PR description to add the checklist, then ensure that you have completed all of the steps. |
2 participants
Ports flutter/flutter#189362 to
material_uinow that the Material/Cupertino code freeze has lifted, per the instructions in flutter/flutter#188444.Original PR description:
TooltipVisibility(visible: false)is the idiomatic way to suppress atooltip's visual overlay — most commonly done when an accessibility
service is active, because the floating tooltip is visual noise for a
screen reader user. The expectation is that the visual tooltip goes away
but the semantic label survives, so e.g. an
IconButton(tooltip: 'Add')still announces "Add" to TalkBack/VoiceOver.
Before this change, the opposite happened: an
IconButtonwrapped inTooltipVisibility(visible: false)announced only "button" — theaccessibility label was dropped entirely.
Root cause
In
_TooltipState.build(), theSemantics(tooltip: ...)node was onlyever attached inside
RawTooltip, andRawTooltipwas only built whenthe tooltip was visible, coupling the accessibility label to the
interactive overlay machinery it shouldn't depend on.
Fix
Add an
else if (!excludeFromSemantics)branch that still attaches aplain
Semantics(tooltip: _tooltipMessage, ...)wrapper when notvisible, mirroring the same construct
RawTooltipitself uses, withoutany of the gesture/overlay wrappers.
excludeFromSemanticsopt-out behavior is preserved in both branches.Tests
Added tests to
tooltip_visibility_test.dart:tooltiplabelwhen
TooltipVisibility(visible: false).excludeFromSemantics: true, even while not visible.IconButton.tooltipunderTooltipVisibility(visible: false).Fixes flutter/flutter#189062
Closes flutter/flutter#189362 (superseded by this PR)