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

[ISSUE#1903][MAS4.2.5] [Labels and Relationships - Settings] Voiceover announce the same label for “Path to ngrok", "Locale", "Localhost Override"and "User ID" present in the "settings" pane #1970

Merged
merged 3 commits into from
Nov 7, 2019

Conversation

denscollo
Copy link
Contributor

Solves #1903

Description

Fixes how Voiceover announces the TextFields' labels in the Settings editor pane. This error was extended to TextFields in other components as well.

Changes made

We added a condition to the setting of the aria-labelledby attribute in the TextField component. This way, Voiceover will read the error message only when related to the rendered element, otherwise, it will read the element's label.

Additionally, we noticed that this issue was happening on windows as well. The same solution applies to both platforms.

Testing

In the following images, you can see how Voiceover reads the labels instead of the error message:

image
image
image

@coveralls
Copy link

coveralls commented Nov 5, 2019

Coverage Status

Coverage increased (+0.005%) to 67.092% when pulling cdaf343 on fix/incorrect-settings-labels-announcement into a070872 on master.

Copy link
Contributor

@tonyanziano tonyanziano left a comment

Choose a reason for hiding this comment

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

The behavior is right, but I think that the code used to achieve the behavior is slightly off. I left a comment explaining why.

@@ -71,7 +71,7 @@ export class TextField extends Component<TextFieldProps, {}> {
<div className={`${styles.inputContainer} ${inputContainerClassName}`}>
{this.labelNode}
<input
aria-labelledby="errormessagesub"
aria-labelledby={errorMessage ? 'errormessagesub' : 'label'}
Copy link
Contributor

@tonyanziano tonyanziano Nov 5, 2019

Choose a reason for hiding this comment

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

I think that this should be:

aria-labelledby={errorMessage ? 'errormessagesub' : undefined}

aria-labelledby requires

A space-separated list of element IDs

that are supposed to describe the element that the attribute has been attached to. Your change is saying that the <input> tag should be describe by some element with id="label", however, if you look at the HTML markup for the .labelNode of the <TextField> component, it does not have an ID associated with it.

So what I think is actually happening here is that the aria-labelledby attribute tries to find the associated element with id="label", fails, and then falls back to default accessibility behavior, which is to look at the nearest <label> tag with the for="<input id>" attribute assigned. This is the correct behavior.

I've tried changing it to aria-labelledby={errorMessage ? 'errormessagesub' : undefined} on Windows, and the behavior remains the same as your change.

You should try it out on Mac, and if it works, then we are good to merge it in. 👍

Copy link
Contributor Author

@denscollo denscollo Nov 7, 2019

Choose a reason for hiding this comment

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

Thanks @tonyanziano for the explanation! We tried it on Mac and it works perfectly, so we just updated the code with that modification.

Copy link
Contributor

@tonyanziano tonyanziano left a comment

Choose a reason for hiding this comment

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

Great! Thanks :)

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

Successfully merging this pull request may close these issues.

4 participants