Popover: remove click-outside and rewrite the lifecycle logic

[jsnajdr]

This PR attempts to untangle the spaghetti the Popover component has become over the years, and ends up as a major rewrite. There are many changes at once, and because everything is related to everything else, I was unable to isolate the changes into individual commits.

The result is a comprehensible Popover component that is ready for inclusion in @automattic/components and reconciliation with Core.

Remove click-outside npm package
This is ancient (2014) component that's used only in Popover and is very easy to replace: add a click listener to document, use Node.contains to check if the event.target is outside the popover's element (stored as ref), and execute a handler if it is outside. See the (un)bindClickoutHandler and onClickout methods.

Move RootChild to a wrapper component
Wrapping component's children in RootChild has some subtle effects on how lifecycle methods and ref callbacks are called. Consider the following:

class WithRootChild extends Component {
  componentDidMount() {
    console.log( 'WithRootChild cdm called' );
  }

  setRef = () => {
    console.log( 'WithRootChild ref set' );
  }

  render() {
    return <RootChild><div ref={ this.setRef } /></RootChild>;
  }
}

class WithoutRootChild extends Component {
  componentDidMount() {
    console.log( 'WithoutRootChild cdm called' );
  }

  setRef = () => {
    console.log( 'WithoutRootChild ref set' );
  }

  render() {
    return <div ref={ this.setRef } />;
  }
}

When using them, you'll see that in WithoutRootChild, the lifecycle method can rely on the ref being set and work with the DOM element:

WithoutRootChild ref set
WithoutRootChild cdm called

But WithRootChild changes the order:

WithRootChild cdm called
WithRootChild ref set

That's because the JSX in RootChild's children is not rendered immediately, but waits until the container div is created and then a portal renders into the container in RootChild's componentDidMount. The swapped order makes working with DOM elements in lifecycle methods pretty much impossible. Creating a self-contained PopoverInner and wrapping it in RootChild makes things normal again.

Get rid of setDOMBehavior method
There's this weird setDOMBehavior method that is used as a ref callback, and yet goes way beyond simply storing a ref: it calls setState by virtue of calling setPosition, focuses the popover element... It was the only way to do these things, because the didMount/didUpdate lifecycles fired too early due to RootChild, but now we can implement these things normally: use standard React.createRefs and do all effects (resize, move, focus) in lifecycles.

Don't use requestAnimationFrame if not needed
When doing React updates (setState) in event listeners (keydown, window scroll or resize), we don't need to defer them with requestAnimationFrame. Doing everything synchronously in the event handler is fine.

Don't set generated id unless explicitly specified by the user
The current Popover adds a generated pop__1234 ID to every popover element, which I believe is not necessary. We don't use it to connect multiple elements together with ARIA attributes or anything like that.

[matticbot]

Test live: https://calypso.live/?branch=update/popover-dom-behavior

[matticbot]

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~8465 bytes removed 📉 [gzipped])

