fix(web): re-apply the editor's accessible name, and correct the README's scope
Copilot's third pass generated no new inline comments; all three of its suppressed ones were right. - `ariaLabel` is now re-applied from the wiring effect, not only at mount. `setOptions` updates Ace's option when the prop changes, but Ace recomputes the textarea's `aria-label` from it inside `TextInput.setAriaLabel()`, which it calls when the cursor moves — so a name that changed while the editor stayed mounted (a tool refreshed in place under a new field title, which `SchemaJsonField` passes straight through) left the visible label and the accessible name disagreeing until the user clicked into the box. `onLoad` keeps doing it too, since an effect runs after paint and this is the control's only name. - The README claimed this editor is on "every surface where JSON is typed or displayed", which is false in two places it now names instead: `ExperimentalFeaturesPanel`'s JSON-RPC *response* is still a read-only `Textarea` (only its request box moved), and `OAuthTokenField` renders a decoded JWT in a `Code` block beside the raw token. Both bypass `ContentViewer` and so are not reached by its JSON branch. Documenting them as exceptions is what keeps the section from sending a maintainer to the wrong abstraction. 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:
parent
b4083ee5fb
commit
12c00d8c63
3 changed files with 42 additions and 5 deletions
|
|
@ -285,7 +285,7 @@ Each customized Mantine component has a `Theme<Name>.ts` file (`Button.ts`, `Tex
|
|||
|
||||
## JSON editing and display (`JsonEditor`)
|
||||
|
||||
Every surface in this client where JSON is typed or displayed renders one element: **`elements/JsonEditor`**, an **Ace** editor (`react-ace` + `ace-builds`, declared in this client because they render React). Ace brings code folding, line numbers, brace auto-closing, and per-line error annotation from its JSON worker — the last three are why hand-writing a nested payload in a bare textarea was the actual pain ([#2151](https://github.com/modelcontextprotocol/inspector/issues/2151)).
|
||||
Every surface in this client where JSON is **typed**, plus `ContentViewer`'s read-only JSON branch, renders one element: **`elements/JsonEditor`**, an **Ace** editor (`react-ace` + `ace-builds`, declared in this client because they render React). Ace brings code folding, line numbers, brace auto-closing, and per-line error annotation from its JSON worker — the last three are why hand-writing a nested payload in a bare textarea was the actual pain ([#2151](https://github.com/modelcontextprotocol/inspector/issues/2151)).
|
||||
|
||||
`JsonEditor` is deliberately **text in, text out**: it never parses. The editing contracts above it disagree about what an unparseable draft means, and neither can be expressed by a component that decides for them:
|
||||
|
||||
|
|
@ -298,7 +298,9 @@ Every surface in this client where JSON is typed or displayed renders one elemen
|
|||
| `groups/ImportServerJsonPanel`, `groups/ExperimentalFeaturesPanel` | The panel owns the text and validates it itself. |
|
||||
| `elements/ContentViewer`'s JSON branch | Read-only — see below. |
|
||||
|
||||
**Read-only mode is what `ContentViewer` renders JSON as**, and through it every JSON payload in the app: Protocol and Network entries, tool results, structured output, resource previews, server cards. Highlighting is *not* what that buys — JSON already highlighted, via the lazily-imported Prism grammar `CodeHighlight` loads. What Ace adds is **folding**, line numbers and a gutter on a large payload. The Prism `json` grammar was dropped in the same change rather than kept beside it: two highlighters for one language drift, and nothing else asks for `json`. Two cases stay on the plain renderer — a `wrap={false}` caller (the server card's fixed-height, single-line box) and untyped text that only *looks* like JSON but does not parse, which in an editor would frame a server's prose as a malformed document.
|
||||
**Read-only mode is what `ContentViewer` renders JSON as**, and through it every payload that reaches the app that way: Protocol and Network entries, tool results, structured output, resource previews, server cards. Highlighting is *not* what that buys — JSON already highlighted, via the lazily-imported Prism grammar `CodeHighlight` loads. What Ace adds is **folding**, line numbers and a gutter on a large payload. The Prism `json` grammar was dropped in the same change rather than kept beside it: two highlighters for one language drift, and nothing else asks for `json`. Two cases stay on the plain renderer — a `wrap={false}` caller (the server card's fixed-height, single-line box) and untyped text that only *looks* like JSON but does not parse, which in an editor would frame a server's prose as a malformed document.
|
||||
|
||||
Two read-only JSON displays deliberately do **not** go through `ContentViewer`, and so are not on this editor — worth knowing before assuming it is the route for all of them. `ExperimentalFeaturesPanel` renders its JSON-RPC **response** in a read-only `Textarea` (its *request* box is on the editor), and `ConnectionInfoContent/OAuthTokenField` renders a decoded JWT in a `Code` block beside the raw token, where the decoded/raw toggle and the copy affordance belong to the field rather than to a viewer. Both are candidates for the same treatment; neither is in [#2151](https://github.com/modelcontextprotocol/inspector/issues/2151)'s scope.
|
||||
|
||||
Five integration details are load-bearing:
|
||||
|
||||
|
|
|
|||
|
|
@ -36,6 +36,25 @@ describe("JsonEditor", () => {
|
|||
expect(screen.getByLabelText(aceLabel(LABEL))).toBeInTheDocument();
|
||||
});
|
||||
|
||||
// Ace recomputes the textarea's `aria-label` from its option only when the
|
||||
// cursor moves, so a name that changes while the editor stays mounted — a
|
||||
// tool refreshed in place under a new field title — would leave the visible
|
||||
// label and the accessible name disagreeing until the user clicked into it.
|
||||
it("re-applies the accessible name when it changes in place", () => {
|
||||
const { rerender } = renderWithMantine(
|
||||
<JsonEditor ariaLabel="Series JSON" value="" onChange={vi.fn()} />,
|
||||
);
|
||||
expect(screen.getByLabelText(aceLabel("Series JSON"))).toBeInTheDocument();
|
||||
|
||||
rerender(
|
||||
<JsonEditor ariaLabel="Data points JSON" value="" onChange={vi.fn()} />,
|
||||
);
|
||||
expect(
|
||||
screen.getByLabelText(aceLabel("Data points JSON")),
|
||||
).toBeInTheDocument();
|
||||
expect(screen.queryByLabelText(aceLabel("Series JSON"))).toBeNull();
|
||||
});
|
||||
|
||||
it("shows the text it is given, verbatim", () => {
|
||||
// Braces, not a bare JSX string: a JSX string attribute does not process
|
||||
// escapes, so `'…\n…'` written inline would be a literal backslash-n.
|
||||
|
|
|
|||
|
|
@ -149,12 +149,28 @@ export function JsonEditor({
|
|||
// wrapper's own id is what makes the `<label for>` focus the editor; the
|
||||
// error wiring follows Mantine's `${id}-error` naming.
|
||||
useEffect(() => {
|
||||
const element = editorRef.current?.textInput.getElement();
|
||||
const editor = editorRef.current;
|
||||
const element = editor?.textInput.getElement();
|
||||
/* v8 ignore next -- the ref is set in `onLoad`, which react-ace calls
|
||||
synchronously on mount, so this effect always sees an element. */
|
||||
if (!element) return;
|
||||
if (!editor || !element) return;
|
||||
element.id = wrapperId;
|
||||
|
||||
// Re-apply the accessible name, not just set it once at mount.
|
||||
//
|
||||
// `setOptions` does update Ace's *option* when `ariaLabel` changes, but Ace
|
||||
// only recomputes the textarea's `aria-label` from it inside
|
||||
// `TextInput.setAriaLabel()`, which it calls when the cursor moves. So a
|
||||
// name that changes while the editor stays mounted — a tool refreshed in
|
||||
// place under a new field title, which `SchemaJsonField` passes straight
|
||||
// through — leaves the visible label and the accessible name disagreeing
|
||||
// until the user happens to click into the box.
|
||||
//
|
||||
// `onLoad` still does this too: an effect runs after paint, and this is the
|
||||
// control's only name, so mount should not present a frame without it.
|
||||
editor.setOption("textInputAriaLabel", ariaLabel);
|
||||
editor.textInput.setAriaLabel();
|
||||
|
||||
// Both the description and the error are rendered by the wrapper and both
|
||||
// describe this control, so both ids belong here — a supplied description
|
||||
// is otherwise never announced, and it must not be dropped just because the
|
||||
|
|
@ -197,7 +213,7 @@ export function JsonEditor({
|
|||
element.removeAttribute("aria-disabled");
|
||||
element.tabIndex = 0;
|
||||
}
|
||||
}, [wrapperId, hasError, description, disabled]);
|
||||
}, [wrapperId, hasError, description, disabled, ariaLabel]);
|
||||
|
||||
return (
|
||||
<Input.Wrapper
|
||||
|
|
|
|||
Loading…
Reference in a new issue