Repository navigation
fix: allow Menu to reopen when hide animation callback never fires - #5051
ErfanBagheri404 wants to merge 1 commit into
Conversation
| setRendered(false); | ||
| prevRendered.current = false; | ||
| focusFirstDOMNode(anchorRef.current); | ||
| }); |
There was a problem hiding this comment.
I suppose, moving prevRendered.current = false before hide animation can introduce a race. if the menu closes while its opening animation is running, the interrupted opening callback can set this back to true, preventing the menu from reopening
could we handle interrupted opening & hiding animations using finished and add a test for quickly closing & reopening the menu?
}).start(({ finished }) => {
if (!finished) {
return;
}
setMenuLayout({ width: 0, height: 0 });
setRendered(false);
prevRendered.current = false;
focusFirstDOMNode(anchorRef.current);
});
When the menu closes mid-opening animation, the interrupted callback was setting prevRendered.current = true, blocking the next open. Only set it when the animation finishes. Addresses review comment on callstack#5051.
MikitasK
left a comment
There was a problem hiding this comment.
Thanks @ErfanBagheri404 🙏
LGTM 👌
just one non-blocking suggestion: regression test covering close & reopen behavior would be helpful
satya164
left a comment
There was a problem hiding this comment.
according to this comment #4763 (comment) - the issue happens in react native 0.79
but the pr says React Native >= 0.80. can you verify that you're fixing the same issue?
also please provide before/after videos.
| @@ -379,7 +387,6 @@ const Menu = ({ | |||
| }).start(() => { | |||
| setMenuLayout({ width: 0, height: 0 }); | |||
| setRendered(false); | |||
| prevRendered.current = false; | |||
| focusFirstDOMNode(anchorRef.current); | |||
| }); | |||
There was a problem hiding this comment.
resetting prevRendered.current but leaving everything else like rendered state and focus restoration doesn't make sense and leaves the menu in a broken state if the callback is never delivered.
though the callback not firing sounds like a big regression in react native so a lot of other things should be broken
c47786e to
3bfeb90
Compare
The hide animation's completion callback is not delivered on React Native >= 0.80, so setRendered(false) never ran and the portal stayed mounted after the menu closed, with the anchor never regaining focus. Reset the portal and focus synchronously in hide(), cancel any in-flight show animation to avoid a late finished callback re-marking the menu as shown, and keep the layout cleanup in the completion callback guarded against reopen.
|
On the version question: the reports in the thread are consistent about the boundary. bjeld hit it after upgrading to RN 0.80 with paper 5.14.5 and downgrading to 0.79.4 fixed it, atheck and adamgajzlerowicz both on 0.80.1, raiyanihiren on 0.81. Nobody reported it below 0.80, so "React Native >= 0.80" matches what users are seeing. The PR is rebased onto the current main now that the Menu component was rewritten to Reanimated. In that rewrite the reopen deadlock is gone, but the broken state you described is still there and is what this fixes now: when the hide callback is not delivered, I can't record before/after videos myself, no RN setup here to film one. The close-reopen regression test in the PR covers the visible behavior; if a video is still a hard requirement for merging, someone with a 0.80 device would need to record it. |
Fixes #4763
On React Native >= 0.80 the hide animation completion callback is not delivered, so prevRendered.current stayed true and the menu could never be shown again after being dismissed.
Reset prevRendered.current before starting the hide animation instead of in its completion callback.