Closed
Bug 1180134
Opened 9 years ago
Closed 9 years ago
Indicate if an animation displayed in the animation-inspector comes from a CSS transition or a CSS animation
Categories
(DevTools :: Inspector: Animations, defect)
DevTools
Inspector: Animations
Tracking
(firefox43 fixed)
RESOLVED
FIXED
Firefox 43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: pbro, Assigned: pbro)
References
(Blocks 2 open bugs)
Details
Attachments
(2 files, 4 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
I think bug 1179111 gives us the ability to know, so it'd be easy enough to indicate this in the UI somehow.
Assignee | ||
Updated•9 years ago
|
Whiteboard: [devtools-platform]
Assignee | ||
Comment 1•9 years ago
|
||
Going to work on this on top of bug 1169563
Assignee: nobody → pbrosset
Depends on: 1169563
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
Here's what animations look like with the patch applied.
Other options are:
- adding the words "Transition" and "Animation" in the timeline, but that takes a lot of space and we're already using ellipsis if the animation names are too long,
- using icons, but coming up with good icons will be hard,
- separating animations and transitions in 2 sections with section headers.
Note that in the future, there will be SVG SMIL animations in here too, as well as script-generated web animations.
Assignee | ||
Comment 4•9 years ago
|
||
Brian, any ideas on this? Do you think colors are enough at this stage?
Flags: needinfo?(bgrinstead)
Comment 5•9 years ago
|
||
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #4)
> Brian, any ideas on this? Do you think colors are enough at this stage?
Looks fine to me - you might combine that with tooltiptext that indicates "opacity - transition" and "anim open - animation"
Flags: needinfo?(bgrinstead)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8650973 -
Attachment is obsolete: true
Assignee | ||
Comment 7•9 years ago
|
||
Removing this flag, the platform work for this has already been done before.
Whiteboard: [devtools-platform]
Assignee | ||
Comment 8•9 years ago
|
||
This patch contains:
- a new 'type' property in each animation actor's state object
- some cleanup in the css and 2 new css classes for transitions and animations to look different
- the ui component responsible for displaying animations uses the actor's state to use the new css classes
- new l10n strings that are used as tooltips on animations in the ui
- some tests
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7e4a530f2fb
Attachment #8651804 -
Attachment is obsolete: true
Attachment #8653940 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #8)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7e4a530f2fb
Damn, I forgot to update one test. I will cancel this try push and will push again later.
Comment 10•9 years ago
|
||
Comment on attachment 8653940 [details] [diff] [review]
Bug_1180134_-_1_-_Color_code_animations_and_transi.diff
Review of attachment 8653940 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/devtools/animationinspector/components.js
@@ +939,5 @@
> createNode({
> parent: iterations,
> attributes: {
> "class": "name",
> + "title": state.type
Here there is a falsy check for state.type but above we string concat state.type + " iterations". What's the case where it's falsy? It appears it would end up adding "null" as a class name.
::: browser/locales/en-US/chrome/browser/devtools/animationinspector.properties
@@ +65,5 @@
> timeline.timeGraduationLabel=%Sms
> +
> +# LOCALIZATION NOTE (timeline.CSSAnimation.nameLabel):
> +# This string is displayed in a tooltip of the animation panel that is shown
> +# when hovering over the name of a CSS Animation in the timeline UI.
Worth adding a note of what the %S is going to be here
@@ +70,5 @@
> +timeline.cssanimation.nameLabel=%S - CSS Animation
> +
> +# LOCALIZATION NOTE (timeline.CSSAnimation.nameLabel):
> +# This string is displayed in a tooltip of the animation panel that is shown
> +# when hovering over the name of a CSS Transition in the timeline UI.
And here
::: browser/themes/shared/devtools/animationinspector.css
@@ +273,5 @@
> top: unset;
> }
>
> .animation-timeline .animation .name {
> + color: white;
Nit: var(--theme-selection-color) is pretty much white and will match up with other colors seen in the toolbox
::: toolkit/devtools/server/actors/animation.js
@@ +43,5 @@
> +// Types of animations.
> +const TYPES = {
> + CSS_ANIMATION: "cssanimation",
> + CSS_TRANSITION: "csstransition",
> + UNKNOWN: "unknown"
This type doesn't seem to be used at all, but null is returned instead. This should either be removed from the const, or we should be returning TYPES.UNKNOWN and looking for that in the tool frontend.
Attachment #8653940 -
Flags: review?(bgrinstead) → feedback+
Assignee | ||
Comment 11•9 years ago
|
||
Thanks for the review. This should address your comments.
Attachment #8653940 -
Attachment is obsolete: true
Attachment #8654837 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 12•9 years ago
|
||
Attached the wrong patch.
Attachment #8654837 -
Attachment is obsolete: true
Attachment #8654837 -
Flags: review?(bgrinstead)
Attachment #8654840 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 13•9 years ago
|
||
Updated•9 years ago
|
Attachment #8654840 -
Flags: review?(bgrinstead) → review+
Comment 15•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Comment 16•9 years ago
|
||
Spot the little typo: timelime --> timeline
Comment 17•9 years ago
|
||
This typo causes the dots to miss the border color...
Assignee | ||
Updated•9 years ago
|
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Alfred Kayser from comment #17)
> This typo causes the dots to miss the border color...
Thanks, filed bug 1233363 to fix this.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•