Skip to content

fix: use measurement clone to avoid disrupting popup animations during alignment #612

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
AnthonyH16 wants to merge 1 commit into
base: master
Choose a base branch
from
+107 −38
Open

fix: use measurement clone to avoid disrupting popup animations during alignment #612

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
75 changes: 37 additions & 38 deletions src/hooks/useAlign.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,35 +177,38 @@ export default function useAlign(
const doc = popupElement.ownerDocument;
const win = getWin(popupElement);

const { position: popupPosition } = win.getComputedStyle(popupElement);

const originLeft = popupElement.style.left;
const originTop = popupElement.style.top;
const originRight = popupElement.style.right;
const originBottom = popupElement.style.bottom;
const originOverflow = popupElement.style.overflow;

// Placement
const placementInfo: AlignType = {
...builtinPlacements[placement],
...popupAlign,
};

// placeholder element
const placeholderElement = doc.createElement('div');
popupElement.parentElement?.appendChild(placeholderElement);
placeholderElement.style.left = `${popupElement.offsetLeft}px`;
placeholderElement.style.top = `${popupElement.offsetTop}px`;
placeholderElement.style.position = popupPosition;
placeholderElement.style.height = `${popupElement.offsetHeight}px`;
placeholderElement.style.width = `${popupElement.offsetWidth}px`;

// Reset first
popupElement.style.left = '0';
popupElement.style.top = '0';
popupElement.style.right = 'auto';
popupElement.style.bottom = 'auto';
popupElement.style.overflow = 'hidden';
// Use a temporary measurement element instead of modifying the popup
// directly. This avoids disrupting CSS animations (transform, transition)
// that may be active on the popup during entrance motion.
// On some platforms (notably Linux), the alignment calculation runs
// mid-animation. Modifying popup styles (left/top/transform) interferes
// with active CSS transitions (transition: all) and transforms
// (transform: scale()), causing getBoundingClientRect() to return
// incorrect values and producing wildly wrong popup positions.
const measureEl = popupElement.cloneNode(false) as HTMLElement;
// Copy layout dimensions to the clone since cloneNode(false) produces
// an empty element whose size would otherwise collapse to 0x0.
// Use offsetWidth/offsetHeight instead of getComputedStyle because
// computed styles may return empty during the initial render cycle.
measureEl.style.width = `${popupElement.offsetWidth}px`;
measureEl.style.height = `${popupElement.offsetHeight}px`;
measureEl.style.left = '0';
measureEl.style.top = '0';
measureEl.style.right = 'auto';
measureEl.style.bottom = 'auto';
measureEl.style.overflow = 'hidden';
measureEl.style.transform = 'none';
measureEl.style.transition = 'none';
measureEl.style.animation = 'none';
measureEl.style.visibility = 'hidden';
measureEl.style.pointerEvents = 'none';
popupElement.parentElement?.appendChild(measureEl);
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 24, 2026

Choose a reason for hiding this comment

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

medium

To prevent DOM leaks if an error occurs during alignment, it's best practice to wrap the measurement logic that uses the temporary measureEl in a try...finally block, ensuring removeChild is always called.

For example:

      popupElement.parentElement?.appendChild(measureEl);
      let popupMirrorRect: DOMRect;

      try {
        // ... measurement logic ...
        popupMirrorRect = measureEl.getBoundingClientRect();
      } finally {
        popupElement.parentElement?.removeChild(measureEl);
      }

      // ... rest of the function ...

Note that popupMirrorRect would need to be declared as let outside the try block to be accessible later.


// Calculate align style, we should consider `transform` case
let targetRect: Rect;
Expand All @@ -227,7 +230,9 @@ export default function useAlign(
height: rect.height,
};
}
const popupRect = popupElement.getBoundingClientRect();
// Measure from the temporary element (not affected by CSS transforms
// or transitions on the popup).
const popupRect = measureEl.getBoundingClientRect();
const { height, width } = win.getComputedStyle(popupElement);
popupRect.x = popupRect.x ?? popupRect.left;
popupRect.y = popupRect.y ?? popupRect.top;
Expand Down Expand Up @@ -281,22 +286,16 @@ export default function useAlign(
? visibleRegionArea
: visibleArea;

// Record right & bottom align data
popupElement.style.left = 'auto';
popupElement.style.top = 'auto';
popupElement.style.right = '0';
popupElement.style.bottom = '0';

const popupMirrorRect = popupElement.getBoundingClientRect();
// Record right & bottom align data using measurement element
measureEl.style.left = 'auto';
measureEl.style.top = 'auto';
measureEl.style.right = '0';
measureEl.style.bottom = '0';

// Reset back
popupElement.style.left = originLeft;
popupElement.style.top = originTop;
popupElement.style.right = originRight;
popupElement.style.bottom = originBottom;
popupElement.style.overflow = originOverflow;
const popupMirrorRect = measureEl.getBoundingClientRect();

popupElement.parentElement?.removeChild(placeholderElement);
// Clean up measurement element (popup styles were never modified)
popupElement.parentElement?.removeChild(measureEl);

// Calculate scale
const scaleX = toNum(
Expand Down
70 changes: 70 additions & 0 deletions tests/align.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,4 +333,74 @@ describe('Trigger.Align', () => {
}),
);
});

