From c9fb461bf07c313a2d46abfe6d81048ef5531305 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 27 Aug 2026 02:44:21 -0400 Subject: [PATCH] fix(web): address Copilot review on the shared JSON editor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- .../elements/ContentViewer/ContentViewer.tsx | 36 ++++++++++++----- .../JsonEditor/JsonEditor.stories.tsx | 29 ++++++++++++++ .../elements/JsonEditor/JsonEditor.test.tsx | 40 +++++++++++++++++++ .../elements/JsonEditor/JsonEditor.tsx | 23 ++++++++++- .../EditReplayModal/EditReplayModal.test.tsx | 5 ++- .../StructuredOutputPanel.stories.tsx | 32 +++++++++++---- 6 files changed, 144 insertions(+), 21 deletions(-) diff --git a/clients/web/src/components/elements/ContentViewer/ContentViewer.tsx b/clients/web/src/components/elements/ContentViewer/ContentViewer.tsx index 998ae42a..7d7fbfc3 100644 --- a/clients/web/src/components/elements/ContentViewer/ContentViewer.tsx +++ b/clients/web/src/components/elements/ContentViewer/ContentViewer.tsx @@ -62,6 +62,12 @@ export interface ContentViewerProps { 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 { return `data:${mimeType};base64,${data}`; } @@ -186,18 +192,26 @@ function JsonContent({ text, copyable }: { text: string; copyable: boolean }) { ariaLabel="JSON content" value={formatJson(text)} readOnly - // Sized to the payload, both ways. A two-line result must not open six - // rows of blank editor — and an uncapped `maxLines` is what keeps a - // large one from growing its *own* scroller inside the host's: every - // consumer here already provides the scroll container it wants (the - // structured-output section caps at `mah` and scrolls; a result panel - // scrolls whole), and a second scrollbar nested in the first is both - // worse to use and a change to layout this swap has no business making. - // It also keeps every line in the DOM, as the preformatted block it - // replaces did — Ace renders only what its own viewport covers, so a - // capped editor would silently drop the rest from find-in-page. + // Sized to the payload, and capped. + // + // `minLines={1}` so a two-line result does not open six rows of blank + // editor. The cap is the more interesting number, and it is a trade: + // + // - Below it the editor grows to fit, so the *host's* scroll container + // is the only one — every consumer already provides the one it wants + // (the structured-output section caps at `mah` and scrolls; a result + // panel scrolls whole), and nesting a second scrollbar inside it is + // 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} - maxLines={Infinity} + maxLines={JSON_DISPLAY_MAX_LINES} /> ); diff --git a/clients/web/src/components/elements/JsonEditor/JsonEditor.stories.tsx b/clients/web/src/components/elements/JsonEditor/JsonEditor.stories.tsx index 7b0128e2..356150b6 100644 --- a/clients/web/src/components/elements/JsonEditor/JsonEditor.stories.tsx +++ b/clients/web/src/components/elements/JsonEditor/JsonEditor.stories.tsx @@ -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 = { args: { ariaLabel: "Payload JSON", @@ -86,6 +91,30 @@ export const Disabled: Story = { disabled: true, 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 }, + ], + }, + }, + }, }; /** diff --git a/clients/web/src/components/elements/JsonEditor/JsonEditor.test.tsx b/clients/web/src/components/elements/JsonEditor/JsonEditor.test.tsx index a8824f02..ae84b0c3 100644 --- a/clients/web/src/components/elements/JsonEditor/JsonEditor.test.tsx +++ b/clients/web/src/components/elements/JsonEditor/JsonEditor.test.tsx @@ -136,6 +136,46 @@ describe("JsonEditor", () => { 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( + , + ); + const input = screen.getByLabelText(aceLabel(LABEL)); + expect(input).toHaveAttribute("tabindex", "0"); + expect(input).not.toHaveAttribute("aria-disabled"); + + rerender( + , + ); + expect(input).toHaveAttribute("tabindex", "-1"); + expect(input).toHaveAttribute("aria-disabled", "true"); + + // …and both are restored when the form re-enables. + rerender( + , + ); + 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(); + 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", () => { const { container } = renderWithMantine(