From 5fa6b28a0a6e089cab64afbcd31caac5f458bdb6 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 01:17:21 +0000 Subject: [PATCH 1/2] fix(collaboration): name CommentThread's `+` reaction picker, not just its tooltip (#3478) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fourth glyph-only control in the component, and the one objectui#3441 walked past. Its content is the literal `'+'`, so the `title` objectstack#5506 gave it (`collaboration.addThumbsUp`) could never become its accessible name: a `button`'s name is computed from CONTENT (accname §2F) before the `title` tooltip is consulted at all (§2I). A screen reader announced "plus button". The copy existed, was localized into all ten packs, and reached only the people who could already see the button. Different failure from #3441's three buttons, which is why it survived that fix — those carried no authored copy anywhere, so every "is the key wired up?" check found the gap. Here the key WAS wired up and the English WAS in COLLAB_DEFAULT_TRANSLATIONS; only its destination was wrong. #3441's own pin recorded the defect without naming it, asserting getByTitle('Add thumbs up') and queryAllByRole('button', { name: 'Add thumbs up' }) toHaveLength(0) in one green case — two assertions that together say "the title is set and it is not the name". - `aria-label` (accname §2C, outranks content) added alongside the existing `title`, on the same key. Zero new keys — the copy was always there. - `title` KEPT, not replaced: `+` says nothing to a sighted mouse user either, so the hover hint does real work. Same key for both, so they cannot drift. - Name stays `addThumbsUp` per the issue ruling: it describes what the button does today (`onReaction(id, '👍')`), not what `styles.reactionPicker` hints it might become. Picker-ification is a separate feature. - Adjacent reaction chips untouched — `${emoji} ${count}` already describes them, so name-from-content is correct there. Directions, predicted before the run and confirmed: with the tests applied and the source untouched, `{ name: 'Add thumbs up' }` found 0 buttons and the RTL accname dump listed the picker as `Name "+"` in both en and zh — direct proof of the content-over-title precedence. Both flip green with the `aria-label`. The getByTitle assertions are green on both sides on purpose (the tooltip must survive), as is the `React with thumbs up` count (#3441's separation holds). #3441's pin was rewritten in place to pin the new both-named state rather than the old separation, since that is the statement this change falsifies. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GTRjn8xBqp75dk7kFupVRt --- ...on-reaction-picker-accessible-name-3478.md | 52 +++++++++++++ packages/collaboration/src/CommentThread.tsx | 30 +++++++- .../__tests__/comment-thread-i18n.test.tsx | 74 +++++++++++++++---- ...mment-thread-no-provider-fallback.test.tsx | 21 ++++++ 4 files changed, 158 insertions(+), 19 deletions(-) create mode 100644 .changeset/collaboration-reaction-picker-accessible-name-3478.md diff --git a/.changeset/collaboration-reaction-picker-accessible-name-3478.md b/.changeset/collaboration-reaction-picker-accessible-name-3478.md new file mode 100644 index 000000000..3f1c7a74b --- /dev/null +++ b/.changeset/collaboration-reaction-picker-accessible-name-3478.md @@ -0,0 +1,52 @@ +--- +'@object-ui/collaboration': patch +--- + +Give `CommentThread`'s `+` reaction picker a real accessible name (objectui#3478) + +The fourth glyph-only control in the same component, and the one objectui#3441 +walked past. Its content is the literal `'+'`, so the `title` objectstack#5506 +gave it (`collaboration.addThumbsUp`) could never become its accessible name: a +`button`'s name is computed from CONTENT (accname §2F) before the `title` +tooltip is consulted at all (§2I). A screen reader announced "plus button". The +copy existed, was localized into all ten packs, and reached only the people who +could already see the button. + +That is a different failure from #3441's three buttons, which is why it survived +that fix — those had no authored copy anywhere, so every "is the key wired up?" +check found the gap. Here the key WAS wired up and the English WAS in +`COLLAB_DEFAULT_TRANSLATIONS`; only its DESTINATION was wrong. #3441's own pin +test recorded the defect without naming it, asserting `getByTitle('Add thumbs +up')` and `queryAllByRole('button', { name: 'Add thumbs up' })).toHaveLength(0)` +in the same green case — two assertions that together say "the title is set and +it is not the name". The docblock read them as pinning the picker apart from the +quick 👍; they were also, unread, the bug report. + +The fix adds `aria-label` (accname §2C, which outranks content) alongside the +existing `title`, on the same key. Zero new keys — the copy was always there. + +The `title` is KEPT rather than replaced, and this is the one control in the +component where carrying both is right instead of redundant: `+` says nothing to +a sighted mouse user either, so the hover hint is doing real work of its own. +(The 👍/❤️ buttons #3441 fixed had no `title` to keep.) Both attributes read the +same key, so the tooltip and the name cannot drift apart. + +The name stays `addThumbsUp` — it describes what the button does today +(`onReaction(id, '👍')`, unconditionally), not what `styles.reactionPicker` +hints it might become. Turning it into an actual emoji picker is a feature +change, and the copy follows the behaviour when that lands. It also stays +distinct from #3441's `reactThumbsUp`: on a comment that already has reactions +both controls are on screen at once, and now that both carry a real accessible +name, sharing one key would be worse than when #3441 declined to — two visibly +different buttons announcing themselves identically. + +The adjacent reaction chips are deliberately untouched. Their content is +`${emoji} ${count}` — already a descriptive name — so name-from-content is the +right answer there, and their `title` adds the count in words. + +Tests assert the computed accessible name via `getByRole('button', { name })` +rather than the presence of an attribute, in English, Chinese and with no +`I18nProvider` mounted; the mirror assertion (`{ name: '+' }` finds nothing) is +what fails if the `aria-label` is ever dropped. #3441's pin was rewritten in +place to pin the new both-named state instead of the old separation, since the +statement it used to make is exactly the one this change falsifies. diff --git a/packages/collaboration/src/CommentThread.tsx b/packages/collaboration/src/CommentThread.tsx index 9be2da61b..e40575ad7 100644 --- a/packages/collaboration/src/CommentThread.tsx +++ b/packages/collaboration/src/CommentThread.tsx @@ -591,9 +591,28 @@ export function CommentThread({ ), }, `${emoji} ${userIds.length}`), ), + // The reaction-bar picker (objectui#3478). Its content is the literal + // `'+'`, so the `title` objectstack#5506 gave it could never become + // its accessible name — name-from-content (accname §2F) is resolved + // before the `title` tooltip (§2I) gets a turn, and a screen reader + // announced "plus button". The copy was localized and correct and + // reached nobody who could not see the glyph. `aria-label` (§2C) + // outranks content, so it is what actually names the control. + // + // The `title` STAYS: `+` tells a sighted mouse user nothing either, + // so the hover hint does real work of its own. Both attributes read + // the SAME key, so name and tooltip cannot drift apart. (The 👍/❤️ + // buttons below had no `title` to keep — hence `aria-label` alone.) + // + // Still `addThumbsUp`: the button dispatches `onReaction(id, '👍')` + // unconditionally today, and the name describes that, not what + // `styles.reactionPicker` hints it might one day become. The chips + // above stay content-named — `${emoji} ${count}` already describes + // them. onReaction && React.createElement('button', { style: styles.reactionPicker, onClick: () => onReaction(comment.id, '👍'), + 'aria-label': t('collaboration.addThumbsUp'), title: t('collaboration.addThumbsUp'), }, '+'), ), @@ -608,15 +627,18 @@ export function CommentThread({ // ever reaches `title` (accname §2F outranks §2I) — so these two were // announced as the bare glyph: "thumbs up" / "red heart" at best, // nothing at all where the SR has no name for the codepoint. Hence - // `aria-label`, which overrides content, rather than the `title` the - // `+` picker above uses. + // `aria-label`, which overrides content. (The `+` picker above had + // only a `title` and was announced as "plus" for exactly the same + // reason, until objectui#3478 gave it an `aria-label` too.) // // Their own key pair, NOT a reuse of `collaboration.addThumbsUp`: the // `+` above happens to fire the same `onReaction(id, '👍')` today, but // it is the reaction PICKER's entry point (`styles.reactionPicker`) // whose copy follows the picker if it ever picks. Sharing one key - // would also give a comment that already has reactions two visible - // controls answering to one name. + // would now be worse than when #3441 wrote this down — since #3478 + // both controls carry a real accessible name, so on a comment that + // already has reactions a shared key means two visibly different + // controls announcing themselves identically. onReaction && React.createElement('button', { style: styles.actionBtn, onClick: () => onReaction(comment.id, '👍'), diff --git a/packages/collaboration/src/__tests__/comment-thread-i18n.test.tsx b/packages/collaboration/src/__tests__/comment-thread-i18n.test.tsx index 1dc596e90..28ed69173 100644 --- a/packages/collaboration/src/__tests__/comment-thread-i18n.test.tsx +++ b/packages/collaboration/src/__tests__/comment-thread-i18n.test.tsx @@ -293,25 +293,32 @@ describe('CommentThread per-comment actions (objectstack#5506)', () => { }); /** - * objectui#3441 — the three emoji-only controls objectstack#5506 left unnamed. + * objectui#3441 / #3478 — the four glyph-only controls objectstack#5506 left + * unnamed: the quick 👍, the quick ❤️, the reply-banner ✕ (all #3441) and the + * reaction-bar `+` picker (#3478). * * These assert the computed ACCESSIBLE NAME (`getByRole('button', { name })`, * which runs dom-accessibility-api's accname implementation), not the presence * of an attribute. That distinction is the whole point of the fix: for a * `button`, name-from-content (accname §2F) is consulted BEFORE the `title` - * tooltip (§2I), so hanging a `title` on `'👍'` the way the `+` picker does - * would have left the computed name as the glyph. `aria-label` is the only one - * of the three that outranks content. + * tooltip (§2I), so a `title` hung on `'👍'` — or on `'+'`, which is what the + * picker had until #3478 — leaves the computed name as the glyph. `aria-label` + * is the only one of the three that outranks content. * * ── Direction ───────────────────────────────────────────────────────────── * RED before / GREEN after in EVERY language, `en` included — unlike the * copy-pin cases above, these names did not exist in any locale on * `origin/main`, so there is no "English was already right" half here. The - * `queryAllByRole(… { name: '👍' })` assertions are the mirror image: they - * pass ONLY after the fix, because the glyph was the name until `aria-label` - * displaced it. + * `queryAllByRole(… { name: '👍' })` / `{ name: '+' }` assertions are the + * mirror image: they pass ONLY after the fix, because the glyph was the name + * until `aria-label` displaced it. + * + * The one deliberate exception is the picker's `getByTitle` assertion, green on + * both sides: `+` keeps its tooltip as well as gaining a name. It is the only + * control here that legitimately carries both — a sighted mouse user gets the + * hover hint the glyph cannot give them either. */ -describe('CommentThread emoji-only control names (objectui#3441)', () => { +describe('CommentThread glyph-only control names (objectui#3441, #3478)', () => { it('names the two quick-reaction buttons in English', () => { renderThread('en'); @@ -371,19 +378,56 @@ describe('CommentThread emoji-only control names (objectui#3441)', () => { }); /** - * The `+` picker keeps `collaboration.addThumbsUp` (objectstack#5506) and the - * quick 👍 gets its own `reactThumbsUp`, even though both dispatch the same - * `onReaction(id, '👍')` today. This case is what pins the two apart: on a - * comment that already has reactions both controls are on screen at once, so - * sharing one key would put two visibly different controls under one name. + * objectui#3478 — the `+` reaction picker, the fourth emoji-only control, and + * the one objectui#3441 left behind. + * + * Its content is the literal `'+'`, so accname §2F named it "plus" and the + * `title` at §2I never got a turn — `collaboration.addThumbsUp` reached the + * tooltip and nothing else. #3441's own pin is what recorded this: it + * asserted `getByTitle('Add thumbs up')` AND + * `queryAllByRole('button', { name: 'Add thumbs up' })).toHaveLength(0)` in + * the same case, two simultaneously-green assertions that say in so many + * words "the title is set and it is not the name". The docblock read them as + * pinning the picker APART from the quick 👍; they were also, unread, the + * bug report. This case now pins the fixed state: `aria-label` names it, + * `title` still shows it, and the two keys stay distinct. + * + * ── Direction ───────────────────────────────────────────────────────────── + * RED before / GREEN after, twice over: the `name: 'Add thumbs up'` lookup + * finds 0 buttons on `origin/main` (it was `toHaveLength(0)` there), and the + * `name: '+'` lookup finds 1 — the glyph IS the name until `aria-label` + * displaces it. The `getByTitle` and `React with thumbs up` assertions are + * green on both sides on purpose: the tooltip must survive the fix (the + * picker is the one control here that legitimately has both), and the two + * controls must stay under two names. */ - it('keeps the reaction-bar picker distinct from the quick thumbs-up', () => { + it('names the reaction-bar picker and keeps its tooltip', () => { renderThread('en'); + // Tooltip survives — green on both sides. expect(screen.getByTitle('Add thumbs up')).toBeTruthy(); - expect(screen.queryAllByRole('button', { name: 'Add thumbs up' })).toHaveLength(0); + // …and is now ALSO the accessible name. Exactly one picker: it renders + // only on a comment that already has reactions, i.e. `c2`. + expect(screen.getAllByRole('button', { name: 'Add thumbs up' }).length).toBe(1); + // The bug, mirrored: `'+'` was the computed name on origin/main. + expect(screen.queryAllByRole('button', { name: '+' })).toHaveLength(0); + // Still two names for two controls — #3441's separation is not undone. expect(screen.getAllByRole('button', { name: 'React with thumbs up' }).length).toBe(2); }); + + /** + * The same control in the session language. `addThumbsUp` is already in every + * locale pack (objectstack#5506 shipped it for the tooltip), so this needed + * no new key — only a call site that lets a screen reader reach it. + */ + it('names the reaction-bar picker in the session language', () => { + renderThread('zh'); + + expect(screen.getByRole('button', { name: '点赞' })).toBeTruthy(); + expect(screen.getByTitle('点赞')).toBeTruthy(); + expect(screen.queryAllByRole('button', { name: '+' })).toHaveLength(0); + expect(screen.queryAllByRole('button', { name: 'Add thumbs up' })).toHaveLength(0); + }); }); /** diff --git a/packages/collaboration/src/__tests__/comment-thread-no-provider-fallback.test.tsx b/packages/collaboration/src/__tests__/comment-thread-no-provider-fallback.test.tsx index 2cbf63656..321103603 100644 --- a/packages/collaboration/src/__tests__/comment-thread-no-provider-fallback.test.tsx +++ b/packages/collaboration/src/__tests__/comment-thread-no-provider-fallback.test.tsx @@ -183,6 +183,27 @@ describe('CommentThread with no I18nProvider — English fallback (objectstack#5 expect(screen.queryAllByRole('button', { name: '✕' })).toHaveLength(0); }); + /** + * objectui#3478 — the fourth glyph-only control, the reaction-bar `+`. + * + * It already had `title: t('collaboration.addThumbsUp')`, which is why the + * defect survived #3441: the key WAS wired up and the English WAS in the + * defaults map, so every copy pin in this file passed. What no assertion + * covered until now is that `title` on a `button` never becomes the name — + * content (accname §2F) wins, and the content is `'+'`. + * + * RED before / GREEN after on the two `role`-based assertions; the + * `getByTitle` above (in the reaction-tooltip case) is green on both sides + * and stays there, because the tooltip is kept, not replaced. + */ + it('names the reaction-bar picker in English, tooltip and all', () => { + renderBare(); + + expect(screen.getAllByRole('button', { name: 'Add thumbs up' }).length).toBe(1); + expect(screen.queryAllByRole('button', { name: '+' })).toHaveLength(0); + expect(screen.getByTitle('Add thumbs up')).toBeTruthy(); + }); + /** * objectui#3441 — with no provider the session language is whatever * react-i18next reports (in practice `'en'`), and the >= 7d branch now hands From f6bc6b86ec4373556ee7f5d24ed15fb1aea219d0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 01:22:22 +0000 Subject: [PATCH 2/2] docs(collaboration): correct two docblock wordings in the #3478 accname pins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - "the only one of the three" was ambiguous once the describe covered four controls; it always meant the three naming mechanisms, now says so. - "+" is not an emoji — call the group glyph-only consistently. Comments only; no assertion or source change (72 passed, unchanged). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GTRjn8xBqp75dk7kFupVRt --- .../src/__tests__/comment-thread-i18n.test.tsx | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/collaboration/src/__tests__/comment-thread-i18n.test.tsx b/packages/collaboration/src/__tests__/comment-thread-i18n.test.tsx index 28ed69173..fddbeda8e 100644 --- a/packages/collaboration/src/__tests__/comment-thread-i18n.test.tsx +++ b/packages/collaboration/src/__tests__/comment-thread-i18n.test.tsx @@ -302,8 +302,9 @@ describe('CommentThread per-comment actions (objectstack#5506)', () => { * of an attribute. That distinction is the whole point of the fix: for a * `button`, name-from-content (accname §2F) is consulted BEFORE the `title` * tooltip (§2I), so a `title` hung on `'👍'` — or on `'+'`, which is what the - * picker had until #3478 — leaves the computed name as the glyph. `aria-label` - * is the only one of the three that outranks content. + * picker had until #3478 — leaves the computed name as the glyph. Of the three + * things that can name a `button` here, `aria-label` is the only one that + * outranks content. * * ── Direction ───────────────────────────────────────────────────────────── * RED before / GREEN after in EVERY language, `en` included — unlike the @@ -378,8 +379,9 @@ describe('CommentThread glyph-only control names (objectui#3441, #3478)', () => }); /** - * objectui#3478 — the `+` reaction picker, the fourth emoji-only control, and - * the one objectui#3441 left behind. + * objectui#3478 — the `+` reaction picker, the fourth glyph-only control + * (`+` is not an emoji, but accname does not care), and the one objectui#3441 + * left behind. * * Its content is the literal `'+'`, so accname §2F named it "plus" and the * `title` at §2I never got a turn — `collaboration.addThumbsUp` reached the