popover focus state weirdness

[jakearchibald]

https://html.spec.whatwg.org/multipage/popover.html#popover-target-attribute-activation-behavior

1. Set popover's popover invoker to node.
2. Run show popover given popover and false.

The invoker is set, then the popover attempts to show. However, that might not succeed for a number of reasons (there are 4 early returns in the show algorithm). If this happens, the invoker is left as-is.

That means, if showPopover() is called later, the invoker will be incorrect.

I guess showPopover() should clear the invoker? Or maybe it needs to be cleared earlier? It isn't really clear to me how the invoker is used.

---

https://html.spec.whatwg.org/multipage/interaction.html#associated-focus-navigation-owner

5. If element is in the popover showing state and has a popover invoker set, then return element.

Is that right? All of the other steps in this algorithm seem to be returning something 'parent', this is the only one that returns itself. That means that a popover's focus navigation scope is itself.

[jakearchibald]

cc @mfreed7

[hidde]

This was just discussed in Open UI.

Minutes

JakeA: this is something I encountered in the spec. This [shares scree] is the algorithm when the button for the popover is pressed… at some point it sets the popover's popover invoker to the button…

JakeA: the popover invoker isn't then later removed, I'm not sure how bad that is?

keith: it is for restoring the focus when the popover is hidden

masonf: it's related to focus… in the implementation there is a previouslyfocusedelement too. The 'invoker' is used to manage order of focus

JakeA: so this probably should not hang around if the popover didn't show?

masonf: probably true… almost like a noop, but we probably should clean it up

JakeA: if the popover is shown by some other means that doesn't change the popover invoker, it would still be there

masonf: yes you're right, good catch

JakeA: when I was trying to figure out what the invoker was for… I saw step 5 in the algorithm for the associated focus navigation owner… step 5 seems to be 'off' ?

JakeA: it seems odd for it to return itself

masonf: I think you're right, it probably should return the invoker there

masonf: would need to read it a little more carefully… the relationship is … the invoker is the owner… so you should probably be returning the invoker here… we probably already do this in the Chromium implementation, otherwise it wouldn't work

masonf: so it sounds like we need to fix a few issues in spec and potentially implementation too

[mfreed7]

Per the meeting, both things seem like issues that we need to correct in both the implementation and spec, and @josepharhar and I will take a look. Thanks for raising these.

[mfreed7]

The invoker is set, then the popover attempts to show. However, that might not succeed for a number of reasons (there are 4 early returns in the show algorithm). If this happens, the invoker is left as-is.

That means, if showPopover() is called later, the invoker will be incorrect.

I guess showPopover() should clear the invoker? Or maybe it needs to be cleared earlier? It isn't really clear to me how the invoker is used.

Ok, taking a closer look at this. I'm not sure exactly which "4 early returns" you are talking about, but certainly there are cases where the attempt to invoke the popover might fail. One is the check popover validity call, but that should always pass given the pre-condition that the popover target element is non-null. However, the event handler does fire, and that could change pre-conditions and result in the popover not actually being shown. (Did I miss another condition?)

This begs the question though: so what? If the invoker is left set even though the popover never was shown, I don't think anything bad happens. The invoker is used for focus management (here), and that first makes sure the popover is in the showing state. So if it isn't, nothing happens, and it doesn't matter that the invoker is set. Right?

I'm not sure anything needs to change here.

https://html.spec.whatwg.org/multipage/interaction.html#associated-focus-navigation-owner

5. If element is in the popover showing state and has a popover invoker set, then return element.

Is that right? All of the other steps in this algorithm seem to be returning something 'parent', this is the only one that returns itself. That means that a popover's focus navigation scope is itself.

Double-checking the Chromium implementation, and it does appear this spec text should return the popover invoker and not the popover. The owner of a popover, for focus navigation purposes, is its invoker. (This should also resolve #9168.) @josepharhar mind putting up a quick PR to do that?

[jakearchibald]

This begs the question though: so what?

1. Try to open a popover via a button.
2. It now has the button as the invoker.
3. beforetoggle is cancelled, so the popover doesn't open.
4. Call showPopover() on the popover.
5. It opens successfully.

However, the invoker is still set to the button, even though it wasn't the invoker. It feels like, at the very least, showPopover should clear the invoker, because there was no invoker.

Does that make sense?

[mfreed7]

Does that make sense?

Yep, makes total sense - that seems bad. Thanks for explaining in more detail! I think I agree that the fix is to always set invoker in showPopover() either to the invoker or to null. I’ll prototype that to make sure, and then we can put up a PR.

[josepharhar]

I opened a PR: #9186