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

[Lens] accessibility screen reader issues #84395

Merged
merged 6 commits into from Dec 3, 2020

Conversation

mbondyra
Copy link
Contributor

@mbondyra mbondyra commented Nov 26, 2020

Summary

Fixes #83872

Hey @myasonik here are my responses to the issues you listed:

  • All of the field lists should be lists (ul > li > button)
    we are not able to make button a direct child of li, as it’s a popover… Is this ok?

image

  • The layer configuration flyout needs to should have a proper heading, not just a button that looks like one

    • Recommendation: Make the button just the back arrow icon, then make the text an h2. You can still put a hover effect and click action on the h2 and the back arrow icon will maintain the semantics you need and be able to accurately describe what's happening.
      Done
  • Select a function buttons act like a radio group... They should be a radio group.
    Eui doesn't provide styling for radio group that we need here. Should we make this a separate issue as it seems like a bigger effort (and also an effort from EUI team side)?

  • "Remove configuration" button should say what configuration it will remove (e.g., "Remove timestamp from Horizontal axis")
    Done

  • The data-test-subj="lns-dimensionTrigger" button has an aria-label of "click to edit configuration or drag to move". Similar to above, it needs to say what configuration it is editing. There's no way to figure that out right now with activating it. Also while drag+drop is not keyboard accessible, it is probably not worth mentioning...
    It is keyboard accessible in groups for reordering (check data table or pie chart - for xy charts we currently have a bug so it’s turned off)
changed to “Click to edit configuration for {label} or drag to move”.

  • All of the "Drop a field or click to add" buttons should have an aria-label adding to which where this field will be added (e.g., `aria-label="Drop a field or click to add to the vertical axis")
    Done

  • All of the "Delete layer" buttons should have an aria-label describing which layer they will trash.
    Added an index (we don’t have other way of identifying layer, except for long and random id)

  • When the visualization is unsaved, the h1 just says "Unsaved". "Unsaved visualization" or "Unsaved workspace" or something along those lines would be better.
    Changed to Unsaved visualization

@mbondyra mbondyra requested a review from a team November 26, 2020 09:18
@mbondyra mbondyra requested a review from a team as a code owner November 26, 2020 09:18
@mbondyra mbondyra added Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0 labels Nov 26, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@mbondyra mbondyra added this to In progress in Lens via automation Nov 26, 2020
@mbondyra mbondyra force-pushed the lens/accessibility-screen-reader branch 3 times, most recently from 74bf08b to a6e204f Compare November 26, 2020 16:57
@myasonik
Copy link
Contributor

Select a function buttons act like a radio group... They should be a radio group.
Eui doesn't provide styling for radio group that we need here. Should we make this a separate issue as it seems like a bigger effort (and also an effort from EUI team side)?

Yeah, I'd open an EUI issue for it to get discussed. There's a chance EUI team will decide it's better to implement it in app though so it might still come back but it can be tracked as a separate effort.

@mbondyra
Copy link
Contributor Author

@elasticmachine merge upstream

@mbondyra mbondyra force-pushed the lens/accessibility-screen-reader branch from 2e96cfd to 2b6c991 Compare November 30, 2020 14:17
@mbondyra mbondyra force-pushed the lens/accessibility-screen-reader branch from 2b6c991 to d842c17 Compare November 30, 2020 14:21
@mbondyra mbondyra removed the request for review from a team November 30, 2020 14:21
@mbondyra
Copy link
Contributor Author

mbondyra commented Dec 1, 2020

@elasticmachine merge upstream

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Looks really good to me, left some small comments.

onClick={closeFlyout}
>
<EuiFlexItem grow={false}>
<EuiButtonIcon
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks a little strange because the button has the little y translation on hover while the heading doesn't: Can we make it look like the old version (no translation)?
Kapture 2020-12-01 at 15 26 55

@mbondyra mbondyra force-pushed the lens/accessibility-screen-reader branch from 905f600 to 8178040 Compare December 1, 2020 16:25
@mbondyra mbondyra force-pushed the lens/accessibility-screen-reader branch from 8178040 to fa93967 Compare December 2, 2020 16:54
Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

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

This PR is 🔥

🚀

@mbondyra
Copy link
Contributor Author

mbondyra commented Dec 3, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 995.8KB 999.6KB +3.8KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested in Chrome and Firefox, works fine, looks fine, LGTM

@mbondyra mbondyra merged commit 487eb2e into elastic:master Dec 3, 2020
Lens automation moved this from In progress to Done Dec 3, 2020
mbondyra added a commit to mbondyra/kibana that referenced this pull request Dec 3, 2020
* [Lens] accessibility screen reader issues

* fix i18n

* fix: no aria-label on divs

* cr fixes

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@mbondyra mbondyra deleted the lens/accessibility-screen-reader branch December 3, 2020 12:29
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 3, 2020
…overy-action-group

* upstream/master: (48 commits)
  [Lens] accessibility screen reader issues (elastic#84395)
  [Logs UI] Fetch single log entries via a search strategy (elastic#81710)
  fix: 🐛 don't add separator befor group on no main items (elastic#83166)
  [Security Solution][Detections] Implements indicator match rule cypress test (elastic#84323)
  [APM] Add APM agent config options (elastic#84678)
  Fixed a11y issue on rollup jobs table selection (elastic#84567)
  [Discover] Refactor getContextUrl to separate file (elastic#84503)
  [Embeddable] Export CSV action for Lens embeddables in dashboard (elastic#83654)
  [TSVB] [Cleanup] Remove extra dateFormat props (elastic#84749)
  [Lens] Migrate legacy es client and remove total hits as int (elastic#84340)
  Improve logging pipeline in @kbn/legacy-logging (elastic#84629)
  Catch @hapi/podium errors (elastic#84575)
  [Discover] Unskip date histogram test (elastic#84727)
  Rename server.xsrf.whitelist to server.xsrf.allowlist (elastic#84791)
  [Enterprise Search] Fix schema errors button (elastic#84842)
  [APM] Removes react-sticky dependency in favor of using CSS (elastic#84589)
  [Maps] Always initialize routes on server-startup (elastic#84806)
  [Fleet] EPM support to handle uploaded file paths (elastic#84708)
  [Snapshot Restore] Fix initial policy form state (elastic#83928)
  Upgrade Node.js to version 14 (elastic#83425)
  ...
mbondyra added a commit that referenced this pull request Dec 3, 2020
* [Lens] accessibility screen reader issues

* fix i18n

* fix: no aria-label on divs

* cr fixes

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0
Projects
No open projects
Lens
  
Done
Development

Successfully merging this pull request may close these issues.

[Lens](Accessibility) Small HTML improvements for better screen reader support
5 participants