Skip to content
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

Closed
terracoda opened this issue Nov 14, 2024 · 8 comments
Closed

Absorption and Emission panel dialog could use some improvement #86

terracoda opened this issue Nov 14, 2024 · 8 comments

Comments

@terracoda
Copy link

Screenshot 2024-11-14 at 14 21 51

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.

Screenshot 2024-11-14 at 14 37 44

@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.

@kathy-phet
Copy link

Reassigning this to @jessegreenberg because it should be handled by the common code component. @jessegreenberg can you take a look here.

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Nov 14, 2024

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.

@pixelzoom
Copy link
Contributor

pixelzoom commented Nov 14, 2024

I posted this to Slack#models-of-the-hydrogen-atom before I saw @jessegreenberg's comment in #86 (comment).

If this is for the “Absorption and Emission” dialog… Keep in mind that PhET currently has no non-modal Dialog. So the implementation in MOTHA is faking a non-modal Dialog using a Panel. There are other sims doing similar fakery, all slightly different implementations, with no common code support. And it would be unfortunate if behavior related to this dialog fakery is put into Panel.

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".

@jessegreenberg
Copy link
Contributor

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.

  • I added the aria-labelledby attribute.
  • I added the role attribute, that was required for NVDA to read the accessible name correctly.
  • I removed "Dialog" from the absorptionAndEmissionDialog.accessibleName string so we don't hear "Dialog" twice (since we get that from the role now).

And it sounds like this in NVDA when the Panel has focus:
'Absorption and Emission, dialog, close button"

I think thats good. @terracoda how does this sound to you in VoiceOver?

@terracoda
Copy link
Author

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.

@jessegreenberg
Copy link
Contributor

@terracoda reviewed and found that the helpText was not easily discoverable on VoiceOver.

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.

@pixelzoom
Copy link
Contributor

pixelzoom commented Nov 16, 2024

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.

@terracoda
Copy link
Author

terracoda commented Nov 16, 2024

Just adding the final visuals movable panel/dialog in the PDOM:
Screenshot 1: Movable dialog with focus
Screenshot 2024-11-16 at 10 13 08
Screenshot 2: Focus inside the movable dialog on the Close button.
Screenshot 2024-11-16 at 10 44 59

The following details will need to be considered for any future work on a truly non-modal dialog:

  1. The dialog will likely need an automatic indication that is is in fact movable. Perhaps use aria-roledescription=movable, similar to custom movable objects like the Yellow Balloon in BASE.
  2. In the current implementation for MotHA, when the "movable dialog" has focus (Screenshot 1), a user cannot use the virtual cursor to enter the dialog's content. Is this a feature or a bug?
  3. In Screenshot 2, focus is in the dialog and, and a user can user the virtual cursor to find non-interactive and interactive content inside the dialog.
  4. VoiceOver Bug? - In current implementation the Close button is sometimes not announced on first focus? Will need further investigation.

Note that in the current implementation the Arrow keys and their alternatives work to move the dialog if focus is directly on it, or if focus is inside the dialog on one of the buttons. This behavior needs further consideration. There could be cases where a group is inside the dialog, and the group would need access to the Arrow keys for item navigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants