diff --git a/src/features/direct-messages/NewMessage.recovery.test.tsx b/src/features/direct-messages/NewMessage.recovery.test.tsx index e98034a6c4..3b2cedb5b1 100644 --- a/src/features/direct-messages/NewMessage.recovery.test.tsx +++ b/src/features/direct-messages/NewMessage.recovery.test.tsx @@ -1328,7 +1328,8 @@ it("waits for uploads before opening a DM and reports failure only on the chip", ), ).toHaveAttribute("role", "status"); expect(screen.getAllByRole("alert")).toHaveLength(1); - expect(send()).toBeDisabled(); + // Focusable while blocked, so its tooltip can say why. + expect(send()).toHaveAttribute("aria-disabled", "true"); expect(t.openDirectMessage).not.toHaveBeenCalled(); expect(t.publish).not.toHaveBeenCalled(); await t.user.click(screen.getByRole("button", { name: "Retry photo.png" })); diff --git a/src/features/messages/ComposerAttachments.module.css b/src/features/messages/ComposerAttachments.module.css index 5680e4ea40..f8b8482d8a 100644 --- a/src/features/messages/ComposerAttachments.module.css +++ b/src/features/messages/ComposerAttachments.module.css @@ -168,24 +168,31 @@ html[data-keyboard-navigation] .preview:focus-visible { } .item[data-upload-error] { - /* Fit the strip when possible, but never squeeze the recovery controls. */ + /* The error and its actions each take a full row from the tile's inner + edge, and the icon gap stays the resting tile's. Fit the strip when + possible, but never squeeze the recovery controls. */ min-width: min-content; flex-wrap: wrap; - column-gap: var(--space-2); row-gap: var(--space-1); padding-inline-end: calc(var(--space-5) / 2); } .item[data-upload-error] .error { + /* With the row gap, 12px below the file row, as the actions sit 12px + below the error. */ flex-basis: 100%; + margin-block-start: var(--space-2); } .item[data-upload-error] .details { /* Ellipsized filenames must not determine the card's minimum width. */ contain: inline-size; } .item[data-upload-error] .actions { + /* As in a toast: 12px from the message, 8px between actions. The actions + never wrap; the tile's min-content width keeps room for both. */ position: static; flex-basis: 100%; - flex-wrap: wrap; + gap: var(--space-2); + margin-block-start: var(--space-2); min-width: min-content; } diff --git a/src/features/messages/ComposerAttachments.test.tsx b/src/features/messages/ComposerAttachments.test.tsx index 5ec95446a4..d86da7bcf4 100644 --- a/src/features/messages/ComposerAttachments.test.tsx +++ b/src/features/messages/ComposerAttachments.test.tsx @@ -355,6 +355,11 @@ it.each([ const removeButton = screen.getByRole("button", { name: `Remove ${name}` }); expect(retryButton.textContent).toBe("Retry"); expect(removeButton.textContent).toBe("Remove"); + // Retry first, then Remove. + expect([...(retryButton.parentElement?.children ?? [])]).toEqual([ + retryButton, + removeButton, + ]); act(() => retryButton.focus()); expect(retryButton.getAttribute("title")).toBeNull(); fireEvent.click(retryButton); @@ -399,7 +404,7 @@ it.each([ expect(screen.getByText(error, { selector: "p" })).toBe(alert); expect(alert.classList.contains("sr-only")).toBe(false); expect( - screen.queryByText("Upload failed. Retry or remove it to send."), + screen.queryByText("Upload failed. Retry or remove this file."), ).toBeNull(); }); @@ -414,7 +419,7 @@ it("explains errors without service details and disables both recovery actions", , ); expect(screen.getByRole("alert").textContent).toBe( - "photo.png: Upload failed. Retry or remove it to send.", + "photo.png: Upload failed. Retry or remove this file.", ); for (const action of ["Retry", "Remove"]) { const button = screen.getByRole("button", { name: `${action} photo.png` }); diff --git a/src/features/messages/ComposerAttachments.tsx b/src/features/messages/ComposerAttachments.tsx index c59266874c..0c73f2e8ef 100644 --- a/src/features/messages/ComposerAttachments.tsx +++ b/src/features/messages/ComposerAttachments.tsx @@ -23,7 +23,7 @@ import { useDraftAttachmentSource, } from "./use-draft-attachment-source"; import { XIcon } from "../../shared/design-system/icons"; -import type { DraftAttachment } from "./attachment-draft"; +import { UPLOAD_FAILED_TEXT, type DraftAttachment } from "./attachment-draft"; import styles from "./ComposerAttachments.module.css"; import { useMediaElementSource } from "./use-media-element-source"; @@ -417,7 +417,7 @@ function AttachmentItem({ {uploadFailed && (

