Animations modified with setKeyframes must not mask !important #23032
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wpt-pr-bot
approved these changes
Apr 16, 2020
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-2152449
branch
2 times, most recently
from
ba93794
to
1579b34
Compare
CSS declarations that are !important have higher priority in the cascade than animation effects. Unfortunately the information about which declarations were and weren't important is lost once the StyleCascade disappears. Specifically, it's not stored on the ComputedStyle. This causes a problem (once again) for the base computed style optimization, since we can't naively add animation effects on top of the base anymore. We might be overwriting something in the base that was important. The previous attempt at fixing this (flag on ElementAnimations) doesn't work properly. For example, it fails to detect the case where an animation initially doesn't conflict with important declarations, but then suddenly does via setKeyframes. To solve this, this CL stores a bitset containing information about important declarations alongside the base ComputedStyle on ElementAnimations. When we're considering whether the base can be used or not, we then check if there's any animation matching the set of important declarations. Persisting that many bits is slightly uncomfortable, but the only viable alternative I see is disabling the optimization when *any* important declaration exists in the base, which is probably a worse option. Sidenote: Initially I tried to always use the base, even when there were conflicts with important declarations. The bitset of important declarations is effectively a set of animations to be skipped, so we should still be able to use the base if we just don't apply the properties present in the set. However, it unfortunately didn't work due to visited/unvisited colors being animated together (crbug.com/1062217). Bug: 552085 Change-Id: I39e2879af8a858ce1bd97eaa2ceb6e222591df79 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2152449 Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org> Reviewed-by: Rune Lillesveen <futhark@chromium.org> Reviewed-by: Robert Flack <flackr@chromium.org> Cr-Commit-Position: refs/heads/master@{#761034}
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-2152449
branch
from
1579b34
to
05efca0
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CSS declarations that are !important have higher priority in the
cascade than animation effects. Unfortunately the information about
which declarations were and weren't important is lost once the
StyleCascade disappears. Specifically, it's not stored on the
ComputedStyle. This causes a problem (once again) for the base computed
style optimization, since we can't naively add animation effects on
top of the base anymore. We might be overwriting something in the base
that was important.
The previous attempt at fixing this (flag on ElementAnimations) doesn't
work properly. For example, it fails to detect the case where an
animation initially doesn't conflict with important declarations, but
then suddenly does via setKeyframes.
To solve this, this CL stores a bitset containing information about
important declarations alongside the base ComputedStyle on
ElementAnimations. When we're considering whether the base can be used
or not, we then check if there's any animation matching the set of
important declarations.
Persisting that many bits is slightly uncomfortable, but the only
viable alternative I see is disabling the optimization when any
important declaration exists in the base, which is probably a worse
option.
Sidenote: Initially I tried to always use the base, even when there
were conflicts with important declarations. The bitset of important
declarations is effectively a set of animations to be skipped, so we
should still be able to use the base if we just don't apply the
properties present in the set. However, it unfortunately didn't work
due to visited/unvisited colors being animated together
(crbug.com/1062217).
Bug: 552085
Change-Id: I39e2879af8a858ce1bd97eaa2ceb6e222591df79
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2152449
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#761034}