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

[selectors-4] Add syntax to establish before-change style for css-transitions on new elements. #8174

Closed
flackr opened this issue Dec 2, 2022 · 54 comments
Labels
Closed Accepted by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-transitions-2 selectors-4 Current Work

Comments

@flackr
Copy link
Contributor

flackr commented Dec 2, 2022

Transitions are ergonomic and easy to use for changes to property values when there is an existing style to transition from. However, when an element is first added to the page or made not display: none it has no previous before-change style so developers have to use script to switch from the initial frame state and some state which specifies the post transition style, e.g.

elem.classList.add('visible'); // Removes display: none, but keeps opacity: 0 and other before styles.
elem.offsetTop; // Force style recalc
elem.classList.add('visiblestyle'); // Adds properties to transition to.

It would be great if you could do this directly from CSS perhaps via a pseudoclass selector. E.g.

dialog {
  transition: opacity 200ms;
  opacity: 1; /* Not necessary as it is the default but added for illustration. */
}
dialog:initial {
  opacity: 0;
}

This :initial pseudo-class would match on the initial frame containing a particular element and be removed immediately after style recalc. This does mean we would have to repeat style on that initial frame - hopefully we could target only elements affected by :initial selectors.

This would bring parity with what you can do via css animations, e.g.:

@keyframes fade-in {
  from { opacity: 0; }
}
dialog {
  transition: opacity 200ms;
  animation: fade-in 200ms;
  opacity: 1; /* Not necessary as it is the default but added for illustration. */
}

Though thinking ahead, I've also heard frustration around the css animation above animating when you load the page - so we should consider where there's a way for developers to differentiate loading vs post load and only apply animations (and initial transition styles) for new elements.

@flackr flackr added the selectors-4 Current Work label Dec 2, 2022
@flackr
Copy link
Contributor Author

flackr commented Dec 2, 2022

Though thinking ahead, I've also heard frustration around the css animation above animating when you load the page - so we should consider where there's a way for developers to differentiate loading vs post load and only apply animations (and initial transition styles) for new elements.

I filed #8175 to talk through this particular area separately.

@lilles
Copy link
Member

lilles commented Dec 6, 2022

It could work if :initial is matching if an element had no style (first frame or display:none) before the element style recalc is done for an element and reapply without :initial immediately. I'm not 100% sure about the consequences where we are looking at multiple passes like for container queries. Whether a multi-pass layout is stateless.

Since the before-change style would be based on the previous frame's style, or perhaps the :initial based styles, we probably just need to have a single :initial style pass even if the style would flip back and forth between display:none for the same update.

@andruud
Copy link
Member

andruud commented Dec 7, 2022

This :initial pseudo-class would match on the initial frame containing a particular element and be removed immediately after style recalc. This does mean we would have to repeat style on that initial frame - hopefully we could target only elements affected by :initial selectors.

Rather than thinking terms in of "frames", it could make sense to express this as a way of bringing into existence a before-change style when there otherwise would be none. It could work like this:

  • We resolve style for <dialog> (normally), :initial does not match, but we do note the existence of relevant :initial pseudo-classes. (This is similar to how ::before/etc is handled in Blink). This is the after-change style, same as today.
  • At the point where we want to trigger a transition, we'd normally say "sorry, no before-change style, no transition". But, since we've noted that :initial comes into play, we do an on-the-spot style resolution to produce that before-change style (this time with :initial matching). Then we calculate the transition normally.

@flackr
Copy link
Contributor Author

flackr commented Dec 8, 2022

@ANDUUD It's a really compelling idea to do this just in time for starting transitions, if it helps. Whether to start a transition also seems to depend on the before-change transition duration (demo) though it's unclear from the spec whether this is right. Anyways, in this case we would always have to calculate the initial style, so does it still make sense to do this if it will happen for every element? Also, will this support combinator selectors from :initial?

@andruud
Copy link
Member

andruud commented Dec 8, 2022

Whether to start a transition also seems to depend on the before-change transition duration (demo) though it's unclear from the spec whether this is right.

