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

[css-view-transitions-1] Should mix-blend-mode be a part of the ua opacity animation? #8924

Closed
vmpstr opened this issue Jun 6, 2023 · 10 comments · Fixed by #9000
Closed
Labels
css-view-transitions-1 View Transitions; Bugs only Needs Edits

Comments

@vmpstr
Copy link
Member

vmpstr commented Jun 6, 2023

This is somewhat related to ISSUE 4 in the spec. We currently specify mix-blend-mode conditionally if we have a crossfade.

This, however, has unintended and sometimes surprising effects for developers trying to customize old/new animations. @argyleink had an example where the old content did a slide out to the left and new content did a slide in from the left: the overlap between the two contents did not "look right", since the expectation is to see a normal blend mode.

My suggestion is to consider putting mix-blend-mode to be a part of the UA animation instead, maybe something like

@keyframes -ua-mix-blend-mode-plus-lighter {
  from { mix-blend-mode: plus-lighter }
  to { mix-blend-mode: plus-lighter }
}

and adding that into the animation-name for ua generated css. As an aside, mix-blend-mode is not animatable, but would need to be at least discretely animatable for this to work.

It's a bit of a hack, but I worry that having mix-blend-mode that is added for pairs automatically may cause more developer pain.

Any thoughts?

@vmpstr vmpstr added the css-view-transitions-1 View Transitions; Bugs only label Jun 6, 2023
@fantasai
Copy link
Collaborator

fantasai commented Jun 6, 2023

I think mix-blend-mode should be animatable. I suspect what happened is that the reformatting of ”Animatable” to “Animation type” didn't catch the compositing spec because it's in the FXTF repo, and it's really supposed to be “discrete”.

@khushalsagar
Copy link
Member

So the idea here is that if authors override the default animation on old/new images, we want it to automatically override the blending as well? Because use of the blend-mode is tightly coupled with the opacity animation. Adding up the pixels on the 2 images works because they precisely overlap each other and their opacity (based on the fade animation) adds up to 1.

This thought process sounds good but it would be better if this could be setup in a way that this also removes isolation from the wrapper. Otherwise we have undue rendering cost that authors will not notice because its not a visual glitch. Can't think of a CSS way to make that happen.

@vmpstr
Copy link
Member Author

vmpstr commented Jun 7, 2023

