-
Notifications
You must be signed in to change notification settings - Fork 3
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
Absorption and Emission panel dialog could use some improvement #86
Comments
Reassigning this to @jessegreenberg because it should be handled by the common code component. @jessegreenberg can you take a look here. |
PhET doesn't have a common code component for non-modal dialogs. So, we should either add this to all Panels or wait until PhET creates a reusable non-modal dialog. I am thinking about whether adding to all Panels make sense. |
I posted this to Slack#models-of-the-hydrogen-atom before I saw @jessegreenberg's comment in #86 (comment).
So I don't think that Dialog behavior should be added to Panel, and it would be best to wait until PhET has a non-modal Dialog. My recommendation is to open an issue for creating a non-modal Dialog (if one doesn't already exist). Note this desired behavior in that issue. Live with the current behavior in MOTHA and close this issue as "won't fix". |
After thinking about this more and discussing with @pixelzoom I agree that it is not appropriate to put this in Panel. We found usages of Panel (in vector-addition and inside of other Dialogs) where it would be undesirable to have an accessible name and aria-labelledby association for it. I added the attribute to AbsorptionAndEmissionDialog quickly so we can finish this sim specific issue. When we work on a general non-modal dialog, we can apply what we learned here.
And it sounds like this in NVDA when the Panel has focus: I think thats good. @terracoda how does this sound to you in VoiceOver? |
Ah, it needed a role of dialog. That makes sense. And I agree, we would not want that behaviour in every panel. Let’s just address the fact that we need to create a non-modal dialog. |
@terracoda reviewed and found that the First, we tried adding an aria-describedby association, but VoiceOver ignored it so we removed it. Then, we moved the accessible name and help text inside of the focusable non-modal dialog. This allows the user to read down through the dialog after focus is on it to find the accessible name and help text. When focus lands on the Dialog, VoiceOver correctly reads the accessible name and role (dialog) because of the aria-labelledby association. Unfortunately, this required more complicated accessibility Nodes and options (75ebaf2). But we can use this information when we create a reusable non-modal dialog in phetsims/sun#916. That issue is already pointing to this one for reference. We are ready to close this but @pixelzoom assigning to you in case you want to review or ask any follow up questions first. |
Thanks, looks great, closing. In 72ad151, I added bit more documentation with links back to this issue -- reminders to my future self, or whoever will be maintaining this code. |
When I move focus from the "Absorption and Emission, checkbox" to the newly opened dialog/panel, the dialog/panel does not actually have a name.
I hear: "Close, and 9 more items, group"
Hearing, "Close" is correct, if we want that to be the first focusable button in this dialog/panel.
Hearing, "and 9 more items, group" is useful. That indicates to the learner there are 9 more things in the dialog to check out.
I think for this panel/dialog we need to associate the name, "Absorption and Emission" to the focusable group via some labeling technique. Maybe aria-labeledby? AND since this panel/dialog has interactive components, it might make sense to put focus on the first button, rather than the close button. The close button could be put at the end, or at the very start, before the name and help text.
@pixelzoom , I could check in with @jessegreenberg to see if there are some simple easy improvements to be had here with the implementation. This is lower-level API stuff.
The text was updated successfully, but these errors were encountered: