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#1908][MAS4.2.10][Focus Order - Add other service] With voiceover consolidated focus goes on "Key and Value". #2002

Merged
merged 11 commits into from
Nov 26, 2019

Conversation

denscollo
Copy link
Contributor

Solves #1908

Description

This pull request fixes the navigation with Voiceover through the key-value pair text fields in the Add other service ... dialog.

Now voiceover goes from the label key to its text field, then to the value label and then to its text field.

Changes made

We moved the labels used for the titles inside the first TextField element of the pairs, this way the label is associated with the input field it corresponds.

Also, we changed how the key-value pairs are displayed in the dialog, replacing a list for a table using rows as the tester suggested.

Finally, we made some changes to the styling to have the same visual representation as before. For this, we moved some style properties inside a new class to be used for the rows.

Testing

In the following recording, you can see Voiceover navigation as it should through the fields.
kvpair

@coveralls
Copy link

coveralls commented Nov 21, 2019

Coverage Status

Coverage decreased (-0.04%) to 68.75% when pulling 4961b64 on fix/kv-pair-voiceover-navigation into e549919 on master.

corinagum
corinagum previously approved these changes Nov 21, 2019
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.

This behavior doesn't seem correct to me. After adding another row of key value pairs, the behavior is that it navigates from the key label, to the input for key0, to the value label, to the input for value0, and then to key1, and to value1.

kv

This seems to me like it would confuse a user relying on the screen reader. I think this one particular component might actually be worth wrapping in a table structure with <td> <th> <tr>, etc, or we could try using a combination of <fieldset> and <legend> to group each pair.

Because with this current implementation we still have the same pattern that existed before when accessing rows 1+.

===

Also, tests for kvPair.spec.tsx are failing, and there were some linting errors in kvPair.tsx.

@denscollo
Copy link
Contributor Author

Thanks @tonyanziano for the feedback! We'll be working on this and let you know when we have it ready.

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.

Looks great now! Thanks.

@tonyanziano tonyanziano merged commit 3ecf25f into master Nov 26, 2019
@tonyanziano tonyanziano deleted the fix/kv-pair-voiceover-navigation branch November 26, 2019 20:38
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.

5 participants