Skip to content

Commit 2d2e652

Browse files
committed
fixup: sync dedup ref during render to close descendant layout-effect gap
Addresses @nrps9909's review on #622. `lastDispatchedOpenRef` was synchronized to a newly committed `rawOpen` inside Trigger's own `useLayoutEffect`. React runs descendant layout effects *before* their parent's on the same commit, so if a target component's `useLayoutEffect([open], () => target.blur())` reached `internalTriggerOpen` during that window, the dedup ref still held the previous value. A legitimate opposite dispatch would then look like a duplicate and be dropped — `onOpenChange` would silently never fire even though the parent had accepted the controlled prop change. Move the sync into the render body. Refs are writable during render; the only race — a discarded concurrent render leaving a stale ref — cannot suppress a real dispatch, because every real dispatch also writes `nextOpen` to the ref. Adds `tests/layout-effect-ordering.test.tsx` covering the scenario described in the review: controlled `hideAction={['focus']}`, focus the target, rerender `popupVisible=false -> true`, and have a descendant layout effect fire `fireEvent.blur(target)`. Expect `onOpenChange` called once with `false`. The test fails on the previous fix head (0 callbacks) and passes with this change (1 callback). Full suite: 19 suites / 136 tests (+1 skipped). Refs #622 (review)
1 parent 29ec54e commit 2d2e652

2 files changed

Lines changed: 157 additions & 10 deletions

File tree

