Skip to content

fix(Popper): prevent flash of incorrectly positioned popper on open #12177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fallmo wants to merge 8 commits into
base: main
Choose a base branch
from
+55 −14
Open

fix(Popper): prevent flash of incorrectly positioned popper on open #12177

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5f4c037
fix(Drawer): update refs in examples
fallmo Nov 22, 2025
c758eaa
Merge branch 'patternfly:main' into main
fallmo Dec 14, 2025
1bb6a2b
fix(Popper): prevent flash of incorrectly positioned popper on open
fallmo Dec 14, 2025
97c8cfc
test: update snapshot tests to wait for popper opacity transition
fallmo Dec 14, 2025
161cb88
fix(Popper): prevent race conditions by properly cleaning up animatio…
fallmo Dec 14, 2025
a9a1aaf
fix(tests): increase tooltip visibility timeout to accomodate for asy…
fallmo Dec 20, 2025
cea95fb
fix(tests): increase tooltip visibility timeout to accommodate async …
fallmo Jan 10, 2026
e5e7048
fix(Popper): rename animation frame ref and adapt clearAnimationFrame…
fallmo Mar 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion packages/react-core/src/components/DatePicker/__tests__/DatePicker.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { screen, render } from '@testing-library/react';
import { screen, render, waitFor } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

import { HelperText, HelperTextItem } from '../../HelperText';
Expand Down Expand Up @@ -93,6 +93,11 @@ test('With popover opened', async () => {

await user.click(screen.getByRole('button', { name: 'Toggle date picker' }));
await screen.findByRole('button', { name: 'Previous month' });
// Wait for popper opacity transition after requestAnimationFrame
await waitFor(() => {
const popover = screen.getByRole('dialog');
expect(popover).toHaveStyle({ opacity: '1' });
});

expect(asFragment()).toMatchSnapshot();
});
Expand Down
7 changes: 6 additions & 1 deletion packages/react-core/src/components/Nav/__tests__/Nav.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { StrictMode } from 'react';
import { render, screen } from '@testing-library/react';
import { render, screen, waitFor } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import '@testing-library/jest-dom';

Expand Down Expand Up @@ -242,6 +242,11 @@ describe('Nav', () => {
);

await user.hover(screen.getByText('My custom node'));
// Wait for popper opacity transition after requestAnimationFrame
await waitFor(() => {
const flyout = screen.getByText('Flyout test').parentElement;
expect(flyout).toHaveStyle({ opacity: '1' });
});
expect(asFragment()).toMatchSnapshot();
});

Expand Down
7 changes: 6 additions & 1 deletion packages/react-core/src/components/SearchInput/__tests__/SearchInput.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { StrictMode } from 'react';
import { render, screen } from '@testing-library/react';
import { render, screen, waitFor } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

import { SearchInput } from '../SearchInput';
Expand Down Expand Up @@ -151,6 +151,11 @@ describe('SearchInput', () => {
expect(screen.getByTestId('test-id')).toContainElement(screen.getByText('First name'));

expect(props.onSearch).toHaveBeenCalled();
// Wait for popper opacity transition after requestAnimationFrame
await waitFor(() => {
const panel = screen.getByText('First name').closest('.pf-v6-c-panel');
expect(panel?.parentElement).toHaveStyle({ opacity: '1' });
});
expect(asFragment()).toMatchSnapshot();
});