@flackr That demo has a mistake which makes it seem like the before-style matters (for the transition properties), but it actually doesn't (fixed demo, the difference is the <div class="target closed"> part). Spec also seems clear that the "matching transition property value" (etc) is taken from the after-change style. So I think we're good?

Also, will this support combinator selectors from :initial?

Right ... so e.g. does .a:initial > .b imply that a before-change style for .b exists? That sounds complicated. Maybe this concept is closer to a pseudo-element: we could use dialog::initial instead to avoid all these questions. Pseudo-elements representing a real element in a certain state is not entirely without precedent (::slotted()), although a pseudo-element being its own originating element is perhaps new. :-)

@flackr
Copy link
Contributor Author

flackr commented Dec 8, 2022

Whether to start a transition also seems to depend on the before-change transition duration (demo) though it's unclear from the spec whether this is right.

@flackr That demo has a mistake which makes it seem like the before-style matters (for the transition properties), but it actually doesn't (fixed demo, the difference is the <div class="target closed"> part). Spec also seems clear that the "matching transition property value" (etc) is taken from the after-change style. So I think we're good?

Yup! That's great! So we'll only need to do this when we have a non-zero transition duration making this a good optimization.

Also, will this support combinator selectors from :initial?

Right ... so e.g. does .a:initial > .b imply that a before-change style for .b exists? That sounds complicated. Maybe this concept is closer to a pseudo-element: we could use dialog::initial instead to avoid all these questions. Pseudo-elements representing a real element in a certain state is not entirely without precedent (::slotted()), although a pseudo-element being its own originating element is perhaps new. :-)

Yes, this sounds perfect. I think it's reasonable to require .a > .b::initial in this particular example, and if we take a more confusing example .a:initial + .b, if .b already existed it should just be using its already established before-change style, and if it didn't the developer could have used .a + .b::initial

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Pseudo-class before-change style for transitions on new elements.

