-
Notifications
You must be signed in to change notification settings - Fork 378
⚡️(frontend) improve accessibility of selected document's sub-menu #1261
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
base: main
Are you sure you want to change the base?
Conversation
3463226
to
a4249b3
Compare
6d0eba4
to
069359a
Compare
069359a
to
154abdd
Compare
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.
✨(frontend) add keyboard navigation for subdocs with focus activation
This commit should have a e2e test attached, to clearly understand what is the purpose of it.
src/frontend/apps/impress/src/components/dropdown-menu/hook/useDropdownFocusManagement.ts
Outdated
Show resolved
Hide resolved
const { | ||
handleMoreOptionsClick, | ||
handleMoreOptionsKeyDown, | ||
handleAddChildClick, | ||
handleAddChildKeyDown, | ||
} = useDocTreeItemHandlers({ | ||
isOpen, | ||
onOpenChange, | ||
createChildDoc, | ||
docId: doc.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.
It make it a bit complex to follow what is going on here.
I think it would be more clear with 2 dedicated components, and in these components the interactions that belong to them.
For this one, it could be ButtonAddChildDoc
:
docs/src/frontend/apps/impress/src/features/docs/doc-tree/components/DocTreeItemActions.tsx
Lines 223 to 244 in 154abdd
<BoxButton | |
as="button" | |
tabIndex={0} | |
data-testid="add-child-doc" | |
onClick={handleAddChildClick} | |
onKeyDown={handleAddChildKeyDown} | |
color="primary" | |
aria-label={ | |
t('Add child document to') + | |
` ${doc.title || t('Untitled document')}` | |
} | |
$hasTransition={false} | |
> | |
<Icon | |
variant="filled" | |
$variation="800" | |
$theme="primary" | |
iconName="add_box" | |
aria-hidden="true" | |
/> | |
</BoxButton> | |
)} |
For this one, it could be ButtonMoreOptions
:
docs/src/frontend/apps/impress/src/features/docs/doc-tree/components/DocTreeItemActions.tsx
Lines 205 to 220 in 154abdd
<Icon | |
onClick={handleMoreOptionsClick} | |
iconName="more_horiz" | |
variant="filled" | |
$theme="primary" | |
$variation="600" | |
className="icon-button" | |
tabIndex={0} | |
role="button" | |
aria-label={ | |
t('More options for') + ` ${doc.title || t('Untitled document')}` | |
} | |
aria-haspopup="true" | |
aria-expanded={isOpen} | |
onKeyDown={handleMoreOptionsKeyDown} | |
/> |
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.
I agree 100%
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.
I think you should rebase, you have made a interesting commit here: 5181bba
We should leverage what was made in this commit instead of creating the same logic again.
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.
This is a good idea yes.
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! I’ve refactored the keyboard logic a bit to make it cleaner and avoid duplication.
- useDropdownKeyboardNav still handles the focus and keyboard navigation within dropdown menus, nothing changed there. It just uses a small shared helper now to handle the selection when pressing Enter or Space.
- useTreeItemKeyboardActivate, which is just about triggering an action when a tree item is focused and Enter/Space is pressed, now reuses that same helper directly.
This way we avoid duplicating the same event listener and cleanup logic, and each hook remains focused on its own specific use case.
Let me know if that sounds good to you !
adds focus-visible and focus-within to make the sub-menu accessible Signed-off-by: Cyril <[email protected]>
enter/space now trigger only on real focus add useTreeItemKeyboardActivate hook Signed-off-by: Cyril <[email protected]>
adds proper aria props and translation keys for accessibility support Signed-off-by: Cyril <[email protected]>
82c61fd
to
4c57253
Compare
4c57253
to
12bb25a
Compare
Ok, I see what you mean. Do you suggest moving the test files from the other commit to this one, or at least the test file that refers to this change? |
d753d01
to
584d8f4
Compare
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.
When there is a subdocs, I cannot unfold the doc with keyboard.
Enregistrement.de.l.ecran.2025-08-12.125242.mp4
src/frontend/apps/impress/src/features/docs/doc-tree/hooks/useDropdownKeyboardNav.ts
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/docs/doc-tree/hooks/useDropdownKeyboardNav.ts
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/docs/doc-tree/hooks/useTreeItemKeyboardActivate.tsx
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/docs/doc-tree/components/DocTreeItemActions.tsx
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/docs/doc-tree/components/ButtonMoreOptions.tsx
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/docs/doc-tree/components/ButtonAddChildDoc.tsx
Outdated
Show resolved
Hide resolved
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.
src/frontend/apps/impress/src/features/docs/doc-tree/components/DocSubPageItem.tsx
Outdated
Show resolved
Hide resolved
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.
I still don't see a test that show the keyboard interaction on the tree menu. #1261 (review)
Last thing, I can see that we have 2 components DropdownMenu
, one coming from ui-kit, and another from src/components
, they have different accessibility behaviors, could we unify this behavior ?
docs/src/frontend/apps/impress/src/features/docs/doc-tree/components/DocTreeItemActions.tsx
Lines 1 to 6 in 584d8f4
import { | |
DropdownMenu, | |
DropdownMenuOption, | |
useTreeContext, | |
} from '@gouvfr-lasuite/ui-kit'; | |
import { useModal } from '@openfun/cunningham-react'; |
70e94ad
to
d1c09dc
Compare
d1c09dc
to
7250d07
Compare
25cdd61
to
af01958
Compare
Fix ! subdocnav.mp4 |
Purpose
This pull request improves the accessibility of the document tree by allowing keyboard users to:
issue : 1160
createsubdoc.mp4
Sub-document navigation is accessible, we can now navigate and open sub-docs using the keyboard.
Implementation note:
A small useTreeItemKeyboardActivate hook listens for Enter or Space only when the current node has focus (node.isFocused). This compensates for a limitation in react-arborist, which doesn't trigger any "activate" behavior on keyboard events.
For the action buttons (like ... or +) that appear when a tree item is focused:
By default, react-arborist uses a "roving tabindex", so these buttons are not reachable with Tab.
To fix this and make them keyboard-accessible:
I added a hook useActionableMode and some algo.
Proposal