[rule view] Add keyframe rules with its associated element

RESOLVED FIXED in Firefox 33

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: gl, Assigned: gl)

Tracking

unspecified
Firefox 33
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox 33+)

Details

Attachments

(1 attachment, 13 obsolete attachments)

44.19 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
Keyframe rules should be displayed in the inspector with its associated element.
(Assignee)

Updated

3 years ago
Assignee: nobody → gabriel.luong
(Assignee)

Updated

3 years ago
Summary: [rule view] Add keyframe rules to the matching elements → [rule view] Add keyframe rules with its associated element
(Assignee)

Comment 1

3 years ago
Created attachment 8448577 [details] [diff] [review]
keyframe-inspector.patch
(Assignee)

Updated

3 years ago
Depends on: 591303
(Assignee)

Comment 2

3 years ago
Created attachment 8449653 [details] [diff] [review]
keyframe-inspector.patch

Added:
- Parsing of multiple animation names
- Expandable keyframe rules similar to Pseudo Elements
Attachment #8448577 - Attachment is obsolete: true
(Assignee)

Comment 3

3 years ago
Created attachment 8450238 [details] [diff] [review]
keyframe-inspector.patch

- Fixed the expandable elements
Attachment #8449653 - Attachment is obsolete: true
(Assignee)

Comment 4

3 years ago
Created attachment 8451762 [details] [diff] [review]
keyframe-inspector.patch

Added handling of overridden keyframe rules. The general rule is that if there are multiple animation names associated with the element, the last one wins.
Attachment #8450238 - Attachment is obsolete: true
(Assignee)

Comment 5

3 years ago
Created attachment 8452704 [details] [diff] [review]
keyframe-inspector.patch

Fixed unit tests with pseudo elements.
Attachment #8451762 - Attachment is obsolete: true
(Assignee)

Comment 6

3 years ago
Created attachment 8453849 [details] [diff] [review]
keyframe-inspector.patch

Added unit tests
Attachment #8452704 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
Created attachment 8453859 [details] [diff] [review]
keyframe-inspector.patch
(Assignee)

Comment 8

3 years ago
Created attachment 8453860 [details] [diff] [review]
keyframe-inspector.patch

Trim trailing whitespaces
Attachment #8453849 - Attachment is obsolete: true
Attachment #8453859 - Attachment is obsolete: true
(Assignee)

Comment 9

3 years ago
Unit test currently covers the folllowing:
- Display of the keyframe rules - headers, and text properties
- Clicking on the twisty in the header

Missing:
- Changing the properties for a given keyframe rule. (Not actually sure how to go about this)
- I was getting a race condition when checking for the dblclick to toggle (expand/collapse) the keyframe rule container. Tried yielding for an event, but no luck so far.
(Assignee)

Updated

3 years ago
Attachment #8453860 - Flags: feedback?(pbrosset)
Attachment #8453860 - Flags: feedback?(fayearthur)
(Assignee)

Comment 10

3 years ago
try https://tbpl.mozilla.org/?tree=Try&rev=c4e2b4e94e1c
(Assignee)

Comment 11

3 years ago
Created attachment 8454008 [details] [diff] [review]
keyframe-inspector.patch

Fixed unit tests. Looking into how to restart the animation after modifying a property.
Attachment #8453860 - Attachment is obsolete: true
Attachment #8453860 - Flags: feedback?(pbrosset)
Attachment #8453860 - Flags: feedback?(fayearthur)
(Assignee)

Comment 12

3 years ago
Created attachment 8455620 [details] [diff] [review]
keyframe-inspector.patch

Added unit tests for dynamic changes to keyframe rules.
This should be applied on top of 591303 which is landing on fx-team any time now.
Attachment #8454008 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8455620 - Flags: review?(pbrosset)
Comment on attachment 8455620 [details] [diff] [review]
keyframe-inspector.patch

Review of attachment 8455620 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good. I had played with the patch applied a few days ago, it worked generally nicely. There were these cases where the animation wouldn't adopt the changed properties, but we can probably go ahead and focus on this in follow-up patches.
I have made a few comments below.

