Popover parent/child relationship doesn't seem correct

[jakearchibald]

The algorithm is really complex, so I might be reading it incorrectly.

Consider markup like this:

<div id="popover-1" popover>Popover 1 <button popovertarget="popover-2">Open popover 2</button></div>
<div id="popover-2" popover>Popover 2</div>
<button popovertarget="popover-1">Open popover 1</button>
<button popovertarget="popover-2">Open popover 2</button>

Demo

1. Open popover 1
2. Open popover 2 using the last button in the source (the one that isn't inside the other popover)

When the spec is deciding which popovers to close, it looks at all buttons on the page rather than the button that was used. This doesn't make sense to me, and doesn't match Chrome's implementation.

https://youtu.be/VO1C5YPITsk - here's a readthrough of the spec.

I'm not sure what the actual intended behaviour is (the spec is complicated and buggy). The note mentions a "tree of popovers", but it seems like only a list is needed? Is there a case where two open popovers have the same parent?

I also feel we should match buttons up to closest popovers early. The current spec looks at the position and state of the button when a new popover is opening, but that feels like the wrong time to check. Eg, a button must be enabled when it's used to open a popup. It doesn't matter if it's disabled when later trying to determine the popover tree. In fact, the button may have been later removed.

[jakearchibald]

cc @mfreed7

[keithamus]

I believe maintaining a tree of popovers is to handle the case of opening or closing nested popovers should close non-ancestral auto popovers, but the tree of popovers from which the descendent lies should not be closed.

[jakearchibald]

Can you give an example?

[mfreed7]

This was discussed just now at OpenUI. Notes:

https://www.w3.org/2023/04/13-openui-minutes.html

I'll summarize the discussion later today.

[mfreed7]

Ok, so we had a very good discussion of this issue at OpenUI - see meeting minutes above. There were a number of issues raised, but at its core, there are essentially two possible behaviors we could choose from:

1. The existing spec's behavior, which determines popover ancestor relationships statically. This means that the DOM structure is all you need to determine whether one popover is the ancestor of another popover. The ancestor relationships do not change depending on which invoker the user clicks.
2. An alternative behavior, which determines popover ancestor relationships dynamically. In this scenario, whether one popover is the ancestor of another depends on the sequence of actions. For example, in the example given in the OP, popover 1 is only an ancestor of popover 2 if the invoker inside popover 1 is clicked by the user. If the other invoker, which is outside popover 1, is clicked by the user, then popover 1 is not an ancestor, and it will close before popover 2 opens.

A number of variations of #2 were proposed, such as adding an option to .showPopover() that allows imperative calls to establish ancestor relationships. (This is, in part, requested by #9111.)

While we didn't reach any firm conclusions, it seemed (to me at least) like the tone of the conversation was trending toward supporting #2. In particular, these use cases were discussed:

• Two popovers that can be either independently triggered, or where one of them can be triggered from a button within the other. Most folks agreed that in the case where they're triggered independently, from a user's perspective it seems the most clear to only show one of them, i.e. not have them be ancestral popovers.
• The example above, but where the two popovers are provided as components, authored by different people/groups. It seems bad if these two components are being used by the page independently, but if an "accidental" linkage is made via an invoking attribute within one of the components, that this changes the behavior even when used independently.

I believe option #2 would simplify the spec and implementation, because we wouldn't have to track all invokers on the page, but only the last used invoker (see related issue #9152). I will start thinking about these changes along with @josepharhar, but if there are objections to this change, it'd be good to know what they are. @annevk @nt1m

[mfreed7]

Side note: I realized why it seems that the Chromium implementation differs from the spec, for the example in the OP. When the non-popover-container invoker for popover-2 is clicked, that is a light-dismiss signal for popover-1, since it is a) outside popover-1 and b) isn't a click on an invoker for popover-1. So when you click on that button:

1. popover-1 is light-dismissed
2. popover-2 is shown by the invoker click

I don't think this changes the logic or new direction set in the comment above, but it does likely mean it isn't a material change in behavior that users will see.

[jakearchibald]

Side note: I realized why it seems that the Chromium implementation differs from the spec, for the example in the OP. When the non-popover-container invoker for popover-2 is clicked, that is a light-dismiss signal for popover-1,

lol yeah it's obvious now you say it. Annoyed that I didn't realise that.

The difference in behaviour can be seen if you interact with the demo via keyboard, which doesn't trigger light-dismiss.

In fact, that seems like a good argument for option 2, as it avoids creating a difference between mouse and keyboard interactions.

I believe option #2 would simplify the spec and implementation, because we wouldn't have to track all invokers on the page, but only the last used invoker (see related issue #9152).

Although we'll have to track the invoker of a popover for focus reasons, I don't think we should use that when deciding which popover 'owns' another.

