fix(apollo-react): name the handle add-button after its label - #1105
Open
abegu wants to merge 3 commits into
Open
fix(apollo-react): name the handle add-button after its label#1105abegu wants to merge 3 commits into
abegu wants to merge 3 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.
BenGSchulz
reviewed
Aug 31, 2026
added 2 commits
September 1, 2026 12:34
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.
abegu
force-pushed
the
fix/handle-button-labelled-aria
branch
from
September 1, 2026 09:34
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
BenGSchulz
approved these changes
Sep 1, 2026
Copilot review caught that the inward-handle render path in ButtonHandle
(connectionPosition !== position) never passed label to HandleButton, so
that path still announced the generic "Add node" instead of the specific
"Add node from {label} handle".
Passing label={label} there directly would have caused a duplicate visual
label, since that path already renders its own via InwardHandleContent.
Added a new ariaLabel prop to HandleButton, decoupled from the visual
label, so the accessible name can be set independently of whether the
caller also wants HandleButton to render its own InlineLabel.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
The accessibility fix is consistently applied across both render paths and call sites, and the new behavior is covered by targeted unit tests.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 node from ${label} handle` : 'Add node'}in both branches (keepButtonMountedand the conditionally-rendered path), falling back to the original generic string when no label is supplied. (Wording chosen over a plainAdd ${label}` so it still reads sensibly for handles named after a condition/case, e.g. "Add node from No matches handle" rather than "Add No matches".)ariaLabelprop toHandleButtonso callers that already render their own adjacent visual label (inward handles, viaInwardHandleContent) can set the accessible name withoutHandleButtonALSO rendering a duplicate visualInlineLabel— wiredariaLabel={label}into that inward-handle call site inButtonHandle.tsx, which previously passed no label at all.HandleButton.test.tsxandButtonHandle.test.tsxcovering: the button is named"Add node from Escalations handle"whenlabel="Escalations", falls back to"Add node"when no label is given, the newariaLabelprop takes precedence and doesn't render a duplicate visual label, and the inward-handle path is named correctly too.Test plan
vitest runonHandleButton.test.tsxandButtonHandle.test.tsx— 47/47 passingbiome check— clean (no new issues)tsc --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.