Repository navigation
Conversation
myabc
added
Accessibility
javascript
github-actions
Bot
added
the
ai: Collaborative 💻
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
myabc
changed the title
myabc
force-pushed
the
implementation/dream-886-focus-model
branch
from
October 22, 2026 21:41
6bd2a45 to
6fc634b
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
myabc
force-pushed
the
implementation/dream-886-focus-model
branch
2 times, most recently
from
October 23, 2026 02:52
738953a to
d0c0ec7
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
myabc
marked this pull request as ready for review
October 23, 2026 07:22
myabc
force-pushed
the
implementation/dream-886-focus-model
branch
from
October 23, 2026 07:44
d0c0ec7 to
0de620a
Compare
Base automatically changed from
implementation/dream-893-selection-scope
to
dev
October 23, 2026 11:54
myabc
force-pushed
the
implementation/dream-886-focus-model
branch
from
October 23, 2026 11:54
0de620a to
2592aa4
Compare
Adds the keyboardNavigationEnabled table configuration flag, off by default. Teaches the fast-table test harness an opt-in productionMarkup mode that renders the sticky thead, its column and configuration cells, and the totals row, with headerRow(), headerCells() and totalsRow() accessors, so keyboard navigation specs can walk production-shaped markup. addInlineCreateRow now appends to the table's tbody[wpInlineCreate], as production does, and creates that body after the totals body only when there is none. https://community.openproject.org/wp/DREAM-886
Adds a DOM-free navigation model for the work package table. nextPosition maps a key press and the current position to a move, expand, collapse or none result, skipping hidden rows and keeping the desired column across rows of different widths. initialPosition picks the first visible work package row, and reconcilePosition keeps focus on the same row after a re-render, rebinding group headers and sums rows by group identifier, else falling back to the nearest visible row. https://community.openproject.org/wp/DREAM-886
Adds an optional groupIdentifier to rendered occurrences and sets it on group header and sums rows, so focus recovery can find a group by its stable identifier instead of its ordinal key. Adds WorkPackageTable#rowsRefreshed$, which emits the work package id once refreshRows replaced at least one of its rows, so consumers can restore state lost when cells are rebuilt in place. WorkPackageTable#notifyRowsRefreshed lets other owners of table rows, such as the totals row, emit an edge row key on the same stream. GroupedRowsBuilder#refreshExpansionState returns the visibility updates instead of applying them, and the table publishes a render after a group collapse even when no row changed visibility, since the group header elements are replaced regardless. https://community.openproject.org/wp/DREAM-886
With keyboardNavigationEnabled on, rows, display field spans, the hierarchy and relation indicators, action links, the ID link, share cells and the header sort link and configuration button no longer take Tab focus. Rows get tabindex -1, display fields lose tabindex and role=button, and the details pane renderer keeps both through a new tabbable option that defaults to true. Adds roving-tabindex.ts with navigableCellClassName, paintRovingTabindex, navigableCells, keyboardNavigationEnabled and isNavigableColumn. One eligibility rule decides which cells are navigable: every query column, and the context-menu column while the actions column or column menu is enabled; never the internal drag or baseline columns. Body, group sums, totals and relation sub-rows apply it alike, so vertical moves keep their column. The drag placeholder is built without the class, and relation sub-rows take their tr out of the Tab order and mark every relation cell. With the flag off the DOM is unchanged. Each roving paint also sweeps late-rendered tabbable descendants (custom elements, modal focus anchors) to tabindex -1, sparing the inline-create body and an open inline editor. insideGrid defines the grid as the header, ledger and totals rows; the inline-create body is outside it. Adds removeTabStops to shared/helpers/dom for the builders and the display field renderer. It covers anchors without an href too, since display fields such as spent time set the href after rendering. The harness renders the inline-create body, builds the totals cells from the table's augmented columns with the same eligibility rule, and accepts non-sortable column specs whose header has no sort link. https://community.openproject.org/wp/DREAM-886
Adds the KeyboardNavigationTransformer behind keyboardNavigationEnabled and turns the flag on for the work package list view. The transformer walks the header row, the rendered occurrences and the totals row with the arrow keys, Home, End, Page Up/Down and Ctrl/Cmd+Home/End, keeps one roving Tab stop (the first work package row until focus moves) and expands or collapses hierarchy parents and groups with ArrowRight/ArrowLeft. A recognised key that cannot move is consumed, so the page does not scroll. Enter activates a work package row the way a plain click does, toggles a group header, clicks a header cell's sort link or the column configuration button, and opens the inline editor of an editable cell through the onCellEnter hook. Keys with Alt or Shift, during composition, with Ctrl/Cmd other than Home/End, or from inner controls pass through. Tab or Shift+Tab from the managed row or cell repaints the roving stop first, sweeping late-rendered tabbable descendants, so Tab leaves the grid instead of stopping on them. Extracts activateRow from RowClickHandler and activateTableField from EditCellHandler so both paths share one implementation, passes the view to state transformers, and makes the group header cell navigable when the flag is on. https://community.openproject.org/wp/DREAM-886
Adds a mousedown listener on the table scroll container that records the clicked row and navigable cell as the keyboard position and paints the roving tabindex there. It neither focuses nor prevents default, so links, toggles and editor activation keep working. Targets outside the table rows, such as the timeline side, the inline-create row and the placeholder row, change nothing. TableEditForm now remembers the position it was opened from as origin when its first field activates and clears it once the last field closes. https://community.openproject.org/wp/DREAM-886
Records whether focus was inside the table before a render pass, a collapse or a row refresh replaces or hides its rows, and afterwards reconciles the remembered position against the new rows. The same occurrence keeps focus, a removed row hands it to the nearest visible row, a hidden row to the group header or parent that hides it, and an empty result to the header row. Group positions rebind by identifier and are dropped when the grouping changes. Ownership is captured against the grid, so focus on the inline-create button counts as outside, and a capture never downgrades while a render is pending, so an editor reset that blurs to body cannot revoke it. DOM focus is restored only when the table owned it, no inline editor is open and the active element is at rest: null, body, disconnected, or on a row the ledger now hides. A still-connected control the user focused keeps focus. A row refresh or collapse between scheduling a full render and its frame commit leaves ownership to the commit. With the flag on, the transformer also handles requested focus after render; SelectionTransformer keeps doing so for tables without the flag. The totals row joins the recovery seam: the sums-row directive captures ownership before it refreshes or clears the row, skips the clear when the row is already empty, and notifies the table afterwards, so focus survives a sums refresh and the totals row is reachable once it renders late. https://community.openproject.org/wp/DREAM-886
Makes the keyboard navigation transformer the single owner of focus return after inline editing. Cancelling an editor, saving it or submitting an unchanged value focuses the cell the editor was opened in again, once the save's row refresh has run. Focus stays where it is when the origin cell is gone, when focus is outside the table, when it moved out of the grid while the save was pending, or when another editor in the table is open. Focus landing outside the grid, including on the inline-create button, marks it as having left, and the mark survives that element being removed. Separates the user's position from the reconciled one. Only pointer bookkeeping, keyboard moves, explicit requests and an editor opening write userPosition; a click or arrow elsewhere during a pending save cancels the return, while a render that merely reconciled the position does not. The origin is reconciled before returning, so a row hidden during the save hands focus to its hiding owner. The origin is the activated field's cell, not the remembered position, so programmatic activation, required-field activation and the inline-create row (which has no origin) return correctly. A failed activation leaves no origin behind. A later field of the same form re-records the origin, and a form that survives a required-column redraw registers itself again so refreshes neither overwrite its cell nor steal its focus. Adds an onSubmitComplete hook to EditForm that the field handler calls after a successful user submit; it does nothing for other forms. With the flag off, the table edit form still focuses the cell through FocusHelper. Drives the harness editing portal through the production field handler, lets specs supply a save and edit more than one field of a row. https://community.openproject.org/wp/DREAM-886
Moves focus to the enclosing navigable cell, or else the row, when keyboard or programmatic focus arriving from outside the grid lands on an inner control the table took out of the Tab order (header sort link, ID link, hierarchy toggle, relation expander, context-menu button, column-configuration button). Menus that return focus to such a control on Escape otherwise left focus where arrow keys do nothing. Focus moving between elements inside the grid, such as a screen reader cursor, is left alone. The redirect does not scroll and is skipped while a pointer press is in progress, so the press keeps its target and its click; mousedown already records the position. Elements without tabindex="-1" and anything inside an open inline editor keep focus. Escape from the work package context menu returns to the roving stop under the flag, also when the menu was opened by pointer on an inner element. The column header menu falls back to its th, or else its own element, when a column has no id element, and Enter on a header without a sort link clicks its plain anchor. https://community.openproject.org/wp/DREAM-886
Adds a feature spec that walks the work package list by keyboard: Tab enters the table on the first row, arrow keys move between rows, cells and the header row, Escape from the column header menu returns to the header cell, Enter on the Subject header sorts, Tab re-enters at the remembered position, Shift+Tab leaves the table, Tab from the last row lands on the inline-create button, Enter on a row replaces the selection, Escape in an inline editor returns focus to the cell, a native Enter, type, Enter round trip saves and returns, a custom element that renders a Tab stop does not catch Tab, and Down from a Work cell lands on the Work total. Adds page-object helpers to tab into the table, reach a row or cell by arrow keys, send keys without clicking, and assert where focus is with retrying matchers and accessible selectors. select_all_work_packages now sends the shortcut to the table's roving Tab stop, falling back to the first row when there is none. Points the work package context menu accessibility examples at the Subject cell, where Escape returns focus to the cell the menu was opened from. https://community.openproject.org/wp/DREAM-886
myabc
force-pushed
the
implementation/dream-886-focus-model
branch
from
October 26, 2026 03:52
2592aa4 to
9d38ed7
Compare
myabc
commented
Oct 26, 2026
Reveals the context-menu icon and the drag handle while the row or one of its cells holds keyboard focus. They were shown on hover and when the action cell itself had focus, so row and cell focus from the navigation model showed no actions (mob review, 2026-10-26). Covers it in the keyboard navigation spec. https://community.openproject.org/wp/DREAM-886
The global j and k shortcuts indexed every body row by the active element. From cell focus the active element is a cell, so j jumped to the first row and k did nothing; from a collapsed group header j targeted a hidden child and stalled. Resolves the active element to its row and skips rows that are not rendered. Covers both cases in the keyboard navigation spec. https://community.openproject.org/wp/DREAM-886
The example clicked the parent's hierarchy indicator, which moved focus to the indicator before the child disappeared, so it passed without exercising the recovery it was named for. The transformer spec pins that case. https://community.openproject.org/wp/DREAM-886
Dev switched @typescript-eslint/no-empty-function off, so the directive on EditForm#onSubmitComplete is reported as unused when CI lints the merge with dev. Documents the empty hook with a body comment instead, which lints clean before and after the rule change. https://community.openproject.org/wp/DREAM-886
planktonic
reviewed
Oct 27, 2026
planktonic
left a comment
planktonic
left a comment
Contributor
There was a problem hiding this comment.
Tested this in firefox & safari, both in combination with voiceover and without, and it works well ✨ The only functional issues I see:
- the screenreader focus and the treegrid focus can become out of sync, and it seems to produce odd results when you switch. This is quite a niche issue, perhaps something for the future
- when using a screenreader, the actions buttons are reachable, but when only using keyboard I couldn’t reach those buttons. Perhaps an issue for our keyboard power-users?
I only had time to look at the JS in the keyboard folder, and I think most of comments are questions on naming :)
I didn’t find a WP table with nested rows in the seed data, so I also couldn’t try that out yet. Are there seeds for that somewhere?
| internalSortColumn, | ||
| } from 'core-app/features/work-packages/components/wp-fast-table/builders/internal-sort-columns'; | ||
|
|
||
| export const navigableCellClassName = 'wp-table--navigable-cell'; |
Contributor
There was a problem hiding this comment.
I see these classnames are not introduced here, so this is a general question, not specific to this PR: is the classname following a BEM naming scheme? (the double dash suggests it is). Since the cell is a child of the wp-table, I would expect this to be wp-table__navigable-cell`. Is this a variant of BEM naming, what’re the convention rules?
Comment on lines
+84
to
+86
| if (element !== focused && !element.closest(inlineCreateSelector) && !element.closest(`.${activeFieldContainerClassName}`)) { | ||
| element.setAttribute('tabindex', '-1'); | ||
| } |
Contributor
There was a problem hiding this comment.
Is this responsible for buttons & links not being reachable by tabbing right now? (I can’t tab to the actions buttons at the end of the row, for example). This clause seems to be taking interactive elements out of the normal tab order.
| groupIdentifier?:string; | ||
| } | ||
|
|
||
| export type NavKey = 'ArrowUp'|'ArrowDown'|'ArrowLeft'|'ArrowRight'|'Home'|'End'|'PageUp'|'PageDown'|'CtrlHome'|'CtrlEnd'; |
Contributor
There was a problem hiding this comment.
Tab (and shift+tab), and enter/return are included in the spec but missing here.
| // See COPYRIGHT and LICENSE files for more details. | ||
| //++ | ||
|
|
||
| export type RowKind = 'header'|'workPackage'|'groupHeader'|'sums'|'totals'; |
Contributor
There was a problem hiding this comment.
For setting focus, does the content of the row make a difference?
| rows:readonly NavigableRow[], | ||
| position:FocusPosition, | ||
| edge:'first'|'last', | ||
| column:'keep'|'edge', |
Contributor
There was a problem hiding this comment.
An edge column seems clear to me, but what’s a keep column? It implies we “lose” the edge?
| } | ||
|
|
||
| function navigateVertically(rows:readonly NavigableRow[], position:FocusPosition, key:NavKey, index:number, pageSize:number):NavResult { | ||
| switch (key) { |
Contributor
There was a problem hiding this comment.
Love the switch statements in these two functions, super clear
| return toResult(positionOn(target, cell, cell), position); | ||
| } | ||
|
|
||
| function moveWithinRow(row:NavigableRow, position:FocusPosition, direction:'Home'|'End'|'ArrowLeft'|'ArrowRight'):NavResult { |
Contributor
There was a problem hiding this comment.
This looks like it’s the other in the pair of navigating cells, so would it make sense to call it navigateHorizontally?
| } | ||
|
|
||
| /** Computes what a navigation key does at the given position. */ | ||
| export function nextPosition( |
Contributor
There was a problem hiding this comment.
Like the comment says: would something like computeNextPosition make it’s purpose clearer?
| return Array.from(rows).find((row) => !table.tbody.contains(row)) ?? null; | ||
| } | ||
|
|
||
| /** The navigable kind of a rendered occurrence, or null for rows navigation skips. */ |
Contributor
There was a problem hiding this comment.
null for rows navigation skips is hard to parse :D Is there a missing apostrophe on row’s/rows’?
| return occurrence.workPackageId === null ? null : 'workPackage'; | ||
| } | ||
|
|
||
| function expandedOf( |
Contributor
There was a problem hiding this comment.
If this returns boolean, the naming convention used in other JS files here seems to be that the function would be called something like isExpanded?
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket
https://community.openproject.org/wp/DREAM-886
What are you trying to accomplish?
tl;dr for humans: Getting past the work package table by keyboard takes dozens of Tab presses today; this makes the whole table one Tab stop that you move around in with the arrow keys, like a spreadsheet.
The table becomes a single Tab stop with a roving
tabindex: Tab enters on the first row (or the remembered position), arrow keys move between rows and cells, Enter activates, and Tab/Shift+Tab leave in one press. BehindkeyboardNavigationEnabled, which only the main work package list view turns on; embedded tables and the BCF list are unchanged until DREAM-896.Behaviour
Clicks set the position too. Other modified keys pass through to existing shortcuts (Ctrl/Cmd+A, Shift+Alt+F10); Shift+Arrow is left for DREAM-894 (#26294).
Screenshots
What approach did you choose and why?
A pure position model (
focus-position.ts) driven by one keydown listener on the scroll container, so header and body rows share one code path. One rule (isNavigableColumn) decides which cells are visited, so vertical moves keep their column. Inner controls (ID link, toggles, menu buttons, sort links) leave the Tab order; focus landing on one moves to its cell or row. ARIA semantics follow in DREAM-885 (#26288).A re-render restores focus only if the table owned it and the user has not moved elsewhere; after an inline save, focus returns to the edited cell.
Two fixes apply without the flag: a form that survives a required-column redraw registers itself again, and Escape from the column header menu on a relation column no longer throws.
Known gaps
After the mob review (2026-10-26)
Three follow-up commits on top of the reviewed ones: row action icons (context menu, drag handle) are shown while the row or one of its cells has keyboard focus (they were hover-only); the global
j/kshortcuts move between visible rows from cell focus too and skip collapsed rows; the pointer-collapse smoke example is dropped because the transformer spec pins that case. A fourth commit drops an ESLint directive that dev no longer needs.Ruled for DREAM-895, not this PR: Tab from a focused row or cell walks that row's interactive controls and leaves the table after the last one.
AI involvement
Collaborative – AI generated a substantial part of the code; I reviewed and understand every line.
Hints for review
focus-position.tsand its spec, thenkeyboard-navigation-transformer.tsandroving-tabindex.ts(all inwp-fast-table/handlers/state/keyboard/).spec/features/work_packages/table/keyboard_navigation_spec.rb. The table page object gains keyboard focus helpers, andselect_all_work_packagesnow sends Ctrl/Cmd+A to the roving stop.tabindex="-1"so Firefox does not stop on it; clicking empty space in it now focuses it.Merge checklist