When a button is clicked to show a popover, we should look to see if that button is within an auto-popover, and make that the 'owner' of the new popover. From that point, the button no longer matters. Eg, if the button is (re)moved, it doesn't change or break the popover relationships.

It feels like the relationship between popovers becomes a simple list:

popover1 -> popover2 -> popover3 -> popover4

Where the previous item in the list 'owns' the next item. So, if a new popover is opened that would be owned by popover2, we know we need to close popover3 and popover4.

[jakearchibald]

If this involves looking at the steps for closing related popovers, it might be worth addressing #9161 at the same time.

[mfreed7]

Side note: I realized why it seems that the Chromium implementation differs from the spec, for the example in the OP. When the non-popover-container invoker for popover-2 is clicked, that is a light-dismiss signal for popover-1,

lol yeah it's obvious now you say it. Annoyed that I didn't realise that.

The interactions between popovers and user actions is more complicated than it seems on the surface. I came to this realization a while ago while working on popover. I think this is actually the biggest utility provided by the API - untangling this behavioral stuff and implementing it once, correctly, rather than making everyone re-invent this wheel.

The difference in behaviour can be seen if you interact with the demo via keyboard, which doesn't trigger light-dismiss.

In fact, that seems like a good argument for option 2, as it avoids creating a difference between mouse and keyboard interactions.

Well, yes, this was kind of why I built the algorithm to not depend on behavior. However, from the meeting, it seemed like people were starting to prefer a "path dependent" algorithm that depends on what the user does. This is another such path dependency - did the user use the keyboard or the mouse? Moving focus with the keyboard doesn't trigger light dismiss (and yes we litigated that extensively) but clicking the mouse does. That's the expected typical behavior - users mostly don't expect moving focus to affect the page state, while they do expect clicking to affect state.

I believe option #2 would simplify the spec and implementation, because we wouldn't have to track all invokers on the page, but only the last used invoker (see related issue #9152).

Although we'll have to track the invoker of a popover for focus reasons, I don't think we should use that when deciding which popover 'owns' another.

When a button is clicked to show a popover, we should look to see if that button is within an auto-popover, and make that the 'owner' of the new popover. From that point, the button no longer matters. Eg, if the button is (re)moved, it doesn't change or break the popover relationships.

It feels like the relationship between popovers becomes a simple list:

popover1 -> popover2 -> popover3 -> popover4

This is close to what's there now, but different in nuanced and important ways. The list above is essentially the auto popover list. The reason you can't just handle this from button clicks, as you suggest above, is that there are other ways to link popovers. For example:

<div popover id=p1>p1
  <div popover id=p2>p2</div>
  <div popover id=p3>p3</div>
</div>
<script>
p1.showPopover();
p2.showPopover();
p3.showPopover(); // Hides p2, leaves p1 open
</script>

I really think using the invoker, rather than examining all of the invokers on the page, is the right way to implement option #1. That also enables showPopover({invoker:foo}).

[jakearchibald]

users mostly don't expect moving focus to affect the page state, while they do expect clicking to affect state.

fwiw, I wasn't disagreeing with this.

The reason you can't just handle this from button clicks, as you suggest above, is that there are other ways to link popovers.

Totally agree. I just didn't mention that stuff as it doesn't seem like there's any disagreement there, but yeah, that wasn't clear.

I really think using the invoker, rather than examining all of the invokers on the page, is the right way to implement option #1.

We might be talking about the same thing, but I really think the invoker should be resolved to a parent popover at 'show' time, rather than 'hide' time (the current spec looks at buttons at 'hide' time). That means, if the invoker button is (re)moved after it's clicked, it doesn't change the relationship of the popover to its parent.

That also enables showPopover({invoker:foo}).

We'd need to figure out what:

el.showPopover({ invoker: foo });
el.showPopover({ invoker: bar });

…does.

[mfreed7]

I really think using the invoker, rather than examining all of the invokers on the page, is the right way to implement option #1.

We might be talking about the same thing, but I really think the invoker should be resolved to a parent popover at 'show' time, rather than 'hide' time (the current spec looks at buttons at 'hide' time). That means, if the invoker button is (re)moved after it's clicked, it doesn't change the relationship of the popover to its parent.

I think we agree, and I think that's what the new PR actually does. The invoker is checked/used at the time that it's target is shown, i.e. when the invoker is clicked. Since the invoker is there, it allows the popover containing that invoker to stay open while the target popover is opened. After that point, both popovers are now open and in the popover stack, and the invoker shouldn't matter for the hierarchy any longer.

That also enables showPopover({invoker:foo}).

We'd need to figure out what:

el.showPopover({ invoker: foo });
el.showPopover({ invoker: bar });

…does.

I suggested {invoker:foo}, but I think actually {ancestor: foo} makes more sense, right? At least it seems more understandable. In your example above, the second showPopover() call is an InvalidStateException because the popover is already open. Right?