Expand Down
18 changes: 15 additions & 3 deletions packages/react-core/src/helpers/Popper/Popper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import * as ReactDOM from 'react-dom';
import { usePopper } from './thirdparty/react-popper/usePopper';
import { Options as OffsetOptions } from './thirdparty/popper-core/modifiers/offset';
import { Placement, Modifier } from './thirdparty/popper-core';
import { clearTimeouts } from '../util';
import { clearAnimationFrames, clearTimeouts } from '../util';
import { css } from '@patternfly/react-styles';
import '@patternfly/react-styles/css/components/Popper/Popper.css';
import { getLanguageDirection } from '../util';
Expand Down Expand Up @@ -234,6 +234,7 @@ export const Popper: React.FunctionComponent<PopperProps> = ({
const transitionTimerRef = useRef(null);
const showTimerRef = useRef(null);
const hideTimerRef = useRef(null);
const rafRef = useRef<number>(null);
Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does rafRef mean? The ref part I understand, but I have no idea what raf is referring to here.

Copy link
Contributor Author

@fallmo fallmo Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey sorry for the delay in circling back to this, yes I think that would be a better name.

Copy link
Contributor Author

@fallmo fallmo Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've went ahead and changed it.

const prevExitDelayRef = useRef<number>(undefined);

const refOrTrigger = refElement || triggerElement;
Expand Down Expand Up @@ -275,6 +276,7 @@ export const Popper: React.FunctionComponent<PopperProps> = ({
useEffect(
() => () => {
clearTimeouts([transitionTimerRef, hideTimerRef, showTimerRef]);
clearAnimationFrames([rafRef]);
},
[]
);
Expand Down Expand Up @@ -469,6 +471,7 @@ export const Popper: React.FunctionComponent<PopperProps> = ({
useEffect(() => {
if (prevExitDelayRef.current < exitDelay) {
clearTimeouts([transitionTimerRef, hideTimerRef]);
clearAnimationFrames([rafRef]);
hideTimerRef.current = setTimeout(() => {
transitionTimerRef.current = setTimeout(() => {
setInternalIsVisible(false);
Expand All @@ -481,16 +484,25 @@ export const Popper: React.FunctionComponent<PopperProps> = ({
const show = () => {
onShow();
clearTimeouts([transitionTimerRef, hideTimerRef]);
clearAnimationFrames([rafRef]);
showTimerRef.current = setTimeout(() => {
setInternalIsVisible(true);
setOpacity(1);
onShown();
// Ensures React has committed the DOM changes and the popper element is rendered
rafRef.current = requestAnimationFrame(() => {
// Ensures Popper.js has calculated and applied the position transform before making element visible
rafRef.current = requestAnimationFrame(() => {
setOpacity(1);
onShown();
rafRef.current = null;
});
});
}, entryDelay);
};

const hide = () => {
onHide();
clearTimeouts([showTimerRef]);
clearAnimationFrames([rafRef]);
hideTimerRef.current = setTimeout(() => {
setOpacity(0);
transitionTimerRef.current = setTimeout(() => {
Expand Down
2 changes: 2 additions & 0 deletions packages/react-core/src/helpers/__mocks__/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,6 @@ export const getUniqueId = () => 'unique_id_mock';

export const clearTimeouts = () => {};

export const clearAnimationFrames = () => {};

export const getLanguageDirection = () => 'ltr';
11 changes: 11 additions & 0 deletions packages/react-core/src/helpers/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,17 @@ export const clearTimeouts = (timeoutRefs: React.RefObject<any>[]) => {
});
};

/**
* @param {React.RefObject<any>[]} animationFrameRefs - Animation frame refs to clear
*/
export const clearAnimationFrames = (animationFrameRefs: React.RefObject<any>[]) => {
animationFrameRefs.forEach((ref) => {
if (ref.current) {
cancelAnimationFrame(ref.current);
}
});
};

Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@fallmo fallmo Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@fallmo fallmo Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed this too.

/**
* Helper function to get the language direction of a given element, useful for figuring out if left-to-right
* or right-to-left specific logic should be applied.
Expand Down
5 changes: 3 additions & 2 deletions packages/react-integration/cypress/integration/button.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ describe('Button Demo Test', () => {
.focus()
.should('have.attr', 'aria-describedby', 'button-with-tooltip-1');
});
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');
Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@fallmo fallmo Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point. Given the current behavior this felt like the safest tradeoff, but I’d definitely welcome other suggestions.

Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@fallmo fallmo Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@thatblindgeye thatblindgeye Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@fallmo fallmo Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created an issue #12264 to document the problem a bit more.

Copy link
Contributor

@thatblindgeye thatblindgeye Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm personally still unable to reproduce so far after 10 minutes in Chrome and Firefox trying to trigger the bug. Could you check if you're seeing the bug with the latest PF updates on our staging site?

Copy link
Contributor Author

@fallmo fallmo Mar 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, yeah, it was quite a pain to reproduce with Firefox and even more of a pain to reproduce in chrome. The flicker happens much more consistently with Safari. After doing some research, turns out it's related to the way the browsers handle paint scheduling. Apparently Safari paints more aggressively/immediately after DOM mutations, whereas chrome is better at batching multiple mutations onto a single paint.

And sure, I'll try to reproduce it using the staging website.

});

it('Verify isAriaDisabled button has tooltip when hovered', () => {
Expand All @@ -18,7 +19,7 @@ describe('Button Demo Test', () => {
.trigger('mouseover')
.should('have.attr', 'aria-describedby', 'button-with-tooltip-1');
});
cy.get('.pf-v6-c-tooltip').should('be.visible');
cy.get('.pf-v6-c-tooltip', { timeout: 6000 }).should('be.visible');
});

it('Verify isAriaDisabled button prevents default actions', () => {
Expand Down
8 changes: 4 additions & 4 deletions packages/react-integration/cypress/integration/overflowmenu.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ describe('OverflowMenu Demo Test', () => {
it('Verify dropdown menu expanded', () => {
cy.get('#simple-overflow-menu button').last().click({ force: true });
cy.get('#simple-overflow-menu .pf-v6-c-menu-toggle').should('have.class', 'pf-m-expanded');
cy.get('.simple-overflow-menu.pf-v6-c-menu').should('be.visible');
cy.get('.simple-overflow-menu.pf-v6-c-menu', { timeout: 6000 }).should('be.visible');
// close overflow menu again
cy.get('#simple-overflow-menu button').last().click({ force: true });
});
Expand Down Expand Up @@ -69,7 +69,7 @@ describe('OverflowMenu Demo Test', () => {
it('Verify dropdown menu expanded', () => {
cy.get('#additional-options-overflow-menu button').last().click({ force: true });
cy.get('#additional-options-overflow-menu .pf-v6-c-menu-toggle').should('have.class', 'pf-m-expanded');
cy.get('.additional-options-overflow-menu.pf-v6-c-menu').should('be.visible');
cy.get('.additional-options-overflow-menu.pf-v6-c-menu', { timeout: 6000 }).should('be.visible');
});
});
});
Expand Down Expand Up @@ -107,7 +107,7 @@ describe('OverflowMenu Demo Test', () => {
it('Verify dropdown menu expanded', () => {
cy.get('#persist-overflow-menu button').last().click({ force: true });
cy.get('#persist-overflow-menu .pf-v6-c-menu-toggle').should('have.class', 'pf-m-expanded');
cy.get('.persist-overflow-menu.pf-v6-c-menu').should('be.visible');
cy.get('.persist-overflow-menu.pf-v6-c-menu', { timeout: 6000 }).should('be.visible');
});
});
});
Expand Down Expand Up @@ -142,7 +142,7 @@ describe('OverflowMenu Demo Test', () => {
it('Verify dropdown menu expanded', () => {
cy.get('#container-breakpoint-overflow-menu button').last().click({ force: true });
cy.get('#container-breakpoint-overflow-menu .pf-v6-c-menu-toggle').should('have.class', 'pf-m-expanded');
cy.get('.container-breakpoint-overflow-menu.pf-v6-c-menu').should('be.visible');
cy.get('.container-breakpoint-overflow-menu.pf-v6-c-menu', { timeout: 6000 }).should('be.visible');
// close overflow menu again
cy.get('#container-breakpoint-overflow-menu button').last().click({ force: true });
});
Expand Down
3 changes: 2 additions & 1 deletion packages/react-integration/cypress/integration/tabsdisable.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ describe('Disabled Tab Demo Test', () => {

it('Verify aria-disabled with tooltip', () => {
cy.get(withTooltip.button).trigger('mouseover');
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');
});
});
Loading