The style inspector should allow updating of single css property values within the UI. We are not talking about user editing here ... more about being able to update single property values. This will enable us to speed up the display of node css property values when inspecting as the whole style inspector UI will only need to be re-rendered on display. This will also allow us to stay at a particular scrolled position. I guess that because this will have such a huge increase in performance it should be marked as Minotaur.

Mihai, sorry to ask for another review but most of the changes are to CssHtmlTree.jsm and you are the reviewer most familiar with the code.

This patch makes the style inspector without categories useable as a highlighter registered tool (it was flickery and slow before this change).

Comment on attachment 555736 [details] [diff] [review] Prevent UI redraw patch 1 Review of attachment 555736 [details] [diff] [review]: ----------------------------------------------------------------- The patch is fine, I like it, and it does improve the performance of the Style Inspector. Good work! When I open the style inspector here: http://mihaisucan.github.com/mozilla-work/test.html I get the following errors in the Error Console: Template error evaluating 'selector.humanReadableText(__element)' In: table#templateRules > loop > foreach > 0 > tr > td > #text > ${selector.humanReadableText(__element)} TypeError: this.win is undefined ... repeated several times. Tested other web sites, like ubuntu.com. Same errors. I am giving the patch r- for now, it's almost ready, only some fixes are needed. ::: browser/base/content/inspector.js @@ +980,5 @@ > // Used to prevent the highlighter from calling aTool.onSelect for every > // highlighted node as this dramatically slows down the highlighter. A 300ms > // delay before calling onSelect appears to give the best result. This > // number could be lower if we were not using CSS transitions. > + const toolDelay = 200; I believe we don't need to reduce this delay, yet. Performance is improved, but it still seems to be really slow on big sites (tested on ubuntu.com). Also, as agreed, in today's meeting, it's best to not update the style panel while hovering elements. Better to only update when the user switches to a new locked element. ::: browser/devtools/styleinspector/CssHtmlTree.jsm @@ +208,5 @@ > + * Sets the values of the panel's css properties > + */ > + refreshPanel: function CssHtmlTree_refreshPanel() > + { > + CssHtmlTree.propertyNames.forEach(function(aName) { Why not iterate through the propertyViews directly? @@ +209,5 @@ > + */ > + refreshPanel: function CssHtmlTree_refreshPanel() > + { > + CssHtmlTree.propertyNames.forEach(function(aName) { > + this.updatePanelPropertyValue(aName); I would better like to see a method like PropertyView.refreshValue() that you call. That should handle its own view, instead of having to manually update various PropertyView elements, like the numRulesNode, from CssHtmlTree.refreshPanel(). @@ +220,5 @@ > + propView.numRulesNode.innerHTML = propView.ruleTitle(propView.__element); > + propView.rulesLinkNode.display = "block"; > + } else { > + propView.rulesLinkNode.display = "none"; > + } I believe there's some kind of bug here, because when I move from one element to another, when I have, say, the color property view open, I don't see the updated list of rules. At least, it's not correctly updated. Screen shot: http://img.i7m.de/show/asr5b.png Notice that "unmatched rules" shows twice. To reproduce switch from an element that has matched color rules (expanded), to an element that has only unmatched color rules. @@ +229,5 @@ > + * Empty Panel property values and breadcrumbs > + */ > + resetPanel: function CssHtmlTree_resetPanel() > + { > + let paths = this.root.querySelector("ol"); I would suggest giving the OL element a class name, and use querySelector() to find it based on the class name. @@ +231,5 @@ > + resetPanel: function CssHtmlTree_resetPanel() > + { > + let paths = this.root.querySelector("ol"); > + paths.innerHTML = "<li>null</li>"; > + CssHtmlTree.propertyNames.forEach(function(aName) { Here I would imagine you iterate through the propertyViews object, and you call again propview.refreshValue(). In refreshValue() you can check if tree.viewedElement is null or not, and based on that set the value displayed in the node. @@ +242,5 @@ > + * > + * @param {String} aName Property name > + * @param {String} aValue Property value > + */ > + setPanelPropertyValue: function CssHtmlTree_setPropertyValue(aName, aValue) With a refreshValue() method in PropertyView you no longer need the setPanelPropertyValue() and updatePanelPropertyValue() methods. @@ +254,5 @@ > + * @param {String} aName Property name > + */ > + updatePanelPropertyValue: function CssHtmlTree_setPropertyValue(aName) > + { > + this.setPanelPropertyValue(aName, this.cssLogic.getPropertyInfo(aName).value); Each PropertyView has the .value getter which is exactly the same as this.cssLogic.getPropertyInfo(aName).value. @@ +334,5 @@ > this.name = aName; > this.getRTLAttr = CssHtmlTree.getRTLAttr; > > this.populated = false; > + this.showingUnmatched = false; Why do you change this.showUnmatched to showingUnmatched? You missed at least one instance where the code still uses showUnmatched. Please explain why you need this change. ::: browser/devtools/styleinspector/csshtmltree.xhtml @@ +147,5 @@ > </loop> > <tr if="${showUnmatchedLink}"> > <td colspan="4"> > <div class="expander unmatched-rule"></div> > + <div class="rule-count"> Why did you rename the div.unmatched container to div.rule-count?