I also don't know if it's possible to select based on the specified css value of a descendant (I don't think it is?) I think isolation rendering cost in this case should be something that a user-agent can optimize.

Since we'd have a normal blending mode, so there would be no other trigger for compositing, so isolation can also be decomposited. Is that right?

@khushalsagar
Copy link
Member

khushalsagar commented Jun 8, 2023

That's a good question. isolation has to induce a stacking context (since that affects paint order) but doesn't necessarily require us to composite the descendants first if they are all using normal blending. Blending is associated if every element has normal blending.

Blend(A, Blend (B, C)) = Blend(Blend(A,B), C)

Thank you @ccameron-chromium for walking me through the math!

In that case, I'm good with this proposal. Just to summarize:

  • The current UA CSS for cross-fading the 2 images is as follows:

    @keyframes -ua-view-transition-fade-out {
    to { opacity: 0; }
    }
    @keyframes -ua-view-transition-fade-in {
    from { opacity: 0; }
    }
    :root::view-transition-old(*) {
    animation-name: -ua-view-transition-fade-out;
    mix-blend-mode: plus-lighter;
    }
    :root::view-transition-new(*) {
    animation-name: -ua-view-transition-fade-in;
    mix-blend-mode: plus-lighter;
    }
    :root::view-transition-image-pair(*) {
        isolation: isolate;
    }
  • We want to change it to this instead:

    @keyframes -ua-view-transition-fade-out {
    to { opacity: 0; }
    }
    @keyframes -ua-view-transition-fade-in {
    from { opacity: 0; }
    }
    @keyframes -ua-mix-blend-mode-plus-lighter {
    from { mix-blend-mode: plus-lighter }
    to { mix-blend-mode: plus-lighter }
    }
    :root::view-transition-old(*) {
    animation-name: -ua-view-transition-fade-out, -ua-mix-blend-mode-plus-lighter;
    }
    :root::view-transition-new(*) {
    animation-name: -ua-view-transition-fade-in, -ua-mix-blend-mode-plus-lighter;
    }
    :root::view-transition-image-pair(*) {
        isolation: isolate;
    }

    Because the blending is tightly coupled to the cross-fade. If the author is overriding the cross-fade, the blend mode is just getting in their way.

Open to suggestions to set up this CSS such that isolation also gets removed if the author removes the cross-fade animation. But we can optimize the rendering internally assuming none of the elements in the sub-tree have normal blending.

@noamr
Copy link
Collaborator

noamr commented Jun 13, 2023

That's a good question. isolation has to induce a stacking context (since that affects paint order) but doesn't necessarily require us to composite the descendants first if they are all using normal blending. Blending is associated if every element has normal blending.

Blend(A, Blend (B, C)) = Blend(Blend(A,B), C)

Thank you @ccameron-chromium for walking me through the math!

In that case, I'm good with this proposal. Just to summarize:

  • The current UA CSS for cross-fading the 2 images is as follows:

    @keyframes -ua-view-transition-fade-out {
    to { opacity: 0; }
    }
    @keyframes -ua-view-transition-fade-in {
    from { opacity: 0; }
    }
    :root::view-transition-old(*) {
    animation-name: -ua-view-transition-fade-out;
    mix-blend-mode: plus-lighter;
    }
    :root::view-transition-new(*) {
    animation-name: -ua-view-transition-fade-in;
    mix-blend-mode: plus-lighter;
    }
    :root::view-transition-image-pair(*) {
        isolation: isolate;
    }
  • We want to change it to this instead:

    @keyframes -ua-view-transition-fade-out {
    to { opacity: 0; }
    }
    @keyframes -ua-view-transition-fade-in {
    from { opacity: 0; }
    }
    @keyframes -ua-mix-blend-mode-plus-lighter {
    from { mix-blend-mode: plus-lighter }
    to { mix-blend-mode: plus-lighter }
    }
    :root::view-transition-old(*) {
    animation-name: -ua-view-transition-fade-out, -ua-mix-blend-mode-plus-lighter;
    }
    :root::view-transition-new(*) {
    animation-name: -ua-view-transition-fade-in, -ua-mix-blend-mode-plus-lighter;
    }
    :root::view-transition-image-pair(*) {
        isolation: isolate;
    }

    Because the blending is tightly coupled to the cross-fade. If the author is overriding the cross-fade, the blend mode is just getting in their way.

This seems like a sensible solution to me.

@bramus
Copy link
Contributor

bramus commented Jun 13, 2023

+1 on the approach, as it hinders authors less.

@fantasai
Copy link
Collaborator

fantasai commented Jun 13, 2023

Would it make sense to fold the blend mode into the first animation? I.e.

@keyframes -ua-view-transition-fade-in {
  from { opacity: 0; mix-blend-mode: plus-lighter }
  to { mix-blend-mode: plus-lighter }
}
@keyframes -ua-view-transition-fade-out {
  from { mix-blend-mode: plus-lighter }
  to { opacity: 0; mix-blend-mode: plus-lighter }
}

Then there's fewer UA animations hanging around... not sure if that's more or less helpful to authors, but I wanted to ask. :)

@khushalsagar
Copy link
Member

I'm good with the suggestion above as well. I don't think authors will notice the difference, but maybe it better documents that blending is tightly coupled with the cross-fade.

@w3c w3c deleted a comment from bramus Jun 14, 2023
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-view-transitions-1] Should mix-blend-mode be a part of the ua opacity animation?, and agreed to the following:

  • RESOLVED: Move mix-blend-mode behavior into the UA opacity animation
