-
Notifications
You must be signed in to change notification settings - Fork 750
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
Conversation
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
@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). |
@denscollo Oh no I haven't. I didn't know that was a thing 😐 I'll retry using VoiceOver navigation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the unnecessary changes are reverted I'll merge this in :)
Looks good.
packages/app/client/src/ui/dialogs/openBotDialog/openBotDialog.scss
Outdated
Show resolved
Hide resolved
@@ -82,7 +82,7 @@ export class AutoComplete extends Component<AutoCompleteProps, AutoCompleteState | |||
<div | |||
className={`${styles.autoCompleteContainer} ${invalidClassName} ${className}`} | |||
id={this.textboxId} | |||
role="combobox" | |||
role="presentation" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Do you want us to change it anyways?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
I wonder why it is behaving so much differently on your machine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
:
Proposed implementation with extra <div>
:
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.
…t/BotFramework-Emulator into fix/synchronize-voiceover-focus
Solves #1889
Description
This pull request fixes the navigation with Voiceover, now it's cursor is synchronized with the keyboard in the following places:
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](https://user-images.githubusercontent.com/38112957/69183937-a8e27c00-0af2-11ea-88ff-41aa4b106e5a.gif)
![connectto](https://user-images.githubusercontent.com/38112957/69184166-17273e80-0af3-11ea-8bce-d22e02f338a7.gif)
![openabot](https://user-images.githubusercontent.com/38112957/69184294-5bb2da00-0af3-11ea-8a3f-588135905c9a.gif)