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

New feature proposal: Popover API #7785

Closed
mfreed7 opened this issue Apr 4, 2022 · 78 comments
Closed

New feature proposal: Popover API #7785

mfreed7 opened this issue Apr 4, 2022 · 78 comments
Labels
addition/proposal New features or enhancements

Comments

@mfreed7
Copy link
Contributor

mfreed7 commented Apr 4, 2022

This is quite related to #6349, which was previously discussed in WHATWG. However, over the course of the last year or so, the OpenUI CG has been working through issues and maturing the proposal with the help of great feedback and brainstorming from both developers and implementers. The latest explainer (https://open-ui.org/components/popup.research.explainer) has changed the shape of the API from a new element (<popup>) to a content attribute (popup), and has changed several of the details, primarily to improve accessibility and broaden the applicability of the API.

Importantly, the OpenUI group resolved that this proposal looks good enough to start prototyping. I have started that process for Chromium.

While there are still many open issues that we're working through, I think the shape of the API is in a good enough shape to bring before WHATWG for implementer interest and (importantly!) input / feedback / guidance. Hence this issue.

For convenience, here are the background and goals sections from the explainer:


Background

There is a need in the Web Platform for an API to create "popup UI". This is a general class of UI that always appear on top of all other content, and have both one-at-a-time and "light-dismiss" behaviors. This document proposes a set of APIs to make this type of UI easy to build.

Goals

Here are the goals for this API:

  • Allow any* element and its (arbitrary) descendants to be rendered on top of all other content in the host web application.
  • Include “light dismiss” management functionality, to remove the element/descendants from the top-layer upon certain actions such as hitting Esc (or any close signal) or clicking outside the element bounds.
  • Allow this “top layer” content to be fully styled, including properties which require compositing with other layers of the host web application (e.g. the box-shadow or backdrop-filter CSS properties).
  • Allow these top layer elements to reside at semantically-relevant positions in the DOM. I.e. it should not be required to re-parent a top layer element as the last child of the document.body simply to escape ancestor containment and transforms.
  • Allow this “top layer” content to be sized and positioned to the author's discretion.
    Include an appropriate user input and focus management experience, with flexibility to modify behaviors such as initial focus.
  • Accessible by default, with the ability to further extend semantics/behaviors as needed for the author's specific use case.
  • Avoid developer footguns, such as improper stacking of dialogs and popups, and incorrect accessibility mappings.
  • Avoid the need for Javascript for the common cases.
@mfreed7 mfreed7 added the agenda+ To be discussed at a triage meeting label Apr 4, 2022
@annevk
Copy link
Member

annevk commented Apr 5, 2022

I just realized that this naming clashes with window.open()'s popup feature as I initially thought this was about the new API for window.open() that we've been discussing. Perhaps it's enough if we can refer to this as viewport popups or some such, or perhaps we need slightly different name.

@annevk annevk added the addition/proposal New features or enhancements label Apr 5, 2022
@mfreed7
Copy link
Contributor Author

mfreed7 commented Apr 5, 2022

Yep, that’s unfortunate. More so because I worked on both of them, and should have thought of this potential confusion earlier. We are anyway bikeshedding the attribute name, so perhaps add your thoughts there?

@past past removed the agenda+ To be discussed at a triage meeting label Apr 8, 2022
@mfreed7
Copy link
Contributor Author

mfreed7 commented Apr 8, 2022

Ok, per the triage meeting, my action item was to organize and summarize the open issues for Popup. Hopefully this helps:

Behavior:

Bikeshedding:

Documentation / spec:

Resolved, just needs implementation/testing:

Relationships to other APIs:

Other:

@mfreed7
Copy link
Contributor Author

mfreed7 commented Aug 17, 2022