The full IRC log of that discussion <fantasai> Topic: Pseudo-class before-change style for transitions on new elements
<Rossen_> q?
<fantasai> github: https://github.com//issues/8174
<fantasai> flackr: when coming from display:none have no display style
<fantasai> ... so can't define a from state
<fantasai> ... can define in CSS animation, but less ergonomic
<fantasai> ... so proposing we add an :initial pseudo-class which establishes the initial style
<fantasai> ... the before-change style for CSS transitions
<fantasai> ... in discussion, decided that if we made this a pseudo-element
<chris> rrsagent, here
<RRSAgent> See https://www.w3.org/2023/01/11-css-irc#T17-10-12
<fantasai> ... we could optimize this by preventing e.g. sibling selectors and only require occasional style calculation when it's used
<fantasai> ... so proposal is to have ::initial establish the initial style
<emilio> q+
<Rossen_> q?
<Rossen_> ack emilio
<fantasai> emilio: This would be a pseudo-element that matches against an element?
<fantasai> emilio: so matches element rules plus magic pseudo-element?
<fantasai> pseudo-element would be applied on top of element rules
<fantasai> emilio: that's pretty different from pseudo-element
<fantasai> emilio: I don't think we have a precedence for this, which is a bit tricky
<fantasai> emilio: I'm also confused when you want it
<fantasai> emilio: this is for coming back from display:none?
<fantasai> emilio: seems implementable, but seems tricky and weird to make it a pseudo-element
<fantasai> emilio: because targetting two element styles on one
<fantasai> emilio: weird wrt cascade
<fantasai> flackr: that's good feedback for pseudo-element approach
<fantasai> flackr: could also take pseudo-class approach, but less easily optimizable?
<fantasai> emilio: in what sense?
<fantasai> flackr: if you have combinator selectors, element showing up in the DOM can cause recalcuation
<fantasai> emilio: but that's the case for all pseudo-element
<fantasai> flackr: this is extra style recalc that doesn't normally exist
<fantasai> emilio: I'm a bit confused, you recalc more if you have :initial along the combinators.... depending how wel you optimize
<fantasai> flackr: I think given :initial is only establishing before-change style and otherwise doesn't apply to the element, not that weird that it conflicts with other styles
<fantasai> flackr: because those other styles can't initiate transitions from display: none
<fantasai> emilio: we have a similar case with pseudo-classes for links
<fantasai> emilio: need to resolve :visited / :unvisitied, need to do unconditionally, but dont' understand why :initial would be slower
<fantasai> flackr: if initial is a pseudo-class, can be used as part of combinator selectors
<fantasai> flackr: so have to resolve all those combinators
<fantasai> flackr: but if pseudo-element, can't be used in those combinators
<fantasai> emilio: for :visited we have special case
<fantasai> emilio: basically :visited on left of sibling is ignored
<fantasai> flackr: ah, we could do that
<fantasai> emilio: the way I think of implementing it is, we have this element ...
<fantasai> emilio: otherwise :initial only matches rightmost compound
<fantasai> flackr: that sounds reasonable to me
<fantasai> emilio: that would be less weird
<futhark> q+
<fantasai> +1 to emilio
<Rossen_> ack futhark
<fantasai> futhark: ignoring the pseudo-class, when resolving selectors, would have already computed style for previous one so would have eixsting computed style
<fantasai> futhark: so :initial wouldn't match on those elements
<fantasai> emilio: Right. :initial can only match in this special case
<fantasai> emilio: "this element is now :initial", now compute
<fantasai> emilio: with :has() have similar dependency...
<fantasai> futhark: Similar if getComputedStyle() in display:none subtree
<fantasai> futhark: You need to ensure you have styles there to do inherit etc.
<fantasai> emilio: not for siblings, but for ancestors
<fantasai> emilio: I'd rather say that it only matches on the rightmost
<flackr> +1
<chrishtr> +1
<fantasai> fantasai: How does this interact with page loading, partly loading?
<fantasai> flackr: applies anytime the element first gets a layout box
<emilio> q+
<fantasai> flackr: this would apply any time you add element to a page
<fantasai> flackr: separate issue for avoiding running things during load
<fantasai> flackr: but this is pre-existing problem for animations, which run on page load
<fantasai> Rossen_: So based on description, this would not fire on display:contents ?
<fantasai> Rossen_: since it doesn't have a box?
<fantasai> flackr: that sounds right
<Zakim> fantasai, you wanted to ask how this interacts with loading
<Rossen_> ack fantasai
<Rossen_> ack emilio
<fantasai> emilio: I think I'd rather special-case it to display:none since that's the actual thing
<PaulG> q+
<futhark> +1 to emilio.
<fantasai> emilio: at least, I don't think display:contents stops animations in the subtree, whereas display:none does
<Rossen_> ack PaulG
<fantasai> PaulG: If we're leaning towards pseudo-element, how would that affect AX tree?
<fantasai> PaulG: if not supposed to be animated at that time, just want to make sure we consider that
<fantasai> flackr: don't think we're leaning towards the pseudo-element approach
<fantasai> flackr: but not intenteded to establish a separate elmeent, just style the real element in the before-change state
<Rossen_> ack fantasai
<fantasai> fantasai: Suggest maybe flackr and emilio can come back with a revised proposal, and we can move to other issues
<fantasai> flackr: fwiw, AX should be equivalent to animations
<fantasai> Rossen_: ok, let's end here
<fantasai> Rossen_: come back when you're ready, thanks for introducing

@Loirooriol
Copy link
Contributor

What does :initial { display: none } do?
When an element would start generating a box, it applies, preventing the box, then stops applying, so the element is going to try to generate a box again, so :initial applies again, and so on?
Just want to make sure there is no infinite loop.

@AutoSponge
Copy link

APA would want to ensure that any pseudo-element creation (especially in a live region) preserves the current patterns for updating the AxTree and doesn't lead to "double reads" by assistive technology.

@flackr
Copy link
Contributor Author

flackr commented Jan 11, 2023

What does :initial { display: none } do? When an element would start generating a box, it applies, preventing the box, then stops applying, so the element is going to try to generate a box again, so :initial applies again, and so on? Just want to make sure there is no infinite loop.

:initial is only used as the before-change style of a transition. So display: none would return to the status quo of the before-change style not existing during the initial layout and thus not starting transitions.

