8000
Skip to content

core: add unsupported css properties to non-composited-animations audit - #11246

Merged
paulirish merged 10 commits into
masterfrom
unsupported-css
Aug 14, 2020
Merged

core: add unsupported css properties to non-composited-animations audit#11246
paulirish merged 10 commits into
masterfrom
unsupported-css

Conversation

@adamraine
@adamraine adamraine commented Aug 10, 2020
Copy link
Copy Markdown
Contributor

Recent changes to chromium add the names of CSS properties which cannot be animated on the compositor to the DevTools trace. This PR adds those CSS property names to the "Unsupported CSS Property" failure reason in the report.

Chromium changes: https://chromium-review.googlesource.com/c/chromium/src/+/2320552
Issue: #2208

Screen Shot 2020-08-13 at 5 21 53 PM

@adamraine
adamraine requested a review from a team as a code owner August 10, 2020 17:10
@adamraine
adamraine requested review from paulirish and removed request for a team August 10, 2020 17:10
Comment thread lighthouse-core/audits/non-composited-animations.js Outdated
Comment thread lighthouse-core/audits/non-composited-animations.js Outdated
@paulirish paulirish left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good.

we can adopt the plural + variable pattern i linked above.

and also, as mentioned, let's add some relevant items to the fixture artifacts.json so we can see this in our samplejson report

a few nits but lgtm otherwise, so i'll approve a tad early.

Comment thread lighthouse-core/audits/non-composited-animations.js Outdated
@@ -19,8 +19,11 @@ const UIStrings = {
=1 {# animated element found}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment says "the number of animations" but the text is number of elements. 🐱

i personally think it'd be better to report either count of noncomp animations or % of animations that were comped

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the comment, but I still think we should list the number of animated elements rather than the number of animations. We aren't listing the animations, only associating failure reasons with an animation name.

Animations can have more than one failure reason and animations with no name can have their failure reasons combined as duplicates. It would not always be clear where the animation count comes from, where the animated element count would be clear.

For example, this report could have an animation count of 4 if there are three unnamed animations all operating on the property height:

<div id="asdf" .... >
Unsupported CSS Property: height
Unsupported CSS Property: width              myAnimation

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

animations with no name can have their failure reasons combined as duplicates

ahhhhhhh

okay yeah that makes it tricky to summarize something that's intuitive. sg.

@@ -19,8 +19,11 @@ const UIStrings = {
=1 {# animated element found}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

animations with no name can have their failure reasons combined as duplicates

ahhhhhhh

okay yeah that makes it tricky to summarize something that's intuitive. sg.

@paulirish
paulirish merged commit e335fa9 into master Aug 14, 2020
@paulirish
paulirish deleted the unsupported-css branch August 14, 2020 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

0