Add onClose property to Popover

[acelaya]

Closes #1799
Depends on #1793

Add a new onClose mandatory prop to Popover, which is called when clicking away or pressing Escape.

When the native popover API is used, this is controlled via the toggle event. Otherwise, we fall back to useClickAway and useKeyPress(['Escape']).

[robertknight]

I had a query about useFocusAway and popover close fallback behavior.

[robertknight]

If the caller fails to update the local state then the open prop will get out of sync with the actual state. We don't currently have any use cases where the caller should "ignore" the close event, so such an event probably indicates a bug. We could improve this in future by logging a warning perhaps.

[acelaya]

If the caller fails to update the local state then the open prop will get out of sync with the actual state. We don't currently have any use cases where the caller should "ignore" the close event, so such an event probably indicates a bug.

Yeah, this is true. And not only that, in the non-native-popover case, if they fail to update the state, the popover won't actually close at all, but at least the state in that case is still in sync.

I'm not sure what's the solution for this to be honest. Perhaps documenting it for now.

We could improve this in future by logging a warning perhaps.

Do you mean logging a warning if the native popover is closed while open is true?

[robertknight]

I'm not sure what's the solution for this to be honest. Perhaps documenting it for now.

I would suggest to just document it as a required usage pattern for now.

Do you mean logging a warning if the native popover is closed while open is true?

I meant that a warning would be logged if the open prop remained true in the next render after a popover is automatically closed.

One scenario that may turn out to be an issue is if the popover gets automatically re-opened for some reason before a debounced render occurs.

[acelaya]

I meant that a warning would be logged if the open prop remained true in the next render after a popover is automatically closed.

Gotcha

[robertknight]

Should this focus-away behavior also be part of the popover's built-in fallback?

[acelaya]

I guess it would be reasonable, but for now I decided to only mimic what the native popover does by default.

If we see we always end up adding this affordance, in other use cases, we can revisit this decision and add useFocusAway to the Popover.

[acelaya]

These are now covered in Popover-test.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (82734c2) to head (f634ec2).

Additional details and impacted files

@@            Coverage Diff            @@
##              main     #1800   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           68        68           
  Lines         1220      1231   +11     
  Branches       465       467    +2     
=========================================
+ Hits          1220      1231   +11     

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