fix(Popper): prevent flash of incorrectly positioned popper on open #12177
Conversation
Update ref types to HTMLSpanElement and initialize with null to fix TS errors Signed-off-by: Mohamed Fall <ps.hackmaster@gmail.com>
Use requestAnimationFrame to ensure Popper.js has calculated and applied position transforms before transitioning opacity to 1. Signed-off-by: Mohamed Fall <ps.hackmaster@gmail.com>
WalkthroughPopper’s show/hide timing was changed to use requestAnimationFrame sequencing and explicit animation-frame cancellation; a new clearAnimationFrames utility and mock were added. Several unit and Cypress tests were updated to wait for the opacity transition/positioning by using waitFor or extended timeouts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-core/src/helpers/Popper/Popper.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Build, test & deploy
|
Preview: https://pf-react-pr-12177.surge.sh A11y report: https://pf-react-pr-12177-a11y.surge.sh |
Update Jest snapshot tests to wait for popper elements to complete their opacity transition after the double requestAnimationFrame fix. This ensures snapshots are taken after poppers are fully visible (opacity: 1) rather than during the transition phase (opacity: 0). Signed-off-by: Mohamed Fall <ps.hackmaster@gmail.com>
…n frames Add cleanup for requestAnimationFrame to prevent race conditions when popper visibility changes rapidly or component unmounts during RAF execution. Signed-off-by: Mohamed Fall <ps.hackmaster@gmail.com>
fallmo
force-pushed
the
main
branch
2 times, most recently
from
a71702d to
f89b313
Compare
…nc nature Signed-off-by: Mohamed Fall <ps.hackmaster@gmail.com>
coderabbitai
bot
requested review from
lboehling,
mcoker,
nicolethoen and
thatblindgeye
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/react-integration/cypress/integration/overflowmenu.spec.ts (1)
35-35: LGTM - Consistent timeout extensions for menu visibility.All four menu visibility checks appropriately extend the timeout to 6000ms for RAF-based positioning. Optionally, consider adding explanatory comments similar to the tooltip tests for consistency.
Also applies to: 72-72, 110-110, 145-145
packages/react-core/src/helpers/util.ts (1)
527-536: LGTM - Well-implemented utility for RAF cleanup.The
clearAnimationFramesfunction correctly mirrors the pattern of the existingclearTimeoutsutility and properly cancels pending animation frames. The implementation is safe and follows best practices.Optional: Enhance JSDoc for clarity
Consider expanding the JSDoc to describe the function's purpose:
/** + * Cancels all pending animation frames tracked by the provided refs. + * * @param {React.RefObject<any>[]} animationFrameRefs - Animation frame refs to clear */
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/react-core/src/components/DatePicker/__tests__/DatePicker.test.tsx(2 hunks)packages/react-core/src/components/Nav/__tests__/Nav.test.tsx(2 hunks)packages/react-core/src/components/SearchInput/__tests__/SearchInput.test.tsx(2 hunks)packages/react-core/src/helpers/Popper/Popper.tsx(5 hunks)packages/react-core/src/helpers/__mocks__/util.ts(1 hunks)packages/react-core/src/helpers/util.ts(1 hunks)packages/react-integration/cypress/integration/button.spec.ts(2 hunks)packages/react-integration/cypress/integration/overflowmenu.spec.ts(4 hunks)packages/react-integration/cypress/integration/tabsdisable.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/react-core/src/helpers/__mocks__/util.ts (1)
packages/react-core/src/helpers/util.ts (1)
clearAnimationFrames(530-536)
packages/react-core/src/helpers/util.ts (1)
packages/react-core/src/helpers/__mocks__/util.ts (1)
clearAnimationFrames(5-5)
🔇 Additional comments (15)
packages/react-integration/cypress/integration/tabsdisable.spec.ts (1)
43-44: LGTM - Appropriate test synchronization for RAF-based positioning.The extended timeout and explanatory comment align with the PR's objective to accommodate asynchronous positioning via requestAnimationFrame.
packages/react-integration/cypress/integration/button.spec.ts (1)
12-13: LGTM - Consistent test synchronization across tooltip tests.Both tooltip visibility checks appropriately extend the timeout to accommodate RAF-based positioning.
Also applies to: 22-22
packages/react-core/src/components/DatePicker/__tests__/DatePicker.test.tsx (2)
1-1: LGTM - Added waitFor for async assertion.Appropriate import addition to support opacity transition synchronization.
96-100: LGTM - Proper synchronization for RAF-based positioning.The waitFor block correctly waits for the popper's opacity transition to complete before snapshot assertion, aligning with the new two-frame RAF sequence in Popper.
packages/react-core/src/helpers/__mocks__/util.ts (1)
5-5: LGTM - Appropriate no-op mock for test environment.The mock follows the established pattern of other utility mocks in this file and appropriately stubs the clearAnimationFrames function for test environments.
packages/react-core/src/components/Nav/__tests__/Nav.test.tsx (2)
2-2: LGTM - Added waitFor import for async assertion.Appropriate import addition to support flyout opacity transition synchronization.
245-249: LGTM - Proper synchronization for flyout positioning.The waitFor block correctly waits for the popper's opacity transition after hover, ensuring the flyout is fully visible before snapshot assertion.
packages/react-core/src/components/SearchInput/__tests__/SearchInput.test.tsx (2)
2-2: LGTM - Added waitFor import for async assertion.Appropriate import addition to support panel opacity transition synchronization.
154-158: LGTM - Proper synchronization for panel positioning.The waitFor block correctly waits for the popper panel's opacity transition after the advanced search panel opens, ensuring proper visibility before snapshot assertion.
packages/react-core/src/helpers/Popper/Popper.tsx (6)
6-6: LGTM!Import of
clearAnimationFramesutility enables consistent RAF cleanup alongside the existing timeout management.
237-237: LGTM!The
rafRefproperly tracks the pendingrequestAnimationFrameID, addressing the previously identified race condition concern.
276-282: LGTM!Cleanup effect properly cancels both timeouts and animation frames on unmount, preventing state updates on unmounted components.
471-482: LGTM!The
clearAnimationFramescall ensures any pending show animation is cancelled when the exit delay changes, maintaining consistent state.
484-500: Well-implemented fix for the positioning flash issue.The double
requestAnimationFramepattern correctly ensures:
- React has committed DOM changes (first RAF)
- Popper.js has calculated and applied transforms (second RAF)
The RAF tracking addresses the race condition identified in the previous review — if
hide()is called at any point,clearAnimationFrames([rafRef])will cancel whichever RAF is currently pending.
502-514: LGTM!The
clearAnimationFrames([rafRef])call inhide()correctly cancels any pending RAF callbacks fromshow(), preventing the popper from becoming visible when it should be hidden.
2 participants
Description
Fixes an issue where popper elements (dropdowns, select, etc.) briefly flash at the top-left of the screen before appearing in their correct position.
bug4.mp4
Issue
When a popper opens, there's a race condition between:
opacity: 0)1Previously, the opacity would transition to
1immediately after settinginternalIsVisible, which could occur before Popper.js finished calculating and applying the position transform.Fix
Added two
requestAnimationFramecalls in theshow()function:Only after both frames do we allow the opacity transition to begin. This guarantees the browser has painted the correctly positioned element before making it visible.
Testing
The issue occurs intermittently and can be hard to reproduce reliably.
If you haven't encountered it, go to the Menus > Dropdown page, open and close the dropdowns back to back, then repeat with Menus > Select. The issue should reproduce within a few attempts.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.