Closed
Bug 1315600
Opened 9 years ago
Closed 9 years ago
Mask behind animation name is too long for animations running on the compositor
Categories
(DevTools :: Inspector: Animations, defect)
DevTools
Inspector: Animations
Tracking
(firefox52 fixed, firefox53 fixed)
RESOLVED
FIXED
Firefox 53
People
(Reporter: birtles, Assigned: daisuke)
Details
Attachments
(3 files)
36.38 KB,
image/png
|
Details | |
58 bytes,
text/x-review-board-request
|
pbro
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
pbro
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details |
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 | ||
Updated•9 years ago
|
Assignee: nobody → daisuke
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•9 years ago
|
||
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)
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•9 years ago
|
||
mozreview-review |
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 12•9 years ago
|
||
mozreview-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+
Assignee | ||
Comment 13•9 years ago
|
||
(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!
Assignee | ||
Comment 14•9 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 17•9 years ago
|
||
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
Comment 18•9 years ago
|
||
Daisuke: I think this should be uplifted to 52 (nightly is 53 now, right?)
Flags: needinfo?(bbirtles)
Updated•9 years ago
|
Flags: needinfo?(bbirtles) → needinfo?(daisuke)
Comment 19•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e4636b63cd36
https://hg.mozilla.org/mozilla-central/rev/85a9d908e91a
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Assignee | ||
Comment 20•9 years ago
|
||
(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)
Comment 21•9 years ago
|
||
(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 22•9 years ago
|
||
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?
Assignee | ||
Comment 23•9 years ago
|
||
(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•9 years ago
|
||
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 25•9 years ago
|
||
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+
Comment 26•9 years ago
|
||
bugherder uplift |
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•