Skip to content

Commit d7767a0

Browse files
committed
fix: address review feedback
- Copy computed width/height to measurement clone to prevent 0x0 dimensions when popup size is content-driven (CodeRabbit) - Use property setter spies in test instead of setProperty proxy to catch both direct assignments and setProperty calls (CodeRabbit) - Add left/top assertions to test for completeness (Gemini)
1 parent 669642c commit d7767a0

2 files changed

Lines changed: 38 additions & 11 deletions

File tree

src/hooks/useAlign.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,11 @@ export default function useAlign(
192192
// (transform: scale()), causing getBoundingClientRect() to return
193193
// incorrect values and producing wildly wrong popup positions.
194194
const measureEl = popupElement.cloneNode(false) as HTMLElement;
195+
// Copy computed dimensions to the clone since cloneNode(false) produces
196+
// an empty element whose size would otherwise collapse to 0x0.
197+
const { height, width } = win.getComputedStyle(popupElement);
198+
measureEl.style.width = width;
199+
measureEl.style.height = height;
195200
measureEl.style.left = '0';
196201
measureEl.style.top = '0';
197202
measureEl.style.right = 'auto';
@@ -226,7 +231,6 @@ export default function useAlign(
226231
// Measure from the temporary element (not affected by CSS transforms
227232
// or transitions on the popup).
228233
const popupRect = measureEl.getBoundingClientRect();
229-
const { height, width } = win.getComputedStyle(popupElement);
230234
popupRect.x = popupRect.x ?? popupRect.left;
231235
popupRect.y = popupRect.y ?? popupRect.top;
232236
const {

tests/align.test.tsx

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -360,22 +360,45 @@ describe('Trigger.Align', () => {
360360
) as HTMLElement;
361361
expect(popupElement).toBeTruthy();
362362

363-
// Spy on popup style mutations during alignment
363+
// Spy on popup style mutations during alignment using property setter
364+
// spies (catches both direct assignment and setProperty)
364365
const styleChanges: string[] = [];
365-
const origSetProperty = popupElement.style.setProperty.bind(popupElement.style);
366-
popupElement.style.setProperty = function(prop: string, val: string, priority?: string) {
367-
styleChanges.push(prop);
368-
return origSetProperty(prop, val, priority || '');
369-
};
366+
const propsToWatch = ['left', 'top', 'transform', 'transition', 'overflow'];
367+
const restoreSpies: (() => void)[] = [];
368+
369+
propsToWatch.forEach((prop) => {
370+
const descriptor = Object.getOwnPropertyDescriptor(
371+
CSSStyleDeclaration.prototype,
372+
prop,
373+
);
374+
if (descriptor?.set) {
375+
const origSet = descriptor.set;
376+
Object.defineProperty(popupElement.style, prop, {
377+
set(value: string) {
378+
styleChanges.push(prop);
379+
origSet.call(this, value);
380+
},
381+
get: descriptor.get,
382+
configurable: true,
383+
});
384+
restoreSpies.push(() => {
385+
Object.defineProperty(popupElement.style, prop, descriptor);
386+
});
387+
}
388+
});
370389

371390
// Trigger re-alignment
372391
triggerResize(popupElement);
373392
await awaitFakeTimer();
374393

375-
// The popup's left/top/transform/transition should not have been
376-
// modified directly during measurement (only the final positioning
377-
// values should be applied via the React state update, not during
378-
// the measurement phase)
394+
// Restore original property descriptors
395+
restoreSpies.forEach((restore) => restore());
396+
397+
// The popup's styles should not have been modified directly during
398+
// measurement (only the final positioning values should be applied
399+
// via the React state update, not during the measurement phase)
400+
expect(styleChanges).not.toContain('left');
401+
expect(styleChanges).not.toContain('top');
379402
expect(styleChanges).not.toContain('transform');
380403
expect(styleChanges).not.toContain('transition');
381404
expect(styleChanges).not.toContain('overflow');

0 commit comments

Comments
 (0)