@flackr
Copy link
Contributor Author

flackr commented Jan 11, 2023

@emilio Per the discussion, the new proposed resolution is the original one with your proposed restrictions:

:initial is a pseudoclass that doesn't match within :has and can only match on the right-most compound selectors, which establishes the before-change style for transitions.

@nt1m
Copy link
Member

nt1m commented Jan 17, 2023

My issue with this proposal is that this is designed around browser engine internals rather than concepts web developers understand. Not sure the definition of "initial" will be easy to grasp for web developers.

My gut feeling is that the spec should be rewritten in a way where the following "just works", since that is what I think most web developers will expect.

div {
  display: none;
  opacity: 0;
  transition: opacity 2s;
}

div.visible {
  opacity: 1;
  display: block;
}

I don't think:

dialog {
  transition: opacity 200ms;
  opacity: 1; /* Not necessary as it is the default but added for illustration. */
}
dialog:initial {
  opacity: 0;
}

is more explicit than:

@keyframes fade-in {
  from { opacity: 0; }
}
dialog {
  animation: fade-in 200ms;
  opacity: 1; /* Not necessary as it is the default but added for illustration. */
}

nor it is much shorter to write.

@nt1m
Copy link
Member

nt1m commented Jan 17, 2023

My initial reaction to the issue was that maybe this should be solved with a CSS property + at-rule (like scroll-linked animations for instance), but I realized we could already do this with @keyframes. Given the other issues around top layer animations etc. maybe this is an option that could be considered, that would solve these issues more globally.

@flackr
Copy link
Contributor Author

flackr commented Jan 17, 2023

My gut feeling is that the spec should be rewritten in a way where the following "just works", since that is what I think most web developers will expect.

If we could make this work, it would still only work for content which was already in the DOM in a previous state. E.g. I couldn't create an entry transition for a dialog which was just added or had never had style computed before.

My initial reaction to the issue was that maybe this should be solved with a CSS property + at-rule (like scroll-linked animations for instance),

Could you write an example of what you're thinking? Or is it basically animation keyframes?

but I realized we could already do this with @Keyframes. Given the other issues around top layer animations etc. maybe this is an option that could be considered, that would solve these issues more globally.

You are absolutely right that with @keyframes you can already do this, apologies if I didn't call out well enough that this use case is already possible with CSS animations - I tried to make that point. This proposal is born out of concerns that keyframes are harder for simple animations. @AmeliaBR brought this up over here.

@astearns astearns removed the Agenda+ label Jan 24, 2023
@lilles
Copy link
Member

lilles commented Jan 31, 2023

:initial as a pseudo class instead of ::initial pseudo element also means we have issues with :is/:where etc:

<style>
  :is(.a, div:initial, .b) {}
</style>
<div></div>
<div class="a"></div>
<div class="b"></div>

Implementations would typically stop matching when a matching selector is found. If :initial is allowed inside :is/:where we would have to match all to detect if the extra before-change style needs to be computed.

If we keep :initial as a pseudo class, I think we should add further restrictions to where :initial matches to avoid having to do unnecessary matching to detect presence of :initial styles.

@chrishtr
Copy link
Contributor

chrishtr commented Feb 1, 2023

Agenda+ to propose resolving on the behavior described in this comment.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 7, 2023
This isn't part of the landed spec [1], and will be replaced by
a combination of these five CSSWG issues:

- w3c/csswg-drafts#4441
- w3c/csswg-drafts#6429
- w3c/csswg-drafts#8174
- w3c/csswg-drafts#8189
- w3c/csswg-drafts#8389

After this CL, you will no longer be able to animate your
popover like this:

```
  [popover] {
    opacity: 0;
    transition: opacity 0.2s;
  }
  [popover]:open {
    opacity: 1;
  }
```

Instead you'll need to use CSS animations or (eventually) transitions
and you'll have to explicitly declare the `display` and `top-layer`
properties:

```
  transition: opacity 0.2s, display 0.2s, top-layer 0.2s;
```

