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.

Sign up for GitHub

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

Jump to bottom

fix(ASSETS-17276):Form elements must have labels. #278

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(ASSETS-17276):Form elements must have labels. #278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion coral-component-numberinput/i18n/translations.js
View file
Open in desktop
Expand Up @@ -13,7 +13,8 @@
export default {
"en-US": {
"Decrement": "Decrement",
"Increment": "Increment"
"Increment": "Increment",
"NumberInput": "Number Input",
},
"fr-FR": {
"Decrement": "Décrémenter",
Expand Down
2 changes: 1 addition & 1 deletion coral-component-numberinput/src/templates/base.html
View file
Open in desktop
@@ -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')}}">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Screen Shot 2023-01-31 at 14 32 46
Screen Shot 2023-01-31 at 14 35 10

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

<span class="_coral-Stepper-buttons" role="presentation" handle="presentation">
<button variant="action" class="_coral-Stepper-stepUp" type="button" is="coral-button" handle="stepUp" title="{{data.i18n.get('Increment')}}" aria-label="{{data.i18n.get('Increment')}}" tabindex="-1" aria-controls="{{data.uid}}">
<js>
Expand Down