From abb18d4d8c839ce7838bc849c4911a8a127773eb Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 13 Nov 2026 01:16:21 +0100 Subject: [PATCH] =?UTF-8?q?fix(downloads):=20GIF=20review=20round=203=20?= =?UTF-8?q?=E2=80=94=20every=20length=20readout,=20live=20regions=20in=20p?= =?UTF-8?q?lace?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- docs/DESIGN.md | 5 +- .../features/player/ClipPanel.dom.test.tsx | 30 ++++++++- frontend/src/features/player/ClipPanel.tsx | 56 +++++++++------- .../files/VideoEditor.gif.dom.test.tsx | 52 ++++++++++++--- frontend/src/modules/files/VideoEditor.tsx | 65 ++++++++++--------- 5 files changed, 142 insertions(+), 66 deletions(-) diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 5c41dfb6..7ee47296 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -651,8 +651,9 @@ of a full-width panel and nothing on the right. (the long side: 320 / 480 / 640 px), the expected size ("about 4–10 MB"; one number when both ends read the same), a note over 15 MB (a chat's link preview will not move) or 30 MB (it may be refused), and over 30 s the reason, with Make GIF unavailable. A refused length is shown rounded up, never as one the cap allows. The - cap is judged in whole milliseconds between the ends as sent, the server's own rule (`lib/gif`, held to - `downloads/gif.py` by `gif.contract.test.ts`). The editor has its own Format row (a video only): with GIF + cap is judged in whole milliseconds between the ends as sent, the server's own rule (the limits in `lib/gif` + are held to `downloads/gif.py` by `gif.contract.test.ts`; the rule itself is written on each side and + tested there against the same boundary cases). The editor has its own Format row (a video only): with GIF each kept segment is a GIF of its own, Join is unavailable with its reason in a tooltip, the precise / fast choice gives way to one line (a GIF is always re-encoded), and the notes name each segment over a limit, in one live region. A shared GIF's page shows the picture itself (no player), its name, kind and size, its diff --git a/frontend/src/features/player/ClipPanel.dom.test.tsx b/frontend/src/features/player/ClipPanel.dom.test.tsx index a1d0bd33..ccf4ef10 100644 --- a/frontend/src/features/player/ClipPanel.dom.test.tsx +++ b/frontend/src/features/player/ClipPanel.dom.test.tsx @@ -14,9 +14,11 @@ vi.mock("react-i18next", async (importOriginal) => { const actual = await importOriginal(); 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(); + // 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(); + 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(); fireEvent.click(screen.getByRole("button", { name: "clip.formatGif" })); diff --git a/frontend/src/features/player/ClipPanel.tsx b/frontend/src/features/player/ClipPanel.tsx index 238aea08..76613fb7 100644 --- a/frontend/src/features/player/ClipPanel.tsx +++ b/frontend/src/features/player/ClipPanel.tsx @@ -432,33 +432,41 @@ export default function ClipPanel({ label: t("clip.gif.side", { n: formatNumber(s, i18n.language) }), }))} /> - - {t(estimateText.single ? "clip.gif.estimateOne" : "clip.gif.estimate", estimateText)} - )} + {/* 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. */} + + {gif && ( + + {t(estimateText.single ? "clip.gif.estimateOne" : "clip.gif.estimate", estimateText)} + + )} + + +
+ {gif && ( +

+ {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")} +

+ )}
- {gif && ( -

- {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")} -

- )}
({ ...(await importOriginal()), - // 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", () => { diff --git a/frontend/src/modules/files/VideoEditor.tsx b/frontend/src/modules/files/VideoEditor.tsx index f0c4ca06..f3534487 100644 --- a/frontend/src/modules/files/VideoEditor.tsx +++ b/frontend/src/modules/files/VideoEditor.tsx @@ -599,7 +599,7 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos /> - {tc(sel.end - sel.start)} + {tc(gif ? gifShownLength(sel.start, sel.end) : sel.end - sel.start)} {(inField.invalid || outField.invalid) && (
@@ -633,7 +633,13 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos })}
- {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, + ), + })}
@@ -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 && ( - - {t( - gifTotalText.single ? "editor.format.estimateOne" : "editor.format.estimate", - gifTotalText, - )} - - )} )} + {/* 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. */} + + {gif && kept.length > 0 && ( + + {t( + gifTotalText.single ? "editor.format.estimateOne" : "editor.format.estimate", + gifTotalText, + )} + + )} +
)} - {/* 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 && ( -
- {gifNotes.length > 0 && ( -
    - {gifNotes.map((g) => ( -
  • - {t(GIF_NOTE_KEY[g.verdict as keyof typeof GIF_NOTE_KEY], { - n: g.n, - length: tc(g.length), - seconds: GIF_MAX_S, - })} -
    • - ))} -
- )} -
- )} +
+ {gif && gifNotes.length > 0 && ( +
    + {gifNotes.map((g) => ( +
  • + {t(GIF_NOTE_KEY[g.verdict as keyof typeof GIF_NOTE_KEY], { + n: g.n, + length: tc(g.length), + seconds: GIF_MAX_S, + })} +
    • + ))} +
+ )} +
{/* crop + precision */}