fast icon indicating copy to clipboard operation
fast copied to clipboard

feat: add label to combobox, ensure aria-label and aria-labelledby are passed to the root

Open chrisdholt opened this issue 3 years ago • 4 comments

Pull Request

📖 Description

This PR adds a label to the combobox and forwards the aria-label and aria-labelledby attributes from the host element to the input which is the focusable element.

When deciding whether or not to add labels to all form elements or remove labels from form elements, I believe that addition here is the best current path forward. Combobox is somewhat the ideal test case - because the input is within the Shadow DOM, the only option for associating a label from outside the template may require cross-root aria delegation. Given this and that a majority of other form elements have labels already, we're pursuing ensuring all all form elements have labels for our v3.0 implementations.

🎫 Issues

closes #6041 related to #4801

👩‍💻 Reviewer Notes

📑 Test Plan

Waiting for #6113 to finalize this work with testing/validation.

✅ Checklist

General

  • [x] I have included a change request file using $ yarn change
  • [ ] I have added tests for my changes.
  • [ ] I have tested my changes.
  • [ ] I have updated the project documentation to reflect my changes.
  • [x] I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

⏭ Next Steps

chrisdholt avatar Jun 22 '22 18:06 chrisdholt

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-ocean-0b7ce3410-6143.centralus.azurestaticapps.net

github-actions[bot] avatar Jun 28 '22 18:06 github-actions[bot]

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-ocean-0b7ce3410-6143.centralus.azurestaticapps.net

github-actions[bot] avatar Jun 28 '22 19:06 github-actions[bot]

Do we plan to remove this in favor of a "field" component that wraps form elements and provides a label as a general mechanism? Is that something we can do now or are their problems with that? If we can't do that now, should we propagate this label pattern everywhere for consistency? There's no reason why a component without a default slot can't have a named slot, for example.

The problem here is specifically accessibility. While I think a field can be helpful, the problem is that there would be no way to tie the label to the internal control receiving focus in this scenario (as we've currently built it). If we have a field, we would associate using something like aria-labelledby, but in this specific case we are delegating focus which means without cross shadow root ARIA delegation, the association doesn't work. So for scenarios where the role exists on the host, there's no problem associating currently because both are essentially in the light DOM. That works for checkbox, radio, select - but for others where we are currently leveraging delegatesFocus, that falls apart. It's possible that we can write up field to solve this problem, but I also am not sure that it should be the required method of achieving things.

In terms of reproducing this pattern with a named slot for label - I'm okay with that, but consistency becomes limiting as well in terms of a developer experience standpoint. Elements with a default slot can currently take text and do the right thing without requiring an additional element or association. If we go the way of a consistent "label" slot, we lose this in some sense.

Happy to look into how "field" can help us avoid the a11y complications, but I'm not sure it can as of yet. Alternatively, we can revisit using delegatesFocus, but the cost of implementation and complexity likely increases there because we're reinventing things like button, inputs, text area. This was intended as a quick win in starting to get labels on things that don't currently have them. Happy to hold off though and look at a more robust solution. As it stands currently, I'm not sure combobox can currently associate a label because the actual element is in the shadow root.

chrisdholt avatar Jun 30 '22 20:06 chrisdholt

I think it's fine to move ahead with this. We need this capability. We can discuss further at another time if we want to enable a label slot for the other components.

EisenbergEffect avatar Jun 30 '22 21:06 EisenbergEffect

Closing this as we re-evaluate form association.

chrisdholt avatar Dec 16 '22 18:12 chrisdholt

The current solution around this I'm using at my work... is this: https://github.com/microsoft/fast/issues/6552

I'm curious what @break-stuff thinks?

usrrname avatar Jul 14 '23 12:07 usrrname

Interesting... @usrrname Are you creating a proxy object in the light DOM that the label then references rather than the custom element?

break-stuff avatar Jul 14 '23 15:07 break-stuff

@break-stuff certain fast components like FASTTextField already instantiate a proxy object... I think I creeped that in the original class it's texted from: https://github.com/microsoft/fast/blob/master/packages/web-components/fast-foundation/src/text-field/text-field.form-associated.ts

usrrname avatar Jul 17 '23 15:07 usrrname