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#1889][MAS4.2.10][Screen Reader-Open bot dialog] Keyboard focus and voiceover focus is not synchronized. #1995

Merged
merged 19 commits into from Dec 2, 2019

Conversation

@denscollo
Copy link
Collaborator

denscollo commented Nov 19, 2019

Solves #1889

Description

This pull request fixes the navigation with Voiceover, now it's cursor is synchronized with the keyboard in the following places:

  • "My Bots" section.
  • "How to Build a Bot" section.
  • "Connect your Bot to LUIS Application" dialog.
  • "Open a Bot" dialog.
  • "New bot configuration" dialog.
  • "Add language understanding" and "Add QnA maker" pop-up.
  • "Settings" section.
  • "Sign in with Azure account" dialog.

Changes made

To fix the behavior of TextFields we removed the styling of position: relative, now the Voiceover's cursor navigates normal thought these elements, without jumping to other places.

To fix the LinkButtons we added the role presentation to the paragraph containing these elements and to other containers around these paragraphs.

The openBotDialog dialog has two custom elements, an AutoComplete to which we added the presentation role and to the form that contains it, and the browse button, for which we added the aria-hidden="true" attribute to its label and made some styling changes so the Voiceover cursor is in the right place.

Testing

In the following recordings, you can see how these changes fixes the Voiceover's cursor following the keyboard's focus.
howtobuildabot
connectto
openabot

santgr11 added 5 commits Nov 14, 2019
Fixes the bot url text field inside the openBotDialog focus sync with voiceover
fix the position of the browse button in openBotDialog so voiceover position it's coursor in the right place
Fix the navigation with voiceover throught the links in the howToBuildABot section
Fixes the voiceover navigation  of the links in the dialogs to add services, add services manually and login to azure
denscollo added 2 commits Nov 19, 2019
@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 19, 2019

Coverage Status

Coverage remained the same at 68.753% when pulling b9adb97 on fix/synchronize-voiceover-focus into 7019832 on master.

@tonyanziano

This comment has been minimized.

Copy link
Contributor

tonyanziano commented Nov 20, 2019

I went back to the original issue and asked if the issue can be re-evaluated, because I'm unable to reproduce any of these focus synchronization issues using VoiceOver on Mac 10.13.

Here's how VoiceOver navigates on my machine:
out

I'm going to see if someone else on the team with a Mac can reproduce the focus issues using VoiceOver.

@denscollo

This comment has been minimized.

Copy link
Collaborator Author

denscollo commented Nov 21, 2019

@tonyanziano are you using vO navigation to test it? (Ctrl + alt + arrow), also we are using MacOS v10.15 (we found that depending on the OS version, the behaviour can change).

@tonyanziano

This comment has been minimized.

Copy link
Contributor

tonyanziano commented Nov 21, 2019

@denscollo Oh no I haven't. I didn't know that was a thing 😐

I'll retry using VoiceOver navigation.

denscollo added 2 commits Nov 22, 2019
Copy link
Contributor

tonyanziano left a comment

Once the unnecessary changes are reverted I'll merge this in :)

Looks good.

@@ -82,7 +82,7 @@ export class AutoComplete extends Component<AutoCompleteProps, AutoCompleteState
<div
className={`${styles.autoCompleteContainer} ${invalidClassName} ${className}`}
id={this.textboxId}
role="combobox"
role="presentation"

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Nov 22, 2019

Contributor

This is unnecessary and breaks the combobox pattern that the AutoComplete component follows:

https://www.w3.org/TR/wai-aria-practices/examples/combobox/aria1.1pattern/listbox-combo.html

This comment has been minimized.

Copy link
@denscollo

denscollo Nov 26, 2019

Author Collaborator

@tonyanziano We ran some tests and if we change the role to combobox, it won't navigate to this control, only to the Title of this control and then to the browse button (as seen on the attached gif below). Also, we reviewed the example that you sent us and it happens the same.

combobox

Do you want us to change it anyways?

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Nov 26, 2019

Contributor

Hey @denscollo ,

Before I suggested the change I tested it, and it seemed to work fine. Here's an example of me testing it out this morning:

combobox

I wonder why it is behaving so much differently on your machine.

This comment has been minimized.

Copy link
@denscollo

denscollo Nov 29, 2019

Author Collaborator

@tonyanziano after reading the aria documentation you left, we noticed that the current implementation of the auto-completed component does not match the pattern. The element is out of the combobox container. By updating the component to follow this pattern the issue was fixed. We'll be pushing the changes in a bit.

issue1889

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Dec 2, 2019

Contributor

Hi @denscollo ,

If you look at the sample code for the combobox on the w3 example I posted above, the DOM structure looks like this:

<div>       <!-- not important to accessibility, just for layout -->
  <div role="combobox">         <!-- container, important to pattern -->
    <input>                     <!-- text field, important to pattern -->
  </div>
  <ul>                          <!-- results, important to pattern -->
</div>

If you look at the DOM structure that currently exists before your changes:

<div role="combobox">         <!-- container, important to pattern -->
  <input>                     <!-- text field, important to pattern -->
  <ul>                        <!-- results, important to pattern -->
</div>

The only thing that we have excluded in the pattern is the outermost container, because we have wrapped the results in the combobox div instead of placing them outside, eliminating the need for the outermost div.

However, if you look at all the required roles, and attributes of the component, our DOM structure still satisfies those requirements.

The problem with your original changes at the top of this comment thread is that you changed the role of the combobox div to role="presentation", which means that we no longer satisfy the component's role / attribute requirements.

I don't think it's necessary for us to add another container to the DOM structure, and mark it with role="presentation" because you are basically adding another element to the DOM and telling screen readers to ignore it. The current DOM implementation achieves the same exact layout without the extra div, and looks the same in the eyes of the screen reader.

Below are examples of VoiceOver's navigation over each of the implementations.

Current implementation without extra <div>:

old-dom

Proposed implementation with extra <div>:

new-dom

You can see that both implementations behave and read the same, so I don't think the DOM structure should be modified to include an extra <div> element.

I am going to revert the CSS changes, and change the DOM structure back to what it was, and merge this PR in for the sake of time.

I'm happy to continue discussing the reasoning behind this change with you offline if you'd like.

corinagum and others added 9 commits Nov 25, 2019
…t/BotFramework-Emulator into fix/synchronize-voiceover-focus
@tonyanziano tonyanziano merged commit 553d64a into master Dec 2, 2019
2 checks passed
2 checks passed
Emulator-CI-PR #93308 succeeded
Details
license/cla All CLA requirements met.
Details
@tonyanziano tonyanziano deleted the fix/synchronize-voiceover-focus branch Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.