{item.file.name}: - {item.error || "Upload failed. Retry or remove it to send."} + {item.error || UPLOAD_FAILED_TEXT}

)}
expect(trigger).toHaveFocus(); }); +it("words a failed upload and explains the disabled Send, until the file is removed", async () => { + const h = await mountUploadComposer(); + pickAttachments(h.container, [attachmentFile()]); + await waitFor(() => expect(h.uploadCalls).toHaveLength(1)); + await act(() => + h.uploadCalls[0]?.result.reject(new TypeError("Failed to fetch")), + ); + expect(screen.getByRole("alert")).toHaveTextContent( + "Upload didn’t finish. Check your connection, then retry.", + ); + expect(screen.queryByText(/Failed to fetch/)).toBeNull(); + // A disabled button gets no hover from the browser, so Send stays focusable + // and its tooltip says what to do. + const send = screen.getByRole("button", { name: "Send message" }); + expect(send).toHaveAttribute("aria-disabled", "true"); + await userEvent.hover(send); + expect(await screen.findByRole("tooltip")).toHaveTextContent( + "Retry or remove the failed file to send.", + ); + await userEvent.unhover(send); + // Focusable is not enabled: neither Send nor Enter sends or uploads again. + fireEvent.change(h.input(), { target: { value: "With the file" } }); + fireEvent.click(send); + fireEvent.keyDown(h.input(), { key: "Enter" }); + await act(async () => {}); + expect(h.sign).not.toHaveBeenCalled(); + expect(h.publish).not.toHaveBeenCalled(); + expect(h.uploadCalls).toHaveLength(1); + expect(h.input()).toHaveValue("With the file"); + expect(screen.getByRole("alert")).toHaveTextContent( + "Upload didn’t finish. Check your connection, then retry.", + ); + fireEvent.click(screen.getByRole("button", { name: "Remove notes.txt" })); + await waitFor(() => + expect( + screen.getByRole("button", { name: "Send message" }), + ).not.toHaveAttribute("aria-disabled", "true"), + ); +}); + it.each(["inactive", "hidden"])( "cancels picker focus when the conversation becomes %s, including a warm return", async (state) => { @@ -3000,21 +3041,22 @@ it("offers message preparation retry after a failed attachment send remounts", a }); it("identifies failed filenames through retry and removal when service errors match", async () => { + const NETWORK = "Upload didn’t finish. Check your connection, then retry."; const h = await mountUploadComposer(); attachByPaste(h.input(), attachmentFile("one.txt")); attachByPaste(h.input(), attachmentFile("two.txt")); await waitFor(() => expect(h.uploadCalls).toHaveLength(1)); await act(async () => { - h.uploadCalls[0]?.result.reject(new Error("HTTP 503")); + h.uploadCalls[0]?.result.reject(new TypeError("Failed to fetch")); }); await waitFor(() => expect(h.uploadCalls).toHaveLength(2)); await act(async () => { - h.uploadCalls[1]?.result.reject(new Error("HTTP 503")); + h.uploadCalls[1]?.result.reject(new TypeError("Failed to fetch")); }); await waitFor(() => expect( screen.getAllByRole("alert").map((node) => node.textContent), - ).toEqual(["one.txt: HTTP 503", "two.txt: HTTP 503"]), + ).toEqual([`one.txt: ${NETWORK}`, `two.txt: ${NETWORK}`]), ); await userEvent.click(screen.getByRole("button", { name: "Retry one.txt" })); await waitFor(() => expect(h.uploadCalls).toHaveLength(3)); @@ -3024,12 +3066,12 @@ it("identifies failed filenames through retry and removal when service errors ma await waitFor(() => expect( screen.getAllByRole("alert").map((node) => node.textContent), - ).toEqual(["one.txt: first failed again", "two.txt: HTTP 503"]), + ).toEqual([`one.txt: ${UPLOAD_FAILED_TEXT}`, `two.txt: ${NETWORK}`]), ); await userEvent.click(screen.getByRole("button", { name: "Remove one.txt" })); expect(h.input()).toHaveFocus(); expect(screen.getAllByRole("alert").map((node) => node.textContent)).toEqual([ - "two.txt: HTTP 503", + `two.txt: ${NETWORK}`, ]); await waitFor(() => expect(within(h.form()).queryByText("one.txt")).not.toBeInTheDocument(), @@ -3057,7 +3099,7 @@ it("retains successful attachment uploads after a later file fails and retries o }); await screen.findAllByRole("alert"); expect(screen.getAllByRole("alert").map((node) => node.textContent)).toEqual([ - "retry.txt: relay down", + `retry.txt: ${UPLOAD_FAILED_TEXT}`, ]); expect( screen.getByRole("dialog", { name: "Upload to #General failed" }), diff --git a/src/features/messages/MessageComposer.tsx b/src/features/messages/MessageComposer.tsx index 873def1d46..4a79ee0844 100644 --- a/src/features/messages/MessageComposer.tsx +++ b/src/features/messages/MessageComposer.tsx @@ -3855,10 +3855,14 @@ function Composer({ ? "Wait for compaction to finish, or interrupt it." : untagged ? recipientHint - : "Send message" + : attachments.blocked + ? "Retry or remove the failed file to send." + : "Send message" } focusableWhenDisabled={ - untagged || (!editing.target && compacting) || undefined + untagged || + (!editing.target && (compacting || attachments.blocked)) || + undefined } disabled={ disabled || diff --git a/src/features/messages/attachment-draft.test.tsx b/src/features/messages/attachment-draft.test.tsx index f09128aa0c..0ed8ec2408 100644 --- a/src/features/messages/attachment-draft.test.tsx +++ b/src/features/messages/attachment-draft.test.tsx @@ -12,8 +12,13 @@ import { import { assert, afterEach, expect, it, vi } from "vitest"; import { ComposerAttachments } from "./ComposerAttachments"; import type { RelaySession } from "../relay/session"; -import type { UploadedAttachment } from "../relay/attachments"; import { + UPLOAD_FAILURES, + UploadError, + type UploadedAttachment, +} from "../relay/attachments"; +import { + UPLOAD_FAILED_TEXT, useAttachmentDraft, attachmentDraft, transferAttachmentDraft, @@ -144,19 +149,132 @@ it("preserves real upload errors while resuming unfinished siblings on return", first.result.current.store.add([file("failed.txt"), file("pending.txt")]), ); await waitFor(() => expect(h.calls).toHaveLength(1)); - await act(async () => h.calls[0]?.result.reject(new Error("relay down"))); + await act(async () => h.calls[0]?.result.reject(new UploadError("failed"))); await waitFor(() => expect(h.calls).toHaveLength(2)); first.unmount(); const returned = renderHook(() => useAttachmentDraft(h.session, "one", "one"), ); await waitFor(() => expect(h.calls).toHaveLength(3)); - expect(returned.result.current.items[0]?.error).toBe("relay down"); + expect(returned.result.current.items[0]?.error).toBe(UPLOAD_FAILURES.failed); expect(returned.result.current.items[0]?.status).toBe("error"); expect(returned.result.current.blocked).toBe(true); expect(h.calls[2]?.file.name).toBe("pending.txt"); }); +it.each([ + [ + "a fetch network failure", + () => new TypeError("Failed to fetch"), + "Upload didn’t finish. Check your connection, then retry.", + ], + [ + "the upload timeout", + // jsdom's DOMException is not an Error; browsers' is, with this name. + () => + Object.assign(new Error("signal timed out"), { name: "TimeoutError" }), + "Upload took too long. Retry, or remove this file.", + ], + [ + "a relay refusal the client already worded", + () => new UploadError("capacity"), + UPLOAD_FAILURES.capacity, + ], + ["an unrecognised error", () => new Error("relay down"), UPLOAD_FAILED_TEXT], + ["a native string", () => "Something native broke", UPLOAD_FAILED_TEXT], +])( + "words %s for the tile, never with the raw text", + async (_name, make, text) => { + const h = fixture(); + const draft = renderHook(() => useAttachmentDraft(h.session, "one", "one")); + act(() => draft.result.current.store.add([file()])); + await waitFor(() => expect(h.calls).toHaveLength(1)); + await act(async () => h.calls[0]?.result.reject(make())); + expect(draft.result.current.items[0]).toMatchObject({ + status: "error", + error: text, + }); + expect(draft.result.current.blocked).toBe(true); + // Retry still starts a fresh upload. + act(() => + draft.result.current.store.retry(draft.result.current.items[0]?.id ?? ""), + ); + await waitFor(() => expect(h.calls).toHaveLength(2)); + }, +); + +const bitmap = (width: number, height: number) => async () => ({ + width, + height, + close() {}, +}); +it.each([ + [ + "the pixel limit", + bitmap(5001, 5000), + "Choose an image with at most 25 million pixels.", + ], + [ + "a browser without a canvas context", + bitmap(10, 10), + "Image preparation is unavailable in this browser.", + ], + [ + "an image the browser cannot decode", + async () => { + throw Object.assign(new Error("The source image could not be decoded."), { + name: "InvalidStateError", + }); + }, + UPLOAD_FAILED_TEXT, + ], +])( + "words a preparation refusal from %s for the tile", + async (_name, decode, text) => { + vi.stubGlobal("createImageBitmap", decode); + const context = vi + .spyOn(HTMLCanvasElement.prototype, "getContext") + .mockReturnValue(null); + try { + // The fixture supplies no `prepare`, so the draft's own preparation runs. + const h = fixture(); + const draft = renderHook(() => + useAttachmentDraft(h.session, "one", "one"), + ); + const photo = new File( + [new Uint8Array([0xff, 0xd8, 0xff, 0xe0])], + "photo.jpg", + { type: "image/jpeg" }, + ) as unknown as globalThis.File; + act(() => draft.result.current.store.add([photo])); + await waitFor(() => + expect(draft.result.current.items[0]?.status).toBe("error"), + ); + expect(draft.result.current.items[0]?.error).toBe(text); + expect(draft.result.current.blocked).toBe(true); + expect(h.upload).not.toHaveBeenCalled(); + } finally { + context.mockRestore(); + vi.unstubAllGlobals(); + } + }, +); + +it("does not mark a cancelled upload as failed", async () => { + const h = fixture(); + const draft = renderHook(() => useAttachmentDraft(h.session, "one", "one")); + act(() => draft.result.current.store.add([file()])); + await waitFor(() => expect(h.calls).toHaveLength(1)); + act(() => + draft.result.current.store.remove(draft.result.current.items[0]?.id ?? ""), + ); + await act(async () => + h.calls[0]?.result.reject(new DOMException("aborted", "AbortError")), + ); + expect(draft.result.current.items).toEqual([]); + expect(draft.result.current.blocked).toBe(false); +}); + it("ignores a host progress report after an upload fails", async () => { const h = fixture(); const draft = renderHook(() => useAttachmentDraft(h.session, "one", "one")); @@ -595,13 +713,13 @@ it("rejects eager upload errors until explicit Retry, including after Send hando const draft = renderHook(() => useAttachmentDraft(h.session, "one", "one")); act(() => draft.result.current.store.add([file()])); await waitFor(() => expect(h.calls).toHaveLength(1)); - await act(async () => h.calls[0]?.result.reject(new Error("relay down"))); + await act(async () => h.calls[0]?.result.reject(new UploadError("failed"))); expect(draft.result.current.blocked).toBe(true); const target = attachmentDraft(h.session, "send", "one"); act(() => transferAttachmentDraft(draft.result.current.store, target)); await expect( target.prepareForSend(new AbortController().signal), - ).rejects.toThrow("relay down"); + ).rejects.toThrow(UPLOAD_FAILURES.failed); expect(h.calls).toHaveLength(1); expect(target.snapshot()[0]?.status).toBe("error"); target.retry(target.snapshot()[0]?.id ?? ""); diff --git a/src/features/messages/attachment-draft.ts b/src/features/messages/attachment-draft.ts index e49c4ea59c..6459a5c02b 100644 --- a/src/features/messages/attachment-draft.ts +++ b/src/features/messages/attachment-draft.ts @@ -7,7 +7,25 @@ import { type UploadedAttachment, type UploadProgress, } from "../relay/attachments"; -import { prepareAttachment } from "./prepare-attachment"; +import { failureKind } from "../relay/user-copy"; +import { PreparationError, prepareAttachment } from "./prepare-attachment"; + +/** The tile's sentence when nothing more specific is known. */ +export const UPLOAD_FAILED_TEXT = "Upload failed. Retry or remove this file."; + +/** Plain-language copy for a failed upload; never the browser's or host's own + * text. An `UploadError`'s or `PreparationError`'s message is already written + * for people. */ +function uploadFailureText(error: unknown) { + if (error instanceof UploadError || error instanceof PreparationError) + return error.message; + const kind = failureKind(error); + if (kind === "connection") + return "Upload didn’t finish. Check your connection, then retry."; + if (kind === "timeout") + return "Upload took too long. Retry, or remove this file."; + return UPLOAD_FAILED_TEXT; +} export type DraftAttachment = Readonly<{ id: string; @@ -247,10 +265,7 @@ export function attachmentDraft( task.update(item.id, { status: "error", transfer: undefined, - error: - error instanceof Error - ? error.message - : "Upload failed. Retry or remove this file.", + error: uploadFailureText(error), }); throw error; } finally { diff --git a/src/features/messages/prepare-attachment.ts b/src/features/messages/prepare-attachment.ts index fd562b98a7..241c58a6c0 100644 --- a/src/features/messages/prepare-attachment.ts +++ b/src/features/messages/prepare-attachment.ts @@ -17,6 +17,10 @@ import { webpNeedsPixelTransform, } from "./image-metadata"; +/** A preparation refusal already written for people, such as the pixel limit; + * callers may show its message as is. */ +export class PreparationError extends Error {} + /** Prepare pixels before the byte-exact upload contract; the relay still validates. */ export async function prepareAttachment( file: File, @@ -57,14 +61,18 @@ export async function prepareAttachment( !image.height || image.width * image.height > 25_000_000 ) - throw new Error("Choose an image with at most 25 million pixels."); + throw new PreparationError( + "Choose an image with at most 25 million pixels.", + ); const canvas = document.createElement("canvas"); canvas.width = image.width; canvas.height = image.height; try { const context = canvas.getContext("2d"); if (!context) - throw new Error("Image preparation is unavailable in this browser."); + throw new PreparationError( + "Image preparation is unavailable in this browser.", + ); context.drawImage(image, 0, 0); // Old Buzz writes lossless WebP; WebKit has no canvas WebP encoder. const target = type; @@ -92,7 +100,10 @@ export async function prepareAttachment( else if (blob.type === "image/jpeg") output = cleanEncodedJpeg(new Uint8Array(await blob.arrayBuffer())); else if (blob.type === target) output = blob; - else throw new Error("This browser cannot prepare this image format."); + else + throw new PreparationError( + "This browser cannot prepare this image format.", + ); } finally { canvas.width = 0; canvas.height = 0; diff --git a/src/features/solo/SoloDraft.test.tsx b/src/features/solo/SoloDraft.test.tsx index 241f698046..dfbc49e96b 100644 --- a/src/features/solo/SoloDraft.test.tsx +++ b/src/features/solo/SoloDraft.test.tsx @@ -2227,7 +2227,11 @@ it("a failed upload blocks a new chat before anything is created", async () => { expect( await screen.findByRole("button", { name: "Retry notes.txt" }), ).toBeTruthy(); - expect(screen.getByRole("button", { name: "Send message" })).toBeDisabled(); + // Focusable while blocked, so its tooltip can say why. + expect(screen.getByRole("button", { name: "Send message" })).toHaveAttribute( + "aria-disabled", + "true", + ); expect(test.channelWrites.create).not.toHaveBeenCalled(); expect(test.messages.send).not.toHaveBeenCalled(); }); diff --git a/tests/browser/composer-attachments.spec.mjs b/tests/browser/composer-attachments.spec.mjs index 0ecf64a1c6..6c9052a32e 100644 --- a/tests/browser/composer-attachments.spec.mjs +++ b/tests/browser/composer-attachments.spec.mjs @@ -17,6 +17,37 @@ async function openPreparationDocument(page, server) { await page.goto(url); } +// Spare width of a failed upload tile, given its alert. The tile fits the +// strip, or is only as wide as its one-row Retry and Remove need; anything +// else widening it, such as a long name or an 18rem floor, makes this negative. +function failedTileRoom(alert) { + const item = alert.closest("li"); + const strip = item.closest("ul"); + const width = (el) => el.getBoundingClientRect().width; + const insets = (el) => { + const style = getComputedStyle(el); + return [ + "paddingLeft", + "paddingRight", + "borderLeftWidth", + "borderRightWidth", + ] + .map((name) => parseFloat(style[name])) + .reduce((total, inset) => total + inset); + }; + // Measure the actions' outer edges, whichever button comes first. + const actions = [ + ...item.querySelectorAll('[aria-label^="Remove "], [aria-label^="Retry "]'), + ].map((action) => action.getBoundingClientRect()); + const left = Math.min(...actions.map((action) => action.left)); + const right = Math.max(...actions.map((action) => action.right)); + const room = Math.max( + width(strip) - insets(strip), + right - left + insets(item), + ); + return Math.round(room - width(item)); +} + // Browser-only boundary: actual File/DataTransfer delivery, editor capture and page wiring. test("picker, pane drop and clipboard files use the same attachment draft and existing Send", async ({ page, @@ -1039,14 +1070,30 @@ test("local previews stay interactive during eager upload and removal collapses await expect(failure).toBeInViewport(); if (width === 640) { // A narrow pane must not inherit an 18rem floor at enlarged text sizes. - expect( - await failure.evaluate((el) => { - const item = el.closest("li").getBoundingClientRect(); - const strip = el.closest("ul").getBoundingClientRect(); - return strip.width - item.width; - }), - ).toBeGreaterThanOrEqual(0); + expect(await failure.evaluate(failedTileRoom)).toBeGreaterThanOrEqual( + 0, + ); } + // Retry and Remove never wrap, and the error spans the tile from its + // inner edge, under the thumbnail rather than the file name. + const [removeTop, retryTop] = await Promise.all( + [failedRemove, retry].map((action) => + action.evaluate((el) => el.getBoundingClientRect().top), + ), + ); + expect(retryTop).toBe(removeTop); + expect( + await failure.evaluate((el) => { + const item = el.closest("li"); + const style = getComputedStyle(item); + return ( + el.getBoundingClientRect().left - + item.getBoundingClientRect().left - + parseFloat(style.borderLeftWidth) - + parseFloat(style.paddingLeft) + ); + }), + ).toBeCloseTo(0, 1); for (const content of [failure, retry, failedRemove]) { expect( await content.evaluate((el) => { @@ -1066,15 +1113,20 @@ test("local previews stay interactive during eager upload and removal collapses await form.screenshot({ path: test.info().outputPath(`upload-failure-${mode}-${width}.png`), }); - // Error actions must stay clear of the following image, at any UI size. + // Error actions, up to the rightmost one, must stay clear of the + // following image at any UI size. expect( - await failedRemove.evaluate((el) => { + await failure.evaluate((el) => { const item = el.closest("li"); const next = item.nextElementSibling; - return ( - next.getBoundingClientRect().left - - el.getBoundingClientRect().right + const right = Math.max( + ...[ + ...item.querySelectorAll( + '[aria-label^="Remove "], [aria-label^="Retry "]', + ), + ].map((action) => action.getBoundingClientRect().right), ); + return next.getBoundingClientRect().left - right; }), ).toBeGreaterThanOrEqual(0); } @@ -1134,13 +1186,7 @@ test("local previews stay interactive during eager upload and removal collapses await expect( form.getByRole("button", { name: `Retry ${failedName}` }), ).toBeVisible(); - expect( - await failure.evaluate( - (el) => - el.closest("ul").getBoundingClientRect().width - - el.closest("li").getBoundingClientRect().width, - ), - ).toBeGreaterThanOrEqual(0); + expect(await failure.evaluate(failedTileRoom)).toBeGreaterThanOrEqual(0); expect( await form .getByText(failedName, { exact: true })