Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Difference between auto popover list and filtered top layer? #8941

Closed
nt1m opened this issue Feb 23, 2023 · 8 comments · Fixed by #8969
Closed

Difference between auto popover list and filtered top layer? #8941

nt1m opened this issue Feb 23, 2023 · 8 comments · Fixed by #8969
Labels
topic: popover The popover attribute and friends

Comments

@nt1m
Copy link
Member

nt1m commented Feb 23, 2023

Is there a difference between the document auto popover list and a filtered top layer? Is there a reason both should exist?

@josepharhar @mfreed7

@josepharhar
Copy link
Contributor

I suppose that having both makes it easier to work with. I can't think of any difference.

Using the top layer list instead shouldn't be observable, so I'm not motivated to change chromium's implementation or make a spec PR myself.

@nt1m
Copy link
Member Author

nt1m commented Feb 23, 2023

@josepharhar There is a slight behavior difference in the hide popover algorithm, the popover is getting removed from the auto popover list before the beforetoggle event, but from the top layer after that event.

Not sure if that's intentional or an oversight

@annevk annevk added topic: popover The popover attribute and friends clarification Standard could be clearer labels Feb 24, 2023
@annevk
Copy link
Member

annevk commented Feb 24, 2023

Indeed. If the intent is that the difference is not observable we should either make modify both at the same time or define the auto popovers as a filter on top of the top layer.

@mfreed7
Copy link
Collaborator

mfreed7 commented Feb 24, 2023

@josepharhar There is a slight behavior difference in the hide popover algorithm, the popover is getting removed from the auto popover list before the beforetoggle event, but from the top layer after that event.

Not sure if that's intentional or an oversight

This isn't, strictly speaking, observable though, right? There's no way to observe the top layer ordering from JS. If you can think of a way to observe the timing of removal from top layer and removal from the popover list, I'd be inclined to call that a bug and we should think about it.

If this isn't an observable change, then for two reasons, I'd lobby for keeping the existing separate popover list:

  1. It makes the spec easier to read. We're still working on expansions of the popover feature, such as adding back popover=hint, and the explicit list makes that easier to grok. Nothing says implementations have to have an explicit list.
  2. The related work on animation of the top layer status ([css-animations-2, css-transitions-2] Entry and exit animations for top-layer elements w3c/csswg-drafts#8189) might also complicate things in the spec, if we write this as a filtering of the top layer list. Maybe not, but I guess that remains to be seen.

As @josepharhar mentioned though, since this isn't observable, I think we're ok changing the spec if it's something you feel strongly about. So long as the change doesn't change behavior and doesn't get in the way of the above items, ok.

@nt1m
Copy link
Member Author

nt1m commented Feb 24, 2023

Ah right, I just remembered the beforetoggle event when hiding is not cancelable, so this doesn't make a difference.

@annevk mentioned maybe we could just define auto popover list in terms of top layer, so we can still refer to it as it is.

Anyway, I don't feel too strongly about this, I mostly filed this issue to see if there were some subtle differences that I missed, but it doesn't sound like it.

@mfreed7
Copy link
Collaborator

mfreed7 commented Feb 25, 2023

@annevk mentioned maybe we could just define auto popover list in terms of top layer, so we can still refer to it as it is.

Ahh, I think defining it this way alleviates my main concerns also, which were about clarity. So I’m ok with doing this or not - up to you.

@annevk
Copy link
Member

annevk commented Feb 26, 2023

@mfreed7 I think the point is that the current text is not clear as the mutations happen in different locations which leads one to think that's intentional.

@annevk
Copy link
Member

annevk commented Feb 28, 2023

Are only elements whose popover attribute is in the auto state supposed to be in this list? Because that doesn't seem to be enforced at the moment. The more I look at this concept, the less clear it is to me.

@annevk annevk removed the clarification Standard could be clearer label Mar 1, 2023
annevk added a commit that referenced this issue Mar 1, 2023
This solves an issue with elements being unconditionally added to this list and makes it clearer that the steps between it being added here and the top layer didn't mean anything.

This also makes hide popover use check popover validity more to rely on the shared infrastructure.

Fixes #8941 and fixes #8964.
annevk added a commit that referenced this issue Mar 3, 2023
This solves an issue with elements being unconditionally added to this list and makes it clearer that the steps between it being added here and the top layer didn't mean anything.

This also makes hide popover use check popover validity more to rely on the shared infrastructure.

Fixes #8941 and fixes #8964.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: popover The popover attribute and friends
Development

Successfully merging a pull request may close this issue.

4 participants