Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 48 additions & 1 deletion frontend/app/e2e/annotate.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand Down
79 changes: 62 additions & 17 deletions frontend/ui-core/src/annotator/AnnotationPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -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 `<button>` receives
no pointer events and cannot take focus, so Radix's trigger
never opens and the reason cannot be read (principles 4 and 9).
`aria-disabled` keeps the hover and the focus, and the press is
refused in the handler. Native `disabled` survives only where
there is nothing to explain — a `Finished` job, whose label is
the explanation, and the in-flight press.
*/
<Button
variant="primary"
size="sm"
className="min-w-36"
data-testid="finish-job"
disabled={
!declares({ allowed_actions: jobActions }, JOB_ACTION.complete) ||
finishJob.isPending
}
{...(finishWithheld === null ? {} : { title: finishWithheld })}
onClick={() => finishJob.mutate()}
>
<CheckCheck className="size-4" />
{jobState === "completed" ? "Finished" : "Finish job"}
</Button>
<Tooltip>
<TooltipTrigger asChild>
<Button
variant="primary"
size="sm"
className={
finishWithheld === null ? "min-w-36" : "min-w-36 cursor-not-allowed opacity-40"
}
data-testid="finish-job"
data-withheld={finishWithheld === null ? "false" : "true"}
disabled={jobState === "completed" || finishJob.isPending}
aria-disabled={finishWithheld !== null || undefined}
onClick={() => {
if (finishWithheld === null && !finishJob.isPending) finishJob.mutate();
}}
>
<CheckCheck className="size-4" />
{jobState === "completed" ? "Finished" : "Finish job"}
</Button>
</TooltipTrigger>
{/* 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 && (
<TooltipContent side="bottom" data-testid="finish-withheld">
{finishWithheld}
</TooltipContent>
)}
</Tooltip>
) : (
/*
The flow verb, and the whole of #383 (decision 2).
Expand Down
79 changes: 77 additions & 2 deletions frontend/ui-core/src/annotator/topBar.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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,
Expand Down Expand Up @@ -152,6 +169,7 @@ function answer(path: string): unknown {

beforeEach(() => {
sent.length = 0;
jobCounts = null;
progress = "unannotated";
jobSettled = false;
assetCount = 1;
Expand Down Expand Up @@ -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 () => {
Expand Down
Loading