Questions/issues with popover spec

[jakearchibald]

I'm reading the popover spec for the first time. Here are some notes & questions. If any of these turn out to be things that should be changed, I'll split each out into their own issue.

https://html.spec.whatwg.org/multipage/popover.html#auto-popover-list

The Document has an auto popover list, which is a list of all the elements in the Document's top layer whose popover attribute is in the auto state, in the same order.

"in the same order" seems confusing here. Does it mean document order? Is this list intended to be live? I'd rather turn this into an algorithm that returns a static list, and makes the ordering clear.

Is "in the Document's top layer" problematic here? If an element is a descendant of an element in the top layer, is it considered "in the top layer"? I'm worried that "in the top layer" is being used to mean "the popover is shown" here, but that might not be clear/true.

https://html.spec.whatwg.org/multipage/popover.html#the-popover-attribute:concept-element-attributes-change-ext

The following attribute change steps are used for all HTML elements:

Nit: This algorithm references arguments that aren't specified up front. I'd like to fix this.

If oldValue and value are in different states, then run the hide popover algorithm given element, true, true, and false.

This seems a bit of a hand-wave, and 'states' doesn't seem to link to the right thing. I'd like to fix this.

https://html.spec.whatwg.org/multipage/popover.html#check-popover-validity

It seems weird to me that showPopover() and hidePopover() throw if the element is already in that state. I think we usually avoid throwing in these cases. <dialog> throws if showModal() is called when it's already in that state, but that seems to be an outlier. <dialog>'s close() doesn't throw if the state is already closed.

[jakearchibald]

https://html.spec.whatwg.org/multipage/popover.html#light-dismiss-open-popovers

Let topmostPopover be the result of running topmost auto popover given target.

This algorithm expects a Document but it's being given an event target (presumably an Element).

Light dismiss open popovers will be called by the Pointer Events spec when the user clicks or touches anywhere on the page.

Does this mean a popover will close if you click it? I couldn't immediately see where that's prevented in the spec.

[mfreed7]

Thanks for the feedback! A few replies:

https://html.spec.whatwg.org/multipage/popover.html#auto-popover-list

"in the same order" seems confusing here...

This is intended to mean "in the same order as the top layer set". (Top layer is an ordered set.)

https://html.spec.whatwg.org/multipage/popover.html#check-popover-validity

It seems weird to me that showPopover() and hidePopover() throw if the element is already in that state. I think we usually avoid throwing in these cases. <dialog> throws if showModal() is called when it's already in that state, but that seems to be an outlier. <dialog>'s close() doesn't throw if the state is already closed.

No strong opinions from me here. The idea was that it might be more helpful to have this be an error state: you're calling a method that will have no effect. One potential point of confusion is that in order to move an existing top layer item to the top of the top layer, you have to remove it and re-add it. So there could be confusion that developers think they should be able to just call showPopover() again to move it to the top.

However, I also see your argument that many other APIs on the platform are idempotent and don't throw in similar situations. @annevk @josepharhar any issues removing these two exceptions? If we do change this, it seems like it'd also make sense to change dialog.showModal. In all cases, the second call to the method should simply be a no-op.

Light dismiss open popovers will be called by the Pointer Events spec when the user clicks or touches anywhere on the page.

Does this mean a popover will close if you click it? I couldn't immediately see where that's prevented in the spec.

No, it'll stay open. If you look at the linked Light dismiss open popovers algorithm, step 7.5 will hide all popovers until the clicked popover. Which will leave it open.

[annevk]

I'll defer to @nt1m (unless he feels the same way, in which case I'll take a look 😊).

[nt1m]

I don't have a strong opinion, my preference goes towards consistency here :)

[jakearchibald]

Consistency with what?

[nt1m]

I don't really mind as long as the end result is consistent, either by removing exceptions for both dialog and popovers, either keeping them for both. Reduces mental overhead for web developers when things are consistent.

[nt1m]

One thing to think about if we remove the exceptions is whether we want to re-run all the steps, especially for show:

• pushing to top layer: if we re-run that step, the top layer position might change (since it will be removed from its current position, then re-added to the top of the top layer)
• focusing steps: the popover/dialog might end up stealing focus from another element

[mfreed7]

One thing to think about if we remove the exceptions is whether we want to re-run all the steps, especially for show:

• pushing to top layer: if we re-run that step, the top layer position might change (since it will be removed from its current position, then re-added to the top of the top layer)
• focusing steps: the popover/dialog might end up stealing focus from another element

