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.
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
a11y adjustments for the Bottom app bar demo #16238
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, assuming that you have tested this with an actual screen reader.
new Semantics( | ||
label: 'Set Bottom App Bar color to ${colorToName[color]}', | ||
container: true, | ||
explicitChildNodes: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: explicitChildNodes: false
is default behavior and should probably just be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone ahead and done that.
I merged this before seeing this comment from mobile. I'm too used to our internal UI where any comments blocks submitting a change.
I'll send a PR with this update and remember to check for final comments before merging next time.
// Generally, an icon button does not need to be wrapped in a Semantics object. | ||
// When the button has a null onPressed callback, it will be disabled by default. | ||
// | ||
// However, for the purposes of this demo, we don't want the button to appear disabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is kinda odd. Why should a screen reader user have a different experience of the UI then somebody who doesn't use a screen reader?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a demo, these are dummy buttons, not disabled buttons.
I decided to tell the screen reader that they are disabled because when I tried it out with a screen reader, I ended up confused. I partially blindfolded myself and tried tapping those buttons. When the reader told me I could double-tap to activate them, I did and I was confused when I couldn't find anything changed.
Since the primary purpose of this demo is to showcase normal Flutter code, I am OK with removing those semantics objects.
I'll let you make the final call, since we should apply any solution we come up with to all no-op buttons in the gallery.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, I would like to see the following:
- Button state is presented the same to screen-reader and non-screen-reader users.
- If you click an enabled button some kind of dummy action should happen to acknowledge the button click.
I've also seen non-screen-reader users get confused when they hit a seemingly enabled button and nothing happens.
That's correct, tested with TalkBack on Android. I assume that we test Flutter's semantics engine to give the same performance on iOS. Let me know if that isn't correct. |
* 'master' of https://github.com/flutter/flutter: Roll engine to aa9ce70 (flutter#16289) Rename chip's border attribute to shape for consistency. (flutter#16276) Added BeveledRectangleBorder ShapeBorder (flutter#16279) Roll engine to version be07059 (flutter#16264) Make Podfiles work with Cocoapods 1.5.0 (flutter#16273) Updated appearance of filled TextFields - added UnderlineInputBorder.borderRadius (flutter#16272) Handle whitespace in entry values in the AAPT badging parser (flutter#16245) a11y adjustments for the Bottom app bar demo (flutter#16238) Play butterfly video from asset instead of network (flutter#16269)
@goderbauer
fixes #16225