Closed
Bug 1122414
Opened 9 years ago
Closed 9 years ago
When a waapi AnimationEffect corresponds to a CSS transition, its name property should be the transition property
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: pbro, Assigned: birtles)
References
Details
Attachments
(3 files)
5.18 KB,
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
5.22 KB,
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
1.48 KB,
patch
|
pbro
:
review+
|
Details | Diff | Splinter Review |
This request comes out of working on the new animation inspector panel in the devtools. It's useful for users to know which css animation or transition a specific widget in the devtools corresponds to. The WebAnimations API defines that an AnimationEffect has a name property (https://w3c.github.io/web-animations/#animationeffect). It turns out that this name is the name of the @keyframes rule when the owner AnimationPlayer corresponds to a CSS animation. It's empty for CSS transitions, because transitions don't have names we can use. My suggestion is to have the AnimationEffect.name return the transitionProperty in that case. It's better than returning nothing and would be useful in the devtools animation inspector to help users relate whatever they are seeing to their CSS. I talked to Brian Birtles about this and he said it sounded reasonable. Of course this would need to be spec'd too at some stage.
Reporter | ||
Comment 1•9 years ago
|
||
It turns out having a name for transitions would be more helpful than just for displaying it to users. When transitions are added and removed from a node, new Animation objects are created everytime. And so if we want the devtools animation inspector to show these as coming from the same transition, we need an identifier. This name would help.
Assignee | ||
Comment 2•9 years ago
|
||
What sort of name do you have in mind? For example, if it's a transition of the 'transform' property should the name be 'transform'?
Flags: needinfo?(pbrosset)
Assignee | ||
Comment 3•9 years ago
|
||
Actually, I just checked the summary and it says just that. Once we expose keyframes DevTools could fill in this information automatically but I guess until then we need to fill in the name.
Flags: needinfo?(pbrosset)
Reporter | ||
Comment 4•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #2) > What sort of name do you have in mind? For example, if it's a transition of > the 'transform' property should the name be 'transform'? Exactly (bonus points if it doesn't return 'all' for Animation objects created by a 'transition:all' property).
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8587793 -
Flags: review?(jwatt)
Assignee | ||
Comment 6•9 years ago
|
||
This is a bit awkward. We lazily set mName to the transition property and then return it. The reasons for this approach are: * We don't really want to eagerly fill in mName for all transitions since in 99% of cases we'll never use it and this will lead to wasted allocations. * The signature of Name() returns a const nsString reference. This is because Name() is used when building CSS Animations (to compare different copies of the same animation when updating). For that case we don't really want to generate unnecessary copies of nsString objects so we return a reference. However, that means for transitions as well we need to return a reference so we can't just generate a temporary string on-demand. As a result we also have to const-cast ourselves so we can update the mName member. We could make mName mutable but seeing as it's only set once, the const_cast seems more appropriate.
Attachment #8587794 -
Flags: review?(jwatt)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8588499 -
Flags: review?(pbrosset)
Reporter | ||
Comment 8•9 years ago
|
||
Comment on attachment 8588499 [details] [diff] [review] part 3 - Update DevTools tests to expect a name for transitions Review of attachment 8588499 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Brian for working on this!
Attachment #8588499 -
Flags: review?(pbrosset) → review+
Updated•9 years ago
|
Attachment #8587793 -
Flags: review?(jwatt) → review+
Updated•9 years ago
|
Attachment #8587794 -
Flags: review?(jwatt) → review+
Comment 9•9 years ago
|
||
Comment on attachment 8587794 [details] [diff] [review] part 2 - Make return the transitionProperty from Animation.name for CSS transitions Review of attachment 8587794 [details] [diff] [review]: ----------------------------------------------------------------- Oh, and can you fix the first line of the review comment? ("Make return the transitionProperty from Animation.name for CSS transitions")
Assignee | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4af1955b7abb https://hg.mozilla.org/integration/mozilla-inbound/rev/71cd1e22d106 https://hg.mozilla.org/integration/mozilla-inbound/rev/e8bd919e841d
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4af1955b7abb https://hg.mozilla.org/mozilla-central/rev/71cd1e22d106 https://hg.mozilla.org/mozilla-central/rev/e8bd919e841d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•