Closed
Bug 1030889
Opened 10 years ago
Closed 10 years ago
[rule view] Add keyframe rules with its associated element
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(relnote-firefox 33+)
RESOLVED
FIXED
Firefox 33
Tracking | Status | |
---|---|---|
relnote-firefox | --- | 33+ |
People
(Reporter: gl, Assigned: gl)
References
Details
Attachments
(1 file, 13 obsolete files)
44.19 KB,
patch
|
gl
:
review+
|
Details | Diff | Splinter Review |
Keyframe rules should be displayed in the inspector with its associated element.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → gabriel.luong
Assignee | ||
Updated•10 years ago
|
Summary: [rule view] Add keyframe rules to the matching elements → [rule view] Add keyframe rules with its associated element
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Added: - Parsing of multiple animation names - Expandable keyframe rules similar to Pseudo Elements
Attachment #8448577 -
Attachment is obsolete: true
Assignee | ||
Comment 3•10 years ago
|
||
- Fixed the expandable elements
Attachment #8449653 -
Attachment is obsolete: true
Assignee | ||
Comment 4•10 years ago
|
||
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•10 years ago
|
||
Fixed unit tests with pseudo elements.
Attachment #8451762 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
Added unit tests
Attachment #8452704 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
Trim trailing whitespaces
Attachment #8453849 -
Attachment is obsolete: true
Attachment #8453859 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 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•10 years ago
|
Attachment #8453860 -
Flags: feedback?(pbrosset)
Attachment #8453860 -
Flags: feedback?(fayearthur)
Assignee | ||
Comment 10•10 years ago
|
||
try https://tbpl.mozilla.org/?tree=Try&rev=c4e2b4e94e1c
Assignee | ||
Comment 11•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
Attachment #8455620 -
Flags: review?(pbrosset)
Comment 13•10 years ago
|
||
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•10 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•10 years ago
|
||
try https://tbpl.mozilla.org/?tree=Try&rev=8fea50bb9b64
Attachment #8455620 -
Attachment is obsolete: true
Attachment #8457845 -
Flags: review?(pbrosset)
Comment 16•10 years ago
|
||
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+
Comment 17•10 years ago
|
||
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•10 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•10 years ago
|
||
Attachment #8457845 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8457879 -
Flags: review+
Assignee | ||
Comment 20•10 years ago
|
||
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•10 years ago
|
||
try looks good https://tbpl.mozilla.org/?tree=Try&rev=2bb5e2f0655f
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-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
Flags: in-testsuite+ → needinfo?(gabriel.luong)
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 24•10 years ago
|
||
(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•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=38d630b79e65
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 26•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/899a7a8eb6e6
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Whiteboard: [fixed-in-fx-team]
Comment 27•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/899a7a8eb6e6
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Comment 28•10 years ago
|
||
Added to the release notes: "Developer Tools: Editable @keyframes rules in the Rules section of the Inspector"
relnote-firefox:
--- → 33+
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•