-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Add a11y test suite #88623
[Lens] Add a11y test suite #88623
Conversation
@@ -171,7 +171,11 @@ export function ReorderProvider({ | |||
<EuiPortal> | |||
<EuiScreenReaderOnly> | |||
<div> | |||
<p id="lnsDragDrop-reorderAnnouncement" aria-live="assertive" aria-atomic={true}> | |||
<p | |||
id={`lnsDragDrop-reorderAnnouncement-${id}`} |
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.
2 things here:
- Why do we need an id on this element? AFAICT it's not referenced anywhere (and if it is, we have to adjust that place as well)
- The test is failing because the id is the group id, so if there are multiple layers there will be duplicates. Probably fixable by incorporating the layer id into the reorder provider id here:
kibana/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx
Line 225 in 898385c
<ReorderProvider id={group.groupId} className={'lnsLayerPanel__group'}>
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.
Looking at the debug page for the failure, there's effectively a duplication of div
s, hence the id duplication.
Removing the id
from the div
will certainly fix the test, but there's still a problem with having twice as much divs with the same content on the page. I'll try to fix this as well.
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, pretty cool to have these around and extra cool they found issues already :) I'm afraid we need to find another workaround for the duplicated id issue because we rely on them for a11y
{state.keyboardReorderMessage} | ||
</p> | ||
<p id={`lnsDragDrop-reorderInstructions-${id}`}> |
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.
We can't merge this because this id is referenced from the buttons:
aria-describedby={`lnsDragDrop-reorderInstructions-${groupId}`} |
We need it to tie the message and the button together
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.
Sorry, I thought I searched for it, but maybe I was wrong with the other one. Will reintroduce it back! 👍
@elasticmachine merge upstream |
@elasticmachine merge upstream |
Pinging @elastic/kibana-app (Team:KibanaApp) |
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.
Code LGTM once green, didn't test again
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
* ✅ Add a11y test suite for Lens + some fixes * 🔧 Removed unused ids * 🐛 Fix component duplication via key prop * :white_mark_check: Add more a11y tests * ♻️ Restored back reorder id + fix multi layer duplicate ids * 🐛 Fix elastic#88806 Co-authored-by: Kibana Machine <[email protected]>
* ✅ Add a11y test suite for Lens + some fixes * 🔧 Removed unused ids * 🐛 Fix component duplication via key prop * :white_mark_check: Add more a11y tests * ♻️ Restored back reorder id + fix multi layer duplicate ids * 🐛 Fix #88806 Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Summary
Fixes #83594 and #88806
This PR introduces the first set of Lens a11y test suite.
TODO:
Checklist