::: browser/devtools/styleinspector/rule-view.js
@@ +1679,5 @@
>    _createEditors: function() {
>      // Run through the current list of rules, attaching
>      // their editors in order.  Create editors if needed.
>      let lastInheritedSource = "";
> +    let lastKeyframe = null;

Why is this needed? I'm not sure to entirely get the logic of it.
This function loops over all rules applied to the element, and if a rule is a keyframes rule, then it displays it in a specific way.
How can the same keyframes rule appear twice in this list? And how would only checking the previous one guarantee that the UI is correct?
It sounds like something that should be handled server-side in the actor.

@@ +1731,5 @@
>          div.insertBefore(twisty, div.firstChild);
>          this.element.appendChild(div);
>        }
>  
> +      let keyframe = rule.keyframe;

This function is long enough as it is, since what you're adding is a separate enough functionality, it deserves its own function.

@@ +1735,5 @@
> +      let keyframe = rule.keyframe;
> +      if (keyframe && keyframe != lastKeyframe) {
> +        lastKeyframe = keyframe;
> +
> +        let div = this.doc.createElementNS(HTML_NS, "div");

`div` is too generic here, please choose a self-explanatory name for this element.

@@ +1747,5 @@
> +
> +        div.insertBefore(twisty, div.firstChild);
> +        this.element.appendChild(div);
> +
> +        let container = this.keyframeContainer =

Setting `this.keyframeContainer` on the instance doesn't seem to be needed since you get rid of it at the end of the function.

@@ +1778,4 @@
>      }
>  
>      this.togglePseudoElementVisibility(this.showPseudoElements);
> +    delete this.keyframeContainer

Please avoid using `delete` here, this has the effect of putting the whole object in "directory mode" for nothing. You don't actually need that property to be entirely removed, just nullified.

