You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(settings): render Banner content after mount for screen readers - #21609
Screen readers reliably announce only changes to a live region that already exists.
The Banner inserted its role="status" or role="alert" element with its text, so the text was often not announced.
This pull request
Adds a ready state to Banner/index.tsx. One useEffect sets it after mount.
Renders the same outer div before and after the effect, with only id, role, aria-live and tabIndex on the first render.
After the effect, adds the existing className, onAnimationEnd and children to that node.
Adds a MutationObserver test in Banner/index.test.tsx for the status (success) and alert (error) regions. It checks that the region is inserted first and its children are added in a later record.
Changes five consumer specs from getByText to await findByText. Banner text now appears one render after a state update outside act. Only the expectations change.
Rollback: Easy. A plain revert; it ships on the next train.
Verification: Strong. A test fails without the fix (verify --revert). 8 checks pass, 1 left to CI. A screenshot or video is attached.
Would not catch: whether NVDA, VoiceOver or Narrator really announce the banner, and a banner set inside a click handler where React runs the effect before a paint.
If applicable, I have modified or added tests which pass locally.
I have added necessary documentation (if appropriate).
I have verified that my changes render correctly in RTL (if appropriate).
I have manually reviewed all AI generated code.
How to review (Optional)
Key files/areas to focus on: Banner/index.tsx and the new live region test.
Suggested review order: Banner/index.tsx, Banner/index.test.tsx, then the five consumer specs.
Risky or complex parts: after a discrete event such as a click, React can run the effect before a paint. A banner set in an event handler can then appear in the same frame as the region. That is the case to check by hand.
Screenshots (Optional)
None. The rendered output after mount does not change.
Other information (Optional)
Not verified: screen reader behavior (NVDA with Firefox, VoiceOver with Safari). The new test checks DOM order only, not announcement. It needs a manual check after merge on "Skip the password next time?" and on one error banner.
One reviewer call: the change uses an effect, not setTimeout, as the ticket requires. Is that enough for the event-handler case above?
Tests:
Pass (/fxa-verify --run): planned fxa-settings unit tests, lint, Prettier and tsc.
Pass (/fxa-verify --run): functional resetPassword.spec.ts, "click resend" (status banner) and "enter unknown email" (alert banner), on the local stack.
Pass (run by hand, 2 workers): all 152 fxa-settings specs related to Banner (1942 tests).
/fxa-verify --revert showed that the new test fails without the fix and passes with it.
Not covered by CI: screen readers (NVDA with Firefox, VoiceOver with Safari). Check them by hand after merge.
Merge Confidence: 4/5
Banner now mounts its status or alert live region before adding its classes, animation handler, and content. This lets assistive technology observe the region before its content appears. A MutationObserver test checks insertion order for success and error banners. Consumer tests now wait for banner text to appear.
The change affects banner rendering and related tests. The supplied context reports passing unit tests, lint, formatting, type checks, and selected functional checks. Screen reader behavior was not verified, and manual checks remain outstanding.
No actionable comments were generated in the recent review.
Walkthrough
Banner initially renders its live region without its content, then renders the banner content after mounting. Tests verify this rendering order and wait asynchronously for banner messages across several pages.
Changes: Banner rendering
Layer / File(s)
Summary
Defer Banner content until after mounting packages/fxa-settings/src/components/Banner/index.tsx, packages/fxa-settings/src/components/Banner/index.test.tsx
Banner keeps its role and live-region attributes on initial render. After mounting, it renders its container classes, banner content, and animation-end handler. Tests check that the live region appears before its heading.
Await banner text in page tests packages/fxa-settings/src/pages/Authorization/container.test.tsx, packages/fxa-settings/src/pages/InlinePasswordlessSyncSetup/container.test.tsx, packages/fxa-settings/src/pages/Pair/Index/index.test.tsx, packages/fxa-settings/src/pages/Signin/SigninTokenCode/index.test.tsx, packages/fxa-settings/src/pages/Signin/SigninTotpCode/index.test.tsx
Page tests use findByText to wait for existing error and success messages.
The change is mergeable, with the planned screen-reader checks still outstanding.
Pre-merge checks: 8
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title "fix(settings): render Banner content after mount for screen readers" follows the required type(scope): subject format. It uses the fix type, the settings scope, and an imperative, lowercase…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Sensitive Data Protection And Privacy
The reviewed changes only add delayed Banner rendering and update asynchronous test assertions. The diff adds no email addresses, UIDs, phone numbers, tokens, secrets, credentials, payment data, loggi…
Security, Audit, And Regulatory Control Integrity
PASS. The PR changes only Banner rendering and test timing. The Banner metrics call remains present with the same GleanMetrics.banner.view behavior, and the other changes only replace synchronous te…
Performance And Algorithmic Complexity Review
The changed production code adds one bounded mount effect and one intentional state update per Banner instance. It does not add loops, data-store calls, retained listeners, unbounded buffers, or exp…
Database Migration Safety
The pull request changes only packages/fxa-settings files. It does not modify or delete any file under packages/db-migrations/databases/*/patches/, and it does not introduce a schema change requir…
Full details: Title check
Explanation
The title "fix(settings): render Banner content after mount for screen readers" follows the required type(scope): subject format. It uses the fix type, the settings scope, and an imperative, lowercase subject with no trailing period. It is 67 characters, within the 72-character limit. The title matches the main change: Banner now renders its content after mount, and the new tests check live-region insertion order.
Full details: Sensitive Data Protection And Privacy
Explanation
The reviewed changes only add delayed Banner rendering and update asynchronous test assertions. The diff adds no email addresses, UIDs, phone numbers, tokens, secrets, credentials, payment data, logging, Sentry events, or new third-party data transfer. Existing Glean handling and Banner props remain unchanged; no collection, retention, deletion, or account-isolation behavior is introduced.
Full details: Security, Audit, And Regulatory Control Integrity
Explanation
PASS. The PR changes only Banner rendering and test timing. The Banner metrics call remains present with the same GleanMetrics.banner.view behavior, and the other changes only replace synchronous test queries with asynchronous queries. The diff introduces no authentication, session or token validation, rate limiting, cryptography, webhook verification, audit-event, PCI DSS, or GDPR control change.
Full details: Performance And Algorithmic Complexity Review
Explanation
The changed production code adds one bounded mount effect and one intentional state update per Banner instance. It does not add loops, data-store calls, retained listeners, unbounded buffers, or expensive work. The extra render is required to insert the live region before its content and is not an unnecessary rerender. The other changes only update tests to await delayed banner content.
Full details: Database Migration Safety
Explanation
The pull request changes only packages/fxa-settings files. It does not modify or delete any file under packages/db-migrations/databases/*/patches/, and it does not introduce a schema change requiring migration patches or a target-patch update.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
CodeRabbit Verify
Run CodeRabbit Verify to test the changes in this pull request.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
Source excerpt: When building a `metricsContext` or forwarding flow identity (`flowId`, `flowBeginTime`, `deviceId`) to the auth-server or Glean, source it from the frozen `flowQueryParams` snapshot (threaded from `App`, the same value the...
Source excerpt: title: Localization :::warning Gettext is on its way out The Backbone client that consumed the gettext `.po` catalogues was deleted in October 2026 ([#21585](https://github.com//pull/21585)).
Because:
- Screen readers reliably announce only changes to a live region that already exists.
- The Banner inserted its live region with its text, so the text was often not announced.
This commit:
- Adds a `ready` state to Banner, set by one useEffect after mount.
- Renders the live region first with only id, role, aria-live and tabIndex.
- Adds className, onAnimationEnd and children to the same node after the effect.
- Adds a MutationObserver test for the status and alert regions.
- Changes five consumer specs from getByText to await findByText.
Closes FXA-10912
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
autocoderabbitSignal to CodeRabbit to perform a CR review
1 participant
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.
Because
role="status"orrole="alert"element with its text, so the text was often not announced.This pull request
readystate toBanner/index.tsx. OneuseEffectsets it after mount.divbefore and after the effect, with onlyid,role,aria-liveandtabIndexon the first render.className,onAnimationEndand children to that node.Banner/index.test.tsxfor thestatus(success) andalert(error) regions. It checks that the region is inserted first and its children are added in a later record.getByTexttoawait findByText. Banner text now appears one render after a state update outsideact. Only the expectations change.Risk
Risk: Medium (the worst of the three below)
Issue that this pull request solves
Closes: FXA-10912
Checklist
Put an
xin the boxes that applyHow to review (Optional)
Banner/index.tsxand the newlive regiontest.Banner/index.tsx,Banner/index.test.tsx, then the five consumer specs.Screenshots (Optional)
None. The rendered output after mount does not change.
Other information (Optional)
Not verified: screen reader behavior (NVDA with Firefox, VoiceOver with Safari). The new test checks DOM order only, not announcement. It needs a manual check after merge on "Skip the password next time?" and on one error banner.
One reviewer call: the change uses an effect, not
setTimeout, as the ticket requires. Is that enough for the event-handler case above?Tests:
/fxa-verify --run): plannedfxa-settingsunit tests, lint, Prettier andtsc./fxa-verify --run): functionalresetPassword.spec.ts, "click resend" (status banner) and "enter unknown email" (alert banner), on the local stack.fxa-settingsspecs related to Banner (1942 tests)./fxa-verify --revertshowed that the new test fails without the fix and passes with it.Merge Confidence: 4/5
Bannernow mounts its status or alert live region before adding its classes, animation handler, and content. This lets assistive technology observe the region before its content appears. AMutationObservertest checks insertion order for success and error banners. Consumer tests now wait for banner text to appear.The change affects banner rendering and related tests. The supplied context reports passing unit tests, lint, formatting, type checks, and selected functional checks. Screen reader behavior was not verified, and manual checks remain outstanding.