-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
Popover: remove click-outside and rewrite the lifecycle logic (try 2) #38279
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~8448 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Async-loaded Components (~11 bytes removed 📉 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
There are two popovers nested inside each other 🤔 And clicking inside the inner one means clicking outside the outer one. I'll check why the previous version handled this correctly. Thanks for testing and catching this! |
e7c393e
to
36deb02
Compare
@blowery That sites dropdown inside inline help that closes the popover when it shoudn't -- that's a great catch! First, there are no nested After selecting a site inside the dropdown, there are two
In the original But with the new First, the site selection is handled and the Second, the clickout handler runs and the I fixed this bug in 36deb02 by running the clickout listener in the capture phase -- before any other events (that run in the bubble phase). The right order is established again. Please test and review 🙂 |
36deb02
to
06cfc9d
Compare
Today I fixed one more subtle bug that I discovered when testing @sgomes' Publicize changes in #37078. There are two nested popovers there: And the render code looks somewhat like this: const [ showOuterPopover, setOuterPopover ] = useState( false );
const [ showInnerPopover, setInnerPopover ] = useState( false );
const anchorRef = useRef();
return (
<>
{ showOuterPopover && <Popover>
<OuterPopoverContent
anchorRef={ anchorRef }
toggleInnerPopover={ setInnerPopover }
/>
</Popover> }
<Popover isVisible={ showInnerPopover } context={ anchorRef.current } >
<InnerPopoverContent />
</Popover>
</>
); The However, when mounting this UI, the inner popover is mounted first, because its render is not conditional, but rather uses the I refactored the |
…omponent The `Popover` component uses `_.defer` to delay certain tasks, like reposition on rerender or focus after show, to the next event loop ticks. However, in certain edge cases (that happen often enough) the `Popover` can be hidden or unmounted before these tasks have a chance to run. In such case, they try to work with a DOM element that no longer exist and can crash. This patch carefully tracks these deferred tasks and cancels them when the `Popover` is being hidden or unmounted.
…g chat The Inline Help Popover automatically closes when Happychat opens. This patch moves the code that does the detection and closing from the legacy `componentWillReceiveProps` to `componentDidUpdate`, which is more appropriate for firing side effects. Accompanies the fix for `Popover` update scheduling.
The code that syncs the `isVisible` prop and `show` state, possibly delayed by `showDelay` timeout, is extracted to the outer component, simplifying the inner implementation a lot (removes approx 50 lines of code). Also ensures that the `RootChild` is rendered (and its `div` element appended to DOM) only after the `Popover` becomes visible. That ensures that when the last of several nested `Popover`s is shown, it's also the last DOM element in the DOM tree, and therefore has higher natural z-index and is displayed over the earlier one.
06cfc9d
to
04f63e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Stats, the contact us popover, and the post scheduling popover all work as expected.
Second attempt at landing the
Popover
refactoring from #37766 that was reverted in #38229.The problem was that the deferred (
_.defer
) calls to set position after component rerender (componentDidUpdate
) were not cancelled when thePopover
component was hidden or unmounted immediately after that rerender. I.e., before the deferred call had a chance to run. Then the deferred callback tried to access a DOM element ref that no longer existed.I know about two practical cases where this happens:
Stats Chart: when hovering over an empty chart bar, the popover is shown on
mouseenter
event, then shows right under the mouse cursor, which triggers amouseleave
event and the popover is hidden. Very quickly. That's a preexisting bug that triggers the "reposition after hide" condition.Here's a screencast that shows the "appear-under-cursor" phenomenon. The hiding event is disabled:
Opening Happychat: Here, click on a "Chat with us" button dispatches a Redux action that opens Happychat, and the Inline Help popover also listens for change in the result of
isHappyChat
Redux selector and closes itself when Happychat appears:Again, rerender of the
InlineHelpPopover
component and its unmount happen very quickly after each other.In the case of Inline Help Popover, however, I'm not 100% sure what exactly is happening. When reasoning about the code, I don't see why crash of the deferred call should prevent opening Happychat. But I can't reproduce the Happychat-not-opening bug on the updated branch.
How I fixed it: In the
Popover
component, I added code that carefully cancels all outstanding deferred calls when thePopover
is hidden or unmounted.Also, in
InlineHelpPopover
, I moved the code that closes the popover fromcomponentWillReceiveProps
(deprecated method) tocomponentDidUpdate
(more appropriate for firing side effects). That should be another safety belt that ensures that the lifecycles execute properly.