Popover: Refactor to render using React portals

[aduth]

This pull request seeks to leverage React portals — a new (unstable) feature introduced in React 16 — to render the existing Popover component. The concept of portals has existed previously in the React ecosystem, through works such as React-portal or Calypso's RootChild component. The idea is that the rendering of the component occurs elsewhere in the DOM, usually at the root (body) element. This has a few benefits:

• Styles from parent nodes will not cascade into the popover
• It is immune from phrasing content restrictions (e.g. div cannot be a descendant of button)

See included changes to existing Popover usage in PostVisibility as an example of the simplification permitted by the second of these benefits.

Testing instructions:

Verify that there are no regressions in the behavior of Popover controls, currently present in:

• Header inserter
• Post settings visibility
• Content inserter

Of note, the content inserter should demonstrate "bounds flipping" behavior. It should try to open upwards (on Demo content where scroll exists), else downward if space does not permit (on New post without any content).

Ensure that unit tests pass:

npm test

[codecov]

Codecov Report

Merging #2160 into master will increase coverage by 0.65%.
The diff coverage is 70.31%.

@@            Coverage Diff             @@
##           master    #2160      +/-   ##
==========================================
+ Coverage   24.06%   24.71%   +0.65%     
==========================================
  Files         149      150       +1     
  Lines        4592     4641      +49     
  Branches      775      786      +11     
==========================================
+ Hits         1105     1147      +42     
- Misses       2946     2951       +5     
- Partials      541      543       +2

Impacted Files	Coverage Δ
element/index.js	100% <ø> (ø)	⬆️
editor/inserter/index.js	0% <0%> (ø)	⬆️
editor/sidebar/post-visibility/index.js	0% <0%> (ø)	⬆️
editor/inserter/menu.js	48.41% <100%> (ø)	⬆️
components/popover/detect-outside.js	25% <25%> (ø)
components/popover/index.js	96.66% <95.55%> (+22.75%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee1083a...ecb7c70. Read the comment docs.

[aduth]

It was necessary to move click outside logic into Popover, which I'd intended to do eventually anyways, but was required to do so ahead of schedule only because otherwise the existing click outside handlers were assuming that the portaled content was "outside" and not allowing interaction within (e.g. selecting a block option from inserter menu). See 70b2298.

[youknowriad]

There's some highly engineered code in this PR :)

I noticed a small regression tough in the header inserter when I hit the "insert" button a second time, the popover is not closed.

[aduth]

@youknowriad Yeah, this has turned into a rabbit hole 😬 I noticed your issue as well and flipped to In Progress. There's a few interesting things going on here:

• Events on portaled nodes still bubble to ancestor elements, which is pretty amazing but also unexpected. In case of Post Visibility, the popover is inadvertently closed when clicking a visibility option because the click event is bubbling up to the button, which toggles itself closed. The solution is to stop propagation of the click event from reaching the button (63e21c6)
• With moving "click outside" logic into the popover itself, clicking on a toggle button is considered "outside" the popover, so we'd want the popover to close, but then because we're clicking the toggle button it immediately reopens itself. The solution is to check, when closing by click outside, that the target of the click outside is not the button (b8099cf, ddae0d2)

[aduth]

Noticing positioning was off on mobile, I started down another rabbit hole of fixing styling, noting an interesting gotcha that avoiding the style cascade is that assumptions about box sizing no longer hold true (i.e. need to apply box-sizing yourself to any portaled content). Which makes sense in retrospect, but was still unexpected.

The result of the work hopefully pays off. Compared to master, the height of the header inserter is more correct (5d2f030) and we restore the arrow, correctly positioned under the "+" icon:

master	update/popover-portal

cc @jasmussen for a style audit (and/or criticism on my desktop-first media query 😲 )

[jasmussen]

Visually I think this looks great 👍👍!

and/or criticism on my desktop-first media query

Can you elaborate? ;) Not sure what to look for.

[aduth]

Can you elaborate? ;) Not sure what to look for.

Just a single selector using the inverse of the medium breakpoint since it'd be unrealistic to unset those properties in the reverse case.

[jasmussen]

Ooooh dang, that's controversial! Our mixin is decidedly mobile first and here you go and except to that rule 🤘

(It's fine, I kinda expected we'd need to do this at some point :) )

[aduth]

@youknowriad Pulling out of commit comment, since it'll be lost on rebase:

Should we do this consistently? I mean inside the popover component?

I see it being a common pain point, but at the same time, it should be the expected behavior. If I render a child and bind an event to the parent, I should expect that event occurring on the child to bubble to the parent.

It's in these specific use-cases cases in which we have button toggling mixed with click outside where it gets particularly sticky.

One uncertainty I have on second glance is if we might just apply event handlers to the Popover's anchor (span) node instead of the portal-ed node, since the same bubbling behavior will cause events to occur on the span.