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

[WIP] [ML] (Accessibility) Read summary info for File contents and Summary stats #31117 #34686

Closed
wants to merge 6 commits into from

Conversation

PhilippBaranovskiy
Copy link
Contributor

@PhilippBaranovskiy PhilippBaranovskiy commented Apr 8, 2019

Fix: #31117

Summary

I added aria labels to inner <span/>s as well as used titleProps parameter for EuiDescriptionList to add tab indexes to <dt/> elements in order to make them focusable.

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@maryia-lapata maryia-lapata left a comment

Choose a reason for hiding this comment

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

Please take a look at the comments

maryia-lapata and others added 4 commits April 9, 2019 06:52
Co-Authored-By: rockfield <philipp.b@ya.ru>
Co-Authored-By: rockfield <philipp.b@ya.ru>
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@PhilippBaranovskiy
Copy link
Contributor Author

@jgowdyelastic Thanks for your review! I've renamed the function as well as have added object destruction as you suggested.
Please, have a look and confirm the PR whether everything looks good for you.

@PhilippBaranovskiy
Copy link
Contributor Author

@maryia-lapata, thank you for the review! I've applied suggestion you made, moreover have updated translate() parameters and defaultMesssage exactly as you suggested.
Please, have a look and confirm PR whether everything looks good for you! Otherwise, let me know what could be improved.

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@maryia-lapata maryia-lapata left a comment

Choose a reason for hiding this comment

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

LGTM, checked with VoiceOver

@peteharverson
Copy link
Contributor

@rockfield @maryia-lapata the various parts of the Summary panel are now tabbable, with aria-label properties added, but there does not seem to be any indication that the tabbable elements (h3 and dt) have focus. Do you know if there is a requirement that we should provide visible indication of which element has focus?

@PhilippBaranovskiy
Copy link
Contributor Author

PhilippBaranovskiy commented Apr 9, 2019

@peteharverson thanks for pointing that out! I'm not sure whether there is a requirement for that,
however visible indication is always better than lack of that 🙆‍♂️
So, I'm looking into styles and checking that.
Thank you so much!

Do you know if there is a requirement that we should provide a visible indication of which element has a focus?

@maryia-lapata

@PhilippBaranovskiy PhilippBaranovskiy changed the title [ML] (Accessibility) Read summary info for File contents and Summary stats #31117 ❌ [ML] (Accessibility) Read summary info for File contents and Summary stats #31117 Apr 9, 2019
@PhilippBaranovskiy PhilippBaranovskiy changed the title ❌ [ML] (Accessibility) Read summary info for File contents and Summary stats #31117 [WIP] [ML] (Accessibility) Read summary info for File contents and Summary stats #31117 Apr 11, 2019
@PhilippBaranovskiy
Copy link
Contributor Author

PhilippBaranovskiy commented Apr 15, 2019

Hello,
I've tested this functionality with NVDA and Jaws screen readers on Windows (which is supposed to be the app that is used by real people in real life) as well as with VoiceOver on Mac -- it works correctly with initial implementation (before this PR).

It does due to the fact that the working with page content happens in a slightly different way neither switching focus from one item to another.
If I need to go to the next piece of content, I just hit the key. That's how I read it line by line (that's what this issue is about).

For instance, for VoiceOver those commands are:

  • Control + + + H: go to the "Summary" heading
  • Control + + + shift ⇧ + H: go back to the "File contents" heading
  • Control + + : to go to the "Number of lines analyzed" and so forth

For NVDA (Win) keys are slightly different:

  • H: go to the "Summary" heading
  • shift ⇧ + H: go back to the "File contents" heading
  • Control + : to go to the "Number of lines analyzed" and further

Also, in this mode the :focus state is not applied to the element as screen readers handle that indication by itself (VoiceOver does, NVDA doesn't). So if even I code that in .css, these changes are not going to be visible.

The way we used to see this process (hitting tab to go throughout the content) is not supposed to work this way.
Tabbing is supposed to go throughout the interactive items such as links, button, selects.

Since all described above, I suggest that I should decline this PR and close initial #31117 issue.
@peteharverson, @maryia-lapata, @rayafratkina, please confirm.

On the following video you can see how the initial implementation (before this PR) works with VoiceOver on Mac:

voice_over

@peteharverson
Copy link
Contributor

I confirmed that using NVDA the Summary section is read out line by line before this PR (using H and arrow down navigation keys).

@rayafratkina can you confirm that it is ok to close this PR without merging, and close the original issue #31117, as it seems that using screenreaders NVDA / VoiceOver on Mac, that this is working correctly as-is?

@rayafratkina
Copy link
Contributor

@peteharverson @rockfield yes, it's ok to close the original issue without making any changes.
Not sure if something changed along the way or it was just a misunderstanding, but the screen is readable and that is all we need.
Thanks for digging into this!

@PhilippBaranovskiy
Copy link
Contributor Author

I'm closing this PR as the initial implementation is correct, — explanation is in the comment above.

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

Successfully merging this pull request may close these issues.

[ML] (Accessibility) Read summary info for File contents and Summary stats
7 participants