Repository navigation
Introduce MultiSelectField (#574) - #718
bedrich-schindler wants to merge 4 commits into
Conversation
GitHub Actions
Inactive
|
f2d9888 to
ba57f69
Compare
4b5a019 to
ee9ad07
Compare
GitHub Actions
Inactive
ee9ad07 to
ed06396
Compare
ed06396 to
befd172
Compare
|
|
adamkudrna
left a comment
adamkudrna
left a comment
There was a problem hiding this comment.
Please refactor the <div contenteditable> to a native <input role="combobox"> per the ARIA Combobox pattern. Following it should solve all accessibility issues of the current solution, mainly the keyboard interaction.
There is the refactoring/multi-select-field-combobox-a11y branch, feel free to use it as inspiration.
| @@ -5,7 +5,7 @@ const TerserPlugin = require('terser-webpack-plugin'); | |||
| const VisualizerPlugin = require('webpack-visualizer-plugin2'); | |||
|
|
|||
| const MAX_DEVELOPMENT_OUTPUT_SIZE = 3400000; | |||
| const MAX_PRODUCTION_OUTPUT_SIZE = 460000; | |||
| const MAX_PRODUCTION_OUTPUT_SIZE = 520000; | |||
| import { useEffect } from 'react'; | ||
| import type { RefObject } from 'react'; | ||
|
|
||
| export const useClickOutside = ( |
There was a problem hiding this comment.
Can we reuse the hook in Popover and Modal? It would be a new issue of course.
| const callback = jest.fn(); | ||
| const { getByTestId } = render(<ComponentWithClickOutside callback={callback} />); | ||
|
|
||
| fireEvent.click(getByTestId('outside')); |
There was a problem hiding this comment.
A more typical use case is clicking anywhere on the page. Clicking another button is rather an edge case in the real world usage. I'm wondering if it makes a difference and deserves an own test case.
There was a problem hiding this comment.
Is it worth an own file? We don't use this pattern in the project.
| </div> | ||
| {isDropdownOpen && ( | ||
| <MultiSelectFieldDropdown | ||
| autoFocusFirstOptionOnOpen={!isSearchEnabled} |
There was a problem hiding this comment.
With search disabled there's a second focus model: focus jumps straight to the first option on open, while with search enabled it stays in the search span. Two models to keep in sync. With aria-activedescendant, both cases collapse into one: a read-only <input> when search is off.
| className={styles.root} | ||
| // `plaintext-only` makes the browser insert pasted or dropped content as plain text | ||
| // to prevent inserting formatted HTML content into the editable element | ||
| contentEditable={disabled ? false : 'plaintext-only'} |
There was a problem hiding this comment.
contentEditable="plaintext-only" isn't supported before Firefox 136. There the invalid value falls back to non-editable and search silently stops working. The value also isn't React-controlled (closeDropdown() writes textContent directly), there's no aria-autocomplete or placeholder, and textContent gets fragile once the browser inserts a <br>.
A controlled <input type="text"> avoids all of that, and can be the combobox itself.
| } | ||
| }} | ||
| ref={ref} | ||
| role="button" |
There was a problem hiding this comment.
As a div role="button" handling only Delete / Backspace, Space scrolls the page and Enter does nothing. The × is aria-hidden and title isn't reliably announced, so a screen reader says "Apple, button" with no hint that it removes.
A real <button aria-label="Remove Apple"> inside the tag gets Enter / Space for free. Spirit also adds a shared hidden description explaining the Delete / Backspace shortcut.
| if (keyDetected( | ||
| event, | ||
| [ | ||
| ...keyBindings.focusNextDropdownItem, |
There was a problem hiding this comment.
Arrow Down / Up only open the dropdown here, and a second press is needed to reach an option. The APG (and Spirit) open it and activate the first / last option in one go. Home / End aren't supported at all.
| - In case of a large amount of options, consider **grouping related items | ||
| together** by nesting them. | ||
|
|
||
| ### Keyboard Interaction |
There was a problem hiding this comment.
Worth adding an Accessibility section documenting the ARIA contract (roles and attributes of the input, listbox, options and tags), like the table in Spirit's README. The keyboard list here would change with the points above.
befd172 to
0e983ca
Compare
|
Also mixup with migration to latest TypeScript and Playwright stack was pushed, no further changes applied. |
This branch was successfully deployed
2 participants
Closes #574