This is good to emphasize. I'm supportive of not throwing exceptions for these cases, e.g. double-showPopover(), but I'm not supportive of changing the top layer position as a result of the second call. That has wide implications for the popover stack and I'm not sure they're all good. I'd prefer the explicit path of calling hidePopover() and then showPopover() to put a popover on top. That's consistent with dialog and fullscreen, which both require the element to be hidden and re-shown to do this.

[jakearchibald]

Moving the throw/don't-throw discussion to #9045

Also identified an event timing issue #9046

[jakearchibald]

The remaining issues:

https://html.spec.whatwg.org/multipage/popover.html#auto-popover-list

The Document has an auto popover list, which is a list of all the elements in the Document's top layer whose popover attribute is in the auto state, in the same order.

"in the same order" seems confusing here. Does it mean document order? Is this list intended to be live? I'd rather turn this into an algorithm that returns a static list, and makes the ordering clear.

Is "in the Document's top layer" problematic here? If an element is a descendant of an element in the top layer, is it considered "in the top layer"? I'm worried that "in the top layer" is being used to mean "the popover is shown" here, but that might not be clear/true.

https://html.spec.whatwg.org/multipage/popover.html#the-popover-attribute:concept-element-attributes-change-ext

The following attribute change steps are used for all HTML elements:

Nit: This algorithm references arguments that aren't specified up front. I'd like to fix this.

If oldValue and value are in different states, then run the hide popover algorithm given element, true, true, and false.

This seems a bit of a hand-wave, and 'states' doesn't seem to link to the right thing. I'd like to fix this.

https://html.spec.whatwg.org/multipage/popover.html#light-dismiss-open-popovers

Let topmostPopover be the result of running topmost auto popover given target.

This algorithm expects a Document but it's being given an event target (presumably an Element).

[annevk]

We changed auto popover list to be in terms of the top layer because the alternative had bugs: 1257236. I could see someone trying to remove it entirely, but I don't think we want to go back to maintaining it in parallel with the top layer.

[mfreed7]

I believe all of the comments just above are mostly spec nits and not behavior changes.

Is "in the Document's top layer" problematic here? If an element is a descendant of an element in the top layer, is it considered "in the top layer"? I'm worried that "in the top layer" is being used to mean "the popover is shown" here, but that might not be clear/true.

The spec might be a bit vague here, and I think that's the gist of your comment. But to be clear, in all browsers, only the element itself is "in the top layer". Descendants of that element aren't considered to be in the top layer. It wouldn't make any sense if they were - the top layer is an ordered set, painted in the order of the set. You'd never be able to put any useful content in the top layer, because the descendent content would be strung out into the top layer set and painted sequentially as individual fixed-position elements.

[jakearchibald]

@annevk

We changed auto popover list to be in terms of the top layer because the alternative had bugs: 1257236. I could see someone trying to remove it entirely, but I don't think we want to go back to maintaining it in parallel with the top layer.

Yeah, I think it's just the wording that's confusing. Changing it to an algorithm should clear it up.

@mfreed7

I believe all of the comments just above are mostly spec nits and not behavior changes.

I think so, although that might depend on what the desired behaviour is.

in all browsers, only the element itself is "in the top layer". Descendants of that element aren't considered to be in the top layer. It wouldn't make any sense if they were - the top layer is an ordered set, painted in the order of the set

Cool. It's just unclear language then. Given the DOM is a tree, saying one thing is 'in' another can easily mean it's a descendant. But, if you're wanting to be clear about treating the top layer as an ordered set, you can correctly reference terms to avoid any confusion. I think it's worth clearing this up, because although top layer is an ordered set, it's also a stacking context.

It wouldn't make any sense if they were - the top layer is an ordered set, painted in the order of the set. You'd never be able to put any useful content in the top layer, because the descendent content would be strung out into the top layer set and painted sequentially as individual fixed-position elements.

That isn't what I was referring to. Think about it: if I asked you "is this element in the body?", do you think I'm asking if it's a direct child, or a descendant? It's ambiguous, but I think most people would assume descendant. I guess you could say that, when it comes to the top layer, the thinking model switches, but I don't think it's clear. Are we thinking of top layer as a set, or a stacking context?

The good news is, as spec writers, we don't have to choose ambiguity. If instead of something like:

Let autoPopoverList be a list of all the elements in the Document's top layer whose popover attribute is in the auto state, in the same order.

it could say:

Let autoPopoverList be a clone of the Document's top layer set, and remove items whose popover attribute is not in the auto state.

Which clears up the ordering an 'in' ambiguity.

