1315600 - Mask behind animation name is too long for animations running on the compositor

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Brian Birtles (:birtles)]

Attachment: Screenshot

As per the attached screenshot, animations with the little thunderbolt on the right, have a background mask that is much longer than the label. (Notice how the same string, "move-up", has a different sized background depending on if it has the ⚡ or not.)

Comment 1

[Daisuke Akatsuka (:daisuke)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf05b062d04e

Comment 2

[Daisuke Akatsuka (:daisuke)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e9dc3f077e2

Comment 3

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1315600 - Part 1: Mask behind animation name is too long for animations running on the compositor.

Review commit: https://reviewboard.mozilla.org/r/92660/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92660/

Comment 4

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1315600 - Part 2: Add test.

Review commit: https://reviewboard.mozilla.org/r/92662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92662/

Comment 5

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8810307 [details]
Bug 1315600 - Part 2: Add test.

I'm sorry, please let me cancel to review since try-server failed a test.

Comment 6

[Daisuke Akatsuka (:daisuke)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a4d2f2503ac

Comment 7

[Daisuke Akatsuka (:daisuke)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5ea9143423f

Comment 8

[Daisuke Akatsuka (:daisuke)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fbcaa376372

Comment 9

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8810306 [details]
Bug 1315600 - Part 1: Mask behind animation name is too long for animations running on the compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92660/diff/1-2/

Comment 10

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8810307 [details]
Bug 1315600 - Part 2: Add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92662/diff/1-2/

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8810306 [details]
Bug 1315600 - Part 1: Mask behind animation name is too long for animations running on the compositor.

https://reviewboard.mozilla.org/r/92660/#review92708

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8810307 [details]
Bug 1315600 - Part 2: Add test.

https://reviewboard.mozilla.org/r/92662/#review92710

::: devtools/client/animationinspector/test/browser_animation_timeline_shows_name_label.js:20
(Diff revision 2)
> +  info("Selecting 'no-compositor' animation "
> +       + "which is not running on compositor");

/devtools/.eslintrc now specifies a line length of 90 chars, so this would fit on one line, no need to wrap it.

::: devtools/client/animationinspector/test/browser_animation_timeline_shows_name_label.js:35
(Diff revision 2)
> +  is(labelEl.textContent, expectedLabelContent,
> +     `Text content of labelEl sould be ${ expectedLabelContent }`);
> +
> +  // Expand timeblockEl to avoid max-width of the label.
> +  timeblockEl.style.width = "10000px";
> +  const orignalLabelWidth = labelEl.clientWidth;

s/orignalLabelWidth/originalLabelWidth

Comment 13

[Daisuke Akatsuka (:daisuke)]

(In reply to Patrick Brosset <:pbro> from comment #12)
> Comment on attachment 8810307 [details]
> Bug 1315600 - Part 2: Add test.
> 
> https://reviewboard.mozilla.org/r/92662/#review92710
> 
> :::
> devtools/client/animationinspector/test/
> browser_animation_timeline_shows_name_label.js:20
> (Diff revision 2)
> > +  info("Selecting 'no-compositor' animation "
> > +       + "which is not running on compositor");
> 
> /devtools/.eslintrc now specifies a line length of 90 chars, so this would
> fit on one line, no need to wrap it.
> 
> :::
> devtools/client/animationinspector/test/
> browser_animation_timeline_shows_name_label.js:35
> (Diff revision 2)
> > +  is(labelEl.textContent, expectedLabelContent,
> > +     `Text content of labelEl sould be ${ expectedLabelContent }`);
> > +
> > +  // Expand timeblockEl to avoid max-width of the label.
> > +  timeblockEl.style.width = "10000px";
> > +  const orignalLabelWidth = labelEl.clientWidth;
> 
> s/orignalLabelWidth/originalLabelWidth

Thank you very much, Patrick!

Comment 14

[Daisuke Akatsuka (:daisuke)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c4f863a208f

Comment 15

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8810306 [details]
Bug 1315600 - Part 1: Mask behind animation name is too long for animations running on the compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92660/diff/2-3/

Comment 16

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8810307 [details]
Bug 1315600 - Part 2: Add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92662/diff/2-3/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4636b63cd36
Part 1: Mask behind animation name is too long for animations running on the compositor. r=pbro
https://hg.mozilla.org/integration/autoland/rev/85a9d908e91a
Part 2: Add test. r=pbro

Comment 18

[Patrick Brosset <:pbro>]

Daisuke: I think this should be uplifted to 52 (nightly is 53 now, right?)

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e4636b63cd36
https://hg.mozilla.org/mozilla-central/rev/85a9d908e91a

Comment 20

[Daisuke Akatsuka (:daisuke)]

(In reply to Patrick Brosset <:pbro> from comment #18)
> Daisuke: I think this should be uplifted to 52 (nightly is 53 now, right?)

Yeah, if possible!
What should I do for that?

Comment 21

[Patrick Brosset <:pbro>]

(In reply to Daisuke Akatsuka (:daisuke) from comment #20)
> (In reply to Patrick Brosset <:pbro> from comment #18)
> > Daisuke: I think this should be uplifted to 52 (nightly is 53 now, right?)
> 
> Yeah, if possible!
> What should I do for that?
There's an aurora-approval-request comment that you can post in the bug by going to an attachment details page. I'll do it for this one so you can see what it looks like.

Comment 22

[Patrick Brosset <:pbro>]

Comment on attachment 8810307 [details]
Bug 1315600 - Part 2: Add test.

Approval Request Comment
[Feature/regressing bug #]: bug 1210795
[User impact if declined]: If declined, users of FF52 opening the animation inspector panel will have a hard time seeing the exact shape of animations (which is useful to understand the easing of animations). The change done in bug 1210795 is very cool, and unfortunately, this bug impacts it, and will prevent users from experiencing it fully. So we should uplift these patches.
[Describe test coverage new/current, TreeHerder]: Tested manually in mozilla-central, and Daisuke also added automated integration tests for it.
[Risks and why]: This is a one liner CSS devtools-only change that will not have impact on the main browsing experience. The only risk I see is the test potentially failing intermittently.
[String/UUID change made/needed]: None.

Comment 23

[Daisuke Akatsuka (:daisuke)]

(In reply to Patrick Brosset <:pbro> from comment #21)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #20)
> > (In reply to Patrick Brosset <:pbro> from comment #18)
> > > Daisuke: I think this should be uplifted to 52 (nightly is 53 now, right?)
> > 
> > Yeah, if possible!
> > What should I do for that?
> There's an aurora-approval-request comment that you can post in the bug by
> going to an attachment details page. I'll do it for this one so you can see
> what it looks like.

Thank you very much!!

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 8810306 [details]
Bug 1315600 - Part 1: Mask behind animation name is too long for animations running on the compositor.

fix visual glitch in devtools animation display, for aurora52

Comment 25

[Julien Cristau [:jcristau]]

Comment on attachment 8810307 [details]
Bug 1315600 - Part 2: Add test.

test for this issue, take in aurora52 along with the fix

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fd05a57731b6
https://hg.mozilla.org/releases/mozilla-aurora/rev/a2b6e3fc0372