[EuiBasicTable] Fix duplicate ID in EuiBasicTable's Select All checkbox(es) #5237
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- the Select All checkbox is shown in both the mobile and regular table header, so it was creating an a11y duplicate ID error
|
NB: Our EUI a11y tests would have caught this issue, but unfortunately they're currently disabled for basic and in-memory table. I gave myself a timeboxed hour or so to try and fix them alongside this PR, but there's too many other failures on the page - I'll have to consult with Trevor on those and open a separate PR later. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5237/ |
cee-chen
changed the title
cee-chen
added a commit
to cee-chen/eui
that referenced
this pull request
Oct 4, 2021
- missing labels on checkbox elements, duplicate checkbox ID (solved in elastic#5237) - These are all a11y solutions that come OOTB in EuiBasicTable, but need to be added for the custom example
cee-chen
added a commit
to cee-chen/eui
that referenced
this pull request
Oct 4, 2021
- missing labels on checkbox elements, duplicate checkbox ID (solved in elastic#5237) - These are all a11y solutions that come OOTB in EuiBasicTable, but need to be added for the custom example
cee-chen
added a commit
to cee-chen/eui
that referenced
this pull request
Oct 4, 2021
- missing labels on checkbox elements, duplicate checkbox ID (solved in elastic#5237) - These are all a11y solutions that come OOTB in EuiBasicTable, but need to be added for the custom example
3 tasks
cee-chen
added a commit
to cee-chen/eui
that referenced
this pull request
Oct 5, 2021
- missing labels on checkbox elements, duplicate checkbox ID (solved in elastic#5237) - These are all a11y solutions that come OOTB in EuiBasicTable, but need to be added for the custom example
cee-chen
pushed a commit
that referenced
this pull request
Oct 5, 2021
…ors on EuiBasicTable and EuiInMemoryTable pages (#5241) * Fix duplicate checkbox `id`s with multiple isSelectable EuiBasicTables on the page - This is possibly only an issue with our docs and the fact that we use the same dataset repeatedly across multiple examples, but solves the error of duplicated IDs and shouldn't negatively affect production users * Fix duplicate popover `id`s with multiple EuiInMemoryTable filters on the page - there's no real need for this popover to have a custom ID instead of a randomized one - remove it - This is possibly only an issue with our docs and the fact that we use the same dataset repeatedly across examples, but solves the error of duplicated IDs and shouldn't negatively affect production users * Fix missing `aria-label` on table pagination - An `aria-label` was being passed to the `PaginationBar` component, but it wasn't actually being correctly used: - `PaginationBar` was never passing an `aria-label` prop down to `EuiTablePagination` or `EuiPagination` - The i18n typing was passing a react element instead of a string via render prop * Fix multiple `landmark-unique` issues on EuiBasicTable & EuiInMemoryTable docs - While we added an aria-label for each table's paginatoin nav, they still need to be unique for multiple tables on the page, which means adding a tableCaption for each demo * Fix `scope-attr-valid` errors on empty table column headings - empty `td`s within a `thead` is valid per https://webaim.org/techniques/tables/data's examples, but should not have the `scope` attr set * Improve demos with empty table headings - I opted to use visually empty headings, populated with EuiScreenReaderOnly text as an example for users looking to implement their own columns + fix odd data-test-subjs caused by either undefined or node column names * Fix various axe failures on custom EuiTable example - missing labels on checkbox elements, duplicate checkbox ID (solved in #5237) - These are all a11y solutions that come OOTB in EuiBasicTable, but need to be added for the custom example * Fix unsemantic headings in EuiBasicTable responsive documentation - these should very likely just be paragraphs, not headings * Re-enable basic table & in-memory table documentations in a11y tests * Add changelog entry * PR feedback: screen reader landmark copy
ym
pushed a commit
to ym/eui
that referenced
this pull request
Oct 29, 2021
…ors on EuiBasicTable and EuiInMemoryTable pages (elastic#5241) * Fix duplicate checkbox `id`s with multiple isSelectable EuiBasicTables on the page - This is possibly only an issue with our docs and the fact that we use the same dataset repeatedly across multiple examples, but solves the error of duplicated IDs and shouldn't negatively affect production users * Fix duplicate popover `id`s with multiple EuiInMemoryTable filters on the page - there's no real need for this popover to have a custom ID instead of a randomized one - remove it - This is possibly only an issue with our docs and the fact that we use the same dataset repeatedly across examples, but solves the error of duplicated IDs and shouldn't negatively affect production users * Fix missing `aria-label` on table pagination - An `aria-label` was being passed to the `PaginationBar` component, but it wasn't actually being correctly used: - `PaginationBar` was never passing an `aria-label` prop down to `EuiTablePagination` or `EuiPagination` - The i18n typing was passing a react element instead of a string via render prop * Fix multiple `landmark-unique` issues on EuiBasicTable & EuiInMemoryTable docs - While we added an aria-label for each table's paginatoin nav, they still need to be unique for multiple tables on the page, which means adding a tableCaption for each demo * Fix `scope-attr-valid` errors on empty table column headings - empty `td`s within a `thead` is valid per https://webaim.org/techniques/tables/data's examples, but should not have the `scope` attr set * Improve demos with empty table headings - I opted to use visually empty headings, populated with EuiScreenReaderOnly text as an example for users looking to implement their own columns + fix odd data-test-subjs caused by either undefined or node column names * Fix various axe failures on custom EuiTable example - missing labels on checkbox elements, duplicate checkbox ID (solved in elastic#5237) - These are all a11y solutions that come OOTB in EuiBasicTable, but need to be added for the custom example * Fix unsemantic headings in EuiBasicTable responsive documentation - these should very likely just be paragraphs, not headings * Re-enable basic table & in-memory table documentations in a11y tests * Add changelog entry * PR feedback: screen reader landmark copy
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Bug fix/follow up to #5196 (comment)
I hadn't realized during my initial testing that two
select allcheckboxes are rendered - one in the mobile table header, and one in the regular/desktop table header 🤦♀️In our latest Kibana upgrade (Bumping EUI to 39.0.0 kibana#113633), its functional accessibility tests are correctly flagging/raising an error due to both checkboxes sharing the same/duplicate ID
The fix here is to generate a separate suffixed ID depending on whether it's a mobile or desktop checkbox
QA
_desktop_mobileoV8Ge12oLE.mp4
Checklist
- [ ] Check against all themes for compatibility in both light and dark modes- [ ] Checked in Chrome, Safari, Edge, and Firefox- [ ] Props have proper autodocs and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examples- [ ] Added or updated jest tests- [ ] Checked for breaking changes and labeled appropriately