ui: Include a tooltip with all our IconButtons
#98
Conversation
| // M3-only params `isSelected` / `selectedIcon`, after fixing | ||
| // https://github.com/flutter/flutter/issues/127145 . (Also, the |
There was a problem hiding this comment.
Cool, good to have reported this. Looks like that issue was assigned just today, so hopefully will get fixed soon.
| // https://github.com/flutter/flutter/issues/127145 . (Also, the | ||
| // `Semantics` seen here would misbehave in M3 for reasons | ||
| // involving a `Semantics` with `container: true` in an underlying | ||
| // [ButtonStyleButton].) |
There was a problem hiding this comment.
Is there an issue to link to for this?
There was a problem hiding this comment.
Good thought, but no, I don't think so. The change in behavior with M3 surprised me, but I haven't yet decided what upstream bug, if any, causes this Semantics to not do what we want. And as you saw, once flutter/flutter#127145 is fixed, we can remove the Semantics in M3 and get the right behavior, and that's good because it's nice and simple. 🙂
Here are some observations about what happens if I set useMaterial3: true near our app root:
IconButton builds something it calls _IconButtonM3, and that's a class that extends ButtonStyleButton. And ButtonStyleButton builds a Semantics with container: true, to introduce a new node in the Semantics tree (doc). I guess that could be a reasonable choice, I'm not sure. 🙂 I can focus the button with VoiceOver, and it just says: "Hide password. Button."—which means my toggled value is ignored. I found three separate ways I could fix the issue, to where it's still focusable and it says "Hide password. Switch button. On. Double-tap to toggle setting.". Those ways are:
- If I remove that
container: truein theButtonStyleButtoncode. Or, - If I wrap my
Semantics(toggled: _obscurePassword,with aMergeSemantics. Or, - If I remove my
Semantics(toggled: _obscurePassword,use the M3IconButton.isSelected, and apply the patch toicon_button.dartthat I mentioned at [Material3] IconButton'sisSelectednot visible to screen readers flutter/flutter#127145 (comment)
If I copy this chunk of code (the one beginning with Semantics(toggled: _obscurePassword) to somewhere outside the password text field (say, between it and the "Log in" button), then the behaviors and fixes I've described replicate there too. I tested that because I noticed something odd happening near the original show/hide button: before applying one of the three fixes I listed above, a different focusable node would get the wrong text: the one for the whole password text field, before I drill down to (traverse forward to?) the show/hide button. VoiceOver would say, "Password. 1. Text field. Double-tap to edit"—or, for the show/hide button's other state, "Password. 0. Text field. Double-tap to edit". In that case, is the framework wanting to present suffixIcon as part of the meaning of the whole text field? (This code looks related…?) But here it's not meant as part of the text field's meaning; it's just a related control we offer.
We should probably always set `tooltip` on our IconButtons; it gives context for users whether or not they're using screen-reader software.
chrisbobbe
force-pushed
the
pr-icon-button-tooltips
branch
from
4e08898 to
dfe2695
Compare
|
Thanks for the review! Merging, and I responded above: #98 (comment) |
Looks like I forgot to get this one in zulip#98, oops.
Looks like I forgot to get this one in #98, oops.
No description provided.