Repository navigation
Questions/issues with popover spec #9036
Description
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.
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.
Activity
jakearchibald commentedon Mar 16, 2023
https://html.spec.whatwg.org/multipage/popover.html#light-dismiss-open-popovers
This algorithm expects a
Documentbut it's being given an event target (presumably anElement).Does this mean a popover will close if you click it? I couldn't immediately see where that's prevented in the spec.
mfreed7 commentedon Mar 16, 2023
Thanks for the feedback! A few replies:
This is intended to mean "in the same order as the top layer set". (Top layer is an ordered set.)
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.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 commentedon Mar 16, 2023
I'll defer to @nt1m (unless he feels the same way, in which case I'll take a look 😊).
nt1m commentedon Mar 16, 2023
I don't have a strong opinion, my preference goes towards consistency here :)
jakearchibald commentedon Mar 16, 2023
Consistency with what?
nt1m commentedon Mar 16, 2023
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 commentedon Mar 16, 2023
One thing to think about if we remove the exceptions is whether we want to re-run all the steps, especially for show:
mfreed7 commentedon Mar 16, 2023
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 callinghidePopover()and thenshowPopover()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 commentedon Mar 17, 2023
Moving the throw/don't-throw discussion to #9045
Also identified an event timing issue #9046
jakearchibald commentedon Mar 20, 2023
The remaining issues:
https://html.spec.whatwg.org/multipage/popover.html#auto-popover-list
"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
Nit: This algorithm references arguments that aren't specified up front. I'd like to fix this.
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
This algorithm expects a
Documentbut it's being given an event target (presumably anElement).annevk commentedon Mar 20, 2023
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 commentedon Mar 20, 2023
I believe all of the comments just above are mostly spec nits and not behavior changes.
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 commentedon Mar 21, 2023
@annevk
Yeah, I think it's just the wording that's confusing. Changing it to an algorithm should clear it up.
@mfreed7
I think so, although that might depend on what the desired behaviour is.
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.
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:
it could say:
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 commentedon Mar 21, 2023
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.
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-indexon 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.
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 isdialog.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:1 remaining item
mfreed7 commentedon Mar 21, 2023
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 commentedon Mar 21, 2023
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 commentedon Mar 21, 2023
@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 commentedon Mar 21, 2023
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 commentedon Mar 21, 2023
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 commentedon Mar 22, 2023
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 commentedon Apr 20, 2023
What are the items that remain that would close this issue?
jakearchibald commentedon Apr 20, 2023
Also this, which passes an element to an algorithm that expects a document.
annevk commentedon Apr 20, 2023
@josepharhar do you feel comfortable tackling these two items?
Make auto popover list an algorithm
josepharhar commentedon May 1, 2023
I created a PR: #9241
Add parameters to popover attribute change steps
josepharhar commentedon May 2, 2023
I opened another PR to fix this here: #9246
Editorial: add parameters to popover attribute change steps