On a related note, please include a commit message for your patches. If you use mq in your hg installation, do hg qref -m "... your message" (or hg qref -e to open your configured editor). Usually the commit message is the bug number followed by the title. In this case: Bug 674856 - The style inspector should not redraw the whole UI every time it is used It is also useful to include your user name and the patch date. hg qref -U -D. (see hg help qref). Thank you!

In order to make this work I needed to separate the display of matched and unmatched rules (I believe this fixes another bug as well). To do this I have had to completely rewrite the patch.

Fixed failing test

Comment on attachment 558466 [details] [diff] [review] Prevent UI redraw patch 3 Review of attachment 558466 [details] [diff] [review]: ----------------------------------------------------------------- Much improved patch. Thank you! General comments: - For some reason I see the pointer cursor everywhere I move my mouse in the style panel. Is that on purpose? - Please ask for ui-review from Stephen Horlander. We now show "N selectors" and "M unmatched selectors", both at once. Not sure if that's ideal. Or maybe we should let this for the bug that does the UI changes? (bug 672748) More comments below. Giving the patch r- for the changes still needed. Looking forward to see the updated patch! Thanks! ::: browser/devtools/styleinspector/CssHtmlTree.jsm @@ +152,2 @@ > /** > * Focus the output display on a specific element. Please update the comment since we are touching this method. It should say something like: "Update the highlighted element. The CssHtmlTree panel will show the style information for the given element." @@ +177,5 @@ > + for (let step = i + batchSize; i < step && i <= max; i++) { > + let name = CssHtmlTree.propertyNames[i]; > + let propView = new PropertyView(this, name); > + CssHtmlTree.processTemplate( > + this.templateProperty, this.propertyContainer, propView, true); Nit: code style should be ... CssHtmlTree.processTemplate(this.templateProperty, this.propertyContainer, propView, true); @@ +187,5 @@ > + // the next batch of 25. > + this.win.setTimeout(displayProperties.bind(this), 0); > + } else { > + this.htmlComplete = true; > + this.refreshPanel(); I am surprised to see you need to call refreshPanel() when everything is done. Shouldn't the initial render of the panel have everything properly displayed? Without any additional refreshPanel() calls being needed. It looks to me like this should be possible. The code is "almost there". @@ +196,5 @@ > } > + }, > + > + /** > + * Sets the values of the panel's CSS properties Maybe "Refresh the panel content."? @@ +329,5 @@ > + this.matchedRulesNode.display = "none"; > + this.unmatchedRulesNode.display = "none"; > + return; > + } > + this.valueNode.innerHTML = this.propertyInfo.value; this.valueNode.innerHTML = this.value; @@ +353,5 @@ > + } > + > + if (this.matchedExpanded && matchedSelectorCount > 0) { > + CssHtmlTree.processTemplate( > + this.templateMatchedRules, this.matchedRulesTable, this); Nit: code style. CssHtmlTree.processTemplate(this.templateMatchedRules, this.matchedRulesTable, this); @@ +358,5 @@ > + this.matchedExpander.setAttribute("open", ""); > + } else { > + this.matchedExpander.removeAttribute("open"); > + this.matchedRulesTable.innerHTML = ""; > + } I would say there's too much fiddling with elements in this method. Why not just add/remove "open" attribute on .matchedRulesNode? Please rename to .matchedRulesContainer. From CSS use the [open] attribute selector to change display:block/node and whatever else is needed. I don't think you need to fiddle with the expander class name from this method. This should all be handled from the style sheet. (same comment applies to how refreshUnmatchedRules() works.) @@ +380,5 @@ > + } > + > + if (this.unmatchedExpanded && unmatchedSelectorCount > 0) { > + CssHtmlTree.processTemplate( > + this.templateUnmatchedRules, this.unmatchedRulesTable, this); Nit: code style. CssHtmlTree.processTemplate(this.templateUnmatchedRules, this.unmatchedRulesTable, this); @@ +416,2 @@ > */ > + unmatchedRuleTitle: function PropertyView_unmatchedRuleTitle() Code indentation level is wrong for the entire method. @@ +431,5 @@ > * Provide access to the SelectorViews that we are currently displaying > */ > get selectorViews() > { > var all = []; Please change this to: let all = []; @@ +438,5 @@ > all.push(new SelectorView(aSelectorInfo)); > } > > this.propertyInfo.matchedSelectors.forEach(convert); > + this.propertyInfo.unmatchedSelectors.forEach(convert); For better performance I suggest two changes here: 1. split the selectorViews() getter into two, one for matched and another one for unmatched selectors. This will allow code that only displayed the matched selectors to not touch the unmatchedSelectors array from propertyInfo. (perf improvement) 2. cache the |all| array. Here we create instances of SelectorView for each CssSelectorInfo, every time the user opens/closes a matched/unmatched view, for any property. This is slow / not ideal for perf reasons. Clear the cached arrays when the refreshUnmatchedRules() and refreshMatchedRules() methods are invoked. @@ +451,3 @@ > { > + this.matchedExpanded = !this.matchedExpanded; > + this.refresh(); this.refreshMatchedRules(); Also, shouldn't that be refreshMatchedSelectors? matchedSelectorsClick() ... and so on. @@ +461,3 @@ > { > + this.unmatchedExpanded = !this.unmatchedExpanded; > + this.refresh(); this.refreshUnmatchedRules(); ::: browser/devtools/styleinspector/csshtmltree.xhtml @@ +141,5 @@ > } > This is a template so the parent does not need to be a table, except that > using a div as the parent causes the DOM to muck with the tr elements > --> > + <div id="templateMatchedRules"> Please update the comment above. @@ +170,5 @@ > + } > + This is a template so the parent does not need to be a table, except that > + using a div as the parent causes the DOM to muck with the tr elements > + --> > + <div id="templateUnmatchedRules"> Please update the comment above. ::: browser/devtools/webconsole/HUDService.jsm @@ +4431,5 @@ > > if (!errstr) { > let stylePanel = StyleInspector.createPanel(); > stylePanel.setAttribute("hudToolId", aJSTerm.hudId); > + stylePanel.selectNode(aNode); Why was the second argument there?

(In reply to Mihai Sucan [:msucan] from comment #8) > Comment on attachment 558466 [details] [diff] [review] > Prevent UI redraw patch 3 > > Review of attachment 558466 [details] [diff] [review]: > ----------------------------------------------------------------- > > Much improved patch. Thank you! General comments: > > - For some reason I see the pointer cursor everywhere I move my mouse in the > style panel. Is that on purpose? > Nope, fixed > - Please ask for ui-review from Stephen Horlander. We now show "N selectors" > and "M unmatched selectors", both at once. Not sure if that's ideal. Or > maybe we should let this for the bug that does the UI changes? (bug 672748) > My plan is to do this in bug 672748. Shorlander said that he wants a green circle on each properties row to indicate matched rules. Now that we have decided to keep unmatched rules we would have a number in a grey circle show unmatched rules and a number in a green circle show unmatched rules. Clicking anywhere on a properties row to expand the rules (or unmatched rules if that is all we have). If rules are expanded and there are unmatched rules there should be a show unmatched rules link. To see MDN articles you will need to hover and then click the ? that pops up. > More comments below. Giving the patch r- for the changes still needed. > Looking forward to see the updated patch! Thanks! > > ::: browser/devtools/styleinspector/CssHtmlTree.jsm > @@ +152,2 @@ > > /** > > * Focus the output display on a specific element. > > Please update the comment since we are touching this method. > > It should say something like: > > "Update the highlighted element. The CssHtmlTree panel will show the style > information for the given element." > Done > @@ +177,5 @@ > > + for (let step = i + batchSize; i < step && i <= max; i++) { > > + let name = CssHtmlTree.propertyNames[i]; > > + let propView = new PropertyView(this, name); > > + CssHtmlTree.processTemplate( > > + this.templateProperty, this.propertyContainer, propView, true); > > Nit: code style should be ... > > CssHtmlTree.processTemplate(this.templateProperty, > this.propertyContainer, propView, true); > Changed > @@ +187,5 @@ > > + // the next batch of 25. > > + this.win.setTimeout(displayProperties.bind(this), 0); > > + } else { > > + this.htmlComplete = true; > > + this.refreshPanel(); > > I am surprised to see you need to call refreshPanel() when everything is > done. > > Shouldn't the initial render of the panel have everything properly > displayed? Without any additional refreshPanel() calls being needed. > > It looks to me like this should be possible. The code is "almost there". > -1 line, + 2 lines ... done ;o) > @@ +196,5 @@ > > } > > + }, > > + > > + /** > > + * Sets the values of the panel's CSS properties > > Maybe "Refresh the panel content."? > Done > @@ +329,5 @@ > > + this.matchedRulesNode.display = "none"; > > + this.unmatchedRulesNode.display = "none"; > > + return; > > + } > > + this.valueNode.innerHTML = this.propertyInfo.value; > > this.valueNode.innerHTML = this.value; > > @@ +353,5 @@ > > + } > > + > > + if (this.matchedExpanded && matchedSelectorCount > 0) { > > + CssHtmlTree.processTemplate( > > + this.templateMatchedRules, this.matchedRulesTable, this); > > Nit: code style. > > CssHtmlTree.processTemplate(this.templateMatchedRules, > this.matchedRulesTable, this); > Done > @@ +358,5 @@ > > + this.matchedExpander.setAttribute("open", ""); > > + } else { > > + this.matchedExpander.removeAttribute("open"); > > + this.matchedRulesTable.innerHTML = ""; > > + } > > I would say there's too much fiddling with elements in this method. Why not > just add/remove "open" attribute on .matchedRulesNode? > > From CSS use the [open] attribute selector to change display:block/node and > whatever else is needed. I don't think you need to fiddle with the expander > class name from this method. This should all be handled from the style sheet. > > (same comment applies to how refreshUnmatchedRules() works.) > Done > Please rename to .matchedRulesContainer. > Done > @@ +380,5 @@ > > + } > > + > > + if (this.unmatchedExpanded && unmatchedSelectorCount > 0) { > > + CssHtmlTree.processTemplate( > > + this.templateUnmatchedRules, this.unmatchedRulesTable, this); > > Nit: code style. > > CssHtmlTree.processTemplate(this.templateUnmatchedRules, > this.unmatchedRulesTable, this); > Done > @@ +416,2 @@ > > */ > > + unmatchedRuleTitle: function PropertyView_unmatchedRuleTitle() > > Code indentation level is wrong for the entire method. > Fixed > @@ +431,5 @@ > > * Provide access to the SelectorViews that we are currently displaying > > */ > > get selectorViews() > > { > > var all = []; > > Please change this to: > > let all = []; > Done > @@ +438,5 @@ > > all.push(new SelectorView(aSelectorInfo)); > > } > > > > this.propertyInfo.matchedSelectors.forEach(convert); > > + this.propertyInfo.unmatchedSelectors.forEach(convert); > > For better performance I suggest two changes here: > > 1. split the selectorViews() getter into two, one for matched and another > one for unmatched selectors. > > This will allow code that only displayed the matched selectors to not touch > the unmatchedSelectors array from propertyInfo. (perf improvement) > > 2. cache the |all| array. Here we create instances of SelectorView for each > CssSelectorInfo, every time the user opens/closes a matched/unmatched view, > for any property. This is slow / not ideal for perf reasons. > > Clear the cached arrays when the refreshUnmatchedRules() and > refreshMatchedRules() methods are invoked. > Done > @@ +451,3 @@ > > { > > + this.matchedExpanded = !this.matchedExpanded; > > + this.refresh(); > > this.refreshMatchedRules(); > > Also, shouldn't that be refreshMatchedSelectors? matchedSelectorsClick() ... > and so on. > > @@ +461,3 @@ > > { > > + this.unmatchedExpanded = !this.unmatchedExpanded; > > + this.refresh(); > > this.refreshUnmatchedRules(); > Updated names > ::: browser/devtools/styleinspector/csshtmltree.xhtml > @@ +141,5 @@ > > } > > This is a template so the parent does not need to be a table, except that > > using a div as the parent causes the DOM to muck with the tr elements > > --> > > + <div id="templateMatchedRules"> > > Please update the comment above. > Done > @@ +170,5 @@ > > + } > > + This is a template so the parent does not need to be a table, except that > > + using a div as the parent causes the DOM to muck with the tr elements > > + --> > > + <div id="templateUnmatchedRules"> > > Please update the comment above. > Done > ::: browser/devtools/webconsole/HUDService.jsm > @@ +4431,5 @@ > > > > if (!errstr) { > > let stylePanel = StyleInspector.createPanel(); > > stylePanel.setAttribute("hudToolId", aJSTerm.hudId); > > + stylePanel.selectNode(aNode); > > Why was the second argument there? selectNode used to have a forceInit parameter that I removed from the main styleinspector patch because we just didn't need it. I guess I missed this call so true was still getting passed. I suppose I could log a separate bug to remove it but that just seems like a waste.

Missed a failing test ... fixed

(In reply to Michael Ratcliffe from comment #9) > Created attachment 559145 [details] [diff] [review] > Prevent UI redraw patch 4 > > (In reply to Mihai Sucan [:msucan] from comment #8) > > Comment on attachment 558466 [details] [diff] [review] > > Prevent UI redraw patch 3 > > > > Review of attachment 558466 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > - Please ask for ui-review from Stephen Horlander. We now show "N selectors" > > and "M unmatched selectors", both at once. Not sure if that's ideal. Or > > maybe we should let this for the bug that does the UI changes? (bug 672748) > > > > My plan is to do this in bug 672748. Shorlander said that he wants a green > circle on each properties row to indicate matched rules. Now that we have > decided to keep unmatched rules we would have a number in a grey circle show > unmatched rules and a number in a green circle show unmatched rules. > Clicking anywhere on a properties row to expand the rules (or unmatched > rules if that is all we have). If rules are expanded and there are unmatched > rules there should be a show unmatched rules link. To see MDN articles you > will need to hover and then click the ? that pops up. Sounds good! For performance reasons, in the updated UI, perhaps you can find a way with Stephen Horlander to not display the counts. It would help performance A LOT, VERY MUCH if we do not have to display the counts. When we display the counts CssLogic is forced to run all the code that determines the selectors, from all rules, from all style sheets, that apply to the highlighted element or to its parents. Tons of stuff to execute. I would strongly suggest a design that just allows the user to expand matched/unmatched selectors (without displaying the counts), and that would offload computation to only when the users *asks* for it. > > ::: browser/devtools/webconsole/HUDService.jsm > > @@ +4431,5 @@ > > > > > > if (!errstr) { > > > let stylePanel = StyleInspector.createPanel(); > > > stylePanel.setAttribute("hudToolId", aJSTerm.hudId); > > > + stylePanel.selectNode(aNode); > > > > Why was the second argument there? > > selectNode used to have a forceInit parameter that I removed from the main > styleinspector patch because we just didn't need it. I guess I missed this > call so true was still getting passed. I suppose I could log a separate bug > to remove it but that just seems like a waste. Yeah, no worries. No need for a separate bug. I just wanted to know why was the second argument there. Thanks for the updated patch!

Comment on attachment 559190 [details] [diff] [review] Prevent UI redraw patch 5 Review of attachment 559190 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the updated patch! Good work! General comment: for each property we display "N selectors" and "M unmatched selectors". Why not show "N matched selectors" and "M unmatched selectors"? I suggest an update for the l10n string. More comments below. Giving the patch r- for the confusion of some property names, and for the problem with the caching of selector views. Looking forward for the updated patch! ::: browser/devtools/styleinspector/CssHtmlTree.jsm @@ +194,3 @@ > } > } > + this.win.setTimeout(displayProperties.bind(this), 0); These timeouts should be a bit longer. Firefox still freezes for me, on Ubuntu.com. Maybe 50 milliseconds? @@ +292,5 @@ > > // The parent element which contains the open attribute > this.element = null; > + // Destination for templateMatchedSelectors. > + this.matchedSelectorContainer = null; There are a few properties that hold refs to template elements, like this one (matchedSelectorContainer). Here you only have this element explained. Please add the rest of elements with descriptions. Thanks! @@ +349,5 @@ > + { > + if (!this.tree.viewedElement) { > + this.valueNode.innerHTML = ""; > + this.matchedSelectorsNode.removeAttribute("visible"); > + this.unmatchedSelectorsNode.removeAttribute("visible"); It would make sense to also clear the DOM: this.matchedSelectorContainer.innerHTML = ""; this.matchedExpander.removeAttribute("open"); this.unmatchedSelectorContainer.innerHTML = ""; this.unmatchedExpander.removeAttribute("open"); @@ +368,5 @@ > + > + if (this.matchedSelectorCount > 0) { > + this.matchedSelectorsNode.setAttribute("visible", ""); > + } else { > + this.matchedSelectorsNode.removeAttribute("visible"); Here I believe you can use the .hidden property/attribute for your purpose. No need for the "visible" attribute (and the associated CSS). (the same for refreshUnmatchedSelectors()) @@ +393,5 @@ > + refreshUnmatchedSelectors: function PropertyView_refreshUnmatchedSelectors() > + { > + this.numUnmatchedSelectors.innerHTML = this.unmatchedSelectorTitle(); > + > + if (this.unmatchedSelectorCount > 0) { numUnmatchedSelectors and unmatchedSelectorCount. That's confusing. :) I suggest you rename numUnmatchedSelectors to unmatchedSelectorCountElement or something better. (same comment for numMatchedSelectors) @@ +410,5 @@ > + } > + > + if (this.prevViewedElement != this.tree.viewedElement) { > + this._unmatchedSelectorViews = []; > + this.prevViewedElement = this.tree.viewedElement; This code breaks. When you call refreshMatchedSelectors() and prevViewedElement != this.tree.viewedElement, then this._matchedSelectorViews is cleared correctly. After that, when refreshUnmatchedSelectors() is called prevViewedElement == this.tree.viewedElement, which means that _unmatchedSelectorViews is never cleared. I suggest you always clear the _unmatchedSelectorViews array in the refreshUnmatchedSelectors() method. Do this before the call to processTemplate() - selector views are used for building the templates. You do not need to keep track of the previous element. (same comment applies to refreshMatchedSelectors()) @@ +463,5 @@ > + > + if (this._matchedSelectorViews.length == 0) { > + this.propertyInfo.matchedSelectors.forEach(convert, this); > + } > + return this._matchedSelectorViews; I would suggest having this._matchedSelectorViews = null as the initial state (when nothing is cached). Then you can do: if (this._matchedSelectorViews) { return this._matchedSelectorViews; } this._matchedSelectorViews = []; ... at the top of the function. It's clearer to read. (same comment applies to the unmatchedSelectorViews getter as well) @@ +490,3 @@ > { > + this.matchedExpanded = !this.matchedExpanded; > + this.refresh(); Shouldn't this line be: this.refreshMatchedSelectors(); ? (I asked for this in my previous review comment. :) ) @@ +500,3 @@ > { > + this.unmatchedExpanded = !this.unmatchedExpanded; > + this.refresh(); this.refreshUnmatchedSelectors(); ::: browser/devtools/styleinspector/csshtmltree.xhtml @@ +116,2 @@ > </div> > + <table save="${matchedSelectorContainer}" dir="${getRTLAttr}"></table> To avoid naming confusions I would suggest: - rename matchedSelectorsNode to matchedSelectorsContainer. - rename matchedSelectorContainer to matchedSelectorsTable. (In my previous comment I only asked to rename matchedSelectorsNode to matchedSelectorsContainer. Perhaps I wasn't clear.) @@ +123,5 @@ > + <div save="${numUnmatchedSelectors}"> > + ${unmatchedSelectorTitle(__element)} > + </div> > + </div> > + <table save="${unmatchedSelectorContainer}" dir="${getRTLAttr}"></table> Same as above. @@ +165,5 @@ > + showUnmatchedRules: true / false, // show a "more unmatched rules" link > + } > + This is a template so the parent does not need to be a table, except that > + using a div as the parent causes the DOM to muck with the tr elements > + --> The comments that describe the two templates (for matched and unmatched selectors) are not accurate. For example there's no "selectors". We use unmatchedSelectorViews actually. showUnmatchedRules doesn't exist (nor is it used in the template).

Please note that for the caching of selector views it would be ideal to only clear those two arrays when a new element is highlighted from CssHtmlTree. There's CssHtmlTree.refreshPanel() that calls PropertyView.refresh() (for each property), and that's where it makes sense to clear the arrays (as suggested in the comment above). Still, it's interesting to note that refresh() (and the two sub-refresh() methods) is called even when only toggling matched/unmatched views. In those cases we'd not want any clearing of the arrays.

(In reply to Mihai Sucan [:msucan] from comment #12) > Comment on attachment 559190 [details] [diff] [review] > Prevent UI redraw patch 5 > > Review of attachment 559190 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks for the updated patch! Good work! > > General comment: for each property we display "N selectors" and "M unmatched > selectors". Why not show "N matched selectors" and "M unmatched selectors"? > I suggest an update for the l10n string. > Done > More comments below. Giving the patch r- for the confusion of some property > names, and for the problem with the caching of selector views. Looking > forward for the updated patch! > > ::: browser/devtools/styleinspector/CssHtmlTree.jsm > @@ +194,3 @@ > > } > > } > > + this.win.setTimeout(displayProperties.bind(this), 0); > > These timeouts should be a bit longer. Firefox still freezes for me, on > Ubuntu.com. > > Maybe 50 milliseconds? > Depends which screen it is on for me ... done > @@ +292,5 @@ > > > > // The parent element which contains the open attribute > > this.element = null; > > + // Destination for templateMatchedSelectors. > > + this.matchedSelectorContainer = null; > > There are a few properties that hold refs to template elements, like this > one (matchedSelectorContainer). Here you only have this element explained. > > Please add the rest of elements with descriptions. Thanks! > I have now added the extra properties and moved them all into the prototype. > @@ +349,5 @@ > > + { > > + if (!this.tree.viewedElement) { > > + this.valueNode.innerHTML = ""; > > + this.matchedSelectorsNode.removeAttribute("visible"); > > + this.unmatchedSelectorsNode.removeAttribute("visible"); > > It would make sense to also clear the DOM: > > this.matchedSelectorContainer.innerHTML = ""; > this.matchedExpander.removeAttribute("open"); > this.unmatchedSelectorContainer.innerHTML = ""; > this.unmatchedExpander.removeAttribute("open"); > Done > @@ +368,5 @@ > > + > > + if (this.matchedSelectorCount > 0) { > > + this.matchedSelectorsNode.setAttribute("visible", ""); > > + } else { > > + this.matchedSelectorsNode.removeAttribute("visible"); > > Here I believe you can use the .hidden property/attribute for your purpose. > No need for the "visible" attribute (and the associated CSS). > > (the same for refreshUnmatchedSelectors()) > My understand was that you shouldn't use hidden if you are going to interact with elements (see https://developer.mozilla.org/en/HTML/Global_attributes#attr-hidden). Seems to work fine though ... done. > @@ +393,5 @@ > > + refreshUnmatchedSelectors: function PropertyView_refreshUnmatchedSelectors() > > + { > > + this.numUnmatchedSelectors.innerHTML = this.unmatchedSelectorTitle(); > > + > > + if (this.unmatchedSelectorCount > 0) { > > numUnmatchedSelectors and unmatchedSelectorCount. That's confusing. :) > > I suggest you rename numUnmatchedSelectors to unmatchedSelectorCountElement > or something better. > > (same comment for numMatchedSelectors) > Done > @@ +410,5 @@ > > + } > > + > > + if (this.prevViewedElement != this.tree.viewedElement) { > > + this._unmatchedSelectorViews = []; > > + this.prevViewedElement = this.tree.viewedElement; > > This code breaks. When you call refreshMatchedSelectors() and > prevViewedElement != this.tree.viewedElement, then > this._matchedSelectorViews is cleared correctly. After that, when > refreshUnmatchedSelectors() is called prevViewedElement == > this.tree.viewedElement, which means that _unmatchedSelectorViews is never > cleared. > > I suggest you always clear the _unmatchedSelectorViews array in the > refreshUnmatchedSelectors() method. Do this before the call to > processTemplate() - selector views are used for building the templates. You > do not need to keep track of the previous element. > > (same comment applies to refreshMatchedSelectors()) > See 1 comment below You are misunderstanding what this is doing. We only need to clear the selector caches when a new element is inspected. By comparing the current element with the previous one we can determine when to clear the cache. This code works as expected. (In reply to Mihai Sucan [:msucan] from comment #13) > Please note that for the caching of selector views it would be ideal to only > clear those two arrays when a new element is highlighted from CssHtmlTree. > There's CssHtmlTree.refreshPanel() that calls PropertyView.refresh() (for > each property), and that's where it makes sense to clear the arrays (as > suggested in the comment above). Still, it's interesting to note that > refresh() (and the two sub-refresh() methods) is called even when only > toggling matched/unmatched views. In those cases we'd not want any clearing > of the arrays. See 1 comment above > @@ +463,5 @@ > > + > > + if (this._matchedSelectorViews.length == 0) { > > + this.propertyInfo.matchedSelectors.forEach(convert, this); > > + } > > + return this._matchedSelectorViews; > > I would suggest having this._matchedSelectorViews = null as the initial > state (when nothing is cached). > > Then you can do: > if (this._matchedSelectorViews) { return this._matchedSelectorViews; } > this._matchedSelectorViews = []; > ... > > at the top of the function. It's clearer to read. > > (same comment applies to the unmatchedSelectorViews getter as well) > I have done something similar, still simple to read. > @@ +490,3 @@ > > { > > + this.matchedExpanded = !this.matchedExpanded; > > + this.refresh(); > > Shouldn't this line be: > > this.refreshMatchedSelectors(); > > ? > > (I asked for this in my previous review comment. :) ) > I must have missed it when I moved the selector refresh code out of refresh(), done. > @@ +500,3 @@ > > { > > + this.unmatchedExpanded = !this.unmatchedExpanded; > > + this.refresh(); > > this.refreshUnmatchedSelectors(); > > ::: browser/devtools/styleinspector/csshtmltree.xhtml > @@ +116,2 @@ > > </div> > > + <table save="${matchedSelectorContainer}" dir="${getRTLAttr}"></table> > > To avoid naming confusions I would suggest: > > - rename matchedSelectorsNode to matchedSelectorsContainer. > - rename matchedSelectorContainer to matchedSelectorsTable. > > (In my previous comment I only asked to rename matchedSelectorsNode to > matchedSelectorsContainer. Perhaps I wasn't clear.) > > @@ +123,5 @@ > > + <div save="${numUnmatchedSelectors}"> > > + ${unmatchedSelectorTitle(__element)} > > + </div> > > + </div> > > + <table save="${unmatchedSelectorContainer}" dir="${getRTLAttr}"></table> > > Same as above. > I misunderstood you last time, these names make more sense now. > @@ +165,5 @@ > > + showUnmatchedRules: true / false, // show a "more unmatched rules" link > > + } > > + This is a template so the parent does not need to be a table, except that > > + using a div as the parent causes the DOM to muck with the tr elements > > + --> > > The comments that describe the two templates (for matched and unmatched > selectors) are not accurate. For example there's no "selectors". We use > unmatchedSelectorViews actually. > > showUnmatchedRules doesn't exist (nor is it used in the template). Fixed

> > @@ +410,5 @@ > > > + } > > > + > > > + if (this.prevViewedElement != this.tree.viewedElement) { > > > + this._unmatchedSelectorViews = []; > > > + this.prevViewedElement = this.tree.viewedElement; > > > > This code breaks. When you call refreshMatchedSelectors() and > > prevViewedElement != this.tree.viewedElement, then > > this._matchedSelectorViews is cleared correctly. After that, when > > refreshUnmatchedSelectors() is called prevViewedElement == > > this.tree.viewedElement, which means that _unmatchedSelectorViews is never > > cleared. > > > > I suggest you always clear the _unmatchedSelectorViews array in the > > refreshUnmatchedSelectors() method. Do this before the call to > > processTemplate() - selector views are used for building the templates. You > > do not need to keep track of the previous element. > > > > (same comment applies to refreshMatchedSelectors()) > > > > See 1 comment below > > You are misunderstanding what this is doing. We only need to clear the > selector caches when a new element is inspected. By comparing the current > element with the previous one we can determine when to clear the cache. This > code works as expected. > > (In reply to Mihai Sucan [:msucan] from comment #13) > > Please note that for the caching of selector views it would be ideal to only > > clear those two arrays when a new element is highlighted from CssHtmlTree. > > There's CssHtmlTree.refreshPanel() that calls PropertyView.refresh() (for > > each property), and that's where it makes sense to clear the arrays (as > > suggested in the comment above). Still, it's interesting to note that > > refresh() (and the two sub-refresh() methods) is called even when only > > toggling matched/unmatched views. In those cases we'd not want any clearing > > of the arrays. > > See 1 comment above > Sorry, you were right ... of course that did not work. This is now fixed.

Comment on attachment 559420 [details] [diff] [review] Prevent UI redraw patch 7 Review of attachment 559420 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for your patch updates. Giving the patch r+ but please address my comments below. Performance: one concern that I still have is about refreshPanel(). That should probably also batch updates into setTimeouts. In highlight() the 25 props batch size should perhaps be smaller (around 15). Not sure how easy these changes would be. Maybe for a follow up bug report? ::: browser/devtools/styleinspector/CssHtmlTree.jsm @@ +401,5 @@ > + */ > + refreshMatchedSelectors: function PropertyView_refreshMatchedSelectors() > + { > + this.matchedSelectorsTitleNode.innerHTML = this.matchedSelectorTitle(); > + this.matchedSelectorsContainer.hidden = this.matchedSelectorCount <= 0; this.matchedSelectorsContainer.hidden = this.matchedSelectorCount == 0; matchedSelectorCount can never be lower than 0. (same comment applies for refreshUnmatchedSelectors()) @@ +415,5 @@ > + > + if (this.prevViewedElementForMatched != this.tree.viewedElement) { > + this._matchedSelectorViews = null; > + this.prevViewedElementForMatched = this.tree.viewedElement; > + } Thanks for your update here, but I believe it's a better solution to clear the _matchedSelectorViews and _unmatchedSelectorViews arrays in the refresh() method. That's called by refreshPanel() from the CssHtmlTree only when the highlighted element is changed. This means it fits our needs. Also, the clearing of the two arrays *must* happen before your call to processTemplate() otherwise the template still uses the old arrays. So, having the two arrays cleared in the refresh() method does exactly what's needed. @@ +419,5 @@ > + } > + }, > + > + /** > + * Refresh the panel unmatched rules Nit: please end descriptions for all comments with a period.

(In reply to Michael Ratcliffe from comment #14) > Created attachment 559414 [details] [diff] [review] > Prevent UI redraw patch 6 > > (In reply to Mihai Sucan [:msucan] from comment #12) > > Comment on attachment 559190 [details] [diff] [review] > > Prevent UI redraw patch 5 > > > > Here I believe you can use the .hidden property/attribute for your purpose. > > No need for the "visible" attribute (and the associated CSS). > > > > (the same for refreshUnmatchedSelectors()) > > > > My understand was that you shouldn't use hidden if you are going to interact > with elements (see > https://developer.mozilla.org/en/HTML/Global_attributes#attr-hidden). > > Seems to work fine though ... done. My understanding is that the attribute fits our needs. You shouldn't use hidden=true to just hide elements for styling purpose, when a different designer might want to show the element. .hidden is "when you don't need that part of the DOM to show" from a programmatic POV - which is exactly the case here.

(In reply to Mihai Sucan [:msucan] from comment #16) > Comment on attachment 559420 [details] [diff] [review] > Prevent UI redraw patch 7 > > Review of attachment 559420 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks for your patch updates. Giving the patch r+ but please address my > comments below. > > Performance: one concern that I still have is about refreshPanel(). That > should probably also batch updates into setTimeouts. In highlight() the 25 > props batch size should perhaps be smaller (around 15). Not sure how easy > these changes would be. Maybe for a follow up bug report? > I have switched to 15. > ::: browser/devtools/styleinspector/CssHtmlTree.jsm > @@ +401,5 @@ > > + */ > > + refreshMatchedSelectors: function PropertyView_refreshMatchedSelectors() > > + { > > + this.matchedSelectorsTitleNode.innerHTML = this.matchedSelectorTitle(); > > + this.matchedSelectorsContainer.hidden = this.matchedSelectorCount <= 0; > > this.matchedSelectorsContainer.hidden = this.matchedSelectorCount == 0; > > matchedSelectorCount can never be lower than 0. > > (same comment applies for refreshUnmatchedSelectors()) > True, changed > @@ +415,5 @@ > > + > > + if (this.prevViewedElementForMatched != this.tree.viewedElement) { > > + this._matchedSelectorViews = null; > > + this.prevViewedElementForMatched = this.tree.viewedElement; > > + } > > Thanks for your update here, but I believe it's a better solution to clear > the _matchedSelectorViews and _unmatchedSelectorViews arrays in the > refresh() method. That's called by refreshPanel() from the CssHtmlTree only > when the highlighted element is changed. This means it fits our needs. > > Also, the clearing of the two arrays *must* happen before your call to > processTemplate() otherwise the template still uses the old arrays. So, > having the two arrays cleared in the refresh() method does exactly what's > needed. > That makes sense ... fixed. > @@ +419,5 @@ > > + } > > + }, > > + > > + /** > > + * Refresh the panel unmatched rules > > Nit: please end descriptions for all comments with a period. Done

Rebased

In CssHtmlTree.jsm I did: - unmatchedSelectorTitle: function PropertyView_matchedSelectorTitle() + unmatchedSelectorTitle: function PropertyView_unmatchedSelectorTitle() - unmatchedSelectorsClick: function PropertyView_matchedSelectorsClick(aEvent) + unmatchedSelectorsClick: function PropertyView_unmatchedSelectorsClick(aEvent) (something I forgot to notice during review) Will land this patch ASAP.

Also, thanks for your patch updates based on my review comments, and for the rebase!

Comment on attachment 560328 [details] [diff] [review] [in-fx-team] Prevent UI redraw patch 10 Pushed: https://hg.mozilla.org/integration/fx-team/rev/e149493b4133

Comment on attachment 560328 [details] [diff] [review] [in-fx-team] Prevent UI redraw patch 10 https://hg.mozilla.org/mozilla-central/rev/e149493b4133