[1] https://html.spec.whatwg.org/multipage/popover.html

Bug: 1307772,1413556
Change-Id: I4877dd69a06f2624bdb463b065b2e2b66cbf1154
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 10, 2023
This isn't part of the landed spec [1], and will be replaced by
a combination of these five CSSWG issues:

- w3c/csswg-drafts#4441
- w3c/csswg-drafts#6429
- w3c/csswg-drafts#8174
- w3c/csswg-drafts#8189
- w3c/csswg-drafts#8389

After this CL, you will no longer be able to animate your
popover like this:

```
  [popover] {
    opacity: 0;
    transition: opacity 0.2s;
  }
  [popover]:open {
    opacity: 1;
  }
```

Instead you'll need to use CSS animations or (eventually) transitions
and you'll have to explicitly declare the `display` and `top-layer`
properties:

```
  transition: opacity 0.2s, display 0.2s, top-layer 0.2s;
```

[1] https://html.spec.whatwg.org/multipage/popover.html

Bug: 1307772,1413556
Change-Id: I4877dd69a06f2624bdb463b065b2e2b66cbf1154
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 13, 2023
This isn't part of the landed spec [1], and will be replaced by
a combination of these five CSSWG issues:

- w3c/csswg-drafts#4441
- w3c/csswg-drafts#6429
- w3c/csswg-drafts#8174
- w3c/csswg-drafts#8189
- w3c/csswg-drafts#8389

After this CL, you will no longer be able to animate your
popover like this:

```
  [popover] {
    opacity: 0;
    transition: opacity 0.2s;
  }
  [popover]:open {
    opacity: 1;
  }
```

Instead you'll need to use CSS animations or (eventually) transitions
and you'll have to explicitly declare the `display` and `top-layer`
properties:

```
  transition: opacity 0.2s, display 0.2s, top-layer 0.2s;
```

[1] https://html.spec.whatwg.org/multipage/popover.html

Bug: 1307772,1413556
Change-Id: I4877dd69a06f2624bdb463b065b2e2b66cbf1154
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 14, 2023
This isn't part of the landed spec [1], and will be replaced by
a combination of these five CSSWG issues:

- w3c/csswg-drafts#4441
- w3c/csswg-drafts#6429
- w3c/csswg-drafts#8174
- w3c/csswg-drafts#8189
- w3c/csswg-drafts#8389

After this CL, you will no longer be able to animate your
popover like this:

```
  [popover] {
    opacity: 0;
    transition: opacity 0.2s;
  }
  [popover]:open {
    opacity: 1;
  }
```

Instead you'll need to use CSS animations or (eventually) transitions
and you'll have to explicitly declare the `display` and `top-layer`
properties:

```
  transition: opacity 0.2s, display 0.2s, top-layer 0.2s;
```

[1] https://html.spec.whatwg.org/multipage/popover.html

Bug: 1307772,1413556
Change-Id: I4877dd69a06f2624bdb463b065b2e2b66cbf1154
@lilles
Copy link
Member

lilles commented Feb 15, 2023

I have done a prototype of :initial in Chrome with the following restrictions:

  • Parses, but always fail matching if not in subject
  • Parses, but always fail matching inside matches-any selectors like :is(), :where(), :has(), :not().
  • Allowed after tree-abiding pseudo elements (::before, ::after, ::marker, ::placeholder, ::file-selector-button)
  • Allowed after ::part()
  • Allowed inside ::slotted()
  • Allowed inside :host()
  • Parse error after other pseudo elements

@kbrilla
Copy link

kbrilla commented Feb 15, 2023

Parses, but always fail matching inside matches-any selectors like :is()

so It will not work in nested rules?

@lilles
Copy link
Member

lilles commented Feb 15, 2023

Parses, but always fail matching inside matches-any selectors like :is()

so It will not work in nested rules?

In some cases that is true for the current prototype, and may be a good argument for allowing :initial to match in (possibly nested) any-matches type of selectors. It comes at the cost that we can not early out of matching matches-any selectors when we are computing styles for the initial styles and we have not matched any :initial pseudo selectors yet.

