fix(settings): defer AlertBar click-outside arming to next tick#20663
Closed
vpomerleau wants to merge 1 commit into
Closed
fix(settings): defer AlertBar click-outside arming to next tick#20663vpomerleau wants to merge 1 commit into
vpomerleau wants to merge 1 commit into
Conversation
Because: - The "Passkey setup was canceled. Try again." banner intermittently failed to appear on the first cancel after a fresh sign-in (QA on Stage 1.337.0). The Cancel-button click that calls alertBar.error() bubbles to <body> in the same task as React commits the alert into the DOM; AlertBar's global click-outside listener then sees the now-attached ref, treats the click as "outside", and closes the alert before the user can see it. Existing unit tests didn't catch this because they mock alertBarInfo and never render the real AlertBar. This commit: - Replaces useClickOutsideEffect in AlertBar with an inline effect keyed on visible that arms the body-click listener one macrotask after visible flips to true, so the click that opened the alert cannot also close it. - Only attaches the listener while the alert is visible (small correctness improvement over the previous always-on body listener). - Preserves the ESC-key close path and the existing modal-coexistence rule (FXA-2463 TODO) for clicks while a Settings Modal is open. - Adds index.race.test.tsx exercising real reactive vars + fake timers to cover the same-click race, the subsequent-click close, the modal coexistence rule, and inside-the-alert clicks. Closes #FXA-13681
Contributor
Author
|
Continued investigating, this looks like the wrong solution. |
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
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
This pull request
useClickOutsideEffectinAlertBarwith an inline effect keyed onvisiblethat arms the body-click listener one macrotask aftervisibleflips totrue, so the click that opened the alert cannot also close it.index.race.test.tsxto exercise real reactive vars + fake timers, covering the same-click race, the subsequent-click close, the modal coexistence rule, and inside-the-alert clicks.Issue that this pull request solves
Closes: FXA-13681
Checklist
Put an `x` in the boxes that apply
How to review (Optional)
Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
This is a follow-up to a previously merged fix for FXA-13681. The first attempt didn't catch the race because existing unit tests mocked `alertBarInfo` and never rendered the real `AlertBar`. The new `index.race.test.tsx` closes that coverage gap.