From bbf2ba995b8651006c1b9ae82b2d2b4c53238350 Mon Sep 17 00:00:00 2001 From: Cristian Tcaci Date: Fri, 4 Sep 2026 22:50:26 +0100 Subject: [PATCH 1/2] fix(review): associate the refresh-lock reason with every disabled decision ReviewDecisionRail takes an explicit decisionDescriptionIds contract for explanations rendered outside the rail, and joins them with its own edit-lock note on Reject, Request edit, Defer and Approve. The attribute is omitted entirely when there is nothing to point at, so a blank id list cannot become a dangling aria-describedby. ReviewMain forwards the aria-describedby it receives from the Review view to the rail while keeping it on the column wrapper, which the view already asserts. The value is read through a function rather than a computed because fallthrough attributes are not reactive, so a cached read would keep pointing at a note that has already been removed. The prop surface of ReviewMain is unchanged, so PaperReviewView needs no edit. Refs #2461 --- .../paper/review/ReviewDecisionRail.spec.ts | 101 ++++++++++ .../views/paper/review/ReviewMain.spec.ts | 183 ++++++++++++++---- .../views/paper/review/ReviewDecisionRail.vue | 28 ++- .../src/views/paper/review/ReviewMain.vue | 24 ++- 4 files changed, 296 insertions(+), 40 deletions(-) diff --git a/frontend/taskdeck-web/src/tests/views/paper/review/ReviewDecisionRail.spec.ts b/frontend/taskdeck-web/src/tests/views/paper/review/ReviewDecisionRail.spec.ts index cc36ff44c..9f3bec09c 100644 --- a/frontend/taskdeck-web/src/tests/views/paper/review/ReviewDecisionRail.spec.ts +++ b/frontend/taskdeck-web/src/tests/views/paper/review/ReviewDecisionRail.spec.ts @@ -24,9 +24,13 @@ function mountRail( dismissable: boolean applyPhase: 'approve' | 'execute' editLock: 'off' | 'editing' | 'saving' + applyOnly: boolean + decisionDescriptionIds: string }> = {}, + options: { attachTo?: boolean } = {}, ) { return mount(ReviewDecisionRail, { + attachTo: options.attachTo ? document.body : undefined, props: { summary: '1 operation · explicit review · atomic apply', ...props, @@ -34,6 +38,14 @@ function mountRail( }) } +/** The decision controls the rail renders in its actionable (non-filing) mode. */ +const DECISION_TESTIDS = [ + 'decision-reject', + 'decision-edit', + 'decision-defer', + 'decision-apply', +] as const + describe('ReviewDecisionRail', () => { it('renders the four decision actions in the default (actionable) state', () => { const wrapper = mountRail() @@ -354,4 +366,93 @@ describe('ReviewDecisionRail', () => { expect(ids[0]).not.toBe(ids[1]) }) }) + + /** + * #2461 — the post-revision refresh lock is drawn by the Review view, above + * the rail. It used to describe only the non-focusable column wrapper, so a + * reviewer inspecting a disabled decision button was told nothing about why + * the whole row had gone inert. + */ + describe('external decision lock description (#2461)', () => { + const EXTERNAL_ID = 'external-refresh-lock' + + function renderExternalNote(): HTMLElement { + const note = document.createElement('p') + note.id = EXTERNAL_ID + note.textContent = 'Refreshing this proposal before your decision.' + document.body.appendChild(note) + return note + } + + it('describes every disabled decision control with the external explanation', () => { + const note = renderExternalNote() + const wrapper = mountRail( + { busy: true, decisionDescriptionIds: EXTERNAL_ID }, + { attachTo: true }, + ) + + for (const testid of DECISION_TESTIDS) { + const button = wrapper.get(`[data-testid="${testid}"]`) + expect(button.attributes('disabled')).toBeDefined() + const ids = (button.attributes('aria-describedby') ?? '').split(' ').filter(Boolean) + expect(ids).toEqual([EXTERNAL_ID]) + // Attached to the real document, so every referenced id must resolve. + for (const id of ids) { + expect(document.getElementById(id)).not.toBeNull() + } + } + + wrapper.unmount() + note.remove() + }) + + it('carries the external explanation and its own edit-lock note together', () => { + const note = renderExternalNote() + const wrapper = mountRail( + { busy: true, editLock: 'editing', decisionDescriptionIds: EXTERNAL_ID }, + { attachTo: true }, + ) + + const noteId = wrapper.get('[data-testid="decision-lock-note"]').attributes('id') + expect(noteId).toBeTruthy() + for (const testid of DECISION_TESTIDS) { + const ids = (wrapper.get(`[data-testid="${testid}"]`).attributes('aria-describedby') ?? '') + .split(' ') + .filter(Boolean) + expect(ids).toEqual([EXTERNAL_ID, noteId]) + for (const id of ids) { + expect(document.getElementById(id)).not.toBeNull() + } + } + + wrapper.unmount() + note.remove() + }) + + it('describes the only remaining control when a receipt leaves Apply alone', () => { + const note = renderExternalNote() + const wrapper = mountRail( + { busy: true, applyOnly: true, decisionDescriptionIds: EXTERNAL_ID }, + { attachTo: true }, + ) + + expect(wrapper.find('[data-testid="decision-reject"]').exists()).toBe(false) + const apply = wrapper.get('[data-testid="decision-apply"]') + expect(apply.attributes('disabled')).toBeDefined() + expect(apply.attributes('aria-describedby')).toBe(EXTERNAL_ID) + + wrapper.unmount() + note.remove() + }) + + it('adds no attribute when there is no explanation to point at', () => { + // A blank string is the shape a caller produces when it joins an empty id + // list. It must not become an aria-describedby that references nothing. + const wrapper = mountRail({ busy: true, decisionDescriptionIds: ' ' }) + for (const testid of DECISION_TESTIDS) { + expect(wrapper.get(`[data-testid="${testid}"]`).attributes('aria-describedby')) + .toBeUndefined() + } + }) + }) }) diff --git a/frontend/taskdeck-web/src/tests/views/paper/review/ReviewMain.spec.ts b/frontend/taskdeck-web/src/tests/views/paper/review/ReviewMain.spec.ts index 0a6bc3ef4..b76fac76d 100644 --- a/frontend/taskdeck-web/src/tests/views/paper/review/ReviewMain.spec.ts +++ b/frontend/taskdeck-web/src/tests/views/paper/review/ReviewMain.spec.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from 'vitest' +import { h, nextTick, ref } from 'vue' import { mount } from '@vue/test-utils' import ReviewMain from '../../../../views/paper/review/ReviewMain.vue' import type { @@ -46,48 +47,60 @@ const sideEffects: SideEffects = { const conflicts: ConflictRow[] = [] const history: HistoryRow[] = [] +type DeepReviewOptions = { + conflicts?: ConflictRow[] + history?: HistoryRow[] + applyPhase?: 'approve' | 'execute' + dismissable?: boolean + attachTo?: boolean + busy?: boolean + attrs?: Record +} + +function mainProps( + confidence: Partial = {}, + deepReview: DeepReviewOptions = {}, +) { + return { + serial: '#2026-04-25-014', + meta: '11:42 PT · awaiting decision', + titleParts: [ + { text: 'Split ' }, + { text: '“dark mode”', emphasis: true }, + { text: ' into 3 cards' }, + ], + lede: 'Lede text.', + decisionSummary: '3 ops · explicit review · atomic apply', + busy: deepReview.busy ?? false, + confidence: { + overall: confidence.overall === undefined ? 0.84 : confidence.overall, + components: confidence.components ?? [], + threshold: null, + note: confidence.note, + source: confidence.source ?? 'model-reported', + }, + before, + after, + fields, + changeSubTitle: '3 changes', + provenance, + proposalId: 'proposal-001', + sideEffects, + conflicts: deepReview.conflicts ?? conflicts, + history: deepReview.history ?? history, + applyPhase: deepReview.applyPhase ?? 'approve', + dismissable: deepReview.dismissable ?? false, + } +} + function mountMain( confidence: Partial = {}, - deepReview: { - conflicts?: ConflictRow[] - history?: HistoryRow[] - applyPhase?: 'approve' | 'execute' - dismissable?: boolean - attachTo?: boolean - } = {}, + deepReview: DeepReviewOptions = {}, ) { return mount(ReviewMain, { attachTo: deepReview.attachTo ? document.body : undefined, - props: { - serial: '#2026-04-25-014', - meta: '11:42 PT · awaiting decision', - titleParts: [ - { text: 'Split ' }, - { text: '“dark mode”', emphasis: true }, - { text: ' into 3 cards' }, - ], - lede: 'Lede text.', - decisionSummary: '3 ops · explicit review · atomic apply', - busy: false, - confidence: { - overall: confidence.overall === undefined ? 0.84 : confidence.overall, - components: confidence.components ?? [], - threshold: null, - note: confidence.note, - source: confidence.source ?? 'model-reported', - }, - before, - after, - fields, - changeSubTitle: '3 changes', - provenance, - proposalId: 'proposal-001', - sideEffects, - conflicts: deepReview.conflicts ?? conflicts, - history: deepReview.history ?? history, - applyPhase: deepReview.applyPhase ?? 'approve', - dismissable: deepReview.dismissable ?? false, - }, + attrs: deepReview.attrs, + props: mainProps(confidence, deepReview), }) } @@ -225,6 +238,102 @@ describe('ReviewMain', () => { }) }) + /** + * #2461 — the Review view renders the post-revision refresh lock above this + * column and describes the column with its id. That attribute lands on this + * wrapper, which is not focusable, so assistive tech inspecting a disabled + * decision button never reached the explanation. + */ + describe('decision lock description (#2461)', () => { + const LOCK_ID = 'paper-review-revision-refresh-lock' + const decisionTestIds = [ + 'decision-reject', + 'decision-edit', + 'decision-defer', + 'decision-apply', + ] + + it('forwards the column description ids to every disabled decision control', () => { + const note = document.createElement('p') + note.id = LOCK_ID + note.textContent = 'Refreshing this proposal before your decision.' + document.body.appendChild(note) + + const wrapper = mountMain( + {}, + { attachTo: true, busy: true, attrs: { 'aria-describedby': LOCK_ID } }, + ) + + // The wrapper keeps the attribute: those notes describe the whole column, + // and the Review view asserts that association. + expect(wrapper.get('[data-testid="paper-review-main"]').attributes('aria-describedby')) + .toBe(LOCK_ID) + + for (const testid of decisionTestIds) { + const button = wrapper.get(`[data-testid="${testid}"]`) + expect(button.attributes('disabled')).toBeDefined() + const ids = (button.attributes('aria-describedby') ?? '').split(' ').filter(Boolean) + expect(ids).toContain(LOCK_ID) + // Attached to the real document, so every referenced id must resolve. + for (const id of ids) { + expect(document.getElementById(id)).not.toBeNull() + } + } + + wrapper.unmount() + note.remove() + }) + + it('leaves the decision controls undescribed when the column has no explanation', () => { + const wrapper = mountMain({}, { attachTo: true, busy: true }) + + expect(wrapper.get('[data-testid="paper-review-main"]').attributes('aria-describedby')) + .toBeUndefined() + for (const testid of decisionTestIds) { + expect(wrapper.get(`[data-testid="${testid}"]`).attributes('aria-describedby')) + .toBeUndefined() + } + + wrapper.unmount() + }) + + it('drops the association from the controls when the lock clears', async () => { + // Fallthrough attributes are not reactive, so a cached read would keep + // pointing at a note the Review view has already removed. Driven from a + // host that re-renders, exactly as the Review view does. + const note = document.createElement('p') + note.id = LOCK_ID + note.textContent = 'Refreshing this proposal before your decision.' + document.body.appendChild(note) + + const describedBy = ref(LOCK_ID) + const host = mount( + { + render: () => + h(ReviewMain, { + ...mainProps({}, { busy: true }), + 'aria-describedby': describedBy.value, + }), + }, + { attachTo: document.body }, + ) + + expect(host.get('[data-testid="decision-apply"]').attributes('aria-describedby')) + .toBe(LOCK_ID) + + note.remove() + describedBy.value = undefined + await nextTick() + + expect(host.get('[data-testid="paper-review-main"]').attributes('aria-describedby')) + .toBeUndefined() + expect(host.get('[data-testid="decision-apply"]').attributes('aria-describedby')) + .toBeUndefined() + + host.unmount() + }) + }) + it('renders malformed enum fallbacks as user-visible attention states', () => { const wrapper = mountMain({}, { conflicts: [{ tone: 'warn', key: 'Unknown conflict', value: 'Review required' }], diff --git a/frontend/taskdeck-web/src/views/paper/review/ReviewDecisionRail.vue b/frontend/taskdeck-web/src/views/paper/review/ReviewDecisionRail.vue index 8db83732d..26545b189 100644 --- a/frontend/taskdeck-web/src/views/paper/review/ReviewDecisionRail.vue +++ b/frontend/taskdeck-web/src/views/paper/review/ReviewDecisionRail.vue @@ -60,6 +60,17 @@ const props = withDefaults( editLock?: EditLock /** An approved receipt leaves Apply as the only remaining decision. */ applyOnly?: boolean + /** + * Space-separated DOM ids of explanations rendered OUTSIDE the rail that say + * why the decision controls are in their current state. The shipped one is + * the post-revision refresh lock, which the Review view draws above this + * column while it re-reads the proposal (#2461). + * + * The caller owns the existence of these ids: the rail forwards exactly what + * it is given, so pass nothing while no such explanation is on screen rather + * than a constant id for an element that may not be rendered. + */ + decisionDescriptionIds?: string }>(), { applyPhase: 'approve', editLock: 'off', applyOnly: false }, ) @@ -82,11 +93,24 @@ const editLockNote = computed(() => ) /** - * `aria-describedby` is only attached while the explanation exists — a dangling + * Every disabled decision control names its reason, wherever that reason is + * drawn. The edit-lock note lives inside the rail; the post-revision refresh + * lock is drawn by the Review view above this column and arrives as + * `decisionDescriptionIds`. Before #2461 that second reason described only the + * non-focusable column wrapper, so assistive tech inspecting a disabled Reject, + * Request edit, Defer or Approve was told nothing about why it was inert. + * + * External ids come first, matching the order the notes appear on screen. + * + * `aria-describedby` is only attached while an explanation exists — a dangling * reference to an absent id is worse than none, because assistive tech reports * nothing and the markup claims otherwise. */ -const decisionDescribedBy = computed(() => (showEditLock.value ? lockNoteId : undefined)) +const decisionDescribedBy = computed(() => { + const ids = (props.decisionDescriptionIds ?? '').split(/\s+/).filter(Boolean) + if (showEditLock.value) ids.push(lockNoteId) + return ids.length > 0 ? ids.join(' ') : undefined +}) /** * Both phase labels are rendered, always, into the SAME grid cell of the diff --git a/frontend/taskdeck-web/src/views/paper/review/ReviewMain.vue b/frontend/taskdeck-web/src/views/paper/review/ReviewMain.vue index 61fe7b89d..cc4434c55 100644 --- a/frontend/taskdeck-web/src/views/paper/review/ReviewMain.vue +++ b/frontend/taskdeck-web/src/views/paper/review/ReviewMain.vue @@ -1,5 +1,5 @@