Quick update on this thread: the feedback from the HTML spec triage meeting on April 7 was that there were too many open issues for Pop-Up, and we should work to resolve them. OpenUI has been hard at work for the last 4 months discussing those issues and proposing ways forward. As of today, the open issues have been reduced to these 7 issues. The complex issues (see bigger list above, or this list of resolved issues) have been discussed at length and resolved (in the OpenUI sense), and the remainder are what I'd consider to be smaller issues. They still need a resolution, but I'm fairly confident we're on a path to that soon.

@josepharhar has been hard at work on a draft spec PR that explains all of the behaviors of the Pop-Up API, as designed and discussed within OpenUI. Stay tuned!

josepharhar added a commit to josepharhar/html that referenced this issue Aug 18, 2022
@josepharhar josepharhar mentioned this issue Aug 24, 2022
3 tasks
@josepharhar
Copy link
Contributor

I have opened a PR: #8221

@mfreed7
Copy link
Contributor Author

mfreed7 commented Sep 30, 2022

I just wanted to give a quick status update:

  1. The HTML spec PR for the Pop-up API is at #8221. We've received some comments, but would really appreciate some good, detailed reviews.
  2. For background and motivation, the best reference would be the explainer, which I've kept up-to-date as this feature has evolved in OpenUI. There is also this recent blog post, which has some nice working examples.
  3. The open issues list remains small, with nothing that I would consider a fundamental or blocking concern.

Generally, we're looking for feedback and input for this feature, to make sure it works as expected, solves the problems it is trying to solve, and plays nicely with the rest of the platform. Thanks in advance!

@annevk
Copy link
Member

annevk commented Oct 4, 2022

One problem I noticed is that the spelling conflicts with https://whatwg.org/style-guide. We have popup as a non-hyphenated-all-lowercase word.

The explainer also doesn't cover IDL attributes for the other attributes, such as defaultopen.

@josepharhar
Copy link
Contributor

One problem I noticed is that the spelling conflicts with https://whatwg.org/style-guide. We have popup as a non-hyphenated-all-lowercase word.

I believe "pop-up" was decided here: openui/open-ui#546 (comment)

@domenic do you have any thoughts about the style guide dictionary?

@annevk
Copy link
Member

annevk commented Oct 4, 2022

E.g., we spell it allow-popups-to-escape-sandbox not allow-pop-ups-to-escape-sandbox. I think it would be confusing if we did something else here. It's also confusing that it reuses the term to mean something completely different, of course.

@domenic
Copy link
Member

domenic commented Oct 5, 2022

I didn't realize we had existing web-developer-exposed uses of the word popups on the web platform. I don't have any guidance on how to resolve that. Maybe go back to my suggestion of a toplayer="" attribute.

@annevk
Copy link
Member

annevk commented Oct 7, 2022

@josepharhar the PR isn't really reviewable for me anymore through GitHub's UI. Perhaps you can squash the commits and force push?

Also, if there's some more history around why a CSS property was not the solution for this feature that would be appreciated. The summary seems to be that it would require some scripting and that there's a conflict with dialog and Fullscreen, but not a lot of details around that are provided. And in particular it seems that rendering would end up relying on CSS anyway.

@josepharhar
Copy link
Contributor

the PR isn't really reviewable for me anymore through GitHub's UI. Perhaps you can squash the commits and force push?

I merged with the latest tip of main, how does it look now? I can also squash all the commits into one but then github won't show me what lines all of the previous comments were made on, which is still helpful to me.

Also, if there's some more history around why a CSS property was not the solution for this feature that would be appreciated.

I believe these issues contain discussion about this:
w3c/csswg-drafts#6965
openui/open-ui#410
openui/open-ui#417

@annevk
Copy link
Member

annevk commented Oct 8, 2022

Screenshot 2022-10-08 at 7 12 51 AM

I strongly suspect some amount of squashing is needed to resolve that, but you don't have to squash all I think.

Thanks for the pointer to the CSS WG issue, I think that explains it best.

w3c/csswg-drafts#7319 also seems relevant here. Left a comment there about naming the pseudo-classes consistently with the API.

@mfreed7
Copy link
Contributor Author

mfreed7 commented Oct 8, 2022

