fix: ensure keyboard entries navigation accounts for group order #989
Conversation
|
This pull request targets the |
|
@Skaiir Please verify this also works with search / filtering. |
|
@nikku It does :) |
|
UX wise this works like a charm:
Can be tried out via |
|
@Skaiir Please target |
|
@nikku This is a bug fix, no? :) |
There was a problem hiding this comment.
Thanks again for the contribution.
What I see (what we have to do) is to move the grouping concern to the menu, because we need it on the menu anyway. There is little merrit in having it inside the PopupMenuList.
6da8468 does the move so we don't do "double the work".
nikku
force-pushed
the
cr-app-an-53-match-keyboard-nav-to-visual-order
branch
from
6da8468 to
140ab2d
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes keyboard navigation in popup menus to ensure entries are navigated in the visual order displayed to the user rather than the original array order. The issue occurred when entries had groups, as the grouping and sorting changed the visual order but keyboard navigation still used the original entry array order.
- Modified keyboard navigation to use the post-grouping order for consistent user experience
- Moved entry grouping logic from PopupMenuList to PopupMenuComponent for better data flow
- Added comprehensive test coverage for grouped entry navigation scenarios
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/features/popup-menu/PopupMenuComponent.js | Moved grouping logic here, updated keyboard navigation to use grouped entries order, exported groupEntries function |
| lib/features/popup-menu/PopupMenuList.js | Removed grouping logic, now receives pre-grouped entries as props |
| lib/features/popup-menu/PopupMenuProvider.ts | Added PopupMenuGroup type definition |
| test/spec/features/popup-menu/PopupMenuComponentSpec.js | Added test for grouped entry navigation and reduced trigger timeout |
| test/spec/features/popup-menu/PopupMenuListSpec.js | Updated test to use groupedEntries prop instead of entries |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| setEntries(newEntries); | ||
| }, [ selectedEntry, setEntries, setSelectedEntry ]); | ||
| }, [ setEntries, setSelectedEntry ]); |
There was a problem hiding this comment.
The dependency array for updateEntries callback is missing the selectedEntry dependency that was removed. While this might be intentional to avoid unnecessary re-renders, the callback still uses selectedEntry indirectly through the keyboard navigation logic. Consider if this could cause stale closure issues.
Copilot uses AI. Check for mistakes.
This ensures we don't double the necessary work.
Ensures our tests are as fast as possible, but not faster.
nikku
force-pushed
the
cr-app-an-53-match-keyboard-nav-to-visual-order
branch
from
140ab2d to
9141939
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@Skaiir The reduced timeout seems to be entirely unnecessary, cf. 9141939. Impressive speedups, indeed: BeforeThis PR |
|
Released as |
fix: ensure popup menu keyboard navigation accounts for entry grouping Related to bpmn-io/diagram-js#989
fix: ensure popup menu keyboard navigation accounts for entry grouping Related to bpmn-io/diagram-js#989
2 participants
Related to bpmn-io/bpmn-js-create-append-anything#53
Proposed Changes
Ensure we use the post-grouping order when navigating the popup menu by keyboard.
Screencast.from.2025-09-19.10-48-42.webm
Ignore the disabled entries it's from another PR.
To try it out:
Checklist
To ensure you provided everything we need to look at your PR:
@bpmn-io/srtoolCloses {LINK_TO_ISSUE}orRelated to {LINK_TO_ISSUE}