Add popover capabilities to SelectNext listbox

[acelaya]

Depends on #1548

Add popover attribute to the SelectNext listbox, and open it via element.togglePopover() when the browser supports it.

For browsers not supporting the native popover, we continue to handle it as before.

TODO

• Test with a screen reader, making sure the use of [popover] does not have any side effects on how components are announced.
• Add/fix tests
• Update documentation mentioning the popover behavior of the listbox in browsers that support it.

Considerations

• Adding the popover attribute to the listbox makes it render in the top layer, and therefore, it is no longer positioned relative to the toggle button.
  Because of that, we have to manually calculate where should it be based on some heuristics, and absolutely position it there.
• Related with the point above, and because of how useArrowKeyNavigation internally works, I had to disable the automatic option focusing that useArrowKeyNavigation provides, and add an extra toggle event listener that mimics that logic.
  EDIT: This has been solved. See Add popover capabilities to SelectNext listbox #1540 (comment)
• Another side effect of rendering the listbox in the top layer, is that it won't be affected by scrolling on inner elements.
  EDIT: This is now mitigated via listboxAsPopover prop. With that we can keep the listbox as is, and enable the "popover mode" only when really needed.
  EDIT 2: Finally we are capturing the scroll event and re-adjust the listbox positioning dynamically.
• At some point, I considered creating a new Popover component, that can be used in other places, but this is complex enough, without having to think in more abstractions. I will delay that to after this PR has been merged, and we have at least other use case for a popover.

[robertknight]

Adding the popover attribute to the listbox makes it render in the top layer, and therefore, it is no longer positioned relative to the toggle button.
Because of that, we have to manually calculate where should it be based on some heuristics, and absolutely position it there.

I think this is fine. In future you could use CSS anchor positioning for this. That is an extremely recent addition to the web platform, so not viable yet.

Without this, the option gets focused first, when the listbox is still positioned at the very top of the page, making the page scroll up. Then the listbox gets repositioned.

My understanding is that after the listbox is opened, if autofocus is enabled in the useArrowKeyNavigation hook, the sequence of events is:

1. The SelectNext component's layout effect runs and schedules a state update for the position values
2. The useArrowKeyNavigation hook's effect runs and autofocuses its preferred element
3. The SelectNext component is re-rendered. This applies the position value update.

So the problem here is the ordering of steps (2) and (3).

If step (1) updated the DOM style synchronously, then the listbox would have its correct position by the time useArrowKeyNavigation runs.

At some point, I considered creating a new Popover component, that can be used in other places, but this is complex enough, without having to think in more abstractions. I will delay that to after this PR has been merged, and we have at least other use case for a popover.

Yes, that makes sense.

[robertknight]

One other thing that occurred to me is that you could try to make the popover and non-popover use cases more similar by rendering the listbox children into their own element which is positioned at the top-level of the DOM, like how React portals work. The downside is that event bubbling won't work, so you'd either have to emulate it, or warn consumers about this hazard.

[acelaya]

My understanding is that after the listbox is opened, if autofocus is enabled in the useArrowKeyNavigation hook, the sequence of events is:

1. The SelectNext component's layout effect runs and schedules a state update for the position values
2. The useArrowKeyNavigation hook's effect runs and autofocuses its preferred element
3. The SelectNext component is re-rendered. This applies the position value update.

So the problem here is the ordering of steps (2) and (3).

If step (1) updated the DOM style synchronously, then the listbox would have its correct position by the time useArrowKeyNavigation runs.

You are right! If the styles are directly set via listboxElement.style.{prop} = ... instead of setting via the style={positioningStyle} prop with a piece of state, then there's no need to re-render and things work in the right order.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (f1f477b) to head (af4a1c2).

Additional details and impacted files

@@            Coverage Diff            @@
##              main     #1540   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           63        63           
  Lines         1008      1041   +33     
  Branches       383       395   +12     
=========================================
+ Hits          1008      1041   +33     

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

[acelaya]

The only uncovered line that codecov is complaining about, is marked as istanbul ignore next.

[robertknight]

The implementation generally looks good and worked as expected in all browsers that I tested. The main feedback I have is to add some comments for some of the more subtle issues in the code: why the listbox is being positioned manually, why the styles are applied synchronously, why we're using a popover in the first place. You don't need to repeat everything in the PR description. Just brief notes to raise awareness will do.

[robertknight]

Assuming most tests don't set this option, it means we'll be explicitly disabling the popover usage, right? We probably want tests to use the default behavior of the component except for tests that are specifically opting out.

[acelaya]

Yep, I agree. However, there are some tests failing when using the popover mode, mostly around testing that focusing works as expected in certain scenarios (first option gets focused when opened, the toggle recovers focus when closed, etc.). I assume it's related with the use of the top layer, but I couldn't confirm it.

I tried a couple of times to make them pass, but finally decided to disable the popover mode by default for the sake of getting existing tests to pass, and focus on popover mode only for new tests.

I will address this in a follow-up PR, as I think you are definitely right, but I wanted to offload some indirect complexity from this one.

[robertknight]

The added comments LGTM. I did have a minor query about the default value of listboxAsPopover in tests. It looks like the default is to turn this behavior off except for tests that opt in. Is that intended?