src/index.tsx

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -387,7 +387,7 @@ export function generateTrigger(
387387

388388
// Track the last synchronously dispatched `nextOpen` so multiple events
389389
// firing in the same batch (e.g. `pointerenter` + `focus`, or `pointerleave`
390-
// + `blur`) only emit one `onOpenChange`. We can't read `mergedOpen` here
390+
// + `blur`) only emit one `onOpenChange`. We can't read `rawOpen` here
391391
// because React state updates are async — within a single batch the second
392392
// call would still see the stale value. A simple `useRef` avoids that
393393
// without requiring `flushSync`, which would emit a React 19 warning when
@@ -397,16 +397,21 @@ export function generateTrigger(
397397
// See https://github.com/ant-design/ant-design/issues/57789
398398
const lastDispatchedOpenRef = React.useRef(rawOpen);
399399

400-
// Keep the ref in sync with `rawOpen` after each render so that
401-
// controlled updates from outside (or any internal state change that
402-
// already committed) reset the dedup baseline. This preserves the
403-
// behaviour fixed in #601 where the dedup state could leak across user
404-
// interactions in controlled mode without re-renders. We track `rawOpen`
405-
// rather than `mergedOpen` so that toggling `disabled` doesn't get
406-
// treated as an "external" open change and re-fire the callbacks.
407-
useLayoutEffect(() => {
400+
// Sync the dedup baseline to `rawOpen` **in render body**, not in a layout
401+
// effect. React runs descendant layout effects *before* their parent's, so
402+
// if we synced in Trigger's own `useLayoutEffect([rawOpen])` a descendant
403+
// effect (e.g. `useLayoutEffect([open], () => target.blur())`) could reach
404+
// `internalTriggerOpen` while the ref still held the previous value and
405+
// legitimately-different callbacks would be discarded as duplicates. Doing
406+
// the sync during render closes that gap. It's safe: refs are mutable
407+
// during render and the only race — a concurrent render being discarded
408+
// with a stale ref — cannot suppress a real dispatch because any real
409+
// dispatch also writes the ref back to `nextOpen`. Tracks `rawOpen` (not
410+
// `mergedOpen`) so toggling `disabled` doesn't re-fire callbacks.
411+
// https://github.com/react-component/trigger/pull/622#pullrequestreview-...
412+
if (lastDispatchedOpenRef.current !== rawOpen) {
408413
lastDispatchedOpenRef.current = rawOpen;
409-
}, [rawOpen]);
414+
}
410415

411416
const internalTriggerOpen = useEvent((nextOpen: boolean) => {
412417
if (lastDispatchedOpenRef.current !== nextOpen) {
Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
/**
2+
* Regression coverage for the layout-effect ordering gap flagged in the
3+
* #622 review by @nrps9909.
4+
*
5+
* The dedup baseline (`lastDispatchedOpenRef`) used to be synchronized inside
6+
* Trigger's own `useLayoutEffect([rawOpen])`. React runs descendant layout
7+
* effects *before* their parent's, so during a render that flipped
8+
* `popupVisible` a descendant `useLayoutEffect` could reach
9+
* `internalTriggerOpen` while the ref still held the previous, stale value —
10+
* a legitimate opposite dispatch would then be discarded as a duplicate and
11+
* `onOpenChange` would never fire.
12+
*
13+
* The fix synchronizes the ref during render, so descendant layout effects
14+
* see the up-to-date baseline.
15+
*
16+
* Concrete scenario from the review:
17+
*
18+
* 1. Render a controlled `<Trigger hideAction={['focus']} popupVisible={false}>`
19+
* and focus the target.
20+
* 2. Rerender with `popupVisible={true}`.
21+
* 3. In the target component's `useLayoutEffect([open])`, call `target.blur()`.
22+
* 4. Assert focus actually left the target *and* `onOpenChange(false)` fired
23+
* exactly once.
24+
*
25+
* Before the fix: focus leaves but the callback count is 0.
26+
* After the fix: the callback fires once.
27+
*/
28+
import { act, cleanup, fireEvent, render } from '@testing-library/react';
29+
import { spyElementPrototypes } from '@rc-component/util/lib/test/domHook';
30+
import * as React from 'react';
31+
import Trigger from '../src';
32+
33+
const flush = async () => {
34+
for (let i = 0; i < 10; i += 1) {
35+
act(() => {
36+
jest.runAllTimers();
37+
});
38+
await act(async () => {
39+
await Promise.resolve();
40+
});
41+
}
42+
};
43+
44+
describe('Trigger.LayoutEffectOrdering (#622 review)', () => {
45+
let eleRect = { width: 100, height: 100 };
46+
let spanRect = { x: 0, y: 0, left: 0, top: 0, width: 1, height: 1 };
47+
let popupRect = { x: 0, y: 0, left: 0, top: 0, width: 100, height: 100 };
48+
49+
beforeAll(() => {
50+
spyElementPrototypes(HTMLElement, {
51+
clientWidth: { get: () => eleRect.width },
52+
clientHeight: { get: () => eleRect.height },
53+
offsetWidth: { get: () => eleRect.width },
54+
offsetHeight: { get: () => eleRect.height },
55+
offsetParent: { get: () => document.body },
56+
});
57+
spyElementPrototypes(HTMLDivElement, {
58+
getBoundingClientRect() {
59+
return popupRect;
60+
},
61+
});
62+
spyElementPrototypes(HTMLSpanElement, {
63+
getBoundingClientRect() {
64+
return spanRect;
65+
},
66+
});
67+
});
68+
69+
beforeEach(() => {
70+
eleRect = { width: 100, height: 100 };
71+
spanRect = { x: 0, y: 0, left: 0, top: 0, width: 1, height: 1 };
72+
popupRect = { x: 0, y: 0, left: 0, top: 0, width: 100, height: 100 };
73+
jest.useFakeTimers();
74+
});
75+
76+
afterEach(() => {
77+
cleanup();
78+
jest.useRealTimers();
79+
});
80+
81+
it('accepts an opposite dispatch from a descendant layout effect after the parent commits a controlled open change', async () => {
82+
const onOpenChange = jest.fn();
83+
84+
// Target that runs a layout effect on every `open` transition. When
85+
// `open` becomes true it blurs itself synchronously — this executes
86+
// *before* Trigger's own layout effects on the same commit, which is
87+
// exactly the ordering window the original PR head mishandled.
88+
// We fire a real blur event on the DOM node (not just `HTMLElement.blur()`)
89+
// to ensure Trigger's `onBlur` handler runs under jsdom.
90+
const Target = React.forwardRef<
91+
HTMLSpanElement,
92+
{ open: boolean } & React.HTMLAttributes<HTMLSpanElement>
93+
>(({ open, ...rest }, forwardedRef) => {
94+
const localRef = React.useRef<HTMLSpanElement>(null);
95+
React.useImperativeHandle(forwardedRef, () => localRef.current!);
96+
React.useLayoutEffect(() => {
97+
if (open && localRef.current) {
98+
fireEvent.blur(localRef.current);
99+
}
100+
}, [open]);
101+
// Forward any Trigger-injected handlers (onFocus/onBlur/etc.) onto
102+
// the underlying span; without this, Trigger's `onBlur` never fires
103+
// and the ordering gap can't be exercised.
104+
return <span {...rest} className="target" ref={localRef} tabIndex={0} />;
105+
});
106+
107+
const Harness: React.FC<{ open: boolean }> = ({ open }) => (
108+
<Trigger
109+
action={[]}
110+
hideAction={['focus']}
111+
popup={<strong>popup</strong>}
112+
popupVisible={open}
113+
onOpenChange={onOpenChange}
114+
>
115+
<Target open={open} />
116+
</Trigger>
117+
);
118+
119+
const { container, rerender } = render(<Harness open={false} />);
120+
const target = container.querySelector('.target') as HTMLSpanElement;
121+
122+
act(() => {
123+
fireEvent.focus(target);
124+
});
125+
await flush();
126+
127+
onOpenChange.mockClear();
128+
129+
// Parent commits false -> true. The descendant layout effect fires blur
130+
// *during that commit*, before Trigger's own effects could have synced
131+
// the dedup ref. With the render-body sync, Trigger sees the up-to-date
132+
// baseline (`rawOpen === true`) and treats the blur-driven dispatch as
133+
// a real transition to false.
134+
act(() => {
135+
rerender(<Harness open />);
136+
});
137+
await flush();
138+
139+
expect(onOpenChange).toHaveBeenCalledTimes(1);
140+
expect(onOpenChange).toHaveBeenLastCalledWith(false);
141+
});
142+
});

0 commit comments

Comments
 (0)