The full IRC log of that discussion <TabAtkins> vmpstr: in the UA stylesheet, we add an animation, a cross-fade that changes opacity of new and old pseudos
<TabAtkins> vmpstr: We also added mix-blend-mode:plus-lighter
<TabAtkins> vmpstr: This way if you have the same content on both sides it doesn't look like it fades in the middle
<TabAtkins> vmpstr: We found since devs can customize this, if they change it to anything but cross-fade they're surprised by the plus-lighter behavior
<TabAtkins> vmpstr: So proposal is to bundle the plus-lighter to be part of the animation
<TabAtkins> vmpstr: A separate set of keyframes that animates from plus-lighter to plus-lighter
<TabAtkins> vmpstr: And put that in the UA stylesheet, so when devs change the animation, they'll get rid of the mix-blend-mdoe automatically
<TabAtkins> vmpstr: Also right now mix-blend-mode is marked as not animatable, but aiui this is an oversight, so we need to change this to discretely animateable;
<khush> q+
<Rossen_> q
<Rossen_> ack Rossen_
<Rossen_> ack khush
<TabAtkins> khush: There's a few syntax options about where to put it in keyframes
<TabAtkins> khush: Form an impl perspective, we also add isolation ot the image-pair elements to get the cross-fade right
<TabAtkins> khush: If you have an isolated node and something below has a non-normal blend mode, you have to do an off-screen compositing.
<dbaron> +1 to the proposed changes; I think all 3 properties in https://drafts.fxtf.org/compositing/ should be Animation type: discrete rather than Animatable: no.
<TabAtkins> khush: That's a lot of rendering work to do if it's not necessary
<TabAtkins> khush: We convinced ourselves that keeping the isolation is fine; if you rmeove the mix-blend-mode it goes back to a fast rendering path
<emilio> q+
<TabAtkins> khush: So wanted to run this past the group
<TabAtkins> emilio: I guess the isolation isn't partiuclarly side-effect-ful bc things are already stacking contexts
<TabAtkins> emilio: But still probably less confusing to authors if we mix it with the opacity aniamtion?
<Rossen_> ack emilio
<TabAtkins> khush: We just couldn't figure out a CSS way to tie the ancestor's isolation to the child's animation
<TabAtkins> emilio: Not sure i follow
<TabAtkins> khush: By default you have isolation on ancestor and mix-blend-mode+opacity on the child
<TabAtkins> khush: If author is overriding the opacity animation, we wanna get rid of mix-blend-mode and isolation both
<TabAtkins> khush: We can easily get rid of mix-blend-mode, but not sure how to get rid of the isolation too
<TabAtkins> emilio: I see. If there's not a great way to do it, it's probably not a big deal.
<TabAtkins> khush: Right, we can optimize it out if ther'es no mix-blend-mode
<TabAtkins> emilio: I suppose if someone writes their own thing they might get confused, but if devtools shows the right thing it's probably okay
<Rossen_> ack fantasai
<Zakim> fantasai, you wanted to react to emilio to comment on one vs two rules
<TabAtkins> fantasai: Should we be creating two different sets of keyframes? Or one set?
<TabAtkins> vmpstr: I think one set makes sense so we're not creating too many UA keyframes
<TabAtkins> fantasai: So we have a proposed resolution?
<TabAtkins> vmpstr: proposed res is to move the mix-blend-mode behavior into the opacity transition
<TabAtkins> Rossen_: Objections?
<TabAtkins> RESOLVED: Move mix-blend-mode behavior into the UA opacity animation
<TabAtkins> fantasai: This depends on the props being discretely animatable; they are, but we changed the format of the animatable line in propdef
<TabAtkins> fantasai: "No" used to mean "discretely"; later we changed it to say "discrete" and "no" really meant no.
<TabAtkins> fantasai: TAb and I went thru CSSWG and fixed everything, but missed FXTF.
<TabAtkins> fantasai: So all of the fxtf drafts need fixing and repubbing
<TabAtkins> fantasai: But many have had changes and no active editor, so that's hard
<TabAtkins> Rossen_: This sounds like a big topic on its own, should track as separate issue
<TabAtkins> (I suggest we move into csswg as we repub them, but we should open an issue for it.)
<dbaron> can we at least resolve to fix the fxtf EDs per the previous edits?
<fantasai> I don't think you actually need a resolution, we already agreed to make those changes
<TabAtkins> vmpstr: Can I confirm that mix-blend-mode *is* meant to be animatable?
<TabAtkins> TabAtkins: Yes, absolutely
<TabAtkins> dbaron: Can we at least resolve to fix the FXTF EDs?
<TabAtkins> TabAtkins: Shouldn't need another, we already have a reoslution to fix this stuff
<TabAtkins> dbaron: And potentially houdini, tho I'm not sure they have properties
<dbaron> dbaron: and maybe also css-houdini-drafts if there are any properties in them
<TabAtkins> Rossen_: Let's just make a tracking issue for this
<TabAtkins> I'd take it on but I'm gonna be too busy the next week and a half before vacation

@khushalsagar
Copy link
Member

Welp, I started a spec change for this and realized that using a single keyframe runs into an issue with animations which have only one of old/new images.

The setup in the spec for opacity animations is as follows:

  1. There are static UA rules here which add fade-in and fade-out animations to all ::view-transition-new and ::view-transition-old elements respectively.
    This means that if a view-transition-name is only present in the old DOM (some widget is exiting the screen), it fades out in-place. And if a view-transition-name is only present in the new DOM (some widget is entering the screen), it fades in in-place.

  2. There are dynamic rules here which are generated based on the state of old/new DOM elements. Part of that is adding mix-blend-mode: plus-lighter to old/new pseudos if both are present.
    We do this dynamically to only do complex blending for the case which needs it.

So we gotta go back to doing the suggestion in the first comment. The behaviour is the same as intended by this resolution, just that the exact syntax has changed. I put up a PR to avoid ambiguity about what's being proposed: #9000.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css-view-transitions-1 View Transitions; Bugs only Needs Edits
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants