[myabc]

Ticket

https://community.openproject.org/wp/DREAM-967

What are you trying to accomplish?

tl;dr for humans: Adds unit specs that lock in how the work package table behaves on paths the DREAM-874 keyboard and accessibility work changed but no spec exercised, so regressions show up before DREAM-897 verification. Spec-only apart from deleting dead code and two redundant guards; no behaviour change.

First slice of DREAM-967. Uncovered code was attributed with git blame to the epic's tickets; this slice pins the reachable epic paths plus grouped row matching.

Metric (production wp-fast-table)	Before	After
Statements	95.9%	97.0%
Branches	88.9%	91.4%
Functions	97.2%	98.0%
Lines	96.4%	97.3%
Uncovered points in DREAM-874 code	136	94

What approach did you choose and why?

• Delete WorkPackageTableEditingContext.stopEditing and TimelineRowBuilder.insert, which have no callers
• Pin PageUp/PageDown by viewport rows, Enter on a group sums row, and focus that rests on the scroll container or an unlisted row
• Pin Tab inside multi-control editors and overlay re-entry
• Pin pure movement edges in focus-position.ts
• Pin details-pane switching on row click (keeps the tab) and edit activation abandoned on teardown
• Pin mutate after destroy, row positions with an unset groups state, and the grouped position check
• Pin child-relation retry after a failed load, grouped highlighting, unchanged baseline cells and deferred hierarchy parents
• Pin grouped row matching by href, multi-value length, % progress values and missing properties
• Drop two guards their callers already rule out

Every new spec was checked by breaking the line it pins and watching it fail.

Behaviour notes for reviewers

These specs pin today's behaviour where it is an open question rather than a decision; the questions go to DREAM-967:

• Hierarchy mode renders a leaf before its filtered-out parent when the leaf comes before its in-window grandparent (currently renders a deferred leaf before its filtered-out parent). This looks like a bug.
• A cell position without a desired column lands in column 0, Tab from a non-walk control in a cell stays native, and an editor overlay without a usable control receives focus itself.

Not pinned: HierarchyTransformer's reset of collapsed classes uses [class^="__hierarchy-group-"], which never matches a table row, so it never runs. Fixing the selector would change behaviour, so it is left for DREAM-967.

Hints for dev review

Each commit is green on its own (typecheck plus the wp-fast-table specs). The full Chromium suite and the keyboard specs on Firefox pass.

AI involvement

Directed – I specified the requirements and AI implemented most of it; I validated via testing rather than a full line-by-line review.

Merge checklist

• Added/updated tests
• Added/updated documentation in Lookbook (patterns, previews, etc)
• Tested major browsers (Chrome, Firefox, Edge, ...)

[Copilot]

🟡 Changes recommended

The console.warn spy is not restored, so it can hide warnings from subsequent tests in the file.

1 open issue

• Restore console.warn after the test · New

What changed in this PR

Adds regression coverage for work package table paths affected by DREAM-874, building on the DREAM-959 refactors before DREAM-897 verification. Existing behavior is preserved, including explicitly documented edge cases.

Changes:

• Covers keyboard navigation, editor focus, teardown, and details-pane switching.
• Adds rendering checks for grouping, hierarchy, relations, baseline cells, and row positions.
• Removes unused methods, a redundant guard, and an unused constructor default.

All paths below are relative to frontend/src/app/features/work-packages/components/wp-fast-table/.

File	Description
wp-table-editing.ts	Removes unused stopEditing method.
wp-fast-table-lifecycle.spec.ts	Tests mutation after destruction.
testing/​work-package-fixture.ts	Supports unchanged baseline fixtures.
semantics/​table-row-positions.spec.ts	Tests positions with unset groups state.
semantics/​body-row-positions.spec.ts	Covers empty groups and missing headers.
row-mutations.ts	Requires the render-pending callback explicitly.
handlers/​state/​keyboard/​keyboard-navigation-transformer.ts	Removes redundant destruction guard.
handlers/​state/​keyboard/​keyboard-navigation-transformer.spec.ts	Covers paging and focus-recovery edges.
handlers/​state/​keyboard/​focus-position.spec.ts	Tests movement boundary cases.
handlers/​state/​keyboard/​cell-interaction.spec.ts	Covers editor Tab behavior and overlay re-entry.
handlers/​row/​click-handler.spec.ts	Tests details-pane switching and tab preservation.
handlers/​cell/​edit-cell-entry.spec.ts	Tests activation cancellation during teardown.
builders/​timeline/​timeline-row-builder.ts	Removes unused insert method.
builders/​rows/​single-row-builder.spec.ts	Tests unchanged baseline presentation.
builders/​relations/​relations-render-pass.spec.ts	Tests retry after child-loading failure.
builders/​modes/​hierarchy/​hierarchy-order.spec.ts	Records current deferred-leaf ordering.
builders/​modes/​grouped/​grouped-render-pass.spec.ts	Covers group matching and member highlighting.

_{Review effort: Balanced}

[planktonic]

Is every row expected to be highlighted with this class individually? Perhaps it would make the test more robust to build a table with multiple rows, and check that they all have the highlight.

Nothing to do with these tests: if they’re all highlighted, what does the highlight mean? :D Does it have some semantic meaning that we could indicate using an (aria-)attribute, or is it purely aesthetic?

[myabc]

Enabling highlighting does this btw (using status as an example):

If this were phrasal content, then we would have <mark> in our toolbox. For rows, I'm really not sure how to handle. I'm also quite sure that colour highlighting flagrantly disregards WCAG colour contrast guidelines. / @bsatarnejad

aria-describedby referencing the column used for highlighting might be one option.

[myabc]

The spec now renders two rather than one row .. does that suffice?

[myabc]

(sorry, that was meant flippantly. I confess that I didn't put enough thought into how extensive the assertions should be with this particular case)

[planktonic]

Yes two rows works 👍

For the highlighted rows, possible alternative aria attributes would be aria-selected (suitable for gridcell, option, row, and tab roles), or perhaps aria-current (“indicates that this element represents the current item within a container or set of related elements.”, from mdn). But does either of those fit the meaning of the highlight? It looks like this highlighting is indicating something else.

(And to be clear, that’s not a suggestion to do it here, just asking for possible future improvements)

[github-actions]

Caution

The provided work package version does not match the core version

Details:

• Work package URL: https://community.openproject.org/wp/DREAM-967
• Work package version: 18.1.0
• Core version: 18.0.0

Please make sure that:

• The work package version OR your pull request target branch is correct

[github-actions]

Warning

Flaky specs

• rspec ./modules/backlogs/spec/features/work_packages/batch_destination_menu_spec.rb[1:2:3]
• rspec ./modules/backlogs/spec/features/work_packages/create_work_package_spec.rb[1:5:1]

🤖 Ask Copilot to investigate

Copy 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.

@copilot The following spec(s) are flaky in CI (first seen on PR #26512, linked for reference only):

- `rspec ./modules/backlogs/spec/features/work_packages/batch_destination_menu_spec.rb[1:2:3]`
- `rspec ./modules/backlogs/spec/features/work_packages/create_work_package_spec.rb[1:5:1]`

Treat this as a standalone task, unrelated to PR #26512. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #26512 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

[planktonic]

Looks great. My remaining unresolved thread is a question for me to understand the UI here better, it’s not something that’s related to/blocking this PR