-
Notifications
You must be signed in to change notification settings - Fork 22.6k
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
Add imperative invoker updates to popover API #37133
base: main
Are you sure you want to change the base?
Conversation
- showPopover and togglePopover can now take an options parameter.
Probably needs a browser compat data update too. |
Preview URLs (comment last updated: 2024-12-09 01:46:06) |
cc @chrisdavidmills I just noticed you mentioned working on this, so feel free to close this if you've already done it or want more detailed changes! |
@lukewarlow Howdy, is there an associated BCD update showing this is implemented anywhere? EDIT ... and only now I see your comment. Are you planning on doing this. |
- : An {{domxref("HTMLElement")}} that triggered the popover, if any. | ||
This provides all the same functionality that using [`popovertarget`](/en-US/docs/Web/HTML/Element/button#popovertarget) would provide. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but I don't understand what this is for, why it is needed, or how it is used.
Say I have a button and I trigger the popover with that button, why does the popover care what triggered it. What if I lie and specify some random element, or omit this? Seems to make zero difference if it is defined or not.
I get popovertarget
- that creates a link from a button (say) to the popover, but in code you create that link by ... calling this method on the thing you want want to show.
Can you create an example in both the docs that shows how this is used, and explain those things?
Also would be good to add an example in https://developer.mozilla.org/en-US/docs/Web/API/Popover_API/Using. If this is somehow like popovertarget
, perhaps drawing the connection would be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukewarlow @chrisdavidmills Either way, this can't merge as is because IMO no one can understand the point of this without doing extra reading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies I meant to respond here, while coming up with the more detailed explanation I actually found a bug in chrome's implementation so I was double checking that my understanding was correct. I'll spend some time to make this more detailed.
Fwiw here's the 4 things this does:
-
It correctly handles nested popover scenarios, if you invoke a popover from within a popover it shouldn't close it. Currently that only works declaratively, this new feature lets you do it imperatively.
-
It fixes up the focus order such that the contents of the popover are the next tab stop once opened. Currently this only works declaratively. This new feature will do this imperatively (this is the bug I found).
-
It might have an impact on light dismiss behaviour. (This might be both a spec and implementation bug).
-
Both imperative and declarative now setup an implicit anchor element for anchor positioning (this patch doesn't cover that I'd prefer if that was a follow-up)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lukewarlow - looking forward to seeing what you come up with. I kind of understand what you are saying in points one and two, but hard to "fully understand" without a live example.
I'm around this week but away in first 2-ish weeks of January and sporadically in between. You're welcome to seek another reviewer if I'm not around when you get to this.
Thanks @lukewarlow. I was holding off on working on this until whatwg/html#10728 was merged into the spec, but I see it has now happened. I can handle the other bits later in the month, or early next month. |
Description
Add imperative invoker updates to popover API
showPopover and togglePopover can now take an options parameter.
Motivation
So people can discover this new functionality.
Additional details
Released in Chromium 133 - https://chromestatus.com/feature/5120638407409664
Related issues and pull requests
Relates to mdn/mdn#598 (this doesn't cover the anchor positioning change)