1030889 - [rule view] Add keyframe rules with its associated element

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

Keyframe rules should be displayed in the inspector with its associated element.

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: keyframe-inspector.patch

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: keyframe-inspector.patch

Added:
- Parsing of multiple animation names
- Expandable keyframe rules similar to Pseudo Elements

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: keyframe-inspector.patch

- Fixed the expandable elements

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 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.

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: keyframe-inspector.patch

Fixed unit tests with pseudo elements.

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: keyframe-inspector.patch

Added unit tests

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: keyframe-inspector.patch

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: keyframe-inspector.patch

Trim trailing whitespaces

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

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.

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

try https://tbpl.mozilla.org/?tree=Try&rev=c4e2b4e94e1c

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: keyframe-inspector.patch

Fixed unit tests. Looking into how to restart the animation after modifying a property.

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 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.

Comment 13

[Patrick Brosset <:pbro>]

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.

Comment 14

[Gabriel Luong [:gl] (ΦωΦ)]

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.

Comment 15

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: keyframe-inspector.patch

try https://tbpl.mozilla.org/?tree=Try&rev=8fea50bb9b64

Comment 16

[Patrick Brosset <:pbro>]

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());

Comment 17

[Patrick Brosset <:pbro>]

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.

Comment 18

[Gabriel Luong [:gl] (ΦωΦ)]

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

Comment 19

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: keyframe-inspector.patch

Comment 20

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: keyframe-inspector.patch

Increased the animation duration so that animationend event will be hit. Minor jsdoc fixes.

Comment 21

[Gabriel Luong [:gl] (ΦωΦ)]

try looks good https://tbpl.mozilla.org/?tree=Try&rev=2bb5e2f0655f

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/a1778d8e2e38

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 24

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 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.

Comment 25

[Gabriel Luong [:gl] (ΦωΦ)]

https://tbpl.mozilla.org/?tree=Try&rev=38d630b79e65

Comment 26

[Gabriel Luong [:gl] (ΦωΦ)]

https://hg.mozilla.org/integration/fx-team/rev/899a7a8eb6e6

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/899a7a8eb6e6

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes: "Developer Tools: Editable @keyframes rules in the Rules section of the Inspector"