-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Additional accessibility support #412
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.
Thank you for this PR, and I think this is great!
I saw some opportunities for improvements and simplifications:
TextEdit can handle emitting its own special events (TextChanged
and TextCursorChanged
).
It should also be possible to easily (and cheaply) only emit events when a change has occurred.
There will also be some unfortunate merge conflicts when I merge #399
Hitting an issue I'd appreciate help with. The changes in 87bd38d break focus events on text fields and text fields only. That is, if I tab onto a text field, nothing is spoken and nothing is reported. Every other widget seems to work fine, though. That particular commit pushes events in I'm starting to get a bit overwhelmed. When you have time for egui again, any chance you might spare some cycles to help track down why pushing unrelated events breaks focus events? bevy_egui_a11y is updated and exhibits the behavior. I've committed the hand-revert on the branch fix-focus-somehow. Thanks. |
Thank you for taking the time for this! I completely understand that it is a bit overwhelming 😅 |
Oh wow, thanks for catching that. I'll fix tomorrow.
|
OK, it's starting to click for me.
One question before I make too many more changes. When would you like an
event dispatched by `Response::widget_info` vs. the widgets themselves?
If possible, I'd like it to either be one or the other, or for there to
be clear guidance for which to choose.
Maybe universal events like focus, click, double-click, change, etc. are
sent from `widget_info`, while events internal to the widget (I.e.
moving cursor in a text field) are sent from the widget itself? Would
that work?
|
…entions. Note that this work isn't complete--I'll correct more cases as I add more widgets and become familiar with their structures.
* Only dispatch `SelectionChanged` if the selection actually changes. * Fix missing focus events.
…t the previous value.
This is one of the more complex PRs I've worked with, so I tried to close conversations I thought might be resolved. Except for #399, I think everything might be resolved. Please let me know if I missed something, or if there's anything else you might want reworked. Haven't updated the demo yet, but in my game, everything now works as expected, and I got rid of the crappy selection cache since those changes are dispatched correctly. There are two more things I need for my game: sliders, and support for entering passwords (I.e. not reading characters as they're entered, or reading the field when it gets focus.) I'll work on those immediately after this PR lands, but I feel like there's a lot in flight with this one, so I'd like to get it merged before I work on those. Hope that's OK. |
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.
Tested this now, and it works really well! Good job!
I did find one bug: while holding down the mouse button in a text edit field the ValueChanged
event is being spammed each frame. I don't know why though, but it seems to be introduced by this PR.
Also: see 77b3a15 for how to simplify the code by a lot.
I went ahead and merged upstream today, resolving the couple merge conflicts that popped up. I think we may be good to go. Please let me know if I missed anything--tracking all the requested changes has been a bit challenging but I think I got them all. |
Did you manage to fix this? |
Unfortunately I can't fix the mouse issue without help. Sorry. I can't see the field to click in it, and my attempts to do so just click wherever the mouse happens to be. Assistance welcome when you have the time. Thanks. |
I'll fix after merge! Thank you so much for this PR! |
Fixed in f4a95b1 |
This is an initial stab at broadening egui's accessibility support. It isn't at all complete, but I'm hoping to merge what I can as I go, and to get some feedback. I'd particularly like to merge regularly so branches don't get out of sync, and particularly because I think something is better than nothing where accessibility is concerned. Currently supported:
TextEdit
, as well as reporting when the selection changes. Still not perfect, but it's something.bevy_egui_a11y has been updated to show off these changes. Note that you can tab through all fields, get meaningful feedback, type into the text field, check the checkbox, arrow around, etc.
I've already started using my fork in my game and it seems to work well, but I really don't want to maintain a fork going forward. Please let me know if you'd rather not go down this road, and I'll stop work and back out the integration in my game.
Thanks.