-
Notifications
You must be signed in to change notification settings - Fork 842
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
[EuiCodeBlock] Improve accessibility of expandable code blocks #8195
[EuiCodeBlock] Improve accessibility of expandable code blocks #8195
Conversation
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.
The changes look clean and the result is as expected, yay! 🥳 The code block is announced correctly both on focus and expansion.
I tried EuiCodeBlock
with and without overflowHeight
and isCopyable
set.
2 questions:
- Do you think it would be sensible to cover this by an automated a11y test?
- Do we want this first approved by an accessibility contractor? Maybe it would be useful to make it a part of all (/most) accessibility tickets and have this check in the PR template?
Role and label:
VoiceOver:
no overflowHeight
set:
Screen.Recording.2024-11-29.at.15.10.26.mov
overflowHeight
set to 1:
Screen.Recording.2024-11-29.at.15.11.01.mov
overflowHeight
set to 1 and isCopyable
to true:
Screen.Recording.2024-11-29.at.15.13.13.mov
Note: IMO the UX for copying the content could be improved. There's no information that the content has been copied on clicking the button.
I have 2 more suggestions and a question below 👇🏻
@L1nBra Could you have a look as well (ref original issue)? |
We could add a general test to cover keyboard navigation, yes. Beyond that we couldn't test much further as we can't assert screen reader output. (unless we implement something like guidepup)
Thanks for the reminder, I wanted to ping the contractor yesterday but forgot 😅 I'm not sure this needs to be a standard per se. 🤔 If we add it we should make it a) optional (default) and b) non-blocker. |
@weronikaolejniczak Thanks for checking in VoiceOver! That's great to have an additional check. Just fyi, we need to keep in mind that VoiceOver is not the screen reader used by the majority of screen reader users. (there is no statistics but the regular Webaim screen reader surveys give us some idea) |
Whatever you feel is worth it here. I would cover keyboard navigation, assert focus and labels, and if you want to go outside of the scope and assert screen reader output, amazing! 👍🏻
That's very true! I agree, we shouldn't make it a requirement, I would add it as a check "If applicable (...)" after the current accessibility check, just for there to be a reminder when we open a PR 😄 I can mention this in our Slack channel and we can discuss there.
That's a very good point, thanks for bringing this up! 💚 Since you tested with NVDA and JAWS, and even provided videos, I figured - why not do this additional check with VoiceOver 🤷🏻♀️ In most surveys it comes as third so it's definitely worth checking. I'm going to leave an approve here, feel free to ping me if something changes after the a11y review and potential test fixes 😄 |
@weronikaolejniczak NVDA and JAWS work fine here. The |
3b23b17
to
785ac3e
Compare
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
Checked with keyboard, NVDA screen reader on Windows 11 Chrome browser from A11y side: |
`v98.1.0-borealis.0`⏩`v98.2.1-borealis.2` _[Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_ --- # `@elastic/eui` ## [`v98.2.1`](https://github.com/elastic/eui/releases/v98.2.1) - Updated the EUI theme color values to use a full 6 char hex code format ([#8244](elastic/eui#8244)) ## [`v98.2.0`](https://github.com/elastic/eui/releases/v98.2.0) - Added two new icons: `contrast` and `contrastHigh` ([#8216](elastic/eui#8216)) - Updated `EuiDataGrid` content to have a transparent background. ([#8220](elastic/eui#8220)) **Accessibility** - When the tooltips components (`EuiTooltip`, `EuiIconTip`) are used inside components that handle the Escape key (like `EuiFlyout` or `EuiModal`), pressing the Escape key will now only close the tooltip and not the entire wrapping component. ([#8140](elastic/eui#8140)) - Improved the accessibility of `EuiCodeBlock`s by ([#8195](elastic/eui#8195)) - adding screen reader only labels - adding `role="dialog"` on in fullscreen mode - ensuring focus is returned on closing fullscreen mode # Borealis updates - [Visual Refresh] Update color token mappings ([#8211](elastic/eui#8211)) - [Visual Refresh] Introduce shared popover arrow styles to Borealis ([#8212](elastic/eui#8212)) - [Visual Refresh] Add forms.maxWidth token ([#8221](elastic/eui#8221)) - [Visual Refresh] Set darker shade for subdued text ([#8224](elastic/eui#8224)) - [Visual Refresh] Remove support for accentSecondary badges ([#8224](elastic/eui#8227)) - [Visual Refresh] Add severity vis colors ([#8247](elastic/eui#8247)) - [Visual Refresh] Fix transparent color variable definitions ([8249](elastic/eui#8249)) - [Visual Refresh] Update EuiToken colors ([8250](elastic/eui#8250)) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Summary
closes #8175.
This PR aims to improve the accessibility of the
EuiCodeBlock
component, especially the expandable/fullscreen functionality.Accessibility issues
height
is set to provide scrolling via arrow keys - similar to a listbox pattern)Changes
<pre>
elements don't have any semantic information (without adding a specificrole
) and hence also don't supportaria-label
to add additional semantic information directly.<code>
elements don't acceptaria-label
and similar attributes either.To address this a screen-reader only text was added inside the element to be read with the content. This includes the
language
prop of the code block to increase context.The fullscreen wrapper was not semantically marked and therefore did not provide any information about context change. The container is fullscreen and it has a focus trap, therefore it should be semantically defined as a dialog. Additionally this allows to apply an
aria-label
to the element, providing context on opening the dialog.This will result in the following experience:
ArrowUp/Down
keys navigate the code block content line by line (screen reader browse modes)Escape
key anywhere in the dialog closes the dialog and returns the focus to the expand buttonEnter
key on it closes the dialog and returns the focus to the expand buttonThis PR adds a small change to the toggle functionality to ensure the focus is returned to the expand button when closing the fullscreen dialog.
Testing
NVDA
Screen.Recording.2024-11-28.at.15.37.22.mov
JAWS
Screen.Recording.2024-11-28.at.15.35.41.mov
QA
overflowHeight
control to50
to reduce the height and enable the expandable code block#generic
#a11y
language
prop)General checklist
Checked in both light and dark modesChecked in mobileAdded documentationProps have proper autodocs (using(https://github.com/elastic/eui/blob/main/wiki/contributing-to-eui/documenting/playgrounds.md)**@default
if default values are missing) and **[playground toggles]Checked Code Sandbox works for any docs examplesUpdated visual regression testsIf applicable, added the breaking change issue label (and filled out the breaking change checklist)If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)