fix(apollo-react): name the handle add-button after its label - #1105
fix(apollo-react): name the handle add-button after its label#1105abegu wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Improves accessibility of the canvas HandleButton (“+” connector button) in apollo-react by deriving its accessible name from the adjacent visual label, so screen readers can distinguish between different add-connector buttons (e.g., Tools vs Escalations).
Changes:
- Update
HandleButtonto usearia-label={label ? \Add ${label}` : 'Add node'}` in both render branches. - Add unit tests verifying the labeled and fallback accessible names.
File summaries
| File | Description |
|---|---|
| packages/apollo-react/src/canvas/components/ButtonHandle/HandleButton.tsx | Derives the add button’s aria-label from label (with fallback) to improve screen reader clarity. |
| packages/apollo-react/src/canvas/components/ButtonHandle/HandleButton.test.tsx | Adds coverage for the new accessible-name behavior and includes some formatting adjustments in existing tests. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| it("names the add button after its label, so 'Tools' vs 'Escalations' vs 'Memory' aren't all announced as the same generic 'Add node'", () => { | ||
| render(<HandleButton visible position={Position.Top} onAction={vi.fn()} label="Escalations" />); | ||
| expect(screen.getByRole('button', { name: 'Add Escalations' })).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('falls back to the generic name when no label is given', () => { | ||
| render(<HandleButton visible position={Position.Top} onAction={vi.fn()} />); | ||
| expect(screen.getByRole('button', { name: 'Add node' })).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Added a test for the keepButtonMounted branch's aria-label (including the aria-hidden state), and shortened the long test name. Pushed in 7a2ca25.
| // `disabled:opacity-0` overrides the button's default `disabled:opacity-50`. | ||
| <CanvasInlineButton | ||
| aria-label="Add node" | ||
| aria-label={label ? `Add ${label}` : 'Add node'} |
There was a problem hiding this comment.
Thanks for the a11y catch.
This might not work well for scenarios where the handle name doesn't follow semantically. For instance a handle might be be named after a condition or case "Case 1", "No matches", etc. So "Add No matches" doesn't quite make sense.
I might suggest something like "Add node from ${label} handle" or similar.
There was a problem hiding this comment.
Good catch — updated to "Add node from {label} handle" so it reads sensibly for condition/case-named handles too, not just resource-type names. Pushed in 7a2ca25.
| <CanvasInlineButton | ||
| aria-label="Add node" | ||
| aria-label={label ? `Add node from ${label} handle` : 'Add node'} | ||
| aria-hidden={visible ? undefined : true} |
Both CanvasInlineButton instances in HandleButton hardcoded
aria-label="Add node" even though the component already receives the
specific label being shown next to it visually ("Tools", "Memory",
"Context", "Escalations", etc). Every add-connector button on an agent
node was announced identically to assistive tech, with no way to tell
which kind of node it would add.
Reported as PC-1801 against a downstream consumer as a "missing
heading" bug (the visual label is a plain, non-semantic span) — but
converting it to a heading wouldn't have fixed anything, since the
label was never programmatically associated with the button. The real
defect is the button's own accessible name.
…label
- aria-label now reads "Add node from {label} handle" instead of
"Add {label}" — the earlier wording assumed label is always a
resource-type name, but handles can be named after a condition/case
("Case 1", "No matches"), where "Add No matches" doesn't parse.
Per review from @BenGSchulz.
- Added a test covering the keepButtonMounted branch's aria-label
(including the aria-hidden state), per Copilot's review — the prior
test only covered the conditionally-rendered branch.
- Shortened the overly long test name to match the file's style, per
the same Copilot note.
7a2ca25 to
6bf5d82
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
packages/apollo-react/src/canvas/components/ButtonHandle/HandleButton.tsx:147
- The PR description says the new accessible name should be
Add ${label}(with fallback toAdd node), but the implementation/tests useAdd node from ${label} handle. Please reconcile this by either updating the implementation/tests to match the described wording or updating the PR description/acceptance criteria so reviewers know the intended final accessible name.
<CanvasInlineButton
aria-label={label ? `Add node from ${label} handle` : 'Add node'}
aria-hidden={visible ? undefined : true}
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| ) : ( | ||
| visible && ( | ||
| <CanvasInlineButton | ||
| aria-label="Add node" | ||
| aria-label={label ? `Add node from ${label} handle` : 'Add node'} | ||
| onClick={handleClick} |
Summary
HandleButton(the "+" connector button rendered next to a node handle, e.g. Tools/Memory/Context/Escalations on an agent node) hardcodesaria-label="Add node"in both of itsCanvasInlineButtonrender paths.labelprop (e.g."Tools","Escalations") used to render the adjacent visual label — but that string was never wired into the button's own accessible name.Changes
aria-label={label ? \Add ${label}` : 'Add node'}in both branches (keepButtonMounted` and the conditionally-rendered path), falling back to the original generic string when no label is supplied.HandleButton.test.tsx: the button is named"Add Escalations"whenlabel="Escalations", and still falls back to"Add node"when no label is given.Test plan
vitest runonHandleButton.test.tsx— 18/18 passingbiome check— cleantsc --noEmit— no new errorsSource
Found while investigating a WCAG 1.3.1 report (PC-1801) filed against a downstream product as "text visually acting as a heading isn't marked up as one" — the visual label in question turned out to be this component's
labelprop, and the actual defect was upstream here: the adjacent button's accessible name never incorporated it. Marking the visual span as a heading wouldn't have fixed the underlying problem (the button's name), so this targets the real cause instead.