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 7 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 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
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.

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?

/**
* 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.

});

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/searchinput.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ describe('Search Input Demo Test', () => {
it('Verify advanced search and its handlers work', () => {
cy.get('#enabled-search .pf-v6-c-panel').should('not.exist');
cy.get('#enabled-search .pf-v6-c-input-group .pf-v6-c-input-group__item > button').eq(0).click();
cy.get('#enabled-search .pf-v6-c-panel').should('be.visible');
// Tooltip visibility is async due to requestAnimationFrame-based positioning
cy.get('#enabled-search .pf-v6-c-panel', { timeout: 6000 }).should('be.visible');

cy.get('#enabled-search .pf-v6-c-form-control > input').eq(0).type('test');
cy.get('#enabled-search .pf-v6-c-text-input-group__text-input').should('have.value', 'username:test');
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