Although, now I write this out, both lines would include an element like <div popover> that isn't showing, but is contained in the top layer via other means, such has a shown dialog or fullscreen'd element. Is that the intent?

[mfreed7]

in all browsers, only the element itself is "in the top layer". Descendants of that element aren't considered to be in the top layer. It wouldn't make any sense if they were - the top layer is an ordered set, painted in the order of the set

Cool. It's just unclear language then. Given the DOM is a tree, saying one thing is 'in' another can easily mean it's a descendant. But, if you're wanting to be clear about treating the top layer as an ordered set, you can correctly reference terms to avoid any confusion. I think it's worth clearing this up, because although top layer is an ordered set, it's also a stacking context.

Agreed - this could be more clear. And as @annevk has pointed out, there's definitely a need to clean up the top layer spec situation. And @atanasov mentioned that this might get discussed tomorrow at CSSWG.

It wouldn't make any sense if they were - the top layer is an ordered set, painted in the order of the set. You'd never be able to put any useful content in the top layer, because the descendent content would be strung out into the top layer set and painted sequentially as individual fixed-position elements.

That isn't what I was referring to. Think about it: if I asked you "is this element in the body?", do you think I'm asking if it's a direct child, or a descendant? It's ambiguous, but I think most people would assume descendant. I guess you could say that, when it comes to the top layer, the thinking model switches, but I don't think it's clear. Are we thinking of top layer as a set, or a stacking context?

Again, agreed, the language could be more clear. The fullscreen spec says the top layer is a "new stacking layer", not a "new stacking context" but perhaps the intention was the same. Technically, the top layer isn't a stacking context itself, since z-index on the elements within the top layer doesn't have any effect. Or perhaps it's a stacking context, but so also is each element in the top layer.

Either way, I'm totally supportive of clarifying the language here. I just want to ensure we keep the existing, interoperable behavior.

The good news is, as spec writers, we don't have to choose ambiguity. If instead of something like:
...
Although, now I write this out, both lines would include an element like <div popover> that isn't showing, but is contained in the top layer via other means, such has a shown dialog or fullscreen'd element. Is that the intent?

That's definitely a bug in the spec, good catch. That was introduced by 1257236. The intention (and implementation in Chromium) does not include a <dialog popover> that is dialog.showModal(), even though that element is in the top layer. I think we need to repair this. @annevk I think we could fix this with @jakearchibald's suggested change above, but use:

and remove items whose popover attribute is not in the auto state, or whose popover visibility state is not showing.

[jakearchibald]

I just want to ensure we keep the existing, interoperable behavior.

I guess by "interoperable" here you mean the Chromium implementation, or is there another?

I think we need to repair this. @annevk I think we could fix this with @jakearchibald's suggested change above, but use:

and remove items whose popover attribute is not in the auto state, or whose popover visibility state is not showing.

LGTM!

[mfreed7]

I just want to ensure we keep the existing, interoperable behavior.

I guess by "interoperable" here you mean the Chromium implementation, or is there another?

Both <dialog> and fullscreen use the top-layer, and both are implemented in all three engines. (This was in reference to the comments about the language of the spec for top-layer things.)

[annevk]

Auto popover list is a filter on top of another list. It doesn't make sense to remove things from it. They get removed from the other list (or no longer match the condition).

[jakearchibald]

@annevk we're talking about the same thing. Another way to describe a filtered list is a clone with items removed. Infra doesn't have the concept of "filter", but we can add it.

[annevk]

Is it? What if items get added? If the current wording doesn't work I think we should figure out how to remove the entire concept. Reverting that commit makes things worse.

[jakearchibald]

I'm not talking about reverting that commit, unless I'm misunderstanding something. From the algorithms, it doesn't seem like the list needs to be "live", a snapshot during an algorithm is enough, and writing up how that snapshot is taken seems less ambiguous.

[annevk]

Looking at the callers I think you're right. We should just turn it into a getter then that does the filtering and we just call it at each call site.

[annevk]

What are the items that remain that would close this issue?

• Turn auto popover list into an algorithm that returns a list (or a set perhaps, don't think it can contain duplicates).

[jakearchibald]

Also this, which passes an element to an algorithm that expects a document.

[annevk]

@josepharhar do you feel comfortable tackling these two items?

[josepharhar]

I created a PR: #9241

[josepharhar]

The following attribute change steps are used for all HTML elements:

Nit: This algorithm references arguments that aren't specified up front. I'd like to fix this.

I opened another PR to fix this here: #9246