Fix Popover not opening when not using the native API

[acelaya]

This PR fixes some regressions introduced in #1800, when onClose was implemented, and the logic to close the popover on click away and on Escape press was moved from the Select component to the Popover.

The issues affect only browsers where the native popover API is not supported.

Context

Originally, the popover was part of Select, and useCickAway() was invoked there with the container wrapping both the toggle button and the popover.

After the Popover was extracted as a standalone component, we realized it had click-away capabilities built-in when using the native popover API, so we decided to move useClickAway there as well for consistency.

However, the element passed to useClickAway became the popover only, as it was no longer aware of any toggle or wrapper.

This introduced the first issue fixed here: Clicking the toggle button now triggers a setOpen(true), but also a click-away, which sets it back to false, so the popover could not be opened anymore.

This has been addressed by ensuring useClickAway is enabled only while the popover is open.

But this introduces the second issue: We now have the oposite problem. The popover can be opened, but when trying to close it by clicking the toggle, it first triggers a click-away, closing the popover, and then triggers the click on the toggle button, opening it again immediately after.

This has been addressed by ensuring useClickAway ignores events triggered on the popover's anchor element, which is usually its toggle button.

Considerations

I think the first solution is non-controversial, but the second one is making some assumptions which may not always be correct.

In the Select case, the anchor element is the toggle button, so the solution applied is correct, but in future we may want to anchor popovers to elements which are not triggers, in which case we would want useClickAway to behave the same when clicking on them.

I'm open to suggestions on alternative solutions for the second issue.

Testing steps

The issue was reproducible only when not using the native API, so you first need to apply this diff:

diff --git a/src/components/feedback/Popover.tsx b/src/components/feedback/Popover.tsx
index 9c5ea3a..2dc8534 100644
--- a/src/components/feedback/Popover.tsx
+++ b/src/components/feedback/Popover.tsx
@@ -262,7 +262,7 @@ export default function Popover({
   variant = 'panel',
   restoreFocusOnClose = true,
   /* eslint-disable-next-line no-prototype-builtins */
-  asNativePopover = HTMLElement.prototype.hasOwnProperty('popover'),
+  asNativePopover = false,
 }: PopoverProps) {
   const popoverRef = useRef<HTMLDivElement | null>(null);

Then go to http://localhost:4001/feedback-popover and verify popovers can be opened/closed by clicking the toggle, clicking away or pressing Escape.

Note

There's an unrelated issue when not using the native popover API, in which popovers never grow more than their anchor element. I'm addressing that separately.

[acelaya]

This was duplicated. The fallback logic now lives in Popover and we don't need to define a default value here anymore.

[robertknight]

But this introduces the second issue: We now have the oposite problem. The popover can be opened, but when trying to close it by clicking the toggle, it first triggers a click-away, closing the popover, and then triggers the click on the toggle button, opening it again immediately after.

This problem actually happens with native popovers in Safari at the moment. Clicking the toggle button for a Select while the popover is open hides the popover and then immediately re-opens it. In Firefox and Chrome clicking the toggle button toggles the popover as expected.

If using the native popovertarget atttribute (or corresponding property) this doesn't happen.

<button popovertarget="mypopover" popovertargetaction="toggle">
  Toggle popover
</button>
<div id="mypopover" popover>Popover content</div>

If using the same terminology as the web APIs, then we are saying that we assume the popover is anchored to element that controls it, which is currently always the case. In future we could evolve the API by making it possible to specify a separate control element vs anchor element. I'm not sure if we'll actually need that.

[acelaya]

This problem actually happens with native popovers in Safari at the moment. Clicking the toggle button for a Select while the popover is open hides the popover and then immediately re-opens it.

Good catch. I guess we could do the same check in the toggle event that I introduced here for onClickAway.

If using the native popovertarget atttribute (or corresponding property) this doesn't happen.

<button popovertarget="mypopover" popovertargetaction="toggle">
  Toggle popover
</button>
<div id="mypopover" popover>Popover content</div>

This would be another option, but I would prefer to not introduce this to avoid mixing a controlled and uncontrolled component.

Perhaps we could eventually have an UncontrolledPopover, which does not have a piece of state/props indicating its openness state and only using native attributes and the popover API, and a ControlledPopover one which receives open: boolean and onToggle: (open: boolean) => void.

But let's do that when all browsers we support can use the popover API. Right now it might be a bit complicated to coordinate.

If using the same terminology as the web APIs, then we are saying that we assume the popover is anchored to element that controls it, which is currently always the case. In future we could evolve the API by making it possible to specify a separate control element vs anchor element. I'm not sure if we'll actually need that.

Perfect, let's do that.

[acelaya]

If using the native popovertarget atttribute (or corresponding property) this doesn't happen.

<button popovertarget="mypopover" popovertargetaction="toggle">
  Toggle popover
</button>
<div id="mypopover" popover>Popover content</div>

I tried this, and it breaks the behavior in Chrome and Firefox.

I even tried making sure onClick is not added if those attributes are, but then the popover is opened empty because rendering the children is still based on the open prop.

So basically we need to either transition to a completely different model where popover events are the source of truth that drive state updates, or we do state the source of truth that drives toggling the native popover, but a mix doesn't work. And the later is the only option as long as we support browsers which do not support the popover API.

Unfortunately, my suggestion of doing the same on the toggle event as I did here on click away, does not work either, because the toggle event always triggers on the popover, not the anchor element, so my guess is that a toggle and a click events are being triggered at the same time.

I may need your help debugging this, as I don't have access to a Safari browser.

[robertknight]

I may need your help debugging this, as I don't have access to a Safari browser.

I would suggest not to worry about the Safari issue for this PR, since it isn't a regression. I can take a look next week and try to understand better what the difference is between Chrome/FF and Safari when using native popovers.

[acelaya]

I would suggest not to worry about the Safari issue for this PR, since it isn't a regression.

Yeah, good point.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (a6de8f1) to head (41c7ccb).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main     #1804   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           68        68           
  Lines         1231      1234    +3     
  Branches       467       468    +1     
=========================================
+ Hits          1231      1234    +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.