@@ +1867,1 @@
>          this.isSelectorEditable) {

This condition is getting complex enough to require its own getter on the prototype of `RuleEditor`. Turns out we already have `get isSelectorEditable`, but it only checks for the right trait on the target. It should also check if `isEditable` and if the rule type is correct.

::: browser/devtools/styleinspector/ruleview.css
@@ +43,5 @@
>    display: none;
>  }
>  
> +.show-pseudo-elements .ruleview-rule-pseudo-element,
> +.show-expandable-element + .ruleview-rule-expandable-container {

I think we can make this expand/collapse container a little more generic and avoid creating this new class.
Why not use something like `.ruleview-expandable-container` for both the pseudo elements AND the keyframes?

::: browser/devtools/styleinspector/test/browser_ruleview_keyframes-rule.js
@@ +3,5 @@
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Test that keyframe rules are displayed correctly in the rule view

This test tends to be a little big I think. It's better to create many smaller tests that only do one thing. This way they are easier to maintain, and it also helps pin-pointing a problem when one of these tests fail.
You could create a first simple test that only verifies that the gutter and keyframe section appears in the UI when there are keyframes in the CSS, then create another test that verifies the content of the keyframes rule.

@@ +10,5 @@
> +
> +let test = asyncTest(function*() {
> +  yield addTab(TEST_URI);
> +
> +  info("Opening the rule-view");

This comment isn't very useful, and `openRuleView` already logs a comment.

@@ +13,5 @@
> +
> +  info("Opening the rule-view");
> +  let {toolbox, inspector, view} = yield openRuleView();
> +
> +  yield testPacman(inspector, view);

These 3 test case functions could use an `info(...)` however.

::: toolkit/devtools/server/actors/styles.js
@@ +430,5 @@
>        }
>      }
>  
> +    // Add all the keyframes rule associated with the element
> +    let animationNames = this.cssLogic._computedStyle.animationName.split(", ");

`_computedStyle` also is a private property of cssLogic, it's weird that we use it here (and in `getComputed`), but this isn't your fault, I think cssLogic is a strange beast and we should rework its API at some stage.
I don't see a harm in making `_computedStyle` public though.

nit: even if this a computed-style value, I think it would be safer just splitting on "," and trimming the strings rather than splitting on ", ".

@@ +434,5 @@
> +    let animationNames = this.cssLogic._computedStyle.animationName.split(", ");
> +    if (animationNames) {
> +      // Traverse through all the available keyframes rule and add
> +      // the keyframes rule that matches the computed animation name
> +      this.cssLogic._keyframesRules.forEach(keyframesRule => {

Why not using a `for ... of` loop here?

@@ +439,5 @@
> +        if (animationNames.indexOf(keyframesRule.name) > -1) {
> +          for (let rule of keyframesRule.cssRules) {
> +            entries.push({
> +              rule: this._styleRef(rule),
> +              keyframe: this._styleRef(keyframesRule)

To be really precise, this property should be named `keyframes`, not `keyframe`, right? (This means changing it in a bunch of places in rule-view.js too)

::: toolkit/devtools/styleinspector/css-logic.js
@@ +143,5 @@
>      this._sheets = {};
>      this._sheetsCached = false;
>      this._matchedRules = null;
>      this._matchedSelectors = null;
> +    this._keyframesRules = null;;

nit: extra semi-colon here.

@@ +261,5 @@
>    _cacheSheets: function CssLogic_cacheSheets()
>    {
>      this._passId++;
>      this.reset();
> +    this._keyframesRules = [];

It looks like this line could be in the reset function instead, or I'm missing something.

Also, it looks like this is a private variable (_ prefix) but isn't exposed publicly by css-logic. Either make it as a public variable and expose it through a public function or getter.

@@ +300,1 @@
>        Array.prototype.forEach.call(aDomSheet.cssRules, function(aDomRule) {

While you're at it, I think it makes sense to remove this forEach and use a `for ... of` loop instead. It'll work with cssRules and be nicer to read (same goes for `Array.prototype.forEach.call(this.viewedDocument.styleSheets` in _cacheSheets by the way).

Other than this, the function name `_cacheSheet` is a little weird now since that function doesn't only do this anymore. And in a way, it was weird before too, since the function that actually caches is `getSheet`, while `_cacheSheets` and `_cacheSheet` really just iterate recursively over stylesheets and rules to detect and ask for caching of interesting things.
Attachment #8455620 - Flags: review?(pbrosset)
(Assignee)

Comment 14

3 years ago
Comment on attachment 8455620 [details] [diff] [review]
keyframe-inspector.patch

Review of attachment 8455620 [details] [diff] [review]:
-----------------------------------------------------------------

Patch incoming.

::: browser/devtools/styleinspector/rule-view.js
@@ +1679,5 @@
>    _createEditors: function() {
>      // Run through the current list of rules, attaching
>      // their editors in order.  Create editors if needed.
>      let lastInheritedSource = "";
> +    let lastKeyframe = null;

We could possibly have 2 keyframes rules (@-moz-keyframes and @keyframes rules) with the same animation name that creates 2 MozKeyframesRule object (we cannot differentiate them because they have the same object type). If we add keyframe rules to the keyframes rule container based on the keyframes animation name, then we would have duplicates in the same container. We use the same logic as inheritedSource, and separate the keyframes rule based on the last seen keyframes object.

@@ +1731,5 @@
>          div.insertBefore(twisty, div.firstChild);
>          this.element.appendChild(div);
>        }
>  
> +      let keyframe = rule.keyframe;

Fixed. Separated out the functionality for creating an expandable container for both pseudo elements and keyframes rule

@@ +1735,5 @@
> +      let keyframe = rule.keyframe;
> +      if (keyframe && keyframe != lastKeyframe) {
> +        lastKeyframe = keyframe;
> +
> +        let div = this.doc.createElementNS(HTML_NS, "div");

Fixed. Used header.

@@ +1747,5 @@
> +
> +        div.insertBefore(twisty, div.firstChild);
> +        this.element.appendChild(div);
> +
> +        let container = this.keyframeContainer =

Fixed. Removed this.keyframeContainer

@@ +1778,4 @@
>      }
>  
>      this.togglePseudoElementVisibility(this.showPseudoElements);
> +    delete this.keyframeContainer

Fixed. Removed delete

@@ +1867,1 @@
>          this.isSelectorEditable) {

Fixed. Added checks to isSelectorEditable.

::: browser/devtools/styleinspector/ruleview.css
@@ +43,5 @@
>    display: none;
>  }
>  
> +.show-pseudo-elements .ruleview-rule-pseudo-element,
> +.show-expandable-element + .ruleview-rule-expandable-container {

Fixed. Made more generic

::: browser/devtools/styleinspector/test/browser_ruleview_keyframes-rule.js
@@ +3,5 @@
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Test that keyframe rules are displayed correctly in the rule view

Fixed. Separated into 2 tests as described.

@@ +10,5 @@
> +
> +let test = asyncTest(function*() {
> +  yield addTab(TEST_URI);
> +
> +  info("Opening the rule-view");

Fixed. Removed open rule-view info.

@@ +13,5 @@
> +
> +  info("Opening the rule-view");
> +  let {toolbox, inspector, view} = yield openRuleView();
> +
> +  yield testPacman(inspector, view);

Fixed. Added info(...)

::: toolkit/devtools/server/actors/styles.js
@@ +430,5 @@
>        }
>      }
>  
> +    // Add all the keyframes rule associated with the element
> +    let animationNames = this.cssLogic._computedStyle.animationName.split(", ");

Fixed. Added a getter for computedStyle, and splitting on "," and trimming the strings using Array.map()

@@ +434,5 @@
> +    let animationNames = this.cssLogic._computedStyle.animationName.split(", ");
> +    if (animationNames) {
> +      // Traverse through all the available keyframes rule and add
> +      // the keyframes rule that matches the computed animation name
> +      this.cssLogic._keyframesRules.forEach(keyframesRule => {

Fixed. Used for .. of

@@ +439,5 @@
> +        if (animationNames.indexOf(keyframesRule.name) > -1) {
> +          for (let rule of keyframesRule.cssRules) {
> +            entries.push({
> +              rule: this._styleRef(rule),
> +              keyframe: this._styleRef(keyframesRule)

Fixed. Used the correct keyframes naming of variables

::: toolkit/devtools/styleinspector/css-logic.js
@@ +143,5 @@
>      this._sheets = {};
>      this._sheetsCached = false;
>      this._matchedRules = null;
>      this._matchedSelectors = null;
> +    this._keyframesRules = null;;

Fixed. Removed extra semi-colon

@@ +261,5 @@
>    _cacheSheets: function CssLogic_cacheSheets()
>    {
>      this._passId++;
>      this.reset();
> +    this._keyframesRules = [];

Fixed. Added a getter for keyframesRules

@@ +300,1 @@
>        Array.prototype.forEach.call(aDomSheet.cssRules, function(aDomRule) {

Fixed. Used for loop and adjusted jsdoc for _cacheSheet.
(Assignee)

Comment 15

3 years ago
Created attachment 8457845 [details] [diff] [review]
keyframe-inspector.patch

try https://tbpl.mozilla.org/?tree=Try&rev=8fea50bb9b64
Attachment #8455620 - Attachment is obsolete: true
Attachment #8457845 - Flags: review?(pbrosset)
Comment on attachment 8457845 [details] [diff] [review]
keyframe-inspector.patch

Review of attachment 8457845 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing what I commented about!
Looks good to me now. Only a few minor comments left, but I think this is otherwise ready to land.

One thing I noticed while playing with the patch: if you create an animation from scratch with the style-editor (in my case, I even added a new stylesheet to the document), and then go back to the element in the inspector, the keyframes section is missing. It's a sufficiently separate case to file a follow up bug to fix it, no need to do it here.

R+ granted try is green.

::: browser/devtools/styleinspector/rule-view.js
@@ +1648,5 @@
>      }
>      return this._showPseudoElements;
>    },
>  
> +  createExpandableContainer: function(aLabel, isPseudo = false) {

Missing jsdoc comment block for this new function

::: browser/devtools/styleinspector/test/doc_keyframeanimation.css
@@ +1,1 @@
> +.box {

I realized the other css files in this dir don't have it, but I think they should. So since you're adding a new file here, please add the license info at the top:

/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

::: browser/devtools/styleinspector/test/head.js
@@ +618,5 @@
>  
>  /**
>   * Get the rule editor from the rule-view given its index
>   * @param {CssRuleView} view The instance of the rule-view panel
>   * @param {Number} index The index of the link to get

Missing jsdoc @param for the new nodeIndex param

::: toolkit/devtools/server/actors/styles.js
@@ +433,5 @@
> +    // Add all the keyframes rule associated with the element
> +    let animationNames = this.cssLogic.computedStyle.animationName.split(",");
> +    animationNames = animationNames.map(name => {
> +      return name.trim();
> +    });

Nicer on one line:

animationNames = animationNames.map(name => name.trim());
Attachment #8457845 - Flags: review?(pbrosset) → review+
Oh, another candidate for a follow-up bug (can you file it please):
- select an element that has an animation applied,
- right click in the rule-view
- select "Select all"
- then copy the text
- paste the text in a text editor
==> The keyframes rule container is missing, only the individual keyframe rules are copied.
(Assignee)

Comment 18

3 years ago
Comment on attachment 8457845 [details] [diff] [review]
keyframe-inspector.patch

Review of attachment 8457845 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/styleinspector/rule-view.js
@@ +1648,5 @@
>      }
>      return this._showPseudoElements;
>    },
>  
> +  createExpandableContainer: function(aLabel, isPseudo = false) {

Fixed. Added jsdoc.

::: browser/devtools/styleinspector/test/doc_keyframeanimation.css
@@ +1,1 @@
> +.box {

Fixed. Added license

::: browser/devtools/styleinspector/test/head.js
@@ +618,5 @@
>  
>  /**
>   * Get the rule editor from the rule-view given its index
>   * @param {CssRuleView} view The instance of the rule-view panel
>   * @param {Number} index The index of the link to get

Fixed. Updated jsdoc

::: toolkit/devtools/server/actors/styles.js
@@ +433,5 @@
> +    // Add all the keyframes rule associated with the element
> +    let animationNames = this.cssLogic.computedStyle.animationName.split(",");
> +    animationNames = animationNames.map(name => {
> +      return name.trim();
> +    });

Fixed. Added one liner
(Assignee)

Comment 19

3 years ago
Created attachment 8457879 [details] [diff] [review]
keyframe-inspector.patch
Attachment #8457845 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8457879 - Flags: review+
(Assignee)

Comment 20

3 years ago
Created attachment 8458832 [details] [diff] [review]
keyframe-inspector.patch

Increased the animation duration so that animationend event will be hit. Minor jsdoc fixes.
Attachment #8457879 - Attachment is obsolete: true
Attachment #8458832 - Flags: review+
(Assignee)

Comment 21

3 years ago
try looks good https://tbpl.mozilla.org/?tree=Try&rev=2bb5e2f0655f
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/a1778d8e2e38
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
I had to back this out in https://hg.mozilla.org/integration/fx-team/rev/b69b7c847c56 for mochitest-oth failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=44143733&tree=Fx-Team
Flags: in-testsuite+ → needinfo?(gabriel.luong)
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 24

3 years ago
Created attachment 8459256 [details] [diff] [review]
keyframe-inspector.patch

(In reply to Wes Kocher (:KWierso) from comment #23)
> I had to back this out in
> https://hg.mozilla.org/integration/fx-team/rev/b69b7c847c56 for
> mochitest-oth failures:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=44143733&tree=Fx-Team

Fixed. Added a check for this.cssLogic.computedStyle since this may be null.
Attachment #8458832 - Attachment is obsolete: true
Attachment #8459256 - Flags: review+
Flags: needinfo?(gabriel.luong)
(Assignee)

Comment 25

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=38d630b79e65
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 26

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/899a7a8eb6e6
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/899a7a8eb6e6
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Added to the release notes: "Developer Tools: Editable @keyframes rules in the Rules section of the Inspector"
relnote-firefox: --- → 33+
Blocks: 1194146
You need to log in before you can comment on or make changes to this bug.