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
Collaborator

denscollo commented Nov 21, 2019

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

This comment has been minimized.

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.

Copy link
Contributor

tonyanziano left a comment

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

This comment has been minimized.

Copy link
Collaborator Author

denscollo commented Nov 22, 2019

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

denscollo and others added 6 commits Nov 25, 2019
Copy link
Contributor

tonyanziano left a comment

Looks great now! Thanks.

@tonyanziano tonyanziano merged commit 3ecf25f into master Nov 26, 2019
2 checks passed
2 checks passed
Emulator-CI-PR #92559 succeeded
Details
license/cla All CLA requirements met.
Details
@tonyanziano tonyanziano deleted the fix/kv-pair-voiceover-navigation branch Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.