Update focusTrap to use new methodology after accessibility discussions

[owenniblock]

Motivation

Precursor to this PR I'm working on: #48

This change alters the behavior of the focusTrap so that it inserts a sentinel <span> element before and after the trap area.

Note that this is a breaking change in that: when there are no focusable elements within the focusTrap the container itself is no longer focused. This is because from an Accessibility point of view - trapping focus and not having any focusable elements within that focus is bad for screenreader users.

I have tested this against @primer/react by locally wiring it up with yarn link and have verified that it behaves as expected however I recommend that we release this as a minor version point as a minimum (not a patch) and then test the change thoroughly before rolling it out.

Questions:

1. What do I need to add to this PR to get it ready for release?
2. If focusTrap is called against a container with no focusable elements, should we raise an error?

cc @github/accessibility

[changeset-bot]

🦋 Changeset detected

Latest commit: 35c9cee

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/behaviors	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[siddharthkp]

Hi 👋

I think this is great! I ran primer-react's test suite with this change (0.0.0-202213135035) and the interaction tests pass.

One thing that I need to confirm with @alliethu is the initial focus when a menu is opened with mouse click (as compared to keyboard)

Right now, when you open a menu with mouse click, we do not focus the first element by default. (we only do that when it's opened with keyboard). This PR changes that.

menu-click-tho.mov

[owenniblock]

Thanks for the attention to detail @siddharthkp - please let me know what you think @alliethu. 🙂

[alliethu]

👋🏼 Hi all! Thank you @owenniblock for tackling this issue and @siddharthkp for your attention to detail. In this instance, I would suggest we not automatically place focus on the first element when the menu has been triggered by a mouse click, as this could lead to confusion for the user since they've not actually made the selection themselves.

I do agree, however, with the focus implementation when the component is accessed through keyboard.

[owenniblock]

@alliethu I'm trying to think through the best way to implement this - I think it might be fiddly to change the implementation based on whether the user clicks the mouse or a uses the keyboard but if it's the right approach I'm happy to find a way to do this.

Having said that, I'm not 100% sure using this to differentiate is the correct approach. Having the first focusable element selected in this change is intentional (certainly in the case of the dialog) but I've been mainly focused on the dialog usage of this work so hadn't really given too much thought on other use-cases. Do you know why we diverge on this functionality currently? Are we certain that mouse users would find this functionality confusing? (I'm guessing there are some edge cases so I'd like to understand the steps that would lead to this being an issue so I can better understand our users!)

Would you object to me discussing this with James? I'd love to get his input here so I can make sure I'm doing the right thing for both dialogs and dropdown menus!

[owenniblock]

@alliethu - I bought this up with James to check what the best approach would be here and he recommended that we keep the functionality where the focus moves to the first item regardless of how the control is opened - unless you strongly disagree I suggest we leave this as it is?

[bolonio]

Based on what you've consulted with James here, all good for me. Unless @alliethu says something different

[alliethu]

👋🏼 @owenniblock apologies on the late response.

Do you know why we diverge on this functionality currently?

Unfortunately, I don't have any insight into this.

Are we certain that mouse users would find this functionality confusing?

No, I'm not certain, and in a perfect world, it would be nice to be able to do usability testing around this assumption (this is something I'm working through).

@owenniblock @siddharthkp I would say to go ahead and move forward with this implementation, based on @jscholes feedback. If we come to find that mouse users are reporting unexpected behavior, we can address it then. 😄

[siddharthkp]

Ship it!