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.

Sign up for GitHub

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

Jump to bottom

[web-animations-1] commitStyles followed by cancel should not trigger a transition #11084

Open
birtles opened this issue Oct 25, 2024 · 8 comments · May be fixed by #11085
Open

[web-animations-1] commitStyles followed by cancel should not trigger a transition #11084

birtles opened this issue Oct 25, 2024 · 8 comments · May be fixed by #11085
Labels
web-animations-1

Comments

@birtles
Copy link
Contributor

birtles commented Oct 25, 2024 •
edited
Loading

In Mozilla bug 1917071 we came across a case where Gecko is firing transitions as a result of calling commitStyles.

Specifically, for the following content:

#box {
  opacity: 0;
  transition: opacity 1s;
}
document.querySelector('button').onclick = () => {
  let animation = document
    .querySelector('#box')
    .animate({ opacity: 1 }, { fill: 'forwards', duration: 1000 });
  animation.onfinish = () => {
    animation.commitStyles();
    animation.cancel();
  };
};

After analysis, it looks like the problem is that although commitStyles is defined as triggering a style flush:

Unlike most other methods defined on this interface, calling this method does trigger a style change event (see § 6.13 Model liveness).

It doesn't define when this takes place. Logically it needs to happen towards the start of the procedure so that the latest styles are applied. The code for Gecko and Blink shows they flush style before updating inline style.

Edit: It looks like the spec does define that pending style changes are applied at the start of the procedure ("If, after applying any pending style changes...").

However, there is no requirement to flush styles at the end of the procedure. Furthermore, my reading of the procedure to update the style attribute doesn't require computing style immediately. As a result, the style attribute may be updated but not yet reflected in the corresponding element's computed style.

If the animation is canceled before style is next computed, the changes to inline style will not be reflected in the before-change style since it only requires updating computed values from declarative animations. As a result, canceling the animation can cause a change to be observed and transitions to be fired.

I'm not exactly sure why the bug doesn't reproduce in Blink. Perhaps it is reflecting the changes to the style attribute as part of producing the before-change style?

I think we need to specify that commitStyles should somehow cause the computed style to be synchronously updated as a result of updating the style attribute—or somehow indicate that commitStyles followed by cancelling an animation should not trigger a transition. We may need to refine this wording when we tackle #5394, however.

/cc @flackr @graouts @emilio @canalun @vmpstr
@birtles
Copy link
Contributor Author

birtles commented Oct 25, 2024

Also, apparently Safari shows the same behavior as Gecko for this case.

@birtles birtles changed the title [web-animations-1] commitStyles should say when styles are flushed and indicate that changes to inline styles are applied immediately [web-animations-1] commitStyles followed by cancel should not trigger a transition Oct 25, 2024
birtles added a commit that referenced this issue Oct 25, 2024
@birtles
[web-animations-1] Require a style flush at the end of commitStyles
e24222b 
Fixes #11084.
@birtles birtles linked a pull request Oct 25, 2024 that will close this issue
Open
birtles added a commit that referenced this issue Oct 25, 2024
@birtles
[web-animations-1] Require a style flush at the end of commitStyles
5212614 
Fixes #11084.
@graouts
Copy link
Contributor

graouts commented Oct 25, 2024

I can confirm that WebKit performs a flush immediately after it was established that we can proceed with this method and decided not to throw a NoModificationAllowedError exception. There is no further flush performed within this procedure.

@flackr
Copy link
Contributor

flackr commented Oct 25, 2024

In the starting of transitions, it says:

Likewise, define the after-change style as the computed values of all properties on the element based on the information known at the start of that style change event, but using the computed values of the animation-* properties from the before-change style, excluding any styles from CSS Transitions in the computation, and inheriting from the after-change style of the parent. Note that this means the after-change style does not differ from the before-change style due to newly created or canceled CSS Animations.

To me, using the the animation-* properties from the before-change style suggests that the intent here is that the after-change style should include the canceled animation in this case, resulting in no change on the style update for which the animation was canceled. Of course this should be spelled out explicitly if this is the way we want to resolve this, but it seems consistent with the way we would prevent transitions starting on style changes applied at the same time as a css animation is removed.

@flackr
Copy link
Contributor

flackr commented Oct 25, 2024 •
edited
Loading

I'm not exactly sure why the bug doesn't reproduce in Blink. Perhaps it is reflecting the changes to the style attribute as part of producing the before-change style?

This seems to be why we don't start a transition in blink, we still consider the canceled animation in CSSAnimations::CanCalculateTransitionUpdateForProperty. This is I think consistent with my suggestion above, #11084 (comment), as if the animation were still active it would produce the same before and after change style.

@birtles
Copy link
Contributor Author

birtles commented Oct 28, 2024

To me, using the the animation-* properties from the before-change style suggests that the intent here is that the after-change style should include the canceled animation in this case, resulting in no change on the style update for which the animation was canceled.

I see. I can understand that for animations created/cancelled by animation-* properties because it's the style change that actually cancels them. But these animations have already been canceled prior to any style change event. Temporarily un-canceling them seems like something different?

@birtles
Copy link
Contributor Author

birtles commented Oct 30, 2024

I'm going to try to set aside a bit of time in the next day or two to run a few tests to see if I can convince myself of the Chrome behavior or otherwise work out what's reasonable to spec.

@flackr
Copy link
Contributor

flackr commented Oct 31, 2024 •
edited
Loading

But these animations have already been canceled prior to any style change event. Temporarily un-canceling them seems like something different?

I think of it as we haven't had a style change event where their previous styles could actually be undone yet, so even though they're canceled we should still consider the styles they had been applying with the next style change event.

@flackr
Copy link
Contributor

flackr commented Nov 1, 2024

Seems like this relates to #6688 where I also brought up that this shouldn't trigger a transition - #6688 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
web-animations-1
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[web-animations-1] Require a style flush at the end of commitStyles w3c/csswg-drafts
4 participants
@graouts @flackr @birtles and others