feat(ui5-toolbar): align keyboard navigation with APG and improve nested item delegation#13231
feat(ui5-toolbar): align keyboard navigation with APG and improve nested item delegation#13231plamenivanov91 wants to merge 20 commits intomainfrom
Conversation
Add comprehensive keyboard navigation to toolbar following WCAG toolbar pattern: - Arrow keys (Left/Right/Up/Down) navigate between toolbar items - Home/End jump to first/last item - Tab/Shift+Tab navigate within toolbar, exit at edges - Roving tabindex pattern (single tab stop, toolbar is one stop) - Focus entry: remembers last focused item on re-entry, defaults to first item on initial entry - Proper handling of complex nested controls (ToolbarSelect) Resolves focus loss when pressing Shift+Tab on complex toolbar children. Ensures all toolbar items are reachable via keyboard navigation. Optimizations: - Extracted _focusItem() helper to reduce code duplication - Simplified Tab/Shift+Tab navigation logic Fixes: #12945
|
🚀 Deployed on https://pr-13231--ui5-webcomponents-preview.netlify.app |
|
Tested this PR. Good progress so far, but found a few issues we should fix before merge. Most of them can be reproduced in the toolbar with toolbar items test page here https://pr-13231--ui5-webcomponents-preview.netlify.app/packages/main/test/pages/toolbaritems
RTL not handled Do not check hardcoded tag names Your Adviced is to use the createInstanceChecker but that we not work here so simple duck-type check would work Add a property like // In ToolbarItemBase.ts
@property({ type: Boolean })
handlesOwnKeyboardNavigation = false;
// In ToolbarSelect.ts
handlesOwnKeyboardNavigation = true; // Override in class
// In Toolbar.ts - check the property, not the tag
_shouldItemHandleOwnNavigation(item: ToolbarItemBase): boolean {
return item.handlesOwnKeyboardNavigation;
}Other complex toolbar items can opt-in without modifying the toolbar code. Happy to discuss these over a call. |
- fix rtl arrow direction mapping for left and right keys - replace hardcoded tag checks with handlesOwnKeyboardNavigation on toolbar items - opt in ui5-toolbar-select to own keyboard handling - fix wrapped toolbar-item focus resolution and current-item detection - skip non-focusable wrapped content from roving tabindex navigation - add cypress coverage for wrapped items, non-focusable content, rtl behavior, and select traversal
Thanks for the thorough review. I fixed the issues. We can also have a call if any improvements need to be done. |
Improve WCAG toolbar keyboard behavior while preventing regressions for nested and overflowed controls. - add key-level ownership hook shouldHandleOwnKeyboardNavigation(event) - use key-aware delegation in toolbar and overflow key handlers - preserve native input/textarea/contenteditable navigation (Arrow/Home/End) - narrow overflow preventDefault to toolbar-consumed navigation only - refactor navigation item collection for standard and overflow paths - update keyboard handling documentation in Toolbar - add cypress regressions for input Home/End in toolbar and overflow - fix toolbar cypress type check for itemsToOverflow membership assertion
|
I tested the PR and found the following issues that should be fixed: In the toolbar items sample:
In the toolbar sample: https://pr-13231--ui5-webcomponents-preview.netlify.app/packages/main/test/pages/toolbar
|
Treat generic toolbar items as focus islands: arrow keys stay inside, while Tab and Shift+Tab move to the next or previous item in the same toolbar. Add and update Cypress coverage to validate arrow, Home/End, RTL, and Tab navigation behavior, including checkbox-group to input transitions.
… coverage Remove Tab item-roving, add arrow wrapping and input boundary behavior, refine ToolbarSelect Home/End handling, apply disabled-item aria/tabindex updates, and extend Cypress keyboard/a11y tests.
plamenivanov91
changed the title
| * @since 2.22.0 | ||
| */ | ||
| @property({ type: Boolean }) | ||
| handlesOwnKeyboardNavigation = false; |
There was a problem hiding this comment.
This is declared as @property({ type: Boolean }) but it behaves like a class-level constant — ToolbarItem and ToolbarSelect set it in their class body and it never changes at runtime. Making it a DOM attribute means it shows up in HTML and consumers can accidentally set it from outside. This should be a plain class field or a getter.
| _handleNavigationTarget(target: HTMLElement) { | ||
| const hostTarget = this._resolveNavigationHost(target); | ||
|
|
||
| if (hostTarget.tagName === "UI5-RADIO-BUTTON") { |
There was a problem hiding this comment.
Hardcoded tag name checks break when tag names are scoped. Use duck-typing or a property-based check instead, same as the handlesOwnKeyboardNavigation approach we agreed on earlier.
| return focusRef ? [focusRef] : []; | ||
| } | ||
|
|
||
| _storeOriginalTabIndex(target: HTMLElement) { |
There was a problem hiding this comment.
This only stores if not already present. If a component dynamically changes its tabindex (e.g., becomes disabled/enabled), the stored "original" becomes stale. Consider refreshing on each render cycle.
| } | ||
|
|
||
| let nextIndex = currentIndex; | ||
| const movingForward = (isRight(e) && !isRTL) || (isLeft(e) && isRTL) || isDown(e); |
There was a problem hiding this comment.
IMO, ArrowUp/Down should not be used for toolbar navigation. They conflict with too many controls — selects, comboboxes, date pickers, anything with a dropdown. Only ArrowLeft/Right should move between toolbar items. This removes a whole class of delegation issues.
|
|
||
| if (!radio.disabled && !radio.readonly && !radio.checked) { | ||
| if (radio.name) { | ||
| RadioButtonGroup.selectItem(radio as unknown as RadioButton, radio.name); |
There was a problem hiding this comment.
This couples too tightly a generic container to a specific control's internals. A better approach would be to dispatch a synthetic arrow-key event to the radio button and let it handle its own selection, or have selection happen programatically with user pressing Space/Enter. We can discuss this with RadioButton owners
|
Tested the latest version. Found some more issues: 1. ArrowUp/Down steals focus from single-child ToolbarItem controls Focus the ComboBox or DatePicker and press ArrowDown — instead of opening the dropdown/picker, the focus moves to the next toolbar item. Same with ArrowUp moving to the previous item. IMO ArrowUp/Down as navigation keys bring too many issues — they conflict with dropdowns, selects, comboboxes, date pickers, and any control that uses vertical arrows for its own interaction. The toolbar should only use ArrowLeft/Right for item-to-item navigation. This aligns with how most toolbar implementations work and avoids the delegation complexity that causes issues 1–4. 2. Tab from DatePicker, then Shift+Tab skips the toolbar entirely Steps:
The problem is that 3. Disabled button traps navigation If a button becomes disabled asynchronously/after the first rerender, arrow navigation gets stuck before or after it - you can't pass through it. The disabled item is not being skipped during navigation after state changes. 4. Last focused item becomes disabled - toolbar gets skipped If the last focused element in a toolbar gets disabled after focus has left the toolbar, Tab/Shift+Tab skips the entire toolbar. The roving tabindex is left on an element that is now disabled and unfocusable, and no fallback is applied. |
4 participants
Implement boundary-aware keyboard delegation for toolbar content with mixed complexity, while preserving intuitive behavior for grouped controls and self-managed components.
What changed:
Tests:
Validation: