From e6724dd8b61e2b4b3f2e352035cb476e4fedb0d5 Mon Sep 17 00:00:00 2001 From: Jesus Armando Anaya Date: Sat, 8 Aug 2026 01:01:02 -0700 Subject: [PATCH] =?UTF-8?q?fix(annotator):=20the=20withheld=20Finish=20job?= =?UTF-8?q?=20explains=20itself=20=E2=80=94=20a=20focusable=20tooltip=20ca?= =?UTF-8?q?rrying=20the=20unresolved=20count=20(#427)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- frontend/app/e2e/annotate.spec.ts | 49 +++++++++++- .../ui-core/src/annotator/AnnotationPage.tsx | 79 +++++++++++++++---- .../ui-core/src/annotator/topBar.test.tsx | 79 ++++++++++++++++++- 3 files changed, 187 insertions(+), 20 deletions(-) diff --git a/frontend/app/e2e/annotate.spec.ts b/frontend/app/e2e/annotate.spec.ts index 9afadd38..0f36fb3b 100644 --- a/frontend/app/e2e/annotate.spec.ts +++ b/frontend/app/e2e/annotate.spec.ts @@ -108,6 +108,12 @@ interface Lifecycle { refuseProgress?: string; /** When set, `POST /jobs/{id}/complete` refuses 409 with this code instead. */ refuseJobComplete?: string; + /** + * Whether every asset is settled, which is what makes the job declare + * `complete`. Defaults true, as the older scenarios assumed; the withheld + * Finish-job scenarios (#427) set it false. + */ + jobSettled?: boolean; } function openedWorld(): Lifecycle { @@ -176,7 +182,10 @@ async function serveApi( batch_id: BATCH, state: lifecycle.job, asset_count: 2, - allowed_actions: jobActions(lifecycle.job, { batchState: lifecycle.batch }), + allowed_actions: jobActions(lifecycle.job, { + batchState: lifecycle.batch, + settled: lifecycle.jobSettled ?? true, + }), }); await page.route("**/api/**", async (route) => { const request = route.request(); @@ -2094,6 +2103,44 @@ test("a refused Accept says why", async ({ page }) => { await expect(page.getByTestId("action-refusal")).toContainText(/already moved on/i); }); +/** + * Principle 9 with principle 4 riding on it (#427): the withheld Finish job + * carries its reason as a real tooltip that opens on **focus**, not only on + * hover — which is only possible because the withheld state is `aria-disabled` + * rather than natively disabled, and only provable in a real browser, where + * focus and Radix's open-on-focus actually run. + */ +test("a withheld Finish job explains itself on focus, with the count", async ({ page }) => { + const sent: Request[] = []; + await openJob(page, sent, progressStore({ "asset-1": "unannotated", "asset-2": "annotated" }), { + batch: "in_annotation", + job: "in_progress", + jobSettled: false, + }); + + // The last frame, the only one Finish job renders on (#416). Frame 1 stays + // unannotated behind us — the one unresolved frame the sentence counts. + await page.getByTestId("next-asset").click(); + await expect(page.getByTestId("asset-position")).toContainText("2/2"); + + const finish = page.getByTestId("finish-job"); + await expect(finish).toHaveAttribute("aria-disabled", "true"); + + // Keyboard first: the reason is reachable without a pointer. + await finish.focus(); + await expect(page.getByTestId("finish-withheld")).toContainText( + "1 frame unresolved — annotate or skip it to finish the job.", + ); + + // The press is refused in the handler, so nothing reaches the wire — the + // `aria-disabled` spelling must not have quietly made the button live. + // `force`, because Playwright itself honours `aria-disabled` and would + // refuse to press at all — which is the assistive-tech contract working, but + // here the claim is about the handler behind it. + await finish.click({ force: true }); + expect(sent.filter((r) => r.method() === "POST" && r.url().endsWith("/complete"))).toEqual([]); +}); + test("a refused Finish job says why, rather than re-enabling in silence", async ({ page }) => { const sent: Request[] = []; // Every frame settled, so `complete` is declared and the button is live — the diff --git a/frontend/ui-core/src/annotator/AnnotationPage.tsx b/frontend/ui-core/src/annotator/AnnotationPage.tsx index 5d3c92ef..f7142b42 100644 --- a/frontend/ui-core/src/annotator/AnnotationPage.tsx +++ b/frontend/ui-core/src/annotator/AnnotationPage.tsx @@ -174,7 +174,7 @@ import { } from "./jobQueries"; import { AddClassDialog, runAddClass } from "./AddClassDialog"; import { FrameGallery } from "./FrameGallery"; -import { PROGRESS_LABEL, progressDotClass, progressTone } from "../screens/batchState"; +import { PROGRESS_LABEL, outstandingWork, progressDotClass, progressTone } from "../screens/batchState"; import type { LabelClassBody, SchemaDiff, SchemaVersion } from "../screens/queries"; import { batchKeys, @@ -581,6 +581,13 @@ interface WorkspaceProps { readonly annotated: number; readonly total: number; readonly unannotated: number; + /** + * With `unannotated`, the other state that blocks the job's `complete` — + * `outstandingWork` sums exactly the two, and the Finish-job tooltip reads + * that sum (#427). The full five-field model arrives from the wire; this + * type names only what the page consumes. + */ + readonly review_pending: number; } | null; /** Held by `JobScreen`, so `mod+c` here and `mod+v` on the next frame is one clipboard. */ readonly clipboard: Clipboard; @@ -1140,12 +1147,25 @@ function Workspace({ * Null on a job that is already `completed`: the label reads `Finished`, and a * tooltip repeating the word in the button is a tooltip nobody needs. Null too * once `complete` is declared, because then it is simply live. + * + * The sentence names the blocker **with its count** (#427): `outstandingWork` + * is `batchState.ts`'s spelling of "how many frames still block completion" — + * `unannotated` plus `review_pending`, the same two states whose settling is + * what makes the kernel declare `complete` — so the number and the disable + * come from one progress read rather than a second derivation here. The + * count-less sentence survives only for the moments the counts query has not + * answered yet (or disagrees with a declaration mid-invalidation). */ + const unresolved = counts === null ? 0 : outstandingWork(counts); const finishWithheld = jobState === "completed" || declares({ allowed_actions: jobActions }, JOB_ACTION.complete) ? null : (withheld ?? - "Every frame has to be annotated, skipped or accepted before this job can finish."); + (unresolved === 0 + ? "Every frame has to be annotated, skipped or accepted before this job can finish." + : unresolved === 1 + ? "1 frame unresolved — annotate or skip it to finish the job." + : `${unresolved} frames unresolved — annotate or skip them to finish the job.`)); /** * Whether pressing the flow verb will actually store anything (#383). @@ -1487,22 +1507,47 @@ function Workspace({ of forty-eight was possible before and is not now — which is the same rule that already governs its filled treatment, applied to whether it is on screen at all. + + **The reason is a real tooltip, and the withheld state is + `aria-disabled`, never the native attribute** (#427). This was a + `title` spread — invisible to the keyboard and to most pointers. + `ZoomWidget` earned the pattern: a disabled ` + + + + + {/* Only while withheld: an enabled Finish job explains itself by + being pressable, and a tooltip repeating the label would be + noise over the one control the frame exists to end on. */} + {finishWithheld !== null && ( + + {finishWithheld} + + )} + ) : ( /* The flow verb, and the whole of #383 (decision 2). diff --git a/frontend/ui-core/src/annotator/topBar.test.tsx b/frontend/ui-core/src/annotator/topBar.test.tsx index e56ed7b8..28cf8196 100644 --- a/frontend/ui-core/src/annotator/topBar.test.tsx +++ b/frontend/ui-core/src/annotator/topBar.test.tsx @@ -59,6 +59,20 @@ let jobSettled = false; */ let assetCount = 1; +/** + * What `/jobs/{id}/progress` answers — the counts the Finish-job tooltip reads + * (#427). Null keeps the route unanswered, which is how the older tests ran and + * what the page treats as "no counts yet". + */ +let jobCounts: { + unannotated: number; + annotated: number; + skipped: number; + review_pending: number; + accepted: number; + total: number; +} | null = null; + /** Whether the frames arrive carrying a box — what `drawn > 0` reads. */ let annotated = false; @@ -77,6 +91,9 @@ const PROGRESS_STATES = [ ] as const satisfies readonly Progress[]; function answer(path: string): unknown { + if (path === `/jobs/${JOB}/progress` && jobCounts !== null) { + return jobCounts; + } if (path === `/jobs/${JOB}`) { return { id: JOB, @@ -152,6 +169,7 @@ function answer(path: string): unknown { beforeEach(() => { sent.length = 0; + jobCounts = null; progress = "unannotated"; jobSettled = false; assetCount = 1; @@ -478,13 +496,70 @@ describe("the flow verb", () => { it("says why Finish job cannot be pressed where it does render (#416, principle 9)", async () => { // The other half: it appears on the last frame whether or not the job can be // finished — it is the filled slot there — so on that frame it owes a reason. + // + // `aria-disabled`, never the native attribute, and a real tooltip rather + // than a `title` (#427): a natively disabled button cannot be hovered or + // focused, so its reason could never be read. The press is refused in the + // handler instead, which the mutation assertion below holds. assetCount = 1; jobSettled = false; await open(); const finish = screen.getByTestId("finish-job"); - expect(finish.hasAttribute("disabled")).toBe(true); - expect(finish.getAttribute("title")).toMatch(/before this job can finish/i); + expect(finish.hasAttribute("disabled")).toBe(false); + expect(finish.getAttribute("aria-disabled")).toBe("true"); + expect(finish.getAttribute("title")).toBeNull(); + + await userEvent.hover(finish); + expect( + (await screen.findAllByText(/before this job can finish/i)).length, + ).toBeGreaterThan(0); + + // After the reason was read: the press is refused in the handler, so the + // `aria-disabled` spelling has not quietly made the button live. + await userEvent.click(finish); + expect(sent.some((request) => request.path.endsWith("/complete"))).toBe(false); + }); + + it("names the blocker with its count, from the same progress the readout shows (#427)", async () => { + assetCount = 1; + jobSettled = false; + jobCounts = { unannotated: 2, annotated: 1, skipped: 0, review_pending: 1, accepted: 0, total: 4 }; + await open(); + + // `outstandingWork`: unannotated + review_pending — the two states whose + // settling is what makes the kernel declare `complete`. + await userEvent.hover(screen.getByTestId("finish-job")); + expect( + (await screen.findAllByText("3 frames unresolved — annotate or skip them to finish the job.")).length, + ).toBeGreaterThan(0); + }); + + it("speaks singular for a single unresolved frame", async () => { + assetCount = 1; + jobSettled = false; + jobCounts = { unannotated: 1, annotated: 3, skipped: 0, review_pending: 0, accepted: 0, total: 4 }; + await open(); + + await userEvent.hover(screen.getByTestId("finish-job")); + expect( + (await screen.findAllByText("1 frame unresolved — annotate or skip it to finish the job.")).length, + ).toBeGreaterThan(0); + }); + + it("carries no tooltip at all once it is live", async () => { + // An enabled Finish job explains itself by being pressable (#427). + assetCount = 1; + jobSettled = true; + progress = "annotated"; + jobCounts = { unannotated: 0, annotated: 4, skipped: 0, review_pending: 0, accepted: 0, total: 4 }; + await open(); + + const finish = screen.getByTestId("finish-job"); + expect(finish.hasAttribute("disabled")).toBe(false); + expect(finish.getAttribute("aria-disabled")).toBeNull(); + await userEvent.hover(finish); + expect(screen.queryByTestId("finish-withheld")).toBeNull(); }); it("hands the filled slot to Finish job on the last frame, and does not render", async () => {