Closed Bug 1315600 Opened 3 years ago Closed 3 years ago

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

Categories

(DevTools :: Inspector: Animations, defect)

Product:
DevTools
Component:
Inspector: Animations
Type:
defect
Priority:
Not set

Tracking

(firefox52 fixed, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: birtles, Assigned: daisuke)

Details

Attachments

(3 files)

Screenshot
36.38 KB, image/png
Details
Bug 1315600 - Part 1: Mask behind animation name is too long for animations running on the compositor.
58 bytes, text/x-review-board-request
pbro
: review+
jcristau
: approval-mozilla-aurora+
Details
Bug 1315600 - Part 2: Add test.
58 bytes, text/x-review-board-request
pbro
: review+
jcristau
: approval-mozilla-aurora+
Details
Attached image 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.)
Assignee: nobody → 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.
Attachment #8810307 - Flags: review?(pbrosset)
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
Attachment #8810306 - Flags: review?(pbrosset) → review+
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
Attachment #8810307 - Flags: review?(pbrosset) → review+
(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!
Keywords: checkin-needed
Pushed by bbirtles@mozilla.com:
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
Keywords: checkin-needed
Daisuke: I think this should be uplifted to 52 (nightly is 53 now, right?)
Flags: needinfo?(bbirtles)
Flags: needinfo?(bbirtles) → needinfo?(daisuke)
https://hg.mozilla.org/mozilla-central/rev/e4636b63cd36
https://hg.mozilla.org/mozilla-central/rev/85a9d908e91a
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
(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?
Flags: needinfo?(daisuke) → needinfo?(pbrosset)
(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.
Flags: needinfo?(pbrosset)
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.
Attachment #8810307 - Flags: approval-mozilla-aurora?
(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 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
Attachment #8810306 - Flags: approval-mozilla-aurora+
Comment on attachment 8810307 [details]
Bug 1315600 - Part 2: Add test.

test for this issue, take in aurora52 along with the fix
Attachment #8810307 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.