fix(web): address Copilot review on the shared JSON editor

- Cap the read-only editor's `maxLines` at 200. Uncapped, Ace renders every
  line of a payload and its viewport virtualization never engages — which
  matters because the Protocol and Network lists are not virtualized and keep
  each entry's payload mounted inside its `Collapse`. 200 is chosen so no real
  payload nests a scrollbar inside its host's while the pathological one stays
  bounded; the StructuredOutputPanel `Large` story now pins both halves (full
  payload in the document, only part of it in the DOM).

- Take a disabled editor out of the tab order and give it `aria-disabled`.
  Ace has no disabled state, so its hidden textarea kept `tabindex="0"` and a
  keyboard user could tab into a field every neighbouring control had dropped
  — landing somewhere that looks editable and swallows what they type. A
  merely read-only editor stays focusable, since a payload has to be
  keyboard-scrollable (WCAG SC 2.1.1).

- Reword a confusing comment in the EditReplayModal test.

The `Disabled` story additionally scopes the axe `color-contrast` rule off:
WCAG 1.4.3 exempts inactive components, and axe cannot see that the dimmed
gutter belongs to one because the disabled state lives on Ace's hidden
textarea. Scoped to that one story so the rule still covers everything else.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-27 02:44:21 -04:00
commit c9fb461bf0
6 changed files with 144 additions and 21 deletions

View file

@ -62,6 +62,12 @@ export interface ContentViewerProps {
wrap?: boolean; wrap?: boolean;
} }
/**
* Rows a read-only JSON payload renders before the editor scrolls internally
* instead of growing. See the note at the `JsonEditor` that uses it.
*/
const JSON_DISPLAY_MAX_LINES = 200;
function buildDataUri(mimeType: string, data: string): string { function buildDataUri(mimeType: string, data: string): string {
return `data:${mimeType};base64,${data}`; return `data:${mimeType};base64,${data}`;
} }
@ -186,18 +192,26 @@ function JsonContent({ text, copyable }: { text: string; copyable: boolean }) {
ariaLabel="JSON content" ariaLabel="JSON content"
value={formatJson(text)} value={formatJson(text)}
readOnly readOnly
// Sized to the payload, both ways. A two-line result must not open six // Sized to the payload, and capped.
// rows of blank editor — and an uncapped `maxLines` is what keeps a //
// large one from growing its *own* scroller inside the host's: every // `minLines={1}` so a two-line result does not open six rows of blank
// consumer here already provides the scroll container it wants (the // editor. The cap is the more interesting number, and it is a trade:
// structured-output section caps at `mah` and scrolls; a result panel //
// scrolls whole), and a second scrollbar nested in the first is both // - Below it the editor grows to fit, so the *host's* scroll container
// worse to use and a change to layout this swap has no business making. // is the only one — every consumer already provides the one it wants
// It also keeps every line in the DOM, as the preformatted block it // (the structured-output section caps at `mah` and scrolls; a result
// replaces did — Ace renders only what its own viewport covers, so a // panel scrolls whole), and nesting a second scrollbar inside it is
// capped editor would silently drop the rest from find-in-page. // worse to use.
// - Above it Ace scrolls internally and renders only the rows its own
// viewport covers. That is the point of having a cap at all: the
// Protocol and Network lists are not virtualized and keep every
// entry's payload mounted inside its `Collapse`, so an uncapped
// editor would put a whole 10k-line response in the DOM per entry.
//
// 200 is chosen so essentially no real payload nests a scrollbar while
// the pathological one stays bounded.
minLines={1} minLines={1}
maxLines={Infinity} maxLines={JSON_DISPLAY_MAX_LINES}
/> />
</CopyableWrapper> </CopyableWrapper>
); );

View file

