C2LC-316: Fix focus trap settings for actions simplification menu toggle.

[the-t-in-rtf]

See C2LC-316 for details.

[the-t-in-rtf]

After our pairing session yesterday, I went further down the road with using references, and hit a few roadblocks. Using onBlur proved pointless as there was never a relatedTarget and the target was the even losing focus. So we had the wrong end of things, i.e. we only were notified when one of the action menu's elements blurred, and not when an outside element blurred.

After wrestling with a few variations, I ended up using an approach similar to our modal, i.e. when the menu is open, a large transparent element is positioned to fill the screen, just behind the menu itself. Clicking anywhere outside the menu closes the menu and also removes the transparent element to avoid side effects.

I also made a first pass at ensuring that the first checkbox in the panel is focused when the menu is open. I could use help with the polish on this, the focus works as expected when opening the menu with the keyboard, but when clicking the toggle with a mouse, the focus seems to move but the highlighting is wrong in Chrome.

[the-t-in-rtf]

In later review, I figured out a few more things:

1. If I reuse the global focus trap manager, it's unusable as it gets flagged as inactive by the program block editor. So we do need to use our own.
2. I still had my own keyDown handler for escape, I removed that in favour of letting the focus trap manager do its job.
3. I hadn't added the focus trap classname to the toggle, so the focus wasn't correct on exit. I fixed that.

I could still use help on getting the first item to properly steal focus when the toggle is clicked (it works on key down). Otherwise, this is ready for review.

[the-t-in-rtf]

the focus works as expected when opening the menu with the keyboard, but when clicking the toggle with a mouse, the focus seems to move but the highlighting is wrong in Chrome.

As discussed in the chats, it seems like the focus position moves when you click the toggle using the mouse. Even though there are no :focus styles applied:

1. If you hit space, the first checkbox is toggled.
2. If you hit tab, focus moves to the second checkbox.

Oddly, if you hit a key such as caps lock, shift, arrows, the focus highlighting will appear on the correct element.

[sbates-idrc]

I'm seeing an issue on Firefox on Windows (which isn't happening on Chrome on Windows): when I open the menu using the spacebar, the spacebar key press toggles the first item in addition to opening the menu.

[sbates-idrc]

@the-t-in-rtf Could you expand a little on the point above:

"If I reuse the global focus trap manager, it's unusable as it gets flagged as inactive by the program block editor. So we do need to use our own."

Thanks.

[the-t-in-rtf]

If I reuse the global focus trap manager, it's unusable as it gets flagged as inactive by the program block editor. So we do need to use our own.

I may have the wrong idea here, but in my debugging sessions, I could see that by the time I was hitting a key in the actions menu, the focus trap manager's active variable was set to false, which causes it to ignore all keystrokes.

After debugging the calls to unsetFocusTrap a few times, I started looking at the call stack and realised that it was the program block editor deactivating the focus trap manager. The program block editor calls that method whenever it updates and actionPanelStepIndex is null. We might be able to fix this by looking at the previous state and only calling unsetFocusTrap if actionPanelStepIndex is null now and was previously not null.

We would need similar logic in the actions menu, to ensure that we don't end up deactivating the focus trap manager when the action panel needs it.

[the-t-in-rtf]

From chats with @sbates-idrc:

Maybe we don't need/want to set focus? If I am understanding your comments on the PR, I think you aren't using blur any longer to close the menu, is that right? When we were working on it yesterday, setting focus on the first item was motivated by using blur to close. And if we aren't using blur, then maybe we don't need to set focus. Just set up the focus trap and ensure that the first menu control is in the tab order after the button to open it. For example, for the popup for the program steps, we don't set focus in that case. Screen reader users I believe are generally the most sensitive to setting focus programmatically, and I think if we don't need to do it, that might be a better user experience.

In other messages, Simon also noted that if you activate the menu by pressing the space key on Firefox in Windows, the first item gets toggled, which I verified as well on the Mac. Given this side effect, I think Simon's argument above for removing the "focus on first" logic is even stronger.

To answer the question, yes, now we use a click on an external element to close the menu, so we don't have a good reason to fiddle with the focus, and I agree that it's to be avoided if possible. I will update the pull accordingly, and see if my idea about using the global focus trap manager works as well.

[the-t-in-rtf]

I removed the focus manipulation code and other leftover bits from previous approaches. The issues with the spacebar no longer happen in Firefox.

I will update the pull accordingly, and see if my idea about using the global focus trap manager works as well.

With the approach I proposed, the shared focus trap manager remains active, which is good, but there's a side effect, suddenly every tab key moves forward two items (and every shift+tab moves backwards by two items), which makes every other checkbox unreachable.

I pushed up the latest changes without the shared focus trap manager changes, so that the pull would be otherwise merge-ready. It only takes a minute to refactor, we can pair on it and look at it together if you want.

[sbates-idrc]

@the-t-in-rtf I removed some more code that is no longer needed: sbates-idrc@e37d40d

Please take a look and let me know if it looks good. I will then merge. Thanks.

[the-t-in-rtf]

@sbates-idrc, thanks for catching that, looks fine to me.

[the-t-in-rtf]

@sbates-idrc, I merged in your changes and updated with develop-0.7 to pick up other fixes.