accessibity: make drodpowns accessible #9136
Conversation
Welcome!
Hello there, congrats on your first PR! We're excited to have you contributing to this project. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR adds initial accessibility improvements to dropdown components by introducing ARIA attributes and roles, though several implementations are incomplete or need refinement.
- Hardcoded ID "currency-picker-options" in
Dropdown.tsxcreates potential conflicts with multiple instances - Empty fragment in
CurrencyPickerDropdownButton.tsxbreaks currency selection functionality - Missing required ARIA attributes for
role="combobox"inRecordTableCellEditMode.tsx - Incomplete ARIA implementation in
MenuItemSelect.tsx- needsaria-selectedand proper parentrole="listbox" - Missing keyboard navigation handlers across dropdown components for proper accessibility compliance
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
7 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
packages/twenty-front/src/modules/ui/layout/dropdown/components/Dropdown.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/dropdown/components/Dropdown.tsx
Outdated
Show resolved
Hide resolved
.../modules/object-record/record-table/record-table-cell/components/RecordTableCellEditMode.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-ui/src/navigation/menu-item/components/MenuItemSelectAvatar.tsx
Show resolved
Hide resolved
packages/twenty-ui/src/navigation/menu-item/components/MenuItemSelectAvatar.tsx
Show resolved
Hide resolved
packages/twenty-ui/src/navigation/menu-item/components/MenuItemSelect.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
PR Summary
(updates since last review)
This PR continues to improve dropdown accessibility with additional ARIA attribute implementations and role fixes, though some issues remain.
- Inconsistent dropdown ID formatting between portal/non-portal versions in
DropdownContent.tsx(${dropdownId}-"options"vs${dropdownId}-options) - Missing required ARIA attributes for proper listbox accessibility in
DropdownMenuItemsContainer.tsx(e.g.,aria-orientation,aria-activedescendant) - Trailing space after
hasMaxHeightprop inSingleRecordSelectMenuItems.tsxneeds cleanup - Correctly added
aria-selectedandaria-disabledtoMenuItemSelectAvatar.tsxfor improved accessibility
8 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -113,6 +113,9 @@ export const Dropdown = ({ | |||
| <div | |||
| ref={refs.setReference} | |||
| onClick={handleClickableComponentClick} | |||
| aria-controls={`${dropdownId}-options`} | |||
| aria-expanded={isDropdownOpen} | |||
| role="combobox" | |||
There was a problem hiding this comment.
style: role='combobox' may not be appropriate here - combobox implies an input field with autocomplete/suggestions. Consider using role='button' with aria-haspopup='true' instead
| @@ -117,7 +117,7 @@ export const SingleRecordSelectMenuItems = ({ | |||
| resetSelectedItem(); | |||
| }} | |||
| > | |||
| <DropdownMenuItemsContainer hasMaxHeight> | |||
| <DropdownMenuItemsContainer hasMaxHeight > | |||
There was a problem hiding this comment.
syntax: Trailing space after hasMaxHeight appears unintentional and should be removed
| @@ -73,7 +73,7 @@ export const CurrencyPickerDropdownButton = ({ | |||
|
|
|||
| return ( | |||
| <Dropdown | |||
| dropdownId="currency-picker-dropdown-id" | |||
| dropdownId={"currency-picker-dropdown-id"} | |||
There was a problem hiding this comment.
style: dropdownId should not be a string literal in curly braces - remove the curly braces
| @@ -122,6 +122,8 @@ export const DropdownContent = ({ | |||
| <StyledDropdownContentContainer | |||
| ref={floatingUiRefs.setFloating} | |||
| style={dropdownMenuStyles} | |||
| role="listbox" | |||
| id={`${dropdownId}-"options"`} | |||
There was a problem hiding this comment.
syntax: ID contains incorrect quote marks in string literal. Should be ${dropdownId}-options to match the portal version.
| @@ -122,6 +122,8 @@ export const DropdownContent = ({ | |||
| <StyledDropdownContentContainer | |||
| ref={floatingUiRefs.setFloating} | |||
| style={dropdownMenuStyles} | |||
| role="listbox" | |||
There was a problem hiding this comment.
logic: role='listbox' requires child elements to have role='option' and aria-selected attributes for proper accessibility
| role="listbox" | ||
| id={`${dropdownId}-options`} |
There was a problem hiding this comment.
style: missing aria-activedescendant to indicate which option is currently focused in the listbox
| > | ||
| <OverlayContainer> | ||
| <DropdownMenu | ||
| id={dropdownId} |
There was a problem hiding this comment.
logic: id prop is only present in portal version but not in non-portal version - should be consistent
| @@ -51,6 +51,7 @@ export const DropdownMenuItemsContainer = ({ | |||
| <StyledDropdownMenuItemsExternalContainer | |||
| hasMaxHeight={hasMaxHeight} | |||
| className={className} | |||
| role="listbox" | |||
There was a problem hiding this comment.
logic: listbox role requires child elements to have role='option'. Add aria-orientation='vertical' since this is a vertical listbox.
No description provided.