// https://github.com/react-component/trigger/issues/XXX
it('should not modify popup styles during alignment measurement', async () => {
// On some platforms (notably Linux), the alignment calculation runs
// mid-CSS-animation. The fix uses a temporary measurement element
// instead of modifying the popup's styles, so CSS animations
// (transform, transition) are never disrupted.

render(
<Trigger
popupVisible
popup={<span className="popup-content" />}
popupAlign={{
points: ['tl', 'bl'],
}}
>
<div className="trigger-target" />
</Trigger>,
);

await awaitFakeTimer();

const popupElement = document.querySelector(
'.rc-trigger-popup',
) as HTMLElement;
expect(popupElement).toBeTruthy();

// Spy on popup style mutations during alignment using property setter
// spies (catches both direct assignment and setProperty)
const styleChanges: string[] = [];
const propsToWatch = ['left', 'top', 'transform', 'transition', 'overflow'];
const restoreSpies: (() => void)[] = [];

propsToWatch.forEach((prop) => {
const descriptor = Object.getOwnPropertyDescriptor(
CSSStyleDeclaration.prototype,
prop,
);
if (descriptor?.set) {
const origSet = descriptor.set;
Object.defineProperty(popupElement.style, prop, {
set(value: string) {
styleChanges.push(prop);
origSet.call(this, value);
},
get: descriptor.get,
configurable: true,
});
restoreSpies.push(() => {
Object.defineProperty(popupElement.style, prop, descriptor);
});
}
});

// Trigger re-alignment
triggerResize(popupElement);
await awaitFakeTimer();

// Restore original property descriptors
restoreSpies.forEach((restore) => restore());

// The popup's styles should not have been modified directly during
// measurement (only the final positioning values should be applied
// via the React state update, not during the measurement phase)
expect(styleChanges).not.toContain('left');
expect(styleChanges).not.toContain('top');
expect(styleChanges).not.toContain('transform');
expect(styleChanges).not.toContain('transition');
expect(styleChanges).not.toContain('overflow');
Comment on lines +402 to +404
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 24, 2026

Choose a reason for hiding this comment

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

medium

To make this test more comprehensive and future-proof, consider also asserting that left and top styles are not modified during measurement. The previous implementation modified these properties, and ensuring they are not touched directly is a key part of this fix.

    expect(styleChanges).not.toContain('left');
    expect(styleChanges).not.toContain('top');
    expect(styleChanges).not.toContain('transform');
    expect(styleChanges).not.toContain('transition');
    expect(styleChanges).not.toContain('overflow');

});
});