[DREAM-959] Paint table row state from one module by myabc · Pull Request #26503 · opf/openproject

myabc · 2026-10-31T02:13:24Z

Note

This PR is stacked on #26380 (DREAM-945). Please review/merge that PR first.

Ticket

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

What are you trying to accomplish?

tl;dr for humans: Works out each table row's screen-reader state in one place, so it stays consistent whenever rows change.

First phase of DREAM-959: one module classifies every table row, and one module writes every row attribute. No user-visible change.

What approach did you choose and why?

A row classifier returns a record for every row across all table sections, reporting undeclared rows instead of skipping them. A row-state painter replaces the table's separate semantics and positions painters: callers paint a scope, positions stay global, and rows refreshed in place carry their index.

Register row kind and expandability on occurrences, so nothing parses keys or toggle classes
Paint totals, inline-create and empty-results rows from the painter
Repaint the row name before focus returns from an edit reset
Paint selection markers from the painter (classes, aria-selected, new data-pressed)
Repaint rows after the drag-handle cells are swapped in, instead of setting their role directly
Paint the column header row from the painter, and drop the row and cell roles from the table and inline-create templates

Selection now runs the full painter. Measured in Chromium at 15 columns: 1.3 ms at 200 rows and 3.6 ms at 500 rows per paint, about twice the old selection-only path. A click currently paints twice (selection and focus); coalescing those belongs to the next phase.

With this, nothing outside the painter writes row roles, levels, expansion, selection, exposure or positions.

Hints for dev review

Start with semantics/row-classifier.ts and semantics/row-state-painter.ts.

AI involvement

Collaborative – AI generated a substantial part of the code; I reviewed and understand every line.

Merge checklist

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

github-actions · 2026-10-31T09:26:15Z

Warning

Flaky specs

