Only use the used invoker to establish popover hierarchy

[josepharhar]

Fixes #9160
Fixes #9168

If this patch is merged, then #9048 is obsolete and can be closed without merging.

Rationale (copied from masons patch):

Instead of using just
the DOM structure to establish the popover hierarchy, the user's behavior should matter. For example, if one popover contains a popover invoker pointing to another popover, it should matter whether that invoker is actually used to open the second popover.

An example:

• Component 1 is a third party widget, which uses popover
• Component 2 is another third party widget, also using popover
• A page wants to use both components separately, from separate invoking buttons.
• Component 1 also wants to be able to use Component 2, via a button within Component 1.

In this example, the page should be able to still independently use these components. So a user clicking the page's button for Component 2 is expected to close Component 1 if it's open, because that's a direct invocation of Component 2. However, if the user clicks the button within Component 1 to get Component 2, it is natural to leave Component 1 open because this is a nested call.

Important note: this often happens to be the behavior even before this change, since the user clicking on the page-level Component 2 invoking button represents a light dismiss signal for Component 1, so it closes either way. But this patch simplifies the implementation, removing the need to track all invokers on the page, and also removing the need to continuously check whether invoker relationships have changed.

• At least two implementers are interested (and none opposed):
  • Chrome
  • WebKit
• Tests are written and can be reviewed and commented upon at:
  • Only use the used invoker to establish popover hierarchy web-platform-tests/wpt#39560
• Implementation bugs are filed:
  • Chromium: https://chromium-review.googlesource.com/c/chromium/src/+/4429412
  • Gecko: …
  • WebKit: …
• MDN issue is filed: …

(See WHATWG Working Mode: Changes for more details.)

---

/popover.html ( diff )

[nt1m]

This seems like an improvement for its simplicity.

I'm trying to think of cases where #9048 would still be relevant, and the only case that comes to mind is element removal (that would change the invoker to null, or be re-appended to a different popover, which would change the parent/child relationship).

Applying #9048 for element removal may be too aggressive in cases where the invoker is re-appended in a way that doesn't change the parent/child relationship, so I guess this is really up to how important we think it is to always preserve hierarchy correctness.

[mfreed7]

This seems like an improvement for its simplicity.

Glad you think so!

I'm trying to think of cases where #9048 would still be relevant, and the only case that comes to mind is element removal (that would change the invoker to null, or be re-appended to a different popover, which would change the parent/child relationship).

Good point. Perhaps we do the #9048 "check and possibly close popover stack" check only when a new popover is being shown or hidden? Perhaps as a part of "hide all popovers until" or something? That should be the only time changes in the hierarchy really matter now, I think.

[nt1m]

Good point. Perhaps we do the #9048 "check and possibly close popover stack" check only when a new popover is being shown or hidden? Perhaps as a part of "hide all popovers until" or something? That should be the only time changes in the hierarchy really matter now, I think.

I haven't looked into this part too deeply regarding user implications, but yeah, calling this before the hierarchy is used seems reasonable.

[annevk]

Please update the commit message to indicate this will close #9168 as well.

[josepharhar]

Please update the commit message to indicate this will close #9168 as well.

done

[annevk]

Can we add an assert here that newPopover is showing? As per a3b356e.

[josepharhar]

I tried adding this assert in chromium either at the beginning or end of "find topmost popover ancestor" and it got hit in several tests.
Why should it be showing?

[annevk]

Because only when it's showing do we know that it's set correctly. We don't clear it at a particularly deterministic time at the moment.

[mfreed7]

So I think the issue is that during showPopover, newPopover won't be showing yet. See showPopover. Step 2 should reset the invoker to null (so we're ok), step 8.2 runs "topmost popover ancestor" (this algo), and step 13 sets it to showing. So a) we're ok, and b) you can't assert that it's showing here.

[annevk]

Thank you!

[nt1m]

Anne convinced me that #9048 shouldn't be needed anymore.

[mfreed7]

Anne convinced me that #9048 shouldn't be needed anymore.

Thanks. Even when showPopover() is called? That's the only time it now seems like it might still help to call "check and possibly close popover stack". I'm thinking in this example:

<div id=p1 popover>
  <button id=b popovertarget=b></button>
</div>
<div id=p2 popover>
  <div id=p3 popover></div>
</div>
<script>
p1.showPopover();
p2.showPopover(); // Popover stack is now: p1, p2
p2.appendChild(b);
b.popoverTarget = a; // This reverses the ancestor relationships
p3.showPopover(); // P1 closes here
</script>

At the end, p1 closes because the ancestor relationship is p2->p1 and p2->p3, so opening p3 closes p1. Perhaps that's fine. This isn't a great example, since it seems "fine". I'm just wondering if there are worse examples where bad things happen. Maybe not?

[nt1m]

Anne convinced me that #9048 shouldn't be needed anymore.

Thanks. Even when showPopover() is called? That's the only time it now seems like it might still help to call "check and possibly close popover stack". I'm thinking in this example:

<div id=p1 popover>
  <button id=b popovertarget=b></button>
</div>
<div id=p2 popover>
  <div id=p3 popover></div>
</div>
<script>
p1.showPopover();
p2.showPopover(); // Popover stack is now: p1, p2
p2.appendChild(b);
b.popoverTarget = a; // This reverses the ancestor relationships
p3.showPopover(); // P1 closes here
</script>

At the end, p1 closes because the ancestor relationship is p2->p1 and p2->p3, so opening p3 closes p1. Perhaps that's fine. This isn't a great example, since it seems "fine". I'm just wondering if there are worse examples where bad things happen. Maybe not?

@annevk Can you elaborate your reasoning here?

[annevk]

Hmm, I was thinking about risky times to use a popover's popover invoker and it not being risky when we check that the popover is showing before using the popover invoker, based on what you wrote in #9171 (comment) (hence the assert suggestion). But I don't think I have a good enough mental model of the popover stack to rule out all issues there.

[mfreed7]

Hmm, I was thinking about risky times to use a popover's popover invoker and it not being risky when we check that the popover is showing before using the popover invoker, based on what you wrote in #9171 (comment) (hence the assert suggestion). But I don't think I have a good enough mental model of the popover stack to rule out all issues there.

Ok. Perhaps the best way forward, for now, is to not do any "checking and closing the popover stack" for now, and revisit this later if problematic use cases arise? I feel like they might not materialize and the current behavior might turn out to be ok. In other words, let's just abandon #9048 for now.

[annevk]

I think that's okay, I do think we should add the assert I suggested after approving this PR.

[mfreed7]

I think that's okay, I do think we should add the assert I suggested after approving this PR.

I think you mean this conversation - see my latest comment. I don't think we can add that assert.

[annevk]

Thank you!