@ -78,6 +78,11 @@ export const WithError: Story = {
}, },
}; };
/**
* Switched off by the form around it — what a tool panel does to every field
* while a call is in flight. Ace has no disabled state, so this is read-only
* plus a dimmed wrapper, `aria-disabled`, and removal from the tab order.
*/
export const Disabled: Story = { export const Disabled: Story = {
args: { args: {
ariaLabel: "Payload JSON", ariaLabel: "Payload JSON",
@ -86,6 +91,30 @@ export const Disabled: Story = {
disabled: true, disabled: true,
onChange: () => {}, onChange: () => {},
}, },
parameters: {
a11y: {
config: {
rules: [
// Re-stated because a story's `a11y.config.rules` replaces the
// preview's rather than merging with it — dropping this would make
// `region` fire here and nowhere else.
{ id: "region", enabled: false },
// WCAG 1.4.3 exempts "inactive user interface components" from the
// contrast minimum, and this whole editor is one: the dimming is the
// affordance that says so. axe cannot see that, because the disabled
// state lives on Ace's hidden textarea while the text it measures is
// in the separately-rendered gutter — so it reports the dimmed line
// numbers as a violation on every row.
//
// Scoped to this story on purpose. Every other story here, and the
// whole read-only path, is still contrast-checked; a preview-level
// disable would retire the rule for the enabled editor too, which is
// where it has something to catch.
{ id: "color-contrast", enabled: false },
],
},
},
},
}; };
/** /**

View file

@ -136,6 +136,46 @@ describe("JsonEditor", () => {
expect(document.querySelector(".json-editor-readonly")).not.toBeNull(); expect(document.querySelector(".json-editor-readonly")).not.toBeNull();
}); });
// Ace has no disabled state, so its hidden textarea keeps `tabindex="0"`
// unless something takes it out — leaving a keyboard user able to tab into
// a field the rest of the form has switched off, with nothing announced.
it("leaves the tab order and says so when disabled", () => {
const { rerender } = renderWithMantine(
<JsonEditor ariaLabel={LABEL} value='{"a":1}' onChange={vi.fn()} />,
);
const input = screen.getByLabelText(aceLabel(LABEL));
expect(input).toHaveAttribute("tabindex", "0");
expect(input).not.toHaveAttribute("aria-disabled");
rerender(
<JsonEditor
ariaLabel={LABEL}
value='{"a":1}'
onChange={vi.fn()}
disabled
/>,
);
expect(input).toHaveAttribute("tabindex", "-1");
expect(input).toHaveAttribute("aria-disabled", "true");
// …and both are restored when the form re-enables.
rerender(
<JsonEditor ariaLabel={LABEL} value='{"a":1}' onChange={vi.fn()} />,
);
expect(input).toHaveAttribute("tabindex", "0");
expect(input).not.toHaveAttribute("aria-disabled");
});
// Read-only is not disabled: a payload the user may need to scroll past the
// bottom of has to be reachable by keyboard (WCAG SC 2.1.1), which is why
// the preformatted block this replaced carried `tabIndex={0}`.
it("stays focusable when merely read-only", () => {
renderWithMantine(<JsonEditor ariaLabel={LABEL} value='{"a":1}' />);
const input = screen.getByLabelText(aceLabel(LABEL));
expect(input).toHaveAttribute("tabindex", "0");
expect(input).not.toHaveAttribute("aria-disabled");
});
it("is read-only and dimmed when disabled", () => { it("is read-only and dimmed when disabled", () => {
const { container } = renderWithMantine( const { container } = renderWithMantine(
<JsonEditor <JsonEditor

View file

@ -176,7 +176,28 @@ export function JsonEditor({
} else { } else {
element.removeAttribute("aria-invalid"); element.removeAttribute("aria-invalid");
} }
}, [wrapperId, hasError, description]);
// `disabled` and `readOnly` are different states here, and only one of them
// leaves the tab order.
//
// A **read-only** editor is a rendering of a payload the user may need to
// read past the bottom of, so it stays focusable — the same reason the
// preformatted block it replaced carries `tabIndex={0}` (WCAG SC 2.1.1).
//
// A **disabled** one is a control the rest of the form has switched off. Ace
// has no disabled state, so without this its hidden textarea keeps
// `tabindex="0"` and a keyboard user tabs into a field every other control
// beside it has dropped out of the order — landing somewhere that looks
// editable and silently swallows what they type. Assistive tech is told
// nothing either, since dimming is a purely visual signal.
if (disabled) {
element.setAttribute("aria-disabled", "true");
element.tabIndex = -1;
} else {
element.removeAttribute("aria-disabled");
element.tabIndex = 0;
}
}, [wrapperId, hasError, description, disabled]);
return ( return (
<Input.Wrapper <Input.Wrapper

View file

@ -100,8 +100,9 @@ describe("EditReplayModal", () => {
expect(onSend).not.toHaveBeenCalled(); expect(onSend).not.toHaveBeenCalled();
}); });
// The draft belongs to the sitting the user opened it in. Reopening must show // The draft belongs to the one opening of the modal it was typed in.
// the entry's params again, not whatever they typed and abandoned last time. // Reopening must show the entry's params again, not whatever was typed and
// abandoned last time.
it("reseeds from the entry when reopened", async () => { it("reseeds from the entry when reopened", async () => {
const { rerender } = renderWithMantine( const { rerender } = renderWithMantine(
<EditReplayModal {...baseProps} params={{ name: "echo" }} />, <EditReplayModal {...baseProps} params={{ name: "echo" }} />,

View file

@ -16,8 +16,10 @@ const flat = {
city: "San Francisco", city: "San Francisco",
}; };
// Long enough that the payload exceeds the section's max height and scrolls // Long enough on both counts that matter: the payload exceeds the section's max
// within it rather than pushing the rest of the result panel down. // height (so it scrolls within it rather than pushing the rest of the result
// panel down) *and* it exceeds the read-only editor's line cap, so Ace renders
// only the rows its own viewport covers.
const large = { const large = {
rows: Array.from({ length: 60 }, (_, index) => ({ rows: Array.from({ length: 60 }, (_, index) => ({
id: index + 1, id: index + 1,
@ -45,8 +47,8 @@ export const Nested: Story = {
canvas.getByRole("heading", { name: "Structured Output" }), canvas.getByRole("heading", { name: "Structured Output" }),
).toBeInTheDocument(); ).toBeInTheDocument();
// The section starts expanded, so the payload is visible without a click. // The section starts expanded, so the payload is visible without a click.
// Asserted on the container's text: once the Prism grammar loads, the JSON // Asserted on the container's text: the JSON renders in Ace, which puts
// is split across per-token elements, so no single node holds the value. // each token in its own element, so no single node holds the value.
await waitFor(() => await waitFor(() =>
expect(canvasElement.textContent).toContain('"Item A"'), expect(canvasElement.textContent).toContain('"Item A"'),
); );
@ -62,14 +64,30 @@ export const Flat: Story = {
// A payload taller than the section's cap scrolls inside it rather than // A payload taller than the section's cap scrolls inside it rather than
// pushing the rest of the result panel down. Asserted on the real geometry, // pushing the rest of the result panel down. Asserted on the real geometry,
// so dropping `mah` (or moving it to the wrong element) fails here. // so dropping `mah` (or moving it to the wrong element) fails here.
//
// It also pins the read-only editor's line cap (#2151). The Protocol and
// Network lists are not virtualized and keep every entry's payload mounted
// inside its `Collapse`, so an uncapped editor would put a whole large response
// in the DOM per entry. The two assertions below are the difference between
// "capped" and "truncated": the full payload is in the *document*, and only
// part of it is in the *DOM*.
export const Large: Story = { export const Large: Story = {
args: { args: {
structuredContent: large, structuredContent: large,
}, },
play: async ({ canvasElement }) => { play: async ({ canvasElement }) => {
await waitFor(() => const editorNode = await waitFor(() => {
expect(canvasElement.textContent).toContain('"Row 60"'), const node = canvasElement.querySelector(".ace_editor");
); if (!node) throw new Error("JSON editor not mounted");
return node as HTMLElement & { env: { editor: { getValue(): string } } };
});
// Nothing is lost: the last row is in the document the editor holds, so it
// is reachable by scrolling, folding, and the copy button.
await expect(editorNode.env.editor.getValue()).toContain('"Row 60"');
// …and it is not rendered, which is the virtualization the cap buys.
await expect(canvasElement.textContent).not.toContain('"Row 60"');
const viewport = canvasElement.querySelector( const viewport = canvasElement.querySelector(
".mantine-ScrollArea-viewport", ".mantine-ScrollArea-viewport",
); );