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..787499ee5 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 @@ -1,4 +1,4 @@ -import { describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it } from 'vitest' import { h } from 'vue' import { mount, type VueWrapper } from '@vue/test-utils' import ReviewDecisionRail from '../../../../views/paper/review/ReviewDecisionRail.vue' @@ -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,95 @@ 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' + + // Registered by each test, run even when an assertion throws, so a leaked + // note id cannot resolve for a later test that expects it to be absent. + const cleanups: Array<() => void> = [] + + afterEach(() => { + for (const cleanup of cleanups.splice(0).reverse()) cleanup() + }) + + function renderExternalNote(): void { + const note = document.createElement('p') + note.id = EXTERNAL_ID + note.textContent = 'Refreshing this proposal before your decision.' + document.body.appendChild(note) + cleanups.push(() => note.remove()) + } + + it('describes every disabled decision control with the external explanation', () => { + renderExternalNote() + const wrapper = mountRail( + { busy: true, decisionDescriptionIds: EXTERNAL_ID }, + { attachTo: true }, + ) + cleanups.push(() => wrapper.unmount()) + + 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() + } + } + }) + + it('carries the external explanation and its own edit-lock note together', () => { + renderExternalNote() + const wrapper = mountRail( + { busy: true, editLock: 'editing', decisionDescriptionIds: EXTERNAL_ID }, + { attachTo: true }, + ) + cleanups.push(() => wrapper.unmount()) + + 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() + } + } + }) + + it('describes the only remaining control when a receipt leaves Apply alone', () => { + renderExternalNote() + const wrapper = mountRail( + { busy: true, applyOnly: true, decisionDescriptionIds: EXTERNAL_ID }, + { attachTo: true }, + ) + cleanups.push(() => wrapper.unmount()) + + 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) + }) + + 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..e525fe432 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 { afterEach, 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,131 @@ 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', + ] + + // Registered by each test, run even when an assertion throws, so a leaked + // note id cannot resolve for a later test that expects it to be absent. + const cleanups: Array<() => void> = [] + + afterEach(() => { + for (const cleanup of cleanups.splice(0).reverse()) cleanup() + }) + + function renderLockNote(): void { + const note = document.createElement('p') + note.id = LOCK_ID + note.textContent = 'Refreshing this proposal before your decision.' + document.body.appendChild(note) + cleanups.push(() => note.remove()) + } + + it('forwards the column description ids to every disabled decision control', () => { + renderLockNote() + + const wrapper = mountMain( + {}, + { attachTo: true, busy: true, attrs: { 'aria-describedby': LOCK_ID } }, + ) + cleanups.push(() => wrapper.unmount()) + + // 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() + } + } + }) + + it('leaves the decision controls undescribed when the column has no explanation', () => { + const wrapper = mountMain({}, { attachTo: true, busy: true }) + cleanups.push(() => wrapper.unmount()) + + 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() + } + }) + + it('never describes ENABLED decision controls, even while a column note is on screen', () => { + // The ids the Review view passes are a union: the refresh-lock note is + // drawn only during the refresh, but the evidence-unavailable note it can + // leave behind outlives the lock. Reject, Request edit and Defer are then + // enabled and have nothing to retry, so "no decision was made, choose the + // current action again" must not be their description (#2461 review). + renderLockNote() + + const wrapper = mountMain( + {}, + { attachTo: true, busy: false, attrs: { 'aria-describedby': LOCK_ID } }, + ) + cleanups.push(() => wrapper.unmount()) + + // The column itself is still described: the note is genuinely about it. + 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')).toBeUndefined() + expect(button.attributes('aria-describedby')).toBeUndefined() + } + }) + + 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. + renderLockNote() + + const describedBy = ref(LOCK_ID) + const host = mount( + { + render: () => + h(ReviewMain, { + ...mainProps({}, { busy: true }), + 'aria-describedby': describedBy.value, + }), + }, + { attachTo: document.body }, + ) + cleanups.push(() => host.unmount()) + + expect(host.get('[data-testid="decision-apply"]').attributes('aria-describedby')) + .toBe(LOCK_ID) + + 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() + }) + }) + 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..b90cff95a 100644 --- a/frontend/taskdeck-web/src/views/paper/review/ReviewDecisionRail.vue +++ b/frontend/taskdeck-web/src/views/paper/review/ReviewDecisionRail.vue @@ -60,6 +60,21 @@ 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 locked. 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 rail attaches exactly what it is given to all four decision controls, + * unconditionally, so the caller owns two obligations. The ids must resolve + * to elements that are actually rendered — a dangling reference reports + * nothing while the markup claims otherwise. And they must describe the + * CURRENT state of those controls: `ReviewMain` therefore forwards them only + * while `busy` holds the controls disabled, because the notes it draws from + * are a union and one of them outlives the lock that produced it. + */ + decisionDescriptionIds?: string }>(), { applyPhase: 'approve', editLock: 'off', applyOnly: false }, ) @@ -82,11 +97,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..a4446a9df 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 @@