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..fddbeda8e 100644 --- a/packages/collaboration/src/__tests__/comment-thread-i18n.test.tsx +++ b/packages/collaboration/src/__tests__/comment-thread-i18n.test.tsx @@ -293,25 +293,33 @@ 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. 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 * 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 +379,57 @@ 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 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 + * 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