peter/siftlode
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(downloads): GIF review round 3 — every length readout, live regions in place

Browse Source 
A refused GIF's length reads rounded up everywhere the editor shows it (the segment and the kept summary of one), and the GIF notes and estimates sit in live regions that are there before GIF is picked, so a note already true then is said. Tests hold the clip panel's readout and which segment a note names; DESIGN.md claims only what the contract test holds.
This commit is contained in:
peter
2026-11-13 01:16:21 +01:00
parent 5bf13c8701
commit abb18d4d8c
5 changed files with 142 additions and 66 deletions
Download Patch File Download Diff File Expand all files Collapse all files
frontend/src/features/player/ClipPanel.dom.test.tsx
+28 -2
View File
@@ -14,9 +14,11 @@ vi.mock("react-i18next", async (importOriginal) => {
const actual = await importOriginal<typeof import("react-i18next")>();
return {
...actual,
// A size option reads as its own words with its unit ("480 px"); every other text is its key.
// A size option reads as its own words with its unit ("480 px"), the length with the length it
// gives; every other text is its key.
useTranslation: () => ({
t: (k: string, o?: { n?: string }) => (k === "clip.gif.side" ? `${o?.n} px` : k),
t: (k: string, o?: { n?: string; length?: string }) =>
k === "clip.gif.side" ? `${o?.n} px` : k === "clip.length" ? `${k} ${o?.length}` : k,
i18n: { language: "en" },
}),
};
@@ -322,6 +324,30 @@ describe("ClipPanel GIF", () => {
expect(enqueueClip).not.toHaveBeenCalled();
});
it("says a refused GIF length so it never reads as one the cap allows", () => {
render(<Harness start={{ start: 100, end: 130.03 }} />);
// A video of it is 0:30.0 long, as it always read…
expect(screen.getByText(/^clip\.length /).textContent).toBe("clip.length 0:30.0");
fireEvent.click(screen.getByRole("button", { name: "clip.formatGif" }));
// …and a GIF of it, refused, reads 0:30.1.
expect(screen.getByText(/^clip\.length /).textContent).toBe("clip.length 0:30.1");
const make = screen.getByRole("button", { name: "clip.gif.make" });
expect(make.getAttribute("aria-disabled")).toBe("true");
});
it("says a note already true when GIF is picked inside a live region that was there before", () => {
render(<Harness start={{ start: 100, end: 131 }} />);
const before = new Map(
[...document.querySelectorAll("[aria-live='polite']")].map((r) => [r, r.textContent]),
);
fireEvent.click(screen.getByRole("button", { name: "clip.formatGif" }));
// The reason and the estimate change inside regions that were there, empty, before: what
// changes inside a region is announced, a region that arrives with its text is not.
const note = screen.getByText("clip.gif.tooLong").closest("[aria-live='polite']")!;
const estimate = screen.getByText("clip.gif.estimate").closest("[aria-live='polite']")!;
for (const region of [note, estimate]) expect(before.get(region)).toBe("");
});
it("says what the expected size means: fine, no moving preview, or past the cap", () => {
render(<Harness start={{ start: 100, end: 110 }} />);
fireEvent.click(screen.getByRole("button", { name: "clip.formatGif" }));
frontend/src/features/player/ClipPanel.tsx
+32 -24
View File
@@ -432,33 +432,41 @@ export default function ClipPanel({
label: t("clip.gif.side", { n: formatNumber(s, i18n.language) }),
}))}
/>
<span className="tabular-nums text-muted" aria-live="polite">
{t(estimateText.single ? "clip.gif.estimateOne" : "clip.gif.estimate", estimateText)}
</span>
</>
)}
{/* The live regions stay mounted whatever the format (laid out as their content alone): a
live region announces what changes inside it, not what arrives together with it, so
a note already true when GIF is picked is said too. */}
<span className="contents" aria-live="polite">
{gif && (
<span className="tabular-nums text-muted">
{t(estimateText.single ? "clip.gif.estimateOne" : "clip.gif.estimate", estimateText)}
</span>
)}
</span>
</div>
<div className="contents" aria-live="polite">
{gif && (
<p
id={gifNoteId}
className={`text-xs ${
verdict === "too_long"
? "text-danger"
: verdict === "ok"
? "text-muted"
: "text-media-mark"
}`}
>
{verdict === "too_long"
? t("clip.gif.tooLong", { seconds: GIF_MAX_S })
: verdict === "may_fail"
? t("clip.gif.mayFail")
: verdict === "no_preview"
? t("clip.gif.noPreview")
: t("clip.gif.hint")}
</p>
)}
</div>
{gif && (
<p
id={gifNoteId}
className={`text-xs ${
verdict === "too_long"
? "text-danger"
: verdict === "ok"
? "text-muted"
: "text-media-mark"
}`}
aria-live="polite"
>
{verdict === "too_long"
? t("clip.gif.tooLong", { seconds: GIF_MAX_S })
: verdict === "may_fail"
? t("clip.gif.mayFail")
: verdict === "no_preview"
? t("clip.gif.noPreview")
: t("clip.gif.hint")}
</p>
)}
<div className="flex flex-wrap items-center gap-2">
<Input
frontend/src/modules/files/VideoEditor.gif.dom.test.tsx
+43 -9
View File
@@ -4,20 +4,25 @@
// cut-list, and never with the precise/fast choice, which a GIF does not have), a whole short video
// is a GIF too, and a segment longer than a GIF may be leaves Create unavailable with its reason.
import { describe, it, expect, vi, beforeAll, afterEach } from "vitest";
import { render, screen, fireEvent, waitFor, cleanup } from "@testing-library/react";
import { render, screen, fireEvent, waitFor, cleanup, within } from "@testing-library/react";
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
vi.mock("react-i18next", async (importOriginal) => ({
...(await importOriginal<typeof import("react-i18next")>()),
// A size option reads as its own words with its unit ("480 px"), a too-long note with the length
// it gives; every other text is its key.
// A size option reads as its own words with its unit ("480 px"), a segment's note with the
// segment it names (and a too-long one with the length it gives), the kept summary with its
// length; every other text is its key.
useTranslation: () => ({
t: (key: string, o?: { n?: string; length?: string }) =>
t: (key: string, o?: { n?: string; length?: string; dur?: string }) =>
key === "clip.gif.side"
? `${o?.n} px`
: key === "editor.format.segTooLong"
? `${key} ${o?.length}`
: key,
? `${key} #${o?.n} ${o?.length}`
: key === "editor.format.segMayFail" || key === "editor.format.segNoPreview"
? `${key} #${o?.n}`
: key === "editor.keptSummary"
? `${key} ${o?.dur}`
: key,
i18n: { language: "en" },
}),
}));
@@ -123,7 +128,7 @@ describe("VideoEditor — GIF", () => {
const create = screen.getByRole("button", { name: "editor.createGif" });
expect(create.getAttribute("aria-disabled")).toBe("true");
const notes = document.getElementById(create.getAttribute("aria-describedby")!)!;
expect(notes.textContent).toBe("editor.format.segTooLong 1:40.0");
expect(notes.textContent).toBe("editor.format.segTooLong #1 1:40.0");
fireEvent.click(create);
expect(enqueue).not.toHaveBeenCalled();
// A video of the same range has no such limit.
@@ -140,7 +145,36 @@ describe("VideoEditor — GIF", () => {
const create = screen.getByRole("button", { name: "editor.createGif" });
expect(create.getAttribute("aria-disabled")).toBe("true");
const notes = document.getElementById(create.getAttribute("aria-describedby")!)!;
expect(notes.textContent).toBe("editor.format.segTooLong 0:30.1");
expect(notes.textContent).toBe("editor.format.segTooLong #1 0:30.1");
// The segment's own length and the kept summary say it the same way, never 0:30.0.
expect(screen.getByText("0:30.1")).toBeTruthy();
expect(screen.getByText("editor.keptSummary 0:30.1")).toBeTruthy();
expect(screen.queryByText("0:30.0")).toBeNull();
});
it("names each segment by its place, the worst note first, a dropped one left out", () => {
const { playhead } = open(80);
playhead(10);
fireEvent.click(screen.getByText("editor.splitHere"));
playhead(30);
fireEvent.click(screen.getByText("editor.splitHere"));
// Segment 1 (0–10) dropped; 2 (10–30) and 3 (30–80) kept.
const track = screen.getByRole("group", { name: "editor.timeline" });
fireEvent.pointerDown(within(track).getAllByRole("button", { pressed: true })[0]!);
fireEvent.click(screen.getByRole("button", { name: "editor.format.gif" }));
fireEvent.click(screen.getByRole("button", { name: "640 px" }));
const notes = [...document.querySelectorAll("li")].map((l) => l.textContent);
// 50 s is too long, 20 s at 640 may pass 30 MB; the dropped segment says nothing.
expect(notes).toEqual(["editor.format.segTooLong #3 0:50.0", "editor.format.segMayFail #2"]);
});
it("says a note already true when GIF is picked inside a live region that was there before", () => {
open(100);
const region = document.querySelector("div[aria-live='polite']")!;
expect(region.textContent).toBe("");
fireEvent.click(screen.getByRole("button", { name: "editor.format.gif" }));
expect(document.querySelector("div[aria-live='polite']")).toBe(region);
expect(region.textContent).toBe("editor.format.segTooLong #1 1:40.0");
});
it("says a GIF failure as one of GIFs, not of clips", async () => {
@@ -187,7 +221,7 @@ describe("VideoEditor — GIF", () => {
// 20 s at 640: up to about 32 MB, said inside the same region.
fireEvent.click(screen.getByRole("button", { name: "640 px" }));
expect(document.querySelector("div[aria-live='polite']")).toBe(region);
expect(region.textContent).toBe("editor.format.segMayFail");
expect(region.textContent).toBe("editor.format.segMayFail #1");
});
it("says one size for a short GIF, and none when nothing is kept", () => {
frontend/src/modules/files/VideoEditor.tsx
+36 -29
View File
@@ -599,7 +599,7 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos
/>
</label>
<span className="text-xs text-muted tabular-nums w-16 text-right">
{tc(sel.end - sel.start)}
{tc(gif ? gifShownLength(sel.start, sel.end) : sel.end - sel.start)}
</span>
{(inField.invalid || outField.invalid) && (
<div className="basis-full">
@@ -633,7 +633,13 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos
})}
<div className="flex-1" />
<span className="text-xs text-muted">
{t("editor.keptSummary", { n: kept.length, dur: tc(keptDur) })}
{t("editor.keptSummary", {
n: kept.length,
// One GIF: its length as the notes say it (a refused one never reads as allowed).
dur: tc(
gif && kept.length === 1 ? gifShownLength(kept[0]!.start, kept[0]!.end) : keptDur,
),
})}
</span>
</div>
@@ -664,37 +670,38 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos
label: t("clip.gif.side", { n: formatNumber(s, i18n.language) }),
}))}
/>
{kept.length > 0 && (
<span className="text-xs text-muted tabular-nums" aria-live="polite">
{t(
gifTotalText.single ? "editor.format.estimateOne" : "editor.format.estimate",
gifTotalText,
)}
</span>
)}
</>
)}
{/* The live regions stay mounted whatever the format (laid out as their content
alone): a live region announces what changes inside it, not what arrives together
with it, so a note already true when GIF is picked is said too. */}
<span className="contents" aria-live="polite">
{gif && kept.length > 0 && (
<span className="text-xs text-muted tabular-nums">
{t(
gifTotalText.single ? "editor.format.estimateOne" : "editor.format.estimate",
gifTotalText,
)}
</span>
)}
</span>
</div>
)}
{/* Mounted for as long as GIF is chosen: a live region announces what changes inside it,
not what arrives together with it, and the first note is the one that counts. */}
{gif && (
<div id={gifNoteId} aria-live="polite">
{gifNotes.length > 0 && (
<ul className="space-y-0.5 text-xs">
{gifNotes.map((g) => (
<li key={g.n} className={g.verdict === "too_long" ? "text-danger" : "text-warn"}>
{t(GIF_NOTE_KEY[g.verdict as keyof typeof GIF_NOTE_KEY], {
n: g.n,
length: tc(g.length),
seconds: GIF_MAX_S,
})}
</li>
))}
</ul>
)}
</div>
)}
<div id={gifNoteId} className="contents" aria-live="polite">
{gif && gifNotes.length > 0 && (
<ul className="space-y-0.5 text-xs">
{gifNotes.map((g) => (
<li key={g.n} className={g.verdict === "too_long" ? "text-danger" : "text-warn"}>
{t(GIF_NOTE_KEY[g.verdict as keyof typeof GIF_NOTE_KEY], {
n: g.n,
length: tc(g.length),
seconds: GIF_MAX_S,
})}
</li>
))}
</ul>
)}
</div>
{/* crop + precision */}
<div className="grid sm:grid-cols-2 gap-3">