Use SelectNext for StudentSelector

[acelaya]

Requires hypothesis/frontend-shared#1330

This PR replaces Select with SelectNext in the StudentSelector.

Before:

After:

SelectNext needs to have a "fixed" size to make sure it does not resize when selected option changes, so I have changed its sizes to be:

• 20rem for extra large devices (this was already like that)
• 16rem for large devices
• 12rem for medium devices
• full (the default) for small and extra small devices, as the components collapse in a column there.

Grabacion.de.pantalla.desde.2023-10-18.11-56-35.webm

Testing steps

Open any assignment and verify students can be selected as with the regular select.

[robertknight]

When testing with VoiceOver I noticed that the text of the selected student was not read when the select control had focus. A minor issue is that when clicking in the iframe, the listbox is not dismissed, although the focus ring does disappear from the selected item. I think we may have discussed this before, but I'm not sure if we found a solution.

Before:

Note the text format is {current value} {label} {control type} {state}.

After:

[acelaya]

When testing with VoiceOver I noticed that the text of the selected student was not read when the select control had focus.

This is probably because of the label I added to mimic the previous aria-label. I guess it should be solved by removing it or making sure its content changes dynamically.

I suppose screen readers read both label and select, but when the label is linked to a button, it "replaces" the button instead of adding.

What I don't know is why it announces the first part, "Student 1 of 2". That is not linked with the select as far as I can tell 🤔

I'll check it just in case.

A minor issue is that when clicking in the iframe, the listbox is not dismissed, although the focus ring does disappear from the selected item. I think we may have discussed this before, but I'm not sure if we found a solution.

We discussed this for the comment popover, but that one is closed only with useClickAway, which is tricky to handle cross-frame.

In the case of SelectNext we also have onFocusAway, but it does not check if the component loses focus, but instead, if the newly focused element is outside of some container.

I guess that, if the focus has moved to a different frame, the events don't trigger as one would expect. I will try to put together a sandbox to test this.

[acelaya]

A minor issue is that when clicking in the iframe, the listbox is not dismissed, although the focus ring does disappear from the selected item.

That issue ☝🏼 should be addressed by this PR hypothesis/frontend-shared#1326

[acelaya]

@robertknight I have found out why it now announces "Student 1 of 2". The reason is that it is a label linked with the select via htmlFor.

Apparently, the previous aria-label on the select itself was "overwriting" the other label when announced by the screen reader, while it is now "combined" with the new sr-only label I added, making it announce both.

My suggestion is that we remove the htmlFor reference from the uppermost label (the one with the "Student 1 of 2" text) and set a dynamic content in the new sr-only label. That mimics the previous behavior reasonably well.

