Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Merged by Bors] - Integrate AccessKit #6874
[Merged by Bors] - Integrate AccessKit #6874
Changes from 8 commits
0e07c28
8b67099
269293b
c78a81d
07c6ec8
8503824
6c8bc71
3465deb
26444b5
9a9cf83
7d7ac22
1cf5730
0fc80fc
ebaac9b
058264b
efd9d80
b86a2c3
b52e587
06fe868
c503915
79bebfa
f84cbf0
6ec2a87
647a408
2f09ebb
3aa8c8e
43db5ac
f466741
f276404
791fbe4
aac5116
9fee1ee
05b03d3
06714ea
7894849
eafe226
2677ae4
3b75e5e
69166c6
1571633
f00c893
6ed0c14
f67de8e
ca99b51
eb6a30d
3b25d00
7ddfea0
0ac0051
f9f251d
293b240
7771ddc
2b6a114
c6e36c1
a375b56
c4e95bc
2917a86
6ea06b6
03a89e3
fed306d
47e8d0e
d7a1011
0349cd7
d9a9adc
5841343
8d43a23
cc2ed5c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It's not clear to me that we should be reading the text here, rather than using the
Name
component (and encouraging people to write to it).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 agree in principle, but I'd like to see these sorts of ideas pushed forward in a general UI overhaul. Put in terms more familiar to those of us who work with accessibility, it's better when designed in from the start vs. bolted on later.
To be clear, I'm not critiquing bevy_ui for not being accessible from the start, particularly since bevy_ui is lacking quite a few things and accessibility is in good company. :) I'd just like to see us address these sorts of issues as part of a greater UI refactor of which accessibility is just one part. Without this groundwork, we wouldn't necessarily be able to meaningfully include accessibility as a design consideration when bevy_ui eventually gets more attention.
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.
Just curious: Is there a reason this is used for
set_name
and notset_description
or something similar?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.
Mainly because I didn't want to take on fixing bevy_ui's shortcomings in this PR. I needed something to test with, and we don't currently have a way of describing UI widgets in any form. So I calculated a name/title and left it there.
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 see. And so the shortcoming you're talking about is that bevy_ui doesn't enforce
Name
components to extract the a11y name from?If it did, would this function instead be used to extract the node's description?
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.
It's more that bevy_ui will eventually need something like Flutter's Semantics class to truly be accessible. Ideally, in that situation, we'll have clearer understandings of how and when we're able to auto-generate names/descriptions, the Semantics component will be nicely integrated with the editor so accessibility properties can be set directly, etc. And I'm trying to take a lighter touch with all those things right now because I don't know what the right answer is.
Also, names and descriptions are very different things, and you don't necessarily want to scattershot the same property value everywhere so e.g. you don't double-announce a name because a screen reader might read a widget like "<name> <description>: button" or similar. Which properties to use and when is fairly complex and differs per widget, but in general setting names is universal and fairly low-hanging fruit. Something more complex I likely wouldn't auto-generate unless the widget semantics clearly call for it.
Ideally, we'll use AccessKit's NodeBuilder or a supported subset of its schema as the core of our Semantics component, and come to consensus about how we automatically fill in properties vs. expecting developers to fill them in themselves.
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.
Makes sense, thanks for the explanation!
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: It might be good to indicate why this component should be used. Seems like it's mainly for the
Role::Label
, but that's not immediately obvious (users may skip this in favor of just relying onText
).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.
If you have any wording suggestions I'm open to them--brain is a bit fried ATM. Alternately I can just remove it in favor of attaching a custom
NodeBuilder
to those nodes in particular. I addedLabel
before I realized how easy it was to just slap on a customNodeBuilder
and make whatever customizations I needed there.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.
Also, I feel like a label is a fairly common UI widget type, and we're going to want this or something like it at some point anyway. So it isn't really an accessibility thing, more a general widget I probably introduced before it's time thing.
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 don't have any wording suggestions off the top of my head. If it's meant to be part of a larger widget ecosystem, then maybe we come back to this (proper documentation) when we're ready to fully implement it? I imagine its usage and design could change a lot between now and when we truly define a "label" widget.