Improve menu item accessibility semantics #176255
Conversation
github-actions
bot
added
the
framework
github-actions
bot
added
the
f: material 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 improves the accessibility of MenuItemButton and SubmenuButton by ensuring they are correctly identified as buttons with their enabled/disabled state announced by screen readers. This is achieved by wrapping the internal _MenuItemLabel with a Semantics widget, setting the button property to true, and passing the enabled state. The logic correctly handles merging semantics from child widgets when no explicit semantics label is provided. The associated tests have been updated to reflect these changes and new tests for the disabled states have been added, ensuring the new behavior is well-covered. The changes are well-implemented and effectively address the accessibility issue.
|
@QuncCccccc Do you remember why we want to remove the button from the menu before? |
|
IIRC, menu items don't announce "button" on windows, so we removed it in that PR. |
mdebbar
left a comment
mdebbar
left a comment
There was a problem hiding this comment.
@chunhtai @QuncCccccc do you think isSemanticButton should be configurable by the user, or just default to the right value for each platform (or both)?
Default value per platform would be:
- Windows -
isSemanticButton: null - Web -
isSemanticButton: true - Others - ??
| label: semanticsLabel, | ||
| excludeSemantics: true, | ||
| enabled: isEnabled, | ||
| button: isSemanticButton, |
There was a problem hiding this comment.
I'm not sure that this is the right place to set the semantics button property. I think we should be replacing the isSemanticButton: null passed to TextButton above (line 1886).
Same for enabled.
There was a problem hiding this comment.
Great suggestion. I updated the PR.
|
I agree we may need to be platform adaptive on this, I think for now Web - |
| label: semanticsLabel, | ||
| excludeSemantics: true, | ||
| enabled: isEnabled, | ||
| button: isSemanticButton, |
There was a problem hiding this comment.
we should programatically assign the button: true based on kIsWeb, this is not something developer can easily pick and know about the consequence, and it seems like there may not be a good reason to provide the flexibility
There was a problem hiding this comment.
Great suggestion. I updated the PR.
- Rename semantics tests to clarify they test platform-adaptive behavior - Remove redundant default semantics tests that would fail on web - Remove out-of-scope disabled semantics tests to keep PR focused - Platform-adaptive tests with `if (kIsWeb)` cover all platforms
flutter-zl
force-pushed
the
button_0923
branch
from
615ba1d to
36fe1f8
Compare
mdebbar
left a comment
mdebbar
left a comment
There was a problem hiding this comment.
We changed the value on other platform from null to false. If those are equivalent, then the PR looks good to me!
| statesController: widget.statesController, | ||
| clipBehavior: widget.clipBehavior, | ||
| isSemanticButton: null, | ||
| isSemanticButton: kIsWeb, |
There was a problem hiding this comment.
Is null equivalent to false?
There was a problem hiding this comment.
good catch. updated.
| onFocusChange: _enabled ? widget.onFocusChange : null, | ||
| onPressed: _enabled ? toggleShowMenu : null, | ||
| isSemanticButton: null, | ||
| isSemanticButton: kIsWeb, |
There was a problem hiding this comment.
good catch. updated.
This PR addresses issue flutter#171231 by enhancing the accessibility semantics for `MenuItemButton` and `SubmenuButton`. Previously, screen readers did not correctly announce the disabled state of these menu items. This ensures that menu items are correctly identified as buttons and their enabled/disabled state is communicated to assistive technologies. **Before change** https://button-0923-before.web.app/ **After change** https://button-0923-after.web.app/
4 participants
This PR addresses issue #171231 by enhancing the accessibility semantics for
MenuItemButtonandSubmenuButton. Previously, screen readers did not correctly announce the disabled state of these menu items.This ensures that menu items are correctly identified as buttons and their enabled/disabled state is communicated to assistive technologies.
Before change
https://button-0923-before.web.app/
After change
https://button-0923-after.web.app/