name                 parsed_size           gzip_size
entry-main              -10965 B  (-0.7%)    -2807 B  (-0.7%)
entry-login             -10965 B  (-1.0%)    -2815 B  (-1.0%)
entry-gutenboarding     -10965 B  (-0.4%)    -2843 B  (-0.4%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

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.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

[sgomes]

Thanks for working on this, @jsnajdr! This is a foundational component that can definitely use some love.

I added some high-level comments on the implementation before a more thorough review.

[sgomes]

scroll and resize events generally always need debouncing/throttling, because in some browsers they are emitted at ridiculous rates that can slow everything down. That may not be a concern if the methods are very simple, but here beyond setting state (which React may offer some amount of buffer for), there are things like measuring nodes on the DOM.

I'd really recommend debouncing/throttling this.

[jsnajdr]

scroll and resize, at least in modern browsers, fire at most once during an animation frame. So, requestAnimationFrame doesn't throttle anything. Are there browsers that fire more often than that? What debouncing delay would you recommend?

[sgomes]

Yes, you're right about modern browsers, they shouldn't emit events too often. To double-check, I tested Chrome, Safari, and Firefox, and none of them seem to emit the resize event more often than once per frame. Old browsers are generally the issue for these things.

However, we don't necessarily want to be emitting these events once per frame anyway. For resize, you're probably going to be blowing the frame budget anyway, as Calypso doesn't resize very smoothly. And scroll handlers rarely go smoothly, too, at least unless they're passive. Chrome will probably activate an intervention and force the scroll handler to be passive, but as far as I know other browsers won't do the same, and it's not easy to feature-detect support for passive event listeners.

I tend to act defensively in generic, foundational components like this one, because you can never be sure what context they're going to be used in, how big of a sub-tree they're going to have, etc.

That said, I'll leave it to your judgment :) If you haven't noticed any runtime performance issues with the updated component then it's probably fine? Debouncing can be added in later if needed.

[jsnajdr]

When I try throttling or debouncing the scroll handler, it has some unpleasant visual effects: the popover repositioning is very visibly lagging behind the scroll:

Therefore, I'd rather keep the synchronous callback, even though it can raise your CPU usage when scrolling. And work toward eliminating the repositioning handler completely in near future.

[sgomes]

Sounds good, thank you for looking into this! 👍

[sgomes]

Any idea why the position needs to be recalculated on scroll? Is this meant to work with scrolling elements other than the document body? If not, fixing popup position to the page rather than the screen could remove the need for this altogether.

[blowery]

It likely needs to be repositioned to ensure the popover remains on screen, if possible. As an element moves towards the top of the screen, the popover may need to be moved below the element to remain in view.

[jsnajdr]

The popover element is a child of body, and is absolutely positioned relative to the viewport. Not relative to the element that is scrolling. That's why it needs to be repositioned on every scroll. I agree that's suboptimal and that we could position the popover in such a way that it scrolls together with the content that it's attached to. But that's for another awesome PR 😉

[sgomes]

Sounds like an excellent future enhancement, if it's doable 🙂

[jsnajdr]

Now, when testing, I discovered that it's the other way around: the whole body is scrolling, so popovers are always properly attached even without the reposition scroll handler.

It's the static elements, sidebar and masterbar, that need this onscroll repositioning.

[sgomes]

Ah, that's interesting! This definitely complicates things.

If I were designing this component from scratch, I'd probably have the component require a prop that indicates whether to attach itself to the body or to the window. This would be used to toggle some CSS classes that either made it position: absolute or position: fixed, with the portal as a direct child of the body. That way neither scenario would require scroll listeners, but as a downside it would require the consumer to know what it wants.

Still, that's a discussion for a future PR, if ever 🙂

[sgomes]

Will this not invalidate layout, because of the transient computePosition call? If I recall correctly, componentDidUpdate is a good time for DOM manipulations, but not a great time for DOM reads.

[jsnajdr]

I think it's the opposite: good time for DOM reads, because the layout is already done and reading from DOM doesn't trigger any flush. If we write to DOM at this time, a second layout will be triggered, but there's no way around it: we need a two-pass layout here. First we layout the popover, and in the second pass we measure its layout and decide about the final layout (positioning) there.

In your #37489, you're postponing DOM reads to componentDidMount, where previously they were reading at the time when ref callbacks are running. Which, according to your profiling results, is too early and causes layout trashing.

[sgomes]

Right, the ref callback time is another thing altogether, which I'm not sure exactly how it fit. But regardless of that, you're right that in #37489 using the componentDidMount hook seemed to work well for DOM reads, which means I was confused, yup.

The two-pass layout is a shame, but a common issue in these types of components.

In any case, I'll give this a performance look-over with the full review. Teaches me to start writing comments before I measure things 🙂

[jsnajdr]

This will be interesting also for @ockham, author of #11054 🙂

[gwwar]

Before we try a second attempt on the refactor I think it'd be worth adding an E2E for the help contact popup.

[KokkieH]

@jsnajdr We've noticed an increase in forum threads being submitted via https://wordpress.com/help/contact that get created in the forums without any "Site I need help with" link. Previously it would have automatically indicated the primary site in someone's account (though we also have a very old enhancement request to have an explicit site selector added for free users in #16015).

This is the only recent PR I can find that mentions the support contact form, and the timing exactly matches when we started seeing this issue in the forums. Could your changes here have inadvertently caused this?

With the pop-up contact form this isn't an issue, as that merely shows a button that takes free users directly to the forums to create their thread, unlike the form at https://wordpress.com/help/contact that essentially creates a thread for them.

[jsnajdr]

@KokkieH This PR has been reverted on Friday (4 days ago), so if the issue is still present today, it can't be caused by this PR. I can try to investigate a bit what might be the cause.

[KokkieH]

Oops, missed the note mentioning the revert. Sorry :)

If you have the bandwidth to check what's going on here everyone in Chiron (and all our forum volunteers) will be in your debt :)