rspec ./spec/features/projects/lists/filters_spec.rb[1:18:1: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 #26503, linked for reference only):

- `rspec ./spec/features/projects/lists/filters_spec.rb[1:18:1:1]`

Treat this as a standalone task, unrelated to PR #26503. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #26503 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.

Copilot

🟡 Changes recommended

Active findings identify inconsistent placeholder accessibility exposure and a lost withdrawal reason for unsupported table modes, requiring fixes and regression coverage.

2 open issues

What changed in this PR

This first phase of DREAM-959 centralizes work-package table row state, building on #26380’s accessible row headings. It aims to keep accessibility attributes consistent across rendering, editing, selection, and structural changes without changing user-visible behavior.

Changes:

Adds row classification and explicit occurrence kinds.
Routes row semantics, selection markers, and positions through a shared painter.
Updates lifecycle integrations and regression tests.

File	Description
frontend/src/app/features/work-packages/components/wp-table/wp-table.component.ts	Repaints headers through the shared painter.
frontend/src/app/features/work-packages/components/wp-table/wp-table.component.spec.ts	Updates painter integration and header tests.
frontend/src/app/features/work-packages/components/wp-table/wp-table.component.html	Removes template-owned row and cell roles.
frontend/src/app/features/work-packages/components/wp-table/wp-table-sums-row/wp-table-sums-row.directive.ts	Delegates totals-row semantics to the painter.
frontend/src/app/features/work-packages/components/wp-table/wp-table-sums-row/wp-table-sums-row.directive.spec.ts	Updates totals-row painting tests.
frontend/src/app/features/work-packages/components/wp-inline-create/wp-inline-create.component.ts	Reconciles state after inline-row refreshes.
frontend/src/app/features/work-packages/components/wp-inline-create/wp-inline-create.component.html	Removes template-owned accessibility roles.
frontend/src/app/features/work-packages/components/wp-inline-create/inline-create-row-builder.ts	Stops assigning structural roles directly.
frontend/src/app/features/work-packages/components/wp-fast-table/wp-fast-table.ts	Integrates classification, painting, and refreshed-row positions.
frontend/src/app/features/work-packages/components/wp-fast-table/wp-fast-table-lifecycle.spec.ts	Tests repainting after drag-handle replacement.
frontend/src/app/features/work-packages/components/wp-fast-table/semantics/treegrid-semantics.ts	Adds scoped painting and centralized selection markers.
frontend/src/app/features/work-packages/components/wp-fast-table/semantics/treegrid-semantics.spec.ts	Tests semantics through the painter.
frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.ts	Reports withdrawal reasons and skips hidden structural rows.
frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.spec.ts	Migrates position tests to the painter API.
frontend/src/app/features/work-packages/components/wp-fast-table/semantics/row-state-painter.ts	Adds the shared row-state painting entry point.
frontend/src/app/features/work-packages/components/wp-fast-table/semantics/row-classifier.ts	Adds row records across table sections.
frontend/src/app/features/work-packages/components/wp-fast-table/semantics/row-classifier.spec.ts	Tests row kinds, state, and undeclared rows.
frontend/src/app/features/work-packages/components/wp-fast-table/rendered-occurrence-ledger.ts	Adds explicit occurrence kinds.
frontend/src/app/features/work-packages/components/wp-fast-table/rendered-occurrence-ledger.spec.ts	Updates occurrence fixtures with kinds.
frontend/src/app/features/work-packages/components/wp-fast-table/helpers/structural-rows.ts	Classifies occurrences without parsing keys.
frontend/src/app/features/work-packages/components/wp-fast-table/helpers/structural-rows.spec.ts	Tests explicit kind classification.
frontend/src/app/features/work-packages/components/wp-fast-table/helpers/row-expansion.ts	Reads explicit expandability markers.
frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/selection-transformer.ts	Delegates selection updates to the painter.
frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/selection-transformer.spec.ts	Updates painter test doubles.
frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/keyboard/navigable-rows.ts	Builds navigation rows from classifier records.
frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/drag-and-drop-transformer.ts	Repaints positions after row moves and recovery.
frontend/src/app/features/work-packages/components/wp-fast-table/builders/rows/single-row-builder.ts	Removes builder-owned selection painting.
frontend/src/app/features/work-packages/components/wp-fast-table/builders/relations/relations-render-pass.ts	Registers relation rows as work-package occurrences.
frontend/src/app/features/work-packages/components/wp-fast-table/builders/primary-render-pass.ts	Registers explicit kinds for primary and structural rows.
frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/single-hierarchy-row-builder.ts	Marks expandable hierarchy rows.
frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/hierarchy-render-pass.ts	Registers hierarchy occurrence kinds.
frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/grouped/grouped-render-pass.ts	Registers group-header and sums kinds.
frontend/src/app/features/work-packages/components/wp-fast-table/builders/drag-and-drop/drag-drop-handle-render-pass.ts	Repaints rows after replacing drag handles.
frontend/src/app/features/work-packages/components/wp-fast-table/builders/cell-builder.ts	Leaves accessible naming to the painter.
frontend/src/app/features/work-packages/components/wp-edit-form/table-edit-form.ts	Repaints row names before returning focus.
frontend/src/app/features/work-packages/components/wp-edit-form/table-edit-form.spec.ts	Tests accessible naming before focus returns.

_{Review effort: Balanced}

Adds a row classifier that returns a record for every row of the table, in document order and across all sections: the header, rendered occurrences, empty results, totals and inline create. Each record carries the row's key, kind, level, expansion, exposure and selection. Undeclared rows are reported as foreign rather than skipped. https://community.openproject.org/wp/DREAM-959

Replaces the table's separate treegrid semantics and row positions with a row-state painter. Callers paint a scope: all rows, or the keys of the rows whose own attributes changed. Row positions are always repainted for the whole table, except for rows refreshed in place, which carry their predecessor's index. The painter reports why positions were withdrawn instead of only logging it. https://community.openproject.org/wp/DREAM-959

Records each occurrence's kind when a render pass registers it, so nothing parses occurrence keys any more. The hierarchy row builder stamps data-expandable when it adds the expand toggle, replacing the DOM query for the toggle's class. Keyboard navigation builds its rows from the row classifier. https://community.openproject.org/wp/DREAM-959

The painter now paints the totals, inline-create and empty-results rows and the exposure of every table section, on each paint. The sums directive and inline create only report that their rows changed. A totals row is exposed exactly when it has cells. Classifies structural rows by structure before exposure, so a hidden totals row is still recognised and can be shown again. Row positions skip structural rows that are not exposed. Row and cell roles are only written when they change. https://community.openproject.org/wp/DREAM-959

The cell builder now only records a row header's display name; the row-state painter turns it into the header's accessible name. Resetting an edited cell repaints that work package's rows before focus returns, so the restored focus lands on a header with its current name. https://community.openproject.org/wp/DREAM-959

The row-state painter now writes the selected and pressed markers of every work package row: the existing classes, aria-selected and a new data-pressed attribute. The selection transformer and the row builder no longer write them; rows built by a render or refresh get their markers when the painter runs after them. https://community.openproject.org/wp/DREAM-959

The drag-handle pass replaces each row's first cell once manual sort positions load, and set the replacement cell's role itself. It now leaves the role to the row-state painter and repaints the occurrences whose cells it replaced. https://community.openproject.org/wp/DREAM-959

The row-state painter now paints the column header row as well, and the table and inline-create templates stop emitting the row roles, levels and cell roles that the painter already writes after every change detection pass that renders those rows. https://community.openproject.org/wp/DREAM-959

Documents "exposed" as reachable by assistive technology on `isExposedRow`, and what `paintSectionExposure` writes, since the painter and the row classifier use the term without defining it. https://community.openproject.org/wp/DREAM-959

github-actions · 2026-11-02T15:38:01Z

Warning

Flaky specs

rspec ./modules/backlogs/spec/features/work_packages/create_work_package_spec.rb[1:2:1]
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 #26503, linked for reference only):

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

Treat this as a standalone task, unrelated to PR #26503. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #26503 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.

lwassermann

A few tactical feedback comments, a few questions. Overall, I find it hard to approve this PR.

The implementation seems as spread out as before somehow. Maybe it's because I'm new to the table rendering code. The classifier, the renderer, the semantics and the positions logic, they all seem to be closely intertwined. Every module that has changes needs to be aware of who else to poke so that the UI stays updated. And every module just reaches through the table to get at the part it needs right now. In this branch we update the whole table, in that we update only the edges and the positioning.

A lot of that is not new to this PR though, and I can't really offer alternatives (let alone alternatives that are achievable in our roadmap timeframe). Maybe you address some of my questions and feedback and I'll have another look and then approve it tomorrow anyway? 🤔

lwassermann · 2026-11-02T13:54:44Z

+        const kind = classifyRow(this.table, row);
+        if (kind !== 'totals' && kind !== 'create' && kind !== 'empty') return;
+        paintRowRole(row, 1);
+        if (kind === 'totals') setOrRemove(row, 'aria-hidden', row.cells.length === 0 ? 'true' : null);


When do we run into this?

As far as I understand the isHidden semantics established in wp-table-sums-row.directive.ts, we use the subscription of WorkPackageViewSumService to hide the whole row. (Which is apparently a boolean despite being called sum…). And if it's not hidden then it should have cells, doesn't it? Or is there some initialization step that renders empty hidden sum rows, that are now announced because of the new aria-label and aria-description attributes?

I see this was moved, and not created anew. I was a bit confused because we seem to have two different hidden-implementations for this now. But those also existed both before. The only new part is that they are in different files now. 🤷

lwassermann · 2026-11-02T14:48:06Z

Since we removed the semantics from the directives responsibility, could we remove some of the tests? And all the aria-hidden assertions?

#26510 removes the role and aria-hidden assertions. I've kept the assertions for those things that the directive actually stills decides: when the row renders or empties, mutating by the totals key, painting before the notification, and the stale-schema and destroyed-table cases). The painter spec should now cover hiding the totals section.

lwassermann · 2026-11-02T16:50:54Z


  private foreignRowReported = false;

+  private withdrawal:string|null = null;


This member is only ever assigned a value when it is returned after. And since it's private it is not used in other modules. Also I find it confusingly named, along with captureWithdrawal. Maybe we could remove it altogether? And is there some other name that explains better what the goal of captureWithdrawal is. Is it a flag? Or is it like the table ledger – a value captured from the last render cycle?

I think I don't quite get what the changes to this module are meant to do. Partly because I find it quite hard to understand what is happening here. I don't find this modules code easy to read 😞

Renamed to committedWithdrawal. It's a value carried over from the last render pass, not a flag, and it now stores that evidence as is.

withdrawal stays because #26507 reads it through a withdrawn getter.

Painting moved into paintPositions, so paint sets that reason in one place.

lwassermann · 2026-11-02T16:59:37Z

-          <thead class="-sticky" role="rowgroup">
-            <tr
-              role="row"
-              aria-level="1">


This feels like a loss of understandability. Looking at the templates and seeing the HTML, I would not expect something else to change it.

Part of that might be my React background, where templates are generally ensured to match the DOM on every render. Templates end up the source-of-truth.

Since there are so many other things already happening in the table rendering, maybe this feature area is just special, and I'll have to get used to that 🤔 😕.

This is a fair criticism – and in this narrow scope, I agree.

However it is a trade-off. wp-fast-table's rows are built outside Angular. Before this change the templates and the imperative code both wrote these roles, and they drifted apart, which is what DREAM-959 attempts to fix. We could add a template comment saying the row-state painter owns these roles, if that helps.

As you've probably already realised, thee is a long history to wp-fast-table. The implementation is a decade old now and, from what I can discern from git history and some other anecdotal information, the imperative rendering came from performance issues with the original AngularJS (NOT Angular) framework. The assumptions made back then most likely don't hold up any more: I would suspect that we could reimplement declaratively, i.e. using templates, and we would see at least equivalent performance.

That said, this does open up other cans of worms (e.g. taking advantage of modern Angular vs Framework agnosticism), so it's something that we should chat about directly.

I have opened an OPEN POINT, DREAM-965 to discuss rendering further.

Renames the `create` and `empty` row kinds to `inlineCreate` and `emptyResults`, the names the row classifier already reported and its edge row keys use, so the classifier no longer translates between two vocabularies for the same rows. https://community.openproject.org/wp/DREAM-959

Keeps the edge row key map private to the row classifier, since no other module reads it, and narrows the row kind with a type predicate instead of a cast. Names the table element `tableElement` so it no longer reads like the fast table it belongs to. https://community.openproject.org/wp/DREAM-959

Stores why the last committed render pass withheld row positions as the `WithdrawnRowPositions` evidence itself, which replaces the `Unpainted` type, and renames it from `captureWithdrawal` to `committedWithdrawal`. Moves the painting into `paintPositions`, so `paint` records the reason it returns in one place. https://community.openproject.org/wp/DREAM-959

Renames the `element` locals that hold the `<table>` in the row-state painter and the row positions to `tableElement`, as in the row classifier, so they no longer read like a row or cell element, and the `header` local that holds the header rows to `headerRows`. https://community.openproject.org/wp/DREAM-959

Declares `ROW_KINDS` as satisfying `RowRecordKind[]` and narrows the `isNavigableKind` input to `RowRecordKind`, so renaming a row kind in the classifier fails to compile instead of silently dropping that row from keyboard navigation. https://community.openproject.org/wp/DREAM-959

myabc · 2026-11-02T20:11:09Z

-export type RowKind = 'header'|'workPackage'|'groupHeader'|'sums'|'totals';
+import type { RowRecordKind } from '../../../semantics/row-classifier';
+
+export const ROW_KINDS = ['header', 'workPackage', 'groupHeader', 'sums', 'totals'] as const satisfies readonly RowRecordKind[];


@lwassermann this should help prevent drift: renaming a RowRecordKind in
the classifier and not updating here will fail the typecheck.

Adds `hidden` to the row classifier's state: hidden occurrences and the drag placeholder, a totals row without navigable cells, and any other row that is not exposed. Keyboard navigation and the painter's `aria-hidden` on the totals row now read that one value instead of each applying its own rule, so a totals row holding only internal cells is hidden from assistive technology as well as from navigation. https://community.openproject.org/wp/DREAM-959

myabc · 2026-11-02T21:48:28Z

@lwassermann thanks for the review and the patience trawling through a lot of code that is less than optimal.

The implementation seems as spread out as before somehow.

I would argue that DREAM-959 in its totality does bring some decent improvements to the structure of the code and delineating responsibilities. The suggestions you made were helpful, and with those in place the split now looks like:

[DREAM-959] Paint table row state from one module #26503 brings painting into one module and now decides whether each row is hidden in one place.
[DREAM-959] Change table rows through one mutation protocol #26507 adds a single mutate protocol (including mutateWorkPackages), so callers stop deciding what to repaint.
[DREAM-959] Clean up the table keyboard modules #26510 cleans up the keyboard side, including folding navigable-rows.

You will see a bunch of "Addressed in #WPID"-type replies where the feedback is addressed up the stack.

I realise now that it may have been a mistake to break this implementation chunk into multiple PRs. Your time is probably best spent reviewing the tip of the stack. In fact, I would be more than ok with folding #26507 and #26510 into #26503 to reduce review complexity. I was tempted to do this pre-emptively, but now that the WP is in review, it needs your call.

DREAM-959 is about cleanup and small improvements, rather than a big rewrite. If the code is even a little bit better structured and easy to reason about, then I would still call that a net success.

I have some ideas for more substantial refactorings, but I'm not sure where that will fit with the roadmap.

myabc force-pushed the code-maintenance/dream-959-fast-table-cleanup-1 branch from 00140ba to febd2ce Compare October 31, 2026 17:24

myabc mentioned this pull request Oct 31, 2026

[DREAM-959] Change table rows through one mutation protocol #26507

Open

9 of 11 tasks

github-actions Bot added the ai: Collaborative 💻 AI generated a substantial part of the code; A human reviewed and understands every line. label Oct 31, 2026

myabc added feature javascript Pull requests that update Javascript code needs review labels Oct 31, 2026

myabc added this pull request to stack #26508 October 31, 2026 20:17

myabc marked this pull request as ready for review October 31, 2026 20:27

myabc mentioned this pull request Oct 31, 2026

[DREAM-959] Clean up the table keyboard modules #26510

Open

8 of 10 tasks

myabc requested a balanced review from Copilot November 1, 2026 00:54

Copilot started reviewing on behalf of myabc November 1, 2026 00:54

View session

Copilot AI reviewed Nov 1, 2026

View reviewed changes

Comment thread frontend/src/app/features/work-packages/components/wp-fast-table/semantics/row-classifier.ts Resolved

Comment thread ...end/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.ts Resolved

myabc mentioned this pull request Nov 1, 2026

[DREAM-967] Pin the table paths DREAM-874 changed #26512

Open

10 of 12 tasks

myabc force-pushed the code-maintenance/dream-959-fast-table-cleanup-1 branch 2 times, most recently from 54f5384 to 444d1cc Compare November 1, 2026 13:50

lwassermann self-requested a review November 2, 2026 11:10

myabc added 9 commits November 2, 2026 15:02

[DREAM-959] Define row and section exposure

1cf4a2e

Documents "exposed" as reachable by assistive technology on `isExposedRow`, and what `paintSectionExposure` writes, since the painter and the row classifier use the term without defining it. https://community.openproject.org/wp/DREAM-959

myabc force-pushed the code-maintenance/dream-959-fast-table-cleanup-1 branch from 6e8057d to 1cf4a2e Compare November 2, 2026 15:18

lwassermann reviewed Nov 2, 2026

View reviewed changes

myabc added 5 commits November 2, 2026 19:39

myabc commented Nov 2, 2026

View reviewed changes

myabc requested a review from lwassermann November 2, 2026 21:32

This was referenced Nov 2, 2026

[DREAM-945] Expose work package row headers #26380

Open

[DREAM-974] Move actions for work packages in manually sorted tables #26569

Draft


		private foreignRowReported = false;

		private withdrawal:string\|null = null;

Conversation

myabc commented Oct 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

What are you trying to accomplish?

What approach did you choose and why?

Hints for dev review

AI involvement

Merge checklist

Uh oh!

github-actions Bot commented Oct 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Nov 2, 2026

Uh oh!

lwassermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myabc commented Nov 2, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

myabc commented Oct 31, 2026 •

edited

Loading