fix(react-popover): prevent incorrect non-modal screen reader announcement#36383
fix(react-popover): prevent incorrect non-modal screen reader announcement#36383PaulGMardling wants to merge 15 commits into
Conversation
PaulGMardling
changed the title
📊 Bundle size report
Unchanged fixtures
|
|
Pull request demo site: URL |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
🕵🏾♀️ visual changes to review in the Visual Change Report
vr-tests-react-components/Positioning 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/Positioning.Positioning end.chromium.png | 910 | Changed |
| vr-tests-react-components/Positioning.Positioning end.updated 2 times.chromium.png | 628 | Changed |
vr-tests-react-components/ProgressBar converged 3 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/ProgressBar converged.Indeterminate + thickness.default.chromium.png | 40 | Changed |
| vr-tests-react-components/ProgressBar converged.Indeterminate + thickness - High Contrast.default.chromium.png | 34 | Changed |
| vr-tests-react-components/ProgressBar converged.Indeterminate + thickness - Dark Mode.default.chromium.png | 2 | Changed |
vr-tests-react-components/TagPicker 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/TagPicker.disabled - Dark Mode.chromium.png | 658 | Changed |
| vr-tests-react-components/TagPicker.disabled.chromium.png | 677 | Changed |
There were 2 duplicate changes discarded. Check the build logs for more information.
eaac4f0 to
ad5e853
Compare
PaulGMardling
changed the title
1a1be75 to
ed33516
Compare
ed33516 to
a2e2d0e
Compare
2357010 to
e0a2c69
Compare
…to fix Narrator style group announcement
…o focusable content + test coverage
…faceAriaAttributes - Remove isolated unit tests for getPopoverSurfaceAriaAttributes function - Keep component-level integration tests that verify aria attributes through rendered component - Update function comment to remove mention of isolated testing - Component logic is now tested as a whole rather than in isolation
…shaking - Change condition from 'process.env.NODE_ENV === production' to '!== production' - Move development-only warning logic inside the production check - Ensures entire dev warning useEffect is dropped in production builds - Follows Fluent UI pattern for dev-only code with guaranteed tree-shaking
- Replace double negation operator (!!) with explicit Boolean() call - Improves code readability and follows codebase conventions - No functional change, same type coercion behavior
- Remove nonInteractiveContentWarning constant from public API (constants.ts) - Inline warning message directly in console.warn() call in usePopover.ts - Removes logging noise from public API exports - Update tests to use inlined warning message instead of constant reference
37dacef to
b02f3bb
Compare
|
I'm not sure I follow the intent of this changes to be honest. The usage of popover itself is explicitly for focusable content. this incorrect non-modal screen reader announcement is referring to a usage of a popover without focusable content, if you have non-focusable content, why are you even using popover? 
Probably tooltip is more of what you're looking for?! I don't see the point in modifying something that is even triggering changes on snapshots in our codebase for a use case that we do not actually support. |
@bsunderhus Thanks, this is a good callout. I agree Popover is generally intended for focusable/interactive content, and Tooltip is often a better fit for purely informational content. The intent of this PR is narrower: it fixes the ARIA semantics PopoverSurface emits for existing non-modal usage. Today we can render role="group" without an accessible name, which leads to a misleading announcement ("contains style group") and does not align with expected ARIA semantics for group. So this change is not trying to endorse non-focusable Popover usage as best practice. It is making the current output less misleading and more correct for assistive tech when that usage exists. The snapshot changes also highlight that we already have tooltip-like scenarios in Fluent UI v9 that currently flow through Popover. That is useful signal for follow-up cleanup, but separate from this correctness fix. This PR addresses the immediate a11y semantics issue (unlabeled role="group" causing misleading announcements). A separate follow-up can audit those callout patterns and migrate clearly non-interactive cases to Tooltip semantics where appropriate. I don't see the point in modifying something that is even triggering changes on snapshots in our codebase for a use case that we do not actually support. Thanks, I hear you. I agree we should avoid churn for unsupported scenarios when possible. In this case, the snapshot changes are a side effect of correcting accessibility semantics, not the reason for the change. We were emitting role="group" without an accessible name in non-modal surfaces, which produced misleading screen reader output ("contains style group"). Keeping that behavior because some usages are not ideal would mean preserving incorrect ARIA output in shipped code. So the tradeoff here is intentional: fix the semantics now, accept the snapshot updates that reflect that correction, and handle broader consumer-pattern alignment (Tooltip vs Popover) as a separate follow-up. |
Co-authored-by: Victor Genaev <vgenaev@gmail.com>
4 participants
Summary
This PR fixes the incorrect Narrator announcement for non-modal Popover when content is non-interactive.
Previously, activating this scenario could produce:
contains style group
What Changed
PopoverSurface no longer applies role="group" unconditionally when trapFocus is false.
role="group" is now applied only when an accessible name is provided via aria-label or aria-labelledby, aligning with WAI-ARIA 1.2 requirements.
If no accessible name is provided, the surface renders as a plain div, so Narrator no longer announces contains style group.
Aria attribute logic was extracted into a pure function named getPopoverSurfaceAriaAttributes for isolated unit testing.
The default Popover story was updated to include aria-label as the recommended usage pattern.
Added a dev-only warning for non-modal popovers that open with non-interactive, non-focusable content to guide consumers toward accessible patterns.
Previous Behavior
On activating a popover with no interactive elements, Narrator announced:
contains style group
New Behavior
Narrator no longer announces contains style group for a non-modal popover surface without an accessible name.
Notes
This change resolves the incorrect announcement and adds consumer guidance for the non-interactive-content edge case.
Work Item
Fixes #18661