fix(ASSETS-17276):Form elements must have labels. #278
Conversation
|
Ready for review @Pareesh @RitwikSrivastava |
| @@ -1,7 +1,7 @@ | |||
| <js> | |||
| data.uid = data.commons.getUID(); | |||
| </js> | |||
| <input is="coral-textfield" handle="input" type="number" class="_coral-Stepper-input" id="{{data.uid}}" step="1"> | |||
| <input is="coral-textfield" handle="input" type="number" class="_coral-Stepper-input" id="{{data.uid}}" step="1" title="{{data.i18n.get('NumberInput')}}"> | |||
There was a problem hiding this comment.
Adding a default generic accessibility name for this control will hide the issue an prevent automated testing from detecting cases where a label has not been provided for the control. The appropriate way to label this control would be to add a labelledBy attribute to the <coral-numberinput /> element that references the DOM element that labels the control, as demonstrated in each example on https://opensource.adobe.com/coral-spectrum/examples/#numberinput. This is not an issue with the coral-component-numberinput, but rather with the instance where it is being used in AEM.
There was a problem hiding this comment.
Thank you for clarifying @majornista. Indeed the instance where this coral-component-numberinput is used is different from other instances by not having a label for the input, otherwise an aria-labelledby attributed is added automatically. Screenshots from the same page but different tabs, with and without label:
It's why I opted for adding a title but you are right about the issue that it would hide.
In this case should this be marked as not an issue?
There was a problem hiding this comment.
This is not an issue for Coral Spectrum, but it is an issue for the number input instance for the Creative Rating within AEM. The fix would be to add a unique id attribute to the "Creative Rating" heading preceding the coral-numberinput and then use that id as the value for the labelledBy attribute on the coral-numberinput instance.
There was a problem hiding this comment.
@ionelc-ensemble Here's a brief explanation of why we use labelledBy and labelled to provide labels to controls in CoralUI and Coral-Spectrum. Coral components are built using web components specification but only implement Custom Elements v1. For composite controls like the coral-numberinput, we can't simply add aria-label, aria-labelledby, or expect to assign an id, to be referenced using a for attribute on a label, to the coral-numberinput element because we would only label the container and not the actual input that gets rendered within it. In order to ensure that aria-label or aria-labelledby get added to the appropriate rendered html element, the BaseFormField defines labelled, labelledBy and describedBy attributes to use instead of aria-label, aria-labelledby, or aria-describedby. These setters locate the appropriate labellable element with the rendered component and add aria-label, aria-labelledby, or aria-describedby to provide an accessible name to the control. I hope this makes sense and helps.
There was a problem hiding this comment.
@majornista that was helpful, thanks for taking the time to show the bigger picture.
While adding an ID to the 'Creative Rating' heading however, I got to the root of the issue: the rating input was missing the string for the label. Added that and everything fell into place.
My concern now is whether that rating input SHOULD be without a label for any reason, in which case I will continue working for a fix here.
Else will closed this PR after validating the new fix that adds the label string
https://jira.corp.adobe.com/browse/ASSETS-17276
https://jira.corp.adobe.com/browse/ASSETS-17274
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Tested with AxeDevTools
Screenshots (if appropriate):
Types of changes
Checklist: