Skip to content

ui: Include a tooltip with all our IconButtons #98

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 24, 2023
Merged

ui: Include a tooltip with all our IconButtons #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
45f2f8d
login: Set `tooltip` on IconButton; write TODO for future simplification
chrisbobbe May 18, 2023
dfe2695
compose: Add tooltips to `_AttachUploadsButton`s
chrisbobbe May 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion lib/widgets/compose_box.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ abstract class _AttachUploadsButton extends StatelessWidget {
final FocusNode contentFocusNode;

IconData get icon;
String get tooltip;

/// Request files from the user, in the way specific to this upload type.
///
Expand Down Expand Up @@ -321,7 +322,7 @@ abstract class _AttachUploadsButton extends StatelessWidget {

@override
Widget build(BuildContext context) {
return IconButton(icon: Icon(icon), onPressed: () => _handlePress(context));
return IconButton(icon: Icon(icon), tooltip: tooltip, onPressed: () => _handlePress(context));
}
}

Expand Down Expand Up @@ -366,6 +367,8 @@ class _AttachFileButton extends _AttachUploadsButton {

@override
IconData get icon => Icons.attach_file;
@override
String get tooltip => 'Attach files';

@override
Future<Iterable<_File>> getFiles(BuildContext context) async {
Expand All @@ -378,6 +381,8 @@ class _AttachMediaButton extends _AttachUploadsButton {

@override
IconData get icon => Icons.image;
@override
String get tooltip => 'Attach images or videos';

@override
Future<Iterable<_File>> getFiles(BuildContext context) async {
Expand All @@ -392,6 +397,8 @@ class _AttachFromCameraButton extends _AttachUploadsButton {

@override
IconData get icon => Icons.camera_alt;
@override
String get tooltip => 'Take a photo';

@override
Future<Iterable<_File>> getFiles(BuildContext context) async {
Expand Down
9 changes: 8 additions & 1 deletion lib/widgets/login.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,8 +373,15 @@ class _PasswordLoginPageState extends State<PasswordLoginPage> {
decoration: InputDecoration(
labelText: 'Password',
helperText: kLayoutPinningHelperText,
suffixIcon: Semantics(label: 'Hide password', toggled: _obscurePassword,
// TODO(material-3): Simplify away `Semantics` by using IconButton's
// M3-only params `isSelected` / `selectedIcon`, after fixing
// https://github.com/flutter/flutter/issues/127145 . (Also, the
Comment on lines +377 to +378
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, good to have reported this. Looks like that issue was assigned just today, so hopefully will get fixed soon.

// `Semantics` seen here would misbehave in M3 for reasons
// involving a `Semantics` with `container: true` in an underlying
// [ButtonStyleButton].)
Comment on lines +378 to +381
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an issue to link to for this?

Copy link
Collaborator Author

@chrisbobbe chrisbobbe May 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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.

suffixIcon: Semantics(toggled: _obscurePassword,
child: IconButton(
tooltip: 'Hide password',
onPressed: _handlePasswordVisibilityPress,
icon: _obscurePassword
? const Icon(Icons.visibility_off)
Expand Down