/* Does not work */
:initial {
  div & {}
}

/* Works */
div {
  & :initial {
  }
}

@flackr
Copy link
Contributor Author

flackr commented Apr 20, 2023

As discussed at the meeting yesterday, let's try to resolve on the name for this. There were a lot of options so I'll group them by theme and give suggested voting emoji for each if you want to vote on it with emoji reactions. Voting is not intended to decide the name but just inform the discussion.

I've intentionally left a few options off since I think we've come up with good reasons not to use them:
@initial: short, but easily confused with initial property values #8174 (comment)
@initial-keyframe: confusing since it's not actually specifying an animation keyframe #8174 (comment)
@before-change: specific to spec term, but still confusing since it suggests it replaces actual before-change style

Generic names suggesting the style before other styles apply:
😄 @initial-style: intended to reduce confusion with above
🎉 @initial-state: similar to above
❤️ @starting-style: avoids overlap with initial

Specific to before-change style being set:
🚀 @initial-before-change: a more specific version of @before-change to avoid confusion

@flackr flackr changed the title [selectors-4] Add pseudo-class to establish before-change style for css-transitions on new elements. [selectors-4] Add syntax to establish before-change style for css-transitions on new elements. Apr 20, 2023
@tabatkins
Copy link
Member

Given the unanimous 10 votes for @starting-style, I think an async resolution is appropriate for closing the loop on the second resolution. @astearns ?

@chrishtr chrishtr added the Async Resolution: Proposed Candidate for auto-resolve with stated time limit label May 8, 2023
@o-t-w
Copy link

o-t-w commented May 13, 2023

How does this syntax work if you want a different transition/animation for entry and exit?

@kizu
Copy link
Member

kizu commented May 18, 2023

@o-t-w If we're transitioning display via just CSS, we can use the styles in the “hidden” state as the exit transition: https://codepen.io/kizu/pen/wvYYmZY

Screen.Recording.2023-05-18.at.11.57.40.mov

If we would be using JS, I don't think CSS would help us a lot with the exit transition, but we could use a class with the exit styles and do “apply this class -> wait for the transition -> remove element”. Which is not ideal, and I wonder if there is something we could do for this (@ending-style? Something else?).

@o-t-w
Copy link

o-t-w commented May 18, 2023

How would it work for an element with a popover attribute being closed?

@kizu
Copy link
Member

kizu commented May 18, 2023

@o-t-w Seems to work fine: https://codepen.io/kizu/pen/wvYYmZY — updated the example.

Screen.Recording.2023-05-18.at.15.20.45.mov

We just have to apply the “exit” state to the .popover-element:not(:popover-open) in this case.

aarongable pushed a commit to chromium/chromium that referenced this issue May 22, 2023
w3c/csswg-drafts#8174 (comment)

Bug: 1412851
Change-Id: Ic689460dac934b93039d1db2d6e30866470a0274
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4474048
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Rune Lillesveen <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1147154}
@bramus
Copy link
Contributor

bramus commented May 23, 2023

@astearns @atanassov I think we can resolve on renaming @initial to @starting-style? The vote results are unanimous and its been two weeks since the async label was added.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [selectors-4] Add syntax to establish before-change style for css-transitions on new elements., and agreed to the following:

  • RESOLVED: rename `@initial` to `@starting-style`
The full IRC log of that discussion <bramus> chrishtr: This is the feature specify style in first frame of transition
<bramus> chrishtr: previously we resolved to add it but needed to bikshed name
<bramus> chrishtr: Vote was unanimous for `@starting-style`
<bramus> chrishtr: with 11 votes
<masonf> now 12 votes
<bramus> Rossen_: looking at the folks that voted, there’s a strong Google representation but that’s ok
<bramus> Rossen_: Any extra feedback or ideas?
<bramus> Proposed resolution: rename `@initial` to `@starting-style`
<bramus> RESVOLVED: rename `@initial` to `@starting-style`
<bramus> RESOLVED: rename `@initial` to `@starting-style`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed Accepted by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-transitions-2 selectors-4 Current Work
Projects
None yet
Development

No branches or pull requests