fix(dock-button): give back focus when menu is closed

[vicgeralds]

Almost fixes #1820 (comment)

fix: https://github.com/Lundalogik/crm-feature/issues/2989

The tricky part is we don't know here how the menu was closed. If the user navigated to an item in the menu, we don't expect focus back.

Review:

• Commits are atomic
• Commits have the correct type for the changes made
• Commits with breaking changes are marked as such

Browsers tested:

(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)

Windows:

• Chrome
• Edge
• Firefox

Linux:

• Chrome
• Firefox

macOS:

• Chrome
• Firefox
• Safari

Mobile:

• Chrome on Android
• iOS

[github-actions]

Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-1843/

[vicgeralds]

Maybe we can check that the popover wasn't closed due to click. I guess we could just set this.hadVisibleFocus = false in a click event handler.

[adrianschmidt]

I think we use a scrim for popover, right? So it shouldn't be possible to focus on something outside the open menu with a single click anyway. You first need to click—either on something inside the menu which closes it, or outside the menu—to close the menu, then click on the thing you want to focus. And if you don't do that second click, but tab instead, we should, ideally, be tabbing to the next this after the trigger.

So I think the best thing to do, would be to always set focus on the button, but not make the focus visible if closing was triggered from a pointer-event. The next best thing would be to always set focus, and either just keep it visible or invisible, as it was. The third best would probably be to always set focus but make it always visible. And the fourth best to only set focus if closing was triggered by a non-pointer event. (@Kiarokh might disagree about the order of these last two…)

I haven't looked at the rest of the code yet, but I'm guessing this.hadVisibleFocus is true if the trigger had visible focus when the menu was opened? Using that is not perfect, but decent enough, because it should guarantee that we set focus for any user that uses only non-pointer input methods (users who use only the keyboard, for example). Those are the users for whom it's most important to handle focus correctly.

[Kiarokh]

It's a bit hard for me to follow everything and I'm not sure how everything works exactly. So sorry if my answer is off.

But if I could wish, my wish would be limiting the focus visible to keyboard interactions only. Clicking to close and open stuff is an entirely different story, and very hard to find a good solution for. To see why, follow the discussion here….

So one of the things I dislike with setting the focus the way we do it today is the green outline ring that we will get from focus-visible styles, even if the menus (or popovers) are opened using a click/tap interaction. The focus-visible styles are for keyboard navigation only; but we get them now for mouse interactions, which is not good. I guess this is what you mean when you say:

but not make the focus visible if closing was triggered from a pointer-event.

Screen.Recording.2022-09-29.at.10.36.45.mov

It is possible to keep the focus "invisible"? And make it only visible if navigation is done using keyboard?

Anyhow, I guess what Lucy wanted to achieve was setting focus back on the trigger, if we close the popover by pressing esc, after having opened the popover by pressing enter on its trigger (all keyboard navigation). Or?

[adrianschmidt]

I think I have a solution where we can always set focus freely, and not worry about what type of event triggered the focus-setting—and control the visibility of the current focus completely separately.

I'll keep you in suspense for a little while though, while I fix up some other things 😜

[Kiarokh]

this is torture! 😄

[vicgeralds]

I think it is!? It's only the focus-setting part that's different? 😄

[adrianschmidt]

I honestly don't know how to reply non-sarcastically to this… 😅

You do realise that you basically just said "if we ignore the difference you think is important, it's the same"? 🤷‍♂️

[adrianschmidt]

I'm logging off now for my vacation. I'm back on Tuesday next week.

I completely trust that you'll find a good solution 😊 And if you don't, I'll totally moan about it when I get back! 🤪

Have a great week! 🤗

[adrianschmidt]

Final remark: Why I think we should set focus the same, regardless of whether focus is visible, is that, if I for example click the scrim to close a menu, or press escape to close it, and then press tab, I want the same thing to happen regardless.

But, sure, it's less critical that focusing is handled correctly when using a mix of mouse and keyboard than when using only keyboard 😊 👍

[vicgeralds]

You too! 🤗

Sorry, my comment didn't make things any clearer. I didn't mean that we should ignore that part. What I meant with same thing as using :focus-visible was specifically styling focus as visible or not based on input method. My point is trying to solve that ourselves may not be the best idea as there's a web standard solution available.
I think they're doing a good job with focus-visible. It might be that they solve some issue that we haven't thought about. It might also be that it's not ready yet for something we want.

About what needs to change in this PR, Adrian convinced me that we shouldn't set focus conditionally only when we want it visible. 👍

I saw that element.focus({ focusVisible }) was added very recently to the html standard: whatwg/html#8087, so there's some hope other browsers will implement it soon (enough?). We could start using that, and continue the discussion in the other PR about how we should style focus as visible or not.