fix(Popper): prevent flash of incorrectly positioned popper on open#12177
fix(Popper): prevent flash of incorrectly positioned popper on open#12177fallmo wants to merge 8 commits intopatternfly:mainfrom
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 now uses requestAnimationFrame sequencing with explicit cancellation; a new utility Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
There was a problem hiding this comment.
Overall I like this change but I do have two concerns/callouts:
-
Any PF consumers who may have written unit tests related to testing popper dependent components are at risk of having their unit tests now fail. I think that if they are in that situation, it'd indicate they are not following testing best practices, but that's the sort of thing I'd love to process with @wise-king-sullyman. It's also likely that if they were trying to test their popper components, that those test could be flakey themselves since they were needing to contend with the flashing issue as well... so this may be a net improvement for even the projects it affects. 🤔
-
I think searchinput.spec.ts also needs to be updated on line 56 to add the timeout
Thanks for the review and thanks for the callouts. Good catch on searchinput.spec.ts, I went ahead and added a timeout. |
…positioning Signed-off-by: Mohamed Fall <ps.hackmaster@gmail.com>
wise-king-sullyman
left a comment
wise-king-sullyman
left a comment
There was a problem hiding this comment.
Thanks for making this PR! I just have a few questions/concerns.
| const transitionTimerRef = useRef(null); | ||
| const showTimerRef = useRef(null); | ||
| const hideTimerRef = useRef(null); | ||
| const rafRef = useRef<number>(null); |
There was a problem hiding this comment.
What does rafRef mean? The ref part I understand, but I have no idea what raf is referring to here.
There was a problem hiding this comment.
Here raf is short for requestAnimationFrame. rafRef stores the ID returned by requestAnimationFrame, which lets us cancel the scheduled frame on cleanup. Would it make sense to rename rafRef to something a bit more descriptive, like animationFrameRef?
There was a problem hiding this comment.
Hey sorry for the delay in circling back to this, yes I think that would be a better name.
There was a problem hiding this comment.
I've went ahead and changed it.
| } | ||
| }); | ||
| }; | ||
|
|
There was a problem hiding this comment.
I see that you're having this take an array as its arg and then forEaching through to cancel each ref in the array, but then in Popper it looks like you're just wrapping the ref by itself in an array with nothing else in it each time.
Am I missing/misunderstanding something here?
There was a problem hiding this comment.
Hey, you're absolutely right here. The clearAnimationFrames function takes an array, but in practice I only pass one value. I was following the pattern I saw with the clearTimeouts function, while also accounting for the possibility that there be varied uses of the function in the future.
There was a problem hiding this comment.
I would probably lean towards simplifying the function since unlike with clearTimeouts we don't actually need to be able to pass multiple refs. If in the future we do need to we can always add that functionality.
Also I would adjust the name to reflect that it's just singular when you make that change.
There was a problem hiding this comment.
I've changed this too.
| }); | ||
| cy.get('.pf-v6-c-tooltip').should('be.visible'); | ||
| // Tooltip visibility is async due to requestAnimationFrame-based positioning | ||
| cy.get('.pf-v6-c-tooltip', { timeout: 6000 }).should('be.visible'); |
There was a problem hiding this comment.
I have some real concerns with this change breaking a lot of tests for consumers if this change is needed. I'm not saying that's an absolute blocker, but it does give me pause. Would love to hear thoughts from others on this.
There was a problem hiding this comment.
Fair point. Given the current behavior this felt like the safest tradeoff, but I’d definitely welcome other suggestions.
There was a problem hiding this comment.
Are these test changes actually needed? I pulled your branch down and have run the tests a good few times with the timeouts removed and I'm not seeing any failures.
There was a problem hiding this comment.
Right, in normal circumstances these changes aren't necessary; the raf calculations happen fast enough that a delay is pretty much non-existent. It's just that in many CI environments, the runners are super slow (cpu-constrained), as a result anything even slightly asynchronous may need a timeout.
There was a problem hiding this comment.
Ah. That actually might make things even more tricky for consumers if they see their tests start failing on CI all of a sudden and they can't locally replicate. Especially since I'm having trouble reproducing the issue I'm not sure if the juice would be worth the squeeze here outside of a breaking change.
@nicolethoen @kmcfaul @thatblindgeye what do you all think?
There was a problem hiding this comment.
If we do implement the changes here, we'd need to at least call out in release highlights that tests may need to be updated with a timeout for Popper related components. Though that may still not prevent confusion if consumer tests suddenly fail.
Even though it's only for some tests, 6 seconds waiting is fairly long, and can potentially still be a lot of updates for consumers to need to make. I also am not able to reproduce the raised issue after several attempts, so I'd be curious what version of PatternFly this bug was seen in (not to say it isn't an issue, it just makes things trickier when we can't reproduce it to confirm a) a fix is needed, and b) what that fix should be).
Hey, thanks for the review ! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-core/src/helpers/__mocks__/util.ts`:
- Line 5: The mock clearAnimationFrame currently has no parameters; update the
exported mock function clearAnimationFrame to accept the same parameter as the
real implementation (animationFrameRef: React.RefObject<number>), check
animationFrameRef?.current, call cancelAnimationFrame on that value if present,
and then clear animationFrameRef.current (set to null) so Popper.tsx cleanup
calls (from Popper.tsx) match the real behavior and TypeScript types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 120d043a-8591-470f-a296-76aa55cb89b6
📒 Files selected for processing (3)
packages/react-core/src/helpers/Popper/Popper.tsxpackages/react-core/src/helpers/__mocks__/util.tspackages/react-core/src/helpers/util.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-core/src/helpers/util.ts
- packages/react-core/src/helpers/Popper/Popper.tsx
|
|
||
| export const clearTimeouts = () => {}; | ||
|
|
||
| export const clearAnimationFrame = () => {}; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Real clearAnimationFrame signature =="
cat -n packages/react-core/src/helpers/util.ts | head -30
echo
echo "== Mock clearAnimationFrame signature =="
cat -n packages/react-core/src/helpers/__mocks__/util.ts
echo
echo "== Jest manual mocks in react-core =="
find packages/react-core -name '__mocks__' -type d
echo
echo "== Test files using helpers/util mock =="
rg -l "__mocks__/util" packages/react-core --type=ts --type=tsx
echo
echo "== Actual clearAnimationFrame call sites =="
rg -n "clearAnimationFrame\(" packages/react-core/src --type=ts --type=tsx -B2 -A2Repository: patternfly/patternfly-react
Length of output: 1928
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Real clearAnimationFrame in util.ts =="
rg -n "clearAnimationFrame" packages/react-core/src/helpers/util.ts -A5 -B2
echo
echo "== Mock util.ts complete file =="
cat packages/react-core/src/helpers/__mocks__/util.ts
echo
echo "== Test files importing helpers util =="
rg -l "from.*helpers/util" packages/react-core/src --type ts
echo
echo "== Popper component clearAnimationFrame usage =="
rg -n "clearAnimationFrame" packages/react-core/src/components/Popper -A3 -B3 --type tsRepository: patternfly/patternfly-react
Length of output: 3514
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== All clearAnimationFrame calls in codebase =="
rg -n "clearAnimationFrame\(" packages/react-core/src -A2 -B2
echo
echo "== Check if Popper folder exists =="
find packages/react-core/src -name "*Popper*" -o -name "*popper*" | head -20
echo
echo "== Test files that might use the mock =="
rg -l "__mocks__" packages/react-core/src --type ts | head -20Repository: patternfly/patternfly-react
Length of output: 2759
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check helpers/__mocks__/index.ts =="
cat packages/react-core/src/helpers/__mocks__/index.ts
echo
echo "== Popper test file looking for mock setup =="
find packages/react-core -path "*/Popper*" -name "*.test.*" -o -path "*/Popper*" -name "*.spec.*"
echo
echo "== Check for jest.mock in test files that use Popper =="
rg -l "jest\.mock.*helpers/util" packages/react-core/src --type ts
echo
echo "== Check Popper.tsx imports =="
head -20 packages/react-core/src/helpers/Popper/Popper.tsxRepository: patternfly/patternfly-react
Length of output: 1199
Mock clearAnimationFrame must accept and handle the animationFrameRef parameter
The mock at line 5 exports a zero-arg no-op, but the real implementation takes animationFrameRef: React.RefObject<number> and calls cancelAnimationFrame(). Popper.tsx passes animationFrameRef at cleanup (lines 279, 474, 487, 505), so the mock will cause TypeScript errors in tests and allow queued animation frame callbacks to execute after unmount.
Fix
+import type * as React from 'react';
+
export const getUniqueId = () => 'unique_id_mock';
export const clearTimeouts = () => {};
-export const clearAnimationFrame = () => {};
+export const clearAnimationFrame = (animationFrameRef: React.RefObject<number>) => {
+ if (animationFrameRef.current) {
+ cancelAnimationFrame(animationFrameRef.current);
+ }
+};
export const getLanguageDirection = () => 'ltr';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/helpers/__mocks__/util.ts` at line 5, The mock
clearAnimationFrame currently has no parameters; update the exported mock
function clearAnimationFrame to accept the same parameter as the real
implementation (animationFrameRef: React.RefObject<number>), check
animationFrameRef?.current, call cancelAnimationFrame on that value if present,
and then clear animationFrameRef.current (set to null) so Popper.tsx cleanup
calls (from Popper.tsx) match the real behavior and TypeScript types.
5 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