diff --git a/lms/static/scripts/frontend_apps/components/StudentSelector.tsx b/lms/static/scripts/frontend_apps/components/StudentSelector.tsx
index aa9469c0..d1d9e8bb 100644
--- a/lms/static/scripts/frontend_apps/components/StudentSelector.tsx
+++ b/lms/static/scripts/frontend_apps/components/StudentSelector.tsx
@@ -52,7 +52,6 @@ export default function StudentSelector<Student extends StudentOption>({
       <label
         className="font-semibold text-xs"
         data-testid="student-selector-label"
-        htmlFor={selectId}
       >
         {hasSelectedStudent ? (
           <>
@@ -78,7 +77,7 @@ export default function StudentSelector<Student extends StudentOption>({
             variant="dark"
           />
           <label className="sr-only" htmlFor={selectId}>
-            Select student
+            {selectedStudent?.displayName} Select student
           </label>
           <SelectNext
             classes="md:!w-[12rem] lg:!w-[16rem] xl:!w-[20rem]"

The only drawback is that clicking in the "Student 1 of 2" label will no longer focus the select, but considering how "small" it is, I think it's negligible.

What do you think?

[robertknight]

Hi @acelaya, I should have been clearer. The inclusion of "Student 1 of 2" in the new caption is not the problematic change here. In fact I think it is fine to leave that as-is. Removing the htmlFor on the label would leave a <label> with no associated element, which would be semantically incorrect. The problem here is the fact that the current value of the select ("aunltd learner" in the above screenshot) is no longer read before the label.

[acelaya]

Hi @acelaya, I should have been clearer. The inclusion of "Student 1 of 2" in the new caption is not the problematic change here. In fact I think it is fine to leave that as-is. Removing the htmlFor on the label would leave a <label> with no associated element, which would be semantically incorrect. The problem here is the fact that the current value of the select ("aunltd learner" in the above screenshot) is no longer read before the label.

Gotcha. Then I think just adding the name of the student in the new label should be enough:

diff --git a/lms/static/scripts/frontend_apps/components/StudentSelector.tsx b/lms/static/scripts/frontend_apps/components/StudentSelector.tsx
index aa9469c0..47cc4eca 100644
--- a/lms/static/scripts/frontend_apps/components/StudentSelector.tsx
+++ b/lms/static/scripts/frontend_apps/components/StudentSelector.tsx
@@ -78,7 +78,7 @@ export default function StudentSelector<Student extends StudentOption>({
             variant="dark"
           />
           <label className="sr-only" htmlFor={selectId}>
-            Select student
+            {selectedStudent?.displayName} Select student
           </label>
           <SelectNext
             classes="md:!w-[12rem] lg:!w-[16rem] xl:!w-[20rem]"

With that it announces mostly the same as before, but prefixed with "Student 1 of 2" or whatever the uppermost label says.

[acelaya]

@robertknight I have pushed the fixes mentioned in previous comments and updated to frontend-shared v6.9.0, which includes some extra fixes needed for the LMS

[robertknight]

Explicitly adding the control value into the label is something that SelectNext users shouldn't need to do themselves, otherwise some consumer will forget. When testing this I wondered why I hadn't encountered this issue with the SelectNext control in the Export tab in the client. After some testing I found that the client's Import/Export tabs do have the same issue when testing in Chrome or Firefox. Under Safari, it seems the button content is treated as being either part of the control's label or its current value.

Aside from not wanting to make each use of SelectNext need to add the current value into the label, there is also just the issue that there should be a logical distinction between the label of a select control and the current value. The way native controls work is that they have:

• A label, which can be obtained from one of several sources (label for=, aria-label, aria-labelledby) according to a priority order
• A current value
• A control type and states

The screen reader then reads these out in order, eg. as {current value}, {label}, {control type}, {state}.

Taking the grade input as an example, the properties can be inspected in the Accessibility panel of the Elements tab:

I wondered how other libraries we've been looking at handle this. The React Spectrum picker uses two aria-labelledby attributes on the listbox button, which gets concatenated by the browser. This is semantically implementing the same solution as you have here, but built into the control itself. This seems like a bit of a hack:

I couldn't easily see how Headless UI's example works because their reference page doesn't have an example with a label.

The example at https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-select-only/ on the other hand uses the combobox pattern. That ARIA role allows for a separation between value and label, and is read out correctly by in all 3 browsers. This allows a native <label> to be used similar to what we are doing here:

I had a look at Chrome's implementation of the <selectlist> control they are working on which is planned as a native styleable select in future. After enabling the "Enable experimental web platform features" flag in Chrome and inspecting the demo at https://codepen.io/una/pen/PoXbgVW, I see that in the accessibility tree, <selectlist> is exposed using a combobox.

Turning our control into a combobox by adding role=combobox on the button appears to resolve the issue and map the control into the expected structure in the accessibility tree. After doing that I noted an issue that most of the examples in the SelectNext demos page are missing labels.

In summary:

• I think we should be using the combobox role for SelectNext, because it allows for creating a control with a notion of both a label and a "current value", which can be read as appropriate by the screen reader. It also seems to be where efforts towards a native styleable select are headed.
• The SelectNext examples in frontend-shared should have labels

[acelaya]

Thanks Rob. Super detailed explanation and investigation.

Putting this in draft again, while I try to refactor SelectNext to be a combobox.

[acelaya]

I have tested this branch with the changes from hypothesis/frontend-shared#1330, and it solves the issues with screen readers.

[acelaya]

I have updated to frontend-shared 6.9.1, which includes the refactored SelectNext with combobox role.

This PR is ready for review again.

[robertknight]

LGTM. The updated SelectNext now works as expected.

[robertknight]

Now that we actually have this UI implemented, I'm not sure there is value in keeping the toolbar page, which is going to get out of sync with the actual implementation. We could decide to remove these prototypes. We'll always have the code in Git if we need to get them back.

[acelaya]

Sounds reasonable.