Thanks for the pointer to the CSS WG issue, I think that explains it best.

There’s also this whole document that goes through the alternatives considered, including the linked section about a CSS property.

@annevk
Copy link
Member

annevk commented Oct 8, 2022

Yeah, that is actually what prompted me to ask for more details. 😊

@mfreed7
Copy link
Contributor Author

mfreed7 commented Oct 8, 2022

Yeah, that is actually what prompted me to ask for more details. 😊

Oh ok! Well then I’m glad w3c/csswg-drafts#6965 also helped. Perhaps I should link to that issue from the alternative doc.

@josepharhar
Copy link
Contributor

I strongly suspect some amount of squashing is needed to resolve that, but you don't have to squash all I think.

Ah I see I was able to reproduce the same error. I squashed everything down to one commit and it looks like it is working now!

mfreed7 added a commit to mfreed7/open-ui that referenced this issue Oct 11, 2022
It was [mentioned](whatwg/html#7785 (comment)) that this CSSWG issue was helpful, so this adds a direct link.
@mfreed7
Copy link
Contributor Author

mfreed7 commented Oct 11, 2022

Yeah, that is actually what prompted me to ask for more details. 😊

Oh ok! Well then I’m glad w3c/csswg-drafts#6965 also helped. Perhaps I should link to that issue from the alternative doc.

openui/open-ui#618

mfreed7 added a commit to openui/open-ui that referenced this issue Oct 12, 2022
It was [mentioned](whatwg/html#7785 (comment)) that this CSSWG issue was helpful, so this adds a direct link.

Co-authored-by: Mason Freed <masonf@chromium.org>
@flackr
Copy link

flackr commented Nov 17, 2022

(There's also a problem that it currently doesn't trigger a style change event (as defined for getAnimations()) and as such it wouldn't actually pick up new CSS Animations or CSS Transitions, only script-originated animations.)

This is a constructive point - I'd like to dig into this one a bit. Given the current algorithm, I think you're right. But we could very easily just add a style recalc after firing the popoverhide event, right?

Calling getAnimations triggers a style change event by spec. Isn't this called at the point when we would need to trigger the style change and pick up the new animation?

@mfreed7
Copy link
Contributor Author

mfreed7 commented Nov 17, 2022

(There's also a problem that it currently doesn't trigger a style change event (as defined for getAnimations()) and as such it wouldn't actually pick up new CSS Animations or CSS Transitions, only script-originated animations.)

This is a constructive point - I'd like to dig into this one a bit. Given the current algorithm, I think you're right. But we could very easily just add a style recalc after firing the popoverhide event, right?

Calling getAnimations triggers a style change event by spec. Isn't this called at the point when we would need to trigger the style change and pick up the new animation?

Thanks @flackr - I just verified that at least in Chromium's implementation, it does work correctly for added CSS animations and transitions.

@flackr
Copy link

flackr commented Nov 17, 2022

As such, I tend to agree with @domenic that we really need to pursue a generic solution for the display:none problem.

How do you propose we do that? The View Transitions API doesn't satisfy the requirements. Is there another approach you'd recommend?

I have a strawman proposal for generic display none animations at w3c/csswg-drafts#6429

@annevk
Copy link
Member

annevk commented Nov 18, 2022

Isn't this true of any solution to this problem?

I'm not sure, but if every solution constraints the way you can use animations, I don't think we're on the right path. @flackr's work on display:none seems promising though.

Isn't this called at the point when we would need to trigger the style change and pick up the new animation?

No, the popover specification doesn't call this method (and shouldn't, since it's a public API). It does the first part of this method, collecting the Animation objects, but not style recalc.

@chrishtr
Copy link
Contributor

I chatted with @graouts about the animation aspect of popover and he pointed out that since we don't know the semantics of the collected Animation objects this might not work well. E.g., if you create a forever-running animation for a popover element you could never hide the element.

The forever-running animation, if started before dismissing the popover, would be ignored, because the algorithm for popovers looks only for animations that start right at that time. If an animation was selected that starts at that time and runs too long, then that would be a site bug, and a mistake that developers can make for any animation. Is it any worse of a failure mode for popovers? Perhaps we should think about adding a UA-supplied timeout when the hide is triggered by a UA feature such as modal dialog or fullscreen, to ensure it completes in a reasonable amount of time.

@chrishtr
Copy link
Contributor

No, the popover specification doesn't call this method (and shouldn't, since it's a public API). It does the first part of this method, collecting the Animation objects, but not style recalc.

You're right - the current spec PR doesn't actually do all of the things that getAnimations() does, but it should (though the Chromium implementation does). That’s a spec bug; we'll make that change shortly and then ping this thread - thanks for clarifying.

In offline discussions, @domenic raised this question as well: what about animations on elements in the popover subtree but not the popover itself? Why can’t we detect those animations?

The answer is that we do detect them. The implementation uses getAnimations({subtree:true}). I think it might be another spec PR bug (or perhaps the same spec bug as I mentioned above); we’ll get it fixed. Thanks Domenic for pointing this out!

@chrishtr
Copy link
Contributor

chrishtr commented Nov 21, 2022

I’d also like to respond to the suggestion that we use a generic transition-to-display:none mechanism for popover exit animations. (aside: such a general mechanism for CSS is a great idea and a lot of developers have asked for it; that is tracked by Rob’s proposal here). If a popover or dialog transitioned only into the display:none state and there were no :open or :closed pseudo classes or top-layer involvement, then indeed we could use this mechanism.

But popovers allow the developer to close into a non-display:none state, also have these pseudo classes, and utilizes the top layer. So we need to keep it in the top layer during the animation regardless of destination style; if not, during the animation it would:

  • Break ancestor clipping and z-index from its presence in the top layer
  • Break developer styles that are designed to display it inline with other content after the animation completes

However, as the spec currently stands, :open is removed at the beginning of the popover dismiss animation, yet the popover remains in the top layer during the animation. Also, during the animation, neither :open nor :closed apply (@domenic mentioned this situation as well in a side chat). I agree these are a little complicated/inconsistent, so I propose the following:

  • :open always applies when the popover is in the top layer. :closed always applies otherwise (change from current spec PR)
  • Popover close animations run while the popover is in the top layer (no change from spec PR)
  • During the close animation, a new pseudo class :closing is applied (change from spec PR). :open still applies (since the popover is still in the top layer). This allows developers and the UA to apply an exit animation by targeting :closing in their animation styles.
  • During an animation to open a popover, the popover is in the top layer during the animation (and hence :open applies) (no change from spec PR).
  • Add a :willopen pseudo class that only applies after the first style recalc of an open animation (the first one here).

These changes should address the concerns above (about multiple states/inconsistency), while at the same time providing a cleaner way for developers to target the animation specifically.

This is all probably kind of confusing, so let’s look at a sample developer style for open and close animations. Open animation:

/* General baseline styles for the popover */
popover {
  display: flex;
  transition: opacity 150ms, transform 150ms; 
}

/* Closed state */
popover:closed {
  display:none; /* UA default style */
}

/* Open state */
popover:open {
  background: green;
}

/* Style indicating first frame of an opening animation */
popover:willopen {
  opacity: 0;
  transform: translateY(-50px);
}

What happens on open is:

  1. UA fires the popovershow event (cancellable)
  2. UA removes :closed and adds :willopen and :open, and places the popover in the top layer
  3. UA forces style recalc (which has the effect of storing off the first animation keyframe)
  4. UA removes :willopen (animations start here, if any)

Close:

/*  Style indicating last frame of a closing animation */
popover:closing {
  opacity: 0;
  transform: translateY(50px); /* Notice how it need not be in the same state as `:willopen` */
}

What happens:

  1. UA forces style recalc and collects running animations
  2. UA fires the popoverhide event (not cancellable)
  3. UA adds :closing
  4. UA forces style recalc, collects running animations, and diffs these animations from step 1
  5. On completion of animations in step 4 (which run of their own accord), UA removes :open & :closing, removes the popover from the top layer, and adds :closed

@chrishtr
Copy link
Contributor

Also, when the popoverhide event fires, the author could elect to create an animation with a delay using setTimeout or requestAnimationFrame, instead of using the delay property on the animation. While it may be regrettable that the author chooses this approach, it's valid and it's not clear how we can determine these animations are directly linked to the popover hiding.

One thing we could do is have the popoverhide event handler be able to return a promise (View Transitions also does this for other use cases), which the UA can use to delay removal of the popover from the top layer. This allows the setTimeout pattern to work, and also allows a script-driven requestAnimationFrame animations to cleanly integrate with the dismiss animation.

That also addresses the first half of this feedback from @heycam:

Having an explicit API for the author to use to indicate when their animations are done (via providing a Promise?) seems more understandable to me than the automatic capture and comparison of running animations, but you definitely lose the conciseness of just adding a transition on a :open rule.

@chrishtr
Copy link
Contributor

chrishtr commented Nov 21, 2022

Next: what about <dialog> (or <details>) animations?

I think we should use the pattern pioneered by popover: it’s either in the open or closed state at all times, and may additionally be closing or opening, with pseudo classes for each. The main thing we’ll need to work through for those is how to add this without breaking existing content.

Finally: why not view transitions?

View transitions are designed for arbitrary DOM changes, and as such have a more complex mechanism for animation setup that works in any situation, but comes at the cost of substantial restrictions to the type of animations that can be applied. For example, contain:layout is required, they always stack on top of everything including other top layer items, and animations of the content of the element (e.g. color and sub-layout) are not supported. (View transitions are awesome BTW, just stating their limitations.)

Popover entry and exit are animations of a persistent element, and therefore can support any animation supported by existing animation APIs. In addition, since the element is persistent, I think developers would expect these APIs to work.

Whereas if we had needed an animation that destroyed one element and created another, but wanted to cross-fade between them, then View Transitions would make more sense.

By the way, the feedback raised here was excellent and useful, thanks all!

p.s. I also didn’t write this comment (or the several previous) alone, it was written jointly with @flackr and @mfreed7.

@josepharhar
Copy link
Contributor

I have removed the animations integration from the PR

@mfreed7
Copy link
Contributor Author

mfreed7 commented Dec 2, 2022

I have removed the animations integration from the PR

As mentioned above, we've decided to simplify the current popover spec PR by removing the animations stuff for now, while we figure out the best path forward to support popover animations. We'll likely open a fresh issue to tackle that subject, once we've had a chance to give it some thought.

@annevk

@mfreed7 mfreed7 changed the title New feature proposal: Popup API New feature proposal: Popover API Dec 8, 2022
@annevk
Copy link
Member

annevk commented Dec 9, 2022

Thanks, that sounds great!

@muratcorlu
Copy link

I couldn't be sure where to write this but I think it would be great if it will be possible to set an element reference for anchor without determining an id for it. Working with string ids in complex web projects (like micro frontends), providing a unique ID for a target element would be a challenge. In that kind of situation, passing the reference of the element to popover would be a safer solution, I believe. I'm not sure if it's ok in principle to accept 2 different types for a single property in native elements (like popoverEl.anchor = el as Element), otherwise, maybe there can be an optional parameter in showPopover(anchorElement?: Element).

If this is somehow possible already and I missed it in the documents, sorry for that.

@annevk
Copy link
Member

annevk commented Mar 3, 2023

The anchor feature was dropped.

@mfreed7
Copy link
Contributor Author

mfreed7 commented Mar 3, 2023

The anchor feature was dropped.

Or… it was moved to the anchor positioning proposal, so it’s not directly part of popover. When it comes back, it’s very likely to follow the pattern of popovertarget and get an element reflection property. So you could do myPopover.anchorElement = myAnchor;. But as @annevk said, that’s not part of this proposal anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements
Development

No branches or pull requests