The current style view(s) are a brain dump of information that is hard to navigate, it would be a big benefit to users if the CSS information was presented in an easier to use format.

Just moving to XUL tree rather than HTML tree

Comment on attachment 460876 [details] [diff] [review] Progress so far on alternative style panel presentation # HG changeset patch # User Joe Walker <jwalker@mozilla.com> # Date 1280480973 -3600 # Node ID e42c10b75358f8c489cc09723ba08a77d95030e0 # Parent c5f35032f22df63a99d63bba7cf791ae309d0ab7 Addition of an alternative way to view style information - see bug 582596 diff -r c5f35032f22d browser/base/content/browser-sets.inc --- a/browser/base/content/browser-sets.inc Fri Jul 30 11:07:42 2010 +0200 +++ b/browser/base/content/browser-sets.inc Fri Jul 30 16:07:43 2010 +0100 @@ -152,6 +152,8 @@ disabled="true"/> <command id="Inspector:Style" oncommand="InspectorUI.toggleStylePanel();"/> + <command id="Inspector:Style2" + oncommand="InspectorUI.style2.togglePanel();"/> </commandset> <broadcasterset id="mainBroadcasterSet"> diff -r c5f35032f22d browser/base/content/browser.js --- a/browser/base/content/browser.js Fri Jul 30 11:07:42 2010 +0200 +++ b/browser/base/content/browser.js Fri Jul 30 16:07:43 2010 +0100 @@ -164,6 +164,7 @@ #include browser-fullZoom.js #include inspector.js +#include style2.js #include browser-places.js #include browser-tabPreviews.js diff -r c5f35032f22d browser/base/content/browser.xul --- a/browser/base/content/browser.xul Fri Jul 30 11:07:42 2010 +0200 +++ b/browser/base/content/browser.xul Fri Jul 30 16:07:43 2010 +0100 @@ -252,6 +252,11 @@ accesskey="&inspectStyleButton.accesskey;" class="toolbarbutton-text" command="Inspector:Style"/> + <toolbarbutton id="inspector-style2-toolbutton" + label="&inspectStyle2Button.label;" + accesskey="&inspectStyle2Button.accesskey;" + class="toolbarbutton-text" + command="Inspector:Style2"/> </toolbar> <tree id="inspector-tree" class="plain" seltype="single" @@ -291,6 +296,52 @@ </hbox> </panel> + <panel id="inspector-style2-panel" + class="dimmable" + orient="vertical" + ignorekeys="true" + noautofocus="true" + noautohide="true" + level="top"> + <toolbar id="inspector-style2-toolbar" + nowindowdrag="true"> + <label>&inspectStyle2PanelTitle.label;</label> + </toolbar> + <!-- + <browser id="inspector-style2-browser" + flex="1" + src="chrome://browser/content/style2.html" + onclick="InspectorUI.style2.control.onClick(event);" + disablehistory="true" /> + --> + <tree id="inspector-style2-tree" class="plain" + seltype="single" + treelines="true" + flex="1"> + <!-- + onselect="InspectorUI.style2.control.onTreeSelected(event);" + --> + <treecols> + <treecol id="style2Property" label="Property" primary="true" + persist="width,hidden,ordinal" flex="1" width="35%"/> + <splitter class="tree-splitter"/> + <treecol id="style2Selector" label="Selector" + persist="width,hidden,ordinal" flex="1" width="17%"/> + <splitter class="tree-splitter"/> + <treecol id="style2Value" label="Value" + persist="width,hidden,ordinal" flex="1" width="30%"/> + <splitter class="tree-splitter"/> + <treecol id="style2Source" label="Source" + persist="width,hidden,ordinal" flex="1" width="18%"/> + </treecols> + <treechildren id="inspector-style2-tree-body"/> + </tree> + <hbox align="end"> + <spacer flex="1" /> + <resizer dir="bottomend" /> + </hbox> + </panel> + <menupopup id="toolbar-context-menu" onpopupshowing="onViewToolbarsPopupShowing(event);"> <menuseparator/> diff -r c5f35032f22d browser/base/content/inspector.js --- a/browser/base/content/inspector.js Fri Jul 30 11:07:42 2010 +0200 +++ b/browser/base/content/inspector.js Fri Jul 30 16:07:43 2010 +0100 @@ -23,6 +23,7 @@ * * Contributor(s): * Rob Campbell <rcampbell@mozilla.com> (original author) + * Joe Walker (jwalker@mozilla.com) * * Alternatively, the contents of this file may be used under the terms of * either the GNU General Public License Version 2 or later (the "GPL"), or @@ -500,6 +501,114 @@ selectEventsSuppressed: false, inspecting: false, + style2: { + /** + * Should the style panel be showing? + * TODO: what is the difference between showPanel and panel.state? + */ + showPanel: true, + + /** + * The Stylesheet inspection logic. Setup by init() + * An instance of CssLogic (see style2.js) + */ + cssLogic: null, + + /** + * An instance of CssHtmlTree (see style2.js). Setup by init() + * But only if browser.xul contains inspector-style2-browser + */ + control: null, + + /** + * An instance of CssXulTreeView (see style2.js). Setup by init() + * But only if browser.xul contains inspector-style2-tree + */ + treeView: null, + + /** + * Reference to panel#inspector-style2-panel from browser.xul + */ + panel: null, + + /** + * TODO: This should probably be a real constructor, but we're all static + */ + init: function IUI_style2_init() + { + this.panel = document.getElementById("inspector-style2-panel"); + this.panel.hidden = false; + + this.cssLogic = new CssLogic(InspectorUI.style); + + let styleWin = document.getElementById("inspector-style2-browser"); + if (styleWin) { + this.control = new CssHtmlTree(styleWin, this.cssLogic); + } + + // Setup the XUL tree + let tree = document.getElementById("inspector-style2-tree"); + if (tree) { + this.treeView = new CssXulTreeView(this.cssLogic); + tree.view = this.treeView; + } + + if (this.showPanel) { + this.openPanel(); + } + }, + + /** + * Toggle the style2 panel. + * Invoked from the toolbar's Style2 button (browser-sets.inc) + */ + togglePanel: function IUI_style2_togglePanel() + { + if (this.showPanel) { + this.panel.hidePopup(); + } else { + this.openPanel(); + if (InspectorUI.treeView.selectedNode) { + this.highlight(InspectorUI.treeView.selectedNode); + } + } + this._showPanel = !this._showPanel; + }, + + highlight: function IUI_style2_highlight(element) + { + this.cssLogic.highlight(element); + + if (this.control) { + this.control.highlight(element); + } + if (this.treeView) { + this.treeView.highlight(element); + } + }, + + /** + * Display this.panel on the screen + */ + openPanel: function IUI_style2_openPanel() + { + if (!this.isOpen()) { + // open at top right of browser panel, offset by 20px from top. + this.panel.openPopup(InspectorUI.browser, "end_before", 0, -20, false, false); + // size panel to 200px wide by half browser height - 60. + this.panel.sizeTo(200, InspectorUI.win.outerHeight / 2 - 60); + } + }, + + /** + * Report on the state of this.panel + */ + isOpen: function IUI_style2_isOpen() + { + return this.panel && this.panel.state === "open"; + } + }, + /** * Toggle the inspector interface elements on or off. * @@ -646,6 +755,7 @@ this.clearStylePanel(); this.openStylePanel(); } + this.style2.init(); if (this._showDOMPanel) { this.openDOMPanel(); } @@ -688,6 +798,9 @@ if (this.isStylePanelOpen) { this.stylePanel.hidePopup(); } + if (this.style2.isOpen()) { + this.style2.panel.hidePopup(); + } this.inspectCmd.setAttribute("checked", false); this.browser = this.win = null; // null out references to browser and window }, @@ -701,6 +814,7 @@ this.attachPageListeners(); this.inspecting = true; this.toggleDimForPanel(this.stylePanel); + this.toggleDimForPanel(this.style2.panel); }, /** @@ -714,8 +828,10 @@ this.detachPageListeners(); this.inspecting = false; this.toggleDimForPanel(this.stylePanel); + this.toggleDimForPanel(this.style2.panel); if (this.treeView.selection) { this.updateStylePanel(this.treeView.selectedNode); + this.style2.highlight(this.treeView.selectedNode); } }, @@ -887,6 +1003,7 @@ this.highlighter.highlightNode(node); this.stopInspecting(); this.updateStylePanel(node); + this.style2.highlight(node); return true; }, @@ -929,6 +1046,7 @@ this.treeView.selectedNode = aNode; this.selectEventsSuppressed = false; this.updateStylePanel(aNode); + this.style2.highlight(aNode); }, /////////////////////////////////////////////////////////////////////////// diff -r c5f35032f22d browser/base/content/style2.html --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/browser/base/content/style2.html Fri Jul 30 16:07:43 2010 +0100 @@ -0,0 +1,117 @@ +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN" + "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd"> + +<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en"> +<head> + <style id="dynamic"></style> + <title>Style2</title> + <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/> + <!-- <link rel="stylesheet" href="chrome://browser/skin/style2.css" type="text/css"/> --> + <style> + body { + font-family: Lucida Grande, sans-serif; + font-size: 11px; + } + table.styles { border: 1px solid black; overflow-y: auto; } + table.styles { border-collapse: collapse; } + table.styles th { background: #EEE; } + table.styles th, + table.styles td { + text-align: left; padding-left: 2px; vertical-align:top; + } + + table.styles .twisty { text-align: center; font-size: 80%; width: 10px; } + + .style-name { text-align: right; } + .style-src { font-size: 90%; color: #666; padding-left: 20px; } + .style-rule { font-family: monospace; padding-left: 20px; } + + .overridden { /*background: #FEA;*/ } + .filename { font-family: monospace; color: #006; } + /*.docpos { font-size: 80%; vertical-align: top; }*/ + .docpos { display: none; } + .simple .specificity, .defaultonly .default { display: none; } + p.specific { margin-top:0px; padding: 2px 0; background: #ddd; } + .greyout { color: #666; } + </style> +</head> +<body role="application"> + +<!-- +The templates below are inserted into this table. +To visually debug the templates without running firefox, remove the display:none +from the templates below and load into a browser +--> +<table id="tableDisplay" class="styles simple"> +</table> + +<!-- +Explanatory headers (currently unused) +TODO: If we have time, we should find a way to have this at the top of the +table, however it needs to be included before the main template and refreshed +each time because otherwise gecko gets confused about the number of columns +due to the row hiding that we're doing. +--> +<table id="headTemplate" style="display:none;"> + <tr> + <th>&nbsp;</th> + <th colspan="2">Name</th> + <th>Value</th> + <th>Source</th> + <th>Selector</th> + </tr> +</table> + +<!-- +The main body of the table +Down triangle (i.e. expanded) = &#9654; +Right triangle (i.e. hidden) = &#9660; +--> +<table id="tableTemplate" class="styles simple" style="display:none;" foreach="style in ${styles}"> + <tr> + <th class="twisty" toggle="${style.property}">&#9654;</th> + <th class="style-name" colspan="2">${Style2.preize(style.property)}</th> + <th save="${style.valueEle}">${style.value}</th> + <th class="style-src" save="${style.sourceEle}">${style.source}</th> + <th class="style-rule" save="${style.selectorEle}">${style.selector}</th> + </tr> + <tr class="hiddenWith${style.property}" save="${style.matchedEle}"> + <td>&nbsp;</td> + <td class="twisty" toggle="${style.property}Matched">&#9654;</td> + <td> + Overridden + <span save="${style.matchedCountEle}">${style.matched ? '(' + style.matched.length + ')' : ''}</span> + </td> + <td>&nbsp;</td> + <td>&nbsp;</td> + <td>&nbsp;</td> + </tr> + <tr class="hiddenWith${style.property}" save="${style.unmatchedEle}"> + <td>&nbsp;</td> + <td class="twisty" toggle="${style.property}Unmatched">&#9654;</td> + <td> + Unmatched + <span save="${style.unmatchedCountEle}">${style.matched ? '(' + style.unmatched.length + ')' : ''}</span> + </td> + <td>&nbsp;</td> + <td>&nbsp;</td> + <td>&nbsp;</td> + </tr> +</table> + +<!-- +We insert one of these for each set of matched and unmatched style rules +--> +<table id="rowTemplate" style="display:none;"> + <tr class="hiddenWith${name}${match} hiddenWith${name}" foreach="rule in ${container}"> + <td>&nbsp;</td> + <td>&nbsp;</td> + <td>&nbsp;</td> + <td>${rule.value}</td> + <td class="style-src">${rule.source}</td> + <td class="style-rule">${Style2.preize(rule.selector)}</td> + </tr> +</table> + +</body> +</html> diff -r c5f35032f22d browser/base/content/style2.js --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/browser/base/content/style2.js Fri Jul 30 16:07:43 2010 +0100 @@ -0,0 +1,1484 @@ +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ +/* vim: set ts=2 et sw=2 tw=80: */ +/* ***** BEGIN LICENSE BLOCK ***** + * Version: MPL 1.1/GPL 2.0/LGPL 2.1 + * + * The contents of this file are subject to the Mozilla Public License Version + * 1.1 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * http://www.mozilla.org/MPL/ + * + * Software distributed under the License is distributed on an "AS IS" basis, + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License + * for the specific language governing rights and limitations under the + * License. + * + * The Original Code is the Mozilla Inspector Module. + * + * The Initial Developer of the Original Code is + * The Mozilla Foundation. + * Portions created by the Initial Developer are Copyright (C) 2010 + * the Initial Developer. All Rights Reserved. + * + * Contributor(s): + * Rob Campbell <rcampbell@mozilla.com> (original author) + * Joe Walker (jwalker@mozilla.com) + * + * Alternatively, the contents of this file may be used under the terms of + * either the GNU General Public License Version 2 or later (the "GPL"), or + * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), + * in which case the provisions of the GPL or the LGPL are applicable instead + * of those above. If you wish to allow use of your version of this file only + * under the terms of either the GPL or the LGPL, and not to allow others to + * use your version of this file under the terms of the MPL, indicate your + * decision by deleting the provisions above and replace them with the notice + * and other provisions required by the GPL or the LGPL. If you do not delete + * the provisions above, a recipient may use your version of this file under + * the terms of any one of the MPL, the GPL or the LGPL. + * + * ***** END LICENSE BLOCK ***** */ + +/* + * Done: + * - remove the HTML table + * - split out the HTML table code + * - update documentation + * - de-dupe multiple selectors on a line + * - include a count in Overloaded / Unmatched + * - fully dig into a property at first open + * - dig line numbers out + * - remove Overloaded / Unmatched twisty where count = 0 + * - Make the property column wider, shrink the others + * + * Working: + * - Try out [selector, value, source] ordering + * - fill in ? for actual style values + * + * Outstanding tasks: + * - make the source be a link + * - provide alternate values for color/font-weight/etc + * - create specificity calculator class + * + * - Add editability + * - Try out getting rid of Matched/Unmatched twisties and replacing with a + * 'Why not used' column + * - Add a stylesheet filter to top + */ + +/** + * Other bits of CSS display logic work simply by digging into the actually + * styles of a selected element and its parents. This method fails to answer + * many questions about why the users styles did not work, specifically because + * it only looks at the things that did. This approach takes into account all + * the available styles, and shows why each rule is not applicable. + * + * <p>This approach can be considerably more costly while a page's stylesheets + * are examined, so this class tries to be smart about the information that it + * caches. + * + * <p>These functions could probably be merged into stylePanel.jsm + */ +let CssLogic = function(stylePanel) +{ + // Instance of stylePanel.jsm + this.stylePanel = stylePanel; + + // Both setup by highlight() + this.viewedElement = null; + this.viewedDocument = null; + + // The cache of examined CSS properties + this.properties = {}; +}; + +CssLogic.prototype = { + /** + * Focus on a new element - remove the style caches + */ + highlight: function CssLogic_highlight(viewedElement) + { + if (this.viewedElement === viewedElement) { + return; + } + + this.viewedElement = viewedElement; + + let doc = XPCNativeWrapper.unwrap(viewedElement.ownerDocument); + if (doc !== this.viewedDocument) { + // New document; clear the cache + this.viewedDocument = doc; + this.properties = {}; + } else { + // Same document, new element; clear the matched/unmatched data + for (let property in this.properties) { + delete this.properties[property].matched; + delete this.properties[property].unmatched; + } + } + + let win = this.viewedDocument.defaultView; + this.computedStyle = win.getComputedStyle(this.viewedElement, ""); + }, + + /** + * Dig through the stylesheets and get a data structure like this: + * { + * property: 'color', + * value: 'red', + * source: 'styles.css', + * selector: 'p.specific', + * matched: // contains all rules that can apply to viewedElement + * [ + * { value:'blue', source:'other.css', selector:'p' }, + * { value:'#F80', source:'other.css', selector:'*' }, + * ... + * ], + * unmatched: // rules that don't apply to viewedElement + * [ + * { value:'white', source:'other.css', selector:'div' }, + * ... + * ], + * allStyles: [ union of matched and unmatched ], + * } + * + * <p>Ideally we would pull out the 'winning' style from the matched array. + * Currently we do not know how to do this + * @param property The CSS property to look for + * @param deep Do we fill in matched|unmatched|allStyles + */ + getCssInfo: function CssLogic_getRulesForProperty(property, deep) + { + if (!this.viewedElement) { + jlog('Warning this.viewedElement = ', this.viewedElement); + return {}; + } + + let cssInfo = this.properties[property]; + if (!cssInfo) { + cssInfo = { + property: property, + value: this.computedStyle.getPropertyValue(property), + source: '', + selector: '', + }; + this.properties[property] = cssInfo; + } + + if (deep) { + if (!cssInfo.allRules) { + cssInfo.allRules = []; + this._addPropertyRules(property, cssInfo.allRules); + } + if (!cssInfo.matched) { + this._markMatches(property, cssInfo); + } + // Go back and try to guess at which rule was used by the browser + // TODO: When matched is sorted by specificity we can just pick the 1st + let winner = []; + let winnerIndex = -1; + for (let i = 0; i < cssInfo.matched.length; i++) { + let rule = cssInfo.matched[i]; + if (rule.value === cssInfo.value) { + winnerIndex = i; + winner.push(rule); + } + } + if (winner.length === 1) { + cssInfo.matched.splice(winnerIndex, 1); + cssInfo.source = winner[0].source; + cssInfo.selector = winner[0].selector; + jlog('winner', winner, cssInfo); + } + } + + return cssInfo; + }, + + /** + * Dig through all the sheets looking for matches for <tt>property</tt> and + * adding the results to <tt>rules</tt> + * @param property {string} The CSS property to look for + * @param rules {rule[]} Array to which we add the matching rules. i.e. the + * <tt>allRules</tt> member of a cssInfo + */ + _addPropertyRules: function CssLogic_addPropertyRules(property, rules) + { + let sheets = this.viewedDocument.styleSheets; + for (let s = 0; s < sheets.length; s++) { + this._addPropertyRulesFromSheet(sheets[s], property, rules); + } + }, + + /** + * Dig through a single sheet, calling self recursively when we find an + * import rule. + * @see _addPropertyRules() + */ + _addPropertyRulesFromSheet: function CssLogic_addPropertyRulesFromSheet(sheet, property, rules) + { + let self = this; + + // Work out a shorthand for the source + // TODO: include line numbers in here + let source = sheet.href; + if (source) { + source = source.split('/'); + source = source[source.length - 1]; + } else { + source = '[HTML]'; + } + + let cssRules = sheet.cssRules; + jlog('Looking into \'' + source + '\'. Rule count: ' + cssRules.length + + '. For ' + property); + + for (let r = 0; r < cssRules.length; r++) { + let cssRule = XPCNativeWrapper.unwrap(cssRules[r]); + let line = ''; + if (this.stylePanel) { + line = this.stylePanel.domUtils.getRuleLine(cssRule); + } + + // Process @import rules. TODO: Do we need to worry about recursion? + if (cssRule.styleSheet) { + this._examineStylesFromSheet(cssRule.styleSheet); + } + + let styles = cssRule.style; + if (styles) { + for (let t = 0; t < styles.length; t++) { + if (styles[t] == property) { + let selectorText = cssRule.selectorText; + selectorText.split(',').forEach(function(selector) { + selector = selector.trim(); + rules.push({ + property: property, + // TODO: Firebug wraps this in a rgbToHex() function. Eh? + value: styles.getPropertyValue(property), + source: source + ':' + line, + href: sheet.href, // TODO: if null then document.location? + line: line, + selector: selector + }); + }); + } + } + } + } + }, + + /** + * Go through the <tt>allRules</tt> member of <tt>cssInfo</tt> and work out + * if each rule applies to <tt>viewedElement</tt> + */ + _markMatches: function CssLogic_markMatches(property, cssInfo) + { + cssInfo.matched = []; + cssInfo.unmatched = []; + + let self = this; + + for (let r = 0; r < cssInfo.allRules.length; r++) { + let rule = cssInfo.allRules[r]; + + // Do we have a selector match? + let matched = false; + let selectorMatches = self.viewedDocument.querySelectorAll(rule.selector); + for (let m = 0; m < selectorMatches.length; m++) { + if (selectorMatches[m] == self.viewedElement) { + matched = true; + } + } + + if (matched) { + cssInfo.matched.push(rule); + } else { + cssInfo.unmatched.push(rule); + } + } + + // TODO: Sort the matched array by selector specificity + // TODO: Sort unmatched array by how close the matches are? + }, +}; + +/** + * This list of styles is taken directly from Firebug. + * We need to decide how to acknowledge this. + * We are also likely to significantly change it so ... + */ +CssLogic.styleGroups = +{ + text: [ + "font-family", + "font-size", + "font-weight", + "font-style", + "color", + "text-transform", + "text-decoration", + "letter-spacing", + "word-spacing", + "line-height", + "text-align", + "vertical-align", + "direction", + "column-count", + "column-gap", + "column-width" + ], + + background: [ + "background-color", + "background-image", + "background-repeat", + "background-position", + "background-attachment", + "opacity" + ], + + box: [ + "width", + "height", + "top", + "right", + "bottom", + "left", + "margin-top", + "margin-right", + "margin-bottom", + "margin-left", + "padding-top", + "padding-right", + "padding-bottom", + "padding-left", + "border-top-width", + "border-right-width", + "border-bottom-width", + "border-left-width", + "border-top-color", + "border-right-color", + "border-bottom-color", + "border-left-color", + "border-top-style", + "border-right-style", + "border-bottom-style", + "border-left-style", + "-moz-border-top-radius", + "-moz-border-right-radius", + "-moz-border-bottom-radius", + "-moz-border-left-radius", + "outline-top-width", + "outline-right-width", + "outline-bottom-width", + "outline-left-width", + "outline-top-color", + "outline-right-color", + "outline-bottom-color", + "outline-left-color", + "outline-top-style", + "outline-right-style", + "outline-bottom-style", + "outline-left-style" + ], + + layout: [ + "position", + "display", + "visibility", + "z-index", + "overflow", + "white-space", + "clip", + "float", + "clear", + "-moz-box-sizing" + ], + + other: [ + "cursor", + "list-style-image", + "list-style-position", + "list-style-type", + "marker-offset", + "user-focus", + "user-select", + "user-modify", + "user-input" + ] +}; + +//////////////////////////////////////////////////////////////////////////////// + +/** + * A 4 layer deep tree to display CSS properties. + * <p>Tree levels: + * <pre> + * - Group: + * - Property: + * - Category: + * - Rule: + * <pre> + * <p>For information about XUL Trees: + * - https://developer.mozilla.org/en/XUL_Tutorial/Tree_View_Details + * - https://developer.mozilla.org/en/XUL_Tutorial/Custom_Tree_Views + * - https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsITreeView + */ +let CssXulTreeView = function(cssLogic) +{ + this.cssLogic = cssLogic; + this.visibleData = []; + for (group in CssLogic.styleGroups) { + this.visibleData.push({ + name: group, + container: true, + open: false, + level: 0 + }); + } + + // See https://developer.mozilla.org/en/nsITreeBoxObject + this.treeBox = null; + this.selection = null; +}; + +/** + * The LEVEL consts apply to the .level property of the objects in visibleData + */ +CssXulTreeView.LEVEL = { + GROUP: 0, // e.g. 'text' See CssLogic.styleGroups + PROPERTY: 1, // e.g. 'vertical-align' A CSS property + MATCH: 2, // Overloaded Match | Unmatched. + RULE: 3, // A rule that may or may not match a selected element +}; + +/** + * Our tree has 4 columns. + * TODO: These are not actually used at the moment, is there a use for this? + */ +CssXulTreeView.COL = { + NAME: 0, // Dictated by the level + SELECTOR: 1, // The CSS selector declared by the rule + VALUE: 2, // What is the CSS value of the rule + SOURCE: 3, // What file and where did the rule come from? +}; + +/** + * Effectively the above in reverse + */ +CssXulTreeView.PROPERTY_FOR_COL = [ 'name', 'selector', 'value', 'source' ]; + +CssXulTreeView.prototype = { + highlight: function(viewedElement) + { + this.viewedElement = viewedElement; + // TODO: Priority: reset the tree, delete the data and re-run the logic in the ctor + }, + + get rowCount() + { + return this.visibleData.length; + }, + + getCellText: function(idx, column) + { + let property = CssXulTreeView.PROPERTY_FOR_COL[column.index]; + return this.visibleData[idx][property] || ''; + }, + + isContainer: function(idx) + { + // TODO: Shouldn't this be this.visibleData[idx].container ? + return this.visibleData[idx].level < CssXulTreeView.LEVEL.RULE; + }, + + isContainerOpen: function(idx) + { + return this.visibleData[idx].open; + }, + + isContainerEmpty: function(idx) + { + let data = this.visibleData[idx]; + // Only LEVEL MATCH nodes with empty contents are empty. + if (data.level !== CssXulTreeView.LEVEL.MATCH) { + return false; + } + return data.contents.length === 0; + }, + + getParentIndex: function(idx) + { + // TODO: Should we cache the parent in the visibility data ??? + let level = this.visibleData[idx].level; + if (level === CssXulTreeView.LEVEL.GROUP) { + return -1; + } + // TODO: I think this version is more correct + for (let t = idx - 1; t >= 0 ; t--) { + if (this.visibleData[t].level < level) { + return t; + } + } + /* MDC published version + for (let t = idx - 1; t >= 0 ; t--) { + if (this.isContainer(t)) { + return t; + } + } + */ + }, + + getLevel: function(idx) + { + return this.visibleData[idx].level; + }, + + hasNextSibling: function(idx, after) + { + let thisLevel = this.getLevel(idx); + for (let t = after + 1; t < this.visibleData.length; t++) { + let nextLevel = this.getLevel(t); + if (nextLevel == thisLevel) { + return true; + } + if (nextLevel < thisLevel) { + break; + } + } + return false; + }, + + toggleOpenState: function(idx) + { + let item = this.visibleData[idx]; + if (item.level === CssXulTreeView.LEVEL.RULE) { + return; + } + + if (item.open) { + // The close algorithm is indifferent to the level - walk down the tree + // starting at the next row looking for the first row of a lower level, + // removing all you find + // TODO: This algorithm has a problem in that it collapses an entire tree + // when a top level node is collapsed. Some trees remember the state of + // the closed nodes so when the top level node is re-opened, they all + // re-open to their previous state. + // This is probably a bug, but a minor one. + item.open = false; + let thisLevel = this.getLevel(idx); + let deletecount = 0; + for (let t = idx + 1; t < this.visibleData.length; t++) { + if (this.getLevel(t) > thisLevel) { + deletecount++; + } + else { + break; + } + } + if (deletecount) { + this.visibleData.splice(idx + 1, deletecount); + this.treeBox.rowCountChanged(idx + 1, -deletecount); + } + } + else { + item.open = true; + + let level = this.visibleData[idx].level; + if (level === CssXulTreeView.LEVEL.GROUP) { + // Add rows for each of the properties in this group + let group = this.visibleData[idx].name; + let properties = CssLogic.styleGroups[group]; + for (let i = 0; i < properties.length; i++) { + let cssInfo = this.cssLogic.getCssInfo(properties[i], false); + cssInfo.name = properties[i]; + cssInfo.container = true; + cssInfo.open = false; + cssInfo.level = CssXulTreeView.LEVEL.PROPERTY; + this.visibleData.splice(idx + i + 1, 0, cssInfo); + } + this.treeBox.rowCountChanged(idx + 1, properties.length); + } + + else if (level === CssXulTreeView.LEVEL.PROPERTY) { + let property = this.visibleData[idx].name; + // We could avoid the deep dig here, but then we would not get the + // source/selector info, and it's likely that the user will be going + // into either matched or unmatched, so we'll need to do it anyway + // TODO: do this in a setTimeout? + let cssInfo = this.cssLogic.getCssInfo(property, true); + jlog('invalidateRow', idx, this.visibleData[idx]); + this.treeBox.invalidateRow(idx); + + this.visibleData.splice(idx + 1, 0, { + name: 'Overloaded Matches (' + cssInfo.matched.length + ')', + parentProperty: property, + container: true, + open: false, + level: CssXulTreeView.LEVEL.MATCH, + isMatched: true, + contents: cssInfo.matched + }, { + name: 'Unmatched (' + cssInfo.unmatched.length + ')', + parentProperty: property, + container: true, + open: false, + level: CssXulTreeView.LEVEL.MATCH, + isMatched: false, + contents: cssInfo.unmatched + }); + this.treeBox.rowCountChanged(idx + 1, 2); + } + + else if (level === CssXulTreeView.LEVEL.MATCH) { + let property = this.visibleData[idx].parentProperty; + let cssInfo = this.cssLogic.getCssInfo(property, true); + + let isMatched = this.visibleData[idx].isMatched; + let rules = isMatched ? cssInfo.matched : cssInfo.unmatched; + + for (let i = 0; i < rules.length; i++) { + let rule = rules[i]; + rule.name = ''; // The name is higher up in the tree + rule.container = false; + rule.open = false; + rule.level = CssXulTreeView.LEVEL.RULE; + this.visibleData.splice(idx + i + 1, 0, rule); + } + this.treeBox.rowCountChanged(idx + 1, rules.length); + } + } + this.treeBox.invalidateRow(idx); + }, + + setTree: function(treeBox) + { + this.treeBox = treeBox; + }, + + isSeparator: function(idx) + { + return false; + }, + + isSorted: function() + { + return false; + }, + + isEditable: function(idx, column) + { + return false; + }, + + getImageSrc: function(idx, column) + { + }, + + getProgressMode : function(idx,column) + { + }, + + getCellValue: function(idx, column) + { + }, + + cycleHeader: function(col, elem) + { + }, + + selectionChanged: function() + { + }, + + cycleCell: function(idx, column) + { + }, + + performAction: function(action) + { + }, + + performActionOnCell: function(action, index, column) + { + }, + + getRowProperties: function(idx, column, prop) + { + }, + + getCellProperties: function(idx, column, prop) + { + }, + + getColumnProperties: function(column, element, prop) + { + }, +}; + +//////////////////////////////////////////////////////////////////////////////// + +/** + * CssHtmlTree is a panel that manages the display of a table sorted by style. + * There should be one instance of CssHtmlTree per style display (of which there will + * generally only be one). + * @params document The main XUL browser document + * @constructor + */ +let CssHtmlTree = function(styleWin, cssLogic) +{ + // 'window' in that it contains a document + this.styleWin = styleWin + this.cssLogic = cssLogic; + + // The document in which we display the results + this.styleDocument = XPCNativeWrapper.unwrap(this.styleWin.contentWindow.document); + // The input template that we use to process the display the results + this.tableTemplate = this.styleDocument.getElementById('tableTemplate'); + // The input template that we use to process the display the results + this.rowTemplate = this.styleDocument.getElementById('rowTemplate'); + // The node in which we display the processed template + this.tableDisplay = this.styleDocument.getElementById('tableDisplay'); + // We edit a custom stylesheet to make the tree work + this.customSheet = this.styleDocument.getElementById('dynamic').sheet; + + // The element that we're inspecting + this.viewedElement = null; + // And the document that it comes from + this.viewedDocument = null; +}; + +CssHtmlTree.prototype = { + /** + * TODO: Not sure if we need this it is currently referenced from browser.xul + */ + onTreeSelected: function() {}, + + /** + * Focus the output display on a specific element + * @param element The highlighted node to get styles for. + */ + highlight: function CssHtmlTree_highlight(element) + { + if (element === this.viewedElement) { + return; + } + + this.viewedElement = element; + this.viewedDocument = XPCNativeWrapper.unwrap(element.ownerDocument); + + this._populateTableTemplate(); + }, + + /** + * Used by the twisties in the tree in the style display window to toggle + * the display of the child rows. + * Handle click events in the html tree panel. + * Invoked from browser.xul + */ + onClick: function CssHtmlTree_onClick(event) + { + // Ignore anything that isn't on a twisty + if (!event.target.classList.contains("twisty")) { + return; + } + + let source = event.target; + // The toggle attribute tells us what style we should be toggling + let name = source.getAttribute("toggle"); + if (name) { + let cssInfo = this.cssLogic.getCssInfo(name, true); + + if (cssInfo.matchedCountEle) { + cssInfo.matchedCountEle.innerHTML = '(' + cssInfo.matched.length + ')'; + } + this._populateRowTemplate(name, cssInfo.matchedEle, + cssInfo.matched, 'Matched'); + + if (cssInfo.unmatchedCountEle) { + cssInfo.unmatchedCountEle.innerHTML = '(' + cssInfo.unmatched.length + ')'; + } + this._populateRowTemplate(name, cssInfo.unmatchedEle, + cssInfo.unmatched, 'Unmatched'); + } + + if (source.collapsed !== 'open') { + this._setDisplay('hiddenWith' + name, 'table-row'); + source.collapsed = 'open'; + source.innerHTML = '&#9660;'; + } else { + this._setDisplay('hiddenWith' + name, 'none'); + source.collapsed = 'collapsed'; + source.innerHTML = '&#9654;'; + } + }, + + _populateRowTemplate: function CssHtmlTree_populateRowTemplate(name, tr, container, match) + { + let data = { container: container, match: match, name: name }; + let newDiv = this._template(this.rowTemplate, data, true); + + jlog('data', data); + jlog('newDiv', newDiv); + + while (newDiv.firstChild) { + tr.parentNode.insertBefore(newDiv.firstChild, tr.nextSibling); + } + }, + + /** + * Process the main template + */ + _populateTableTemplate: function CssHtmlTree_populateTableTemplate() + { + let styles = []; + + for (let group in CssLogic.styleGroups) { + let properties = CssLogic.styleGroups[group]; + for (let i = 0; i < properties.length; i++) { + let cssInfo = this.cssLogic.getCssInfo(properties[i], false); + styles.push(cssInfo); + } + } + + // Sort the styles by name - remove this when we split things into groups + styles.sort(function(cssInfo1, cssInfo2) { + return cssInfo1.property.localeCompare(cssInfo2.property); + }); + + let newTable = this._template(this.tableTemplate, { styles: styles }, true); + + this.tableDisplay.innerHTML = ''; + while (newTable.firstChild) { + while (newTable.firstChild.firstChild) { + this.tableDisplay.appendChild(newTable.firstChild.firstChild); + } + newTable.removeChild(newTable.firstChild); + } + }, + + /** + * Hide all the rows + */ + _hideHidden: function CssHtmlTree_hideHidden() + { + let self = this; + for (let group in CssLogic.styleGroups) { + CssLogic.styleGroups[group].forEach(function(property) { + self._setDisplay('hiddenWith' + property, 'none'); + self._setDisplay('hiddenWith' + property + 'Matched', 'none'); + self._setDisplay('hiddenWith' + property + 'Unmatched', 'none'); + }); + } + }, + + /** + * Set the display of a given <tt>className</tt> to the given + * <tt>newDisplay</tt> using stylesheet manipulation. + */ + _setDisplay: function CssHtmlTree_setDisplay(className, newDisplay) + { + return; + + // Loop over the rules until we find a match, then remove it + for (let r = 0; r < this.customSheet.cssRules.length; r++) { + let selector = this.customSheet.cssRules[r].selectorText; + if (selector === '.' + className || selector === 'tr.' + className) { + this.customSheet.deleteRule(r); + } + } + + let prefix = newDisplay === 'none' ? 'tr' : ''; + let ruleText = prefix + '.' + className + ' { display: ' + newDisplay + '; }'; + this.customSheet.insertRule(ruleText, 0); + }, + + /** + * Clone the given template node, and process it by resolving ${} references + * in the template + */ + _template: function CssHtmlTree_template(template, data, returnDummyParent) { + data = data || {}; + let duplicated = template.cloneNode(true); + + // Looping might ask for a parentNode, so we give it a dummy + let parent = template.ownerDocument.createElement('div'); + parent.appendChild(duplicated); + + let templater = new Templater(); + templater.processNode(duplicated, data); + return returnDummyParent ? parent : duplicated; + }, +}; + +/** + * 'static' function used by the style2.html template to turn breaking dashes + * into non-breaking ones (i.e. unicode decimal 8722). + * This function might be filled out to swap breaking spaces into non-breaking + * ones too, however for now it's only for style names. + */ +CssHtmlTree.preize = function(name) +{ + return name.replace(/-/g, '\u2212'); +}; + +//////////////////////////////////////////////////////////////////////////////// + +function Templater() { +}; + +/** + * Recursive function to walk the tree processing the attributes as it goes. + */ +Templater.prototype.processNode = function(node, data) { + var self = this; + var recurse = true; + // Process attributes + if (node.attributes && node.attributes.length) { + // It's good to clean up the attributes when we've processed them, + // but if we do it straight away, we mess up the array index + var attrs = Array.prototype.slice.call(node.attributes); + for (var i = 0; i < attrs.length; i++) { + var value = attrs[i].value; + var name = attrs[i].name; + + if (name === 'save') { + // Save attributes are a setter using the node + value = self.stripBraces(value); + self.property(value, data, node); + node.removeAttribute('save'); + } else if (name === 'if') { + value = self.stripBraces(value); + try { + var reply = self.envEval(value, data, attrs[i].value); + recurse = !!reply; + } catch (ex) { + this.error('Error with \'', value, '\'', ex); + recurse = false; + } + if (!recurse) { + node.parentNode.removeChild(node); + } + node.removeAttribute('if'); + } else if (name === 'foreach') { + var paramName = 'param'; + if (value.charAt(0) === '$') { + // No custom loop variable name. Use the default: 'param' + value = self.stripBraces(value); + } else { + // Extract the loop variable name from 'NAME in ${ARRAY}' + var nameArr = value.split(' in '); + paramName = nameArr[0].trim(); + value = self.stripBraces(nameArr[1].trim()); + } + recurse = false; + try { + var array = self.envEval(value, data, attrs[i].value); + array.forEach(function(param) { + var clone = node.cloneNode(true); + clone.removeAttribute('foreach'); + node.parentNode.insertBefore(clone, node); + data[paramName] = param; + self.processNode(clone, data); + delete data[paramName]; + }); + node.parentNode.removeChild(node); + } catch (ex) { + this.error('Error with \'', value, '\'', ex); + recurse = false; + } + node.removeAttribute('foreach'); + } else if (name.substring(0, 2) === 'on') { + // Event registration relies on property doing a bind + value = self.stripBraces(value); + var func = self.property(value, data); + if (typeof func !== 'function') { + this.error('Expected ' + value + + ' to resolve to a function, but got ', typeof func); + } + node.removeAttribute(name); + var capture = node.hasAttribute('capture' + name.substring(2)); + node.addEventListener(name.substring(2), func, capture); + } else { + // Replace references in other attributes + var newValue = value.replace(/\$\{[^}]*\}/g, function(path) { + return self.envEval(path.slice(2, -1), data, value); + }); + // Remove '_' prefix of attribute names so the DOM won't try + // to use them before we've processed the template + if (name.indexOf('_') === 0) { + node.removeAttribute(name); + node.setAttribute(name.substring(1), newValue); + } else if (value !== newValue) { + attrs[i].value = newValue; + } + } + } + } + + // Process child nodes + if (recurse) { + self.processChildren(node, data); + } + + // Process TextNodes + if (node.nodeType === Node.TEXT_NODE) { + // Replace references in other attributes + value = node.data; // TODO: is this more correct than textContent? + // We can't use the string.replace() with function trick because we need + // to support functions that return DOM nodes, so we can't have the + // conversion to a string. + // Instead we process the string as an array of parts. In order to split + // the string up, we first replace ${ with \uF001$ and } with \uF002 + // We can then split using \uF001 or \uF002 to get an array of strings + // where scripts are prefixed with $. + // \uF001 and \uF002 are just unicode chars reserved for private use. + value = value.replace(/\$\{([^}]*)\}/g, '\uF001$$$1\uF002'); + var parts = value.split(/\uF001|\uF002/); + if (parts.length > 1) { + parts.forEach(function(part) { + if (part === null || part === undefined || part === '') { + return; + } + if (part.charAt(0) === '$') { + part = self.envEval(part.slice(1), data, node.data); + } + if (part === null) { + part = "null"; + } + if (part === undefined) { + part = "undefined"; + } + // Hmmm isDOMElement ... + if (typeof part.cloneNode !== 'function') { + part = node.ownerDocument.createTextNode(part.toString()); + } + node.parentNode.insertBefore(part, node); + }); + node.parentNode.removeChild(node); + } + } +}; + +/** + * Loop through the child nodes of the given node, calling processNode on them + * all. Note this first clones the set of nodes, so the set of nodes that we + * visit will be unaffected by additions or removals. + * @param node The node from which to find children to visit. + * @param data The data to pass to processNode + */ +Templater.prototype.processChildren = function(node, data) { + var children = Array.prototype.slice.call(node.childNodes); + for (var i = 0; i < children.length; i++) { + this.processNode(children[i], data); + } +}; + +/** + * Warn of string does not begin '${' and end '}' + * @return The string stripped of ${ and }, or untouched if it does not match + */ +Templater.prototype.stripBraces = function(str) { + if (!str.match(/\$\{.*\}/g)) { + this.error('Expected ' + str + ' to match ${...}'); + return str; + } + return str.slice(2, -1); +}; + +/** + * Combined getter and setter that works with a path through some data set. + * For example:<ul> + * <li>property('a.b', { a: { b: 99 }}); // returns 99 + * <li>property('a', { a: { b: 99 }}); // returns { b: 99 } + * <li>property('a', { a: { b: 99 }}, 42); // returns 99 and alters the + * input data to be { a: { b: 42 }} + * </ul> + * @param path An array of strings indicating the path through the data, or + * a string to be cut into an array using <tt>split('.')</tt> + * @param data An object to look in for the <tt>path</tt> + * @param newValue (optional) If undefined, this value will replace the + * original value for the data at the path specified. + * @returns The value pointed to by <tt>path</tt> before any + * <tt>newValue</tt> is applied. + */ +Templater.prototype.property = function(path, data, newValue) { + if (typeof path === 'string') { + path = path.split('.'); + } + var value = data[path[0]]; + if (path.length === 1) { + if (newValue !== undefined) { + data[path[0]] = newValue; + } + if (typeof value === 'function') { + return value.bind(data); + } + return value; + } + if (!value) { + this.error('Can\'t find path=', path, " in data=", data); + return null; + } + return this.property(path.slice(1), value, newValue); +}; + +/** + * Like eval, but that creates a context of the variables in <tt>env</tt> in + * which the script is evaluated. + * WARNING: This script uses 'with' which is generally regarded to be evil. + * The alternative is to create a Function at runtime that takes X parameters + * according to the X keys in the env object, and then call that function using + * the values in the env object. This is likely to be slow, but workable. + * @param script The string to be evaluated + * @param env The environment in which to eval the script. + * @param context Optional debugging string in case of failure + * @returns The return value of the script + */ +Templater.prototype.envEval = function(script, env, context) { + with (env) { + try { + return eval(script); + } + catch (ex) { + var message = 'Error evaluating \'' + script + '\''; + if (context) { + message += ' within \'' + context + '\''; + } + this.error(message + ': ' + ex); + return message; + } + } +}; + +/** + * A generic way of reporting errors, for easy overloading in different + * environments. + */ +Templater.prototype.error = function() { + jlog(arguments); +}; + +//////////////////////////////////////////////////////////////////////////////// + + +var lastTime = new Date().getTime(); +var lastName = null; +function jprofile(name) { + var now = new Date().getTime(); + var elapsed = now - lastTime; + lastTime = now; + if (lastName) { + if (name) { + Services.console.logStringMessage('Last job: ' + lastName + ' took ' + (elapsed / 1000) + ' sec. Starting ' + name); + } else { + Services.console.logStringMessage('Last job: ' + lastName + ' took ' + (elapsed / 1000) + ' sec.'); + } + } else { + Services.console.logStringMessage('Starting ' + name); + } + lastName = name; +} + +function jlog() { + var output = ''; + for (var i = 0; i < arguments.length; i++) { + if (i !== 0) { + output += '\n'; + } + output += toDescriptiveString(arguments[i], 4); + } + Services.console.logStringMessage(output); +} + +/** + * This function pretty-prints simple data or whole object graphs, for example + * as an aid in debugging. + * @see http://getahead.org/dwr/browser/util/todescriptivestring + */ +var toDescriptiveString = function(data, showLevels, options) { + if (showLevels === undefined) { + showLevels = 1; + } + var opt = {}; + if (options && typeof options == "object") { + opt = options; + } + var defaultoptions = { + escapeHtml:false, + baseIndent: "", + childIndent: "\u00A0\u00A0", + lineTerminator: "\n", + oneLineMaxItems: 20, + shortStringMaxLength: 13, + longStringMaxLength: 53, + propertyNameMaxLength: 30 + }; + for (var p in defaultoptions) { + if (!(p in opt)) { + opt[p] = defaultoptions[p]; + } + } + + var skipDomProperties = { + document:true, ownerDocument:true, + all:true, + parentElement:true, parentNode:true, offsetParent:true, + children:true, firstChild:true, lastChild:true, + previousSibling:true, nextSibling:true, + innerHTML:true, outerHTML:true, + innerText:true, outerText:true, textContent:true, + attributes:true, + style:true, currentStyle:true, runtimeStyle:true, + parentTextEdit:true + }; + + var shown = []; + function isShown(obj) { + if (typeof obj !== 'object') { + return false; + } + for (var i = 0; i < shown.length; i++) { + if (shown[i] === obj) { + return true; + } + } + shown.push(obj); + return false; + } + + function recursive(data, showLevels, indentDepth, options) { + var reply = ""; + try { + // string + if (typeof data == "string") { + var str = data; + if (showLevels == 0 && str.length > options.shortStringMaxLength) { + str = str.substring(0, options.shortStringMaxLength - 3) + "..."; + } + else if (str.length > options.longStringMaxLength) { + str = str.substring(0, options.longStringMaxLength - 3) + "..."; + } + if (options.escapeHtml) { + // Do the escape separately for every line as escapeHtml() on some + // browsers (IE) will strip line breaks and we want to preserve them + var lines = str.split("\n"); + for (var i = 0; i < lines.length; i++) { + lines[i] = lines[i]. + replace(/&/g,'&amp;'). + replace(/</g,'&lt;'). + replace(/>/g,'&gt;'); + } + str = lines.join("\n"); + } + if (showLevels == 0) { // Short format + str = str.replace(/\n|\r|\t/g, function(ch) { + switch (ch) { + case "\n": return "\\n"; + case "\r": return ""; + case "\t": return "\\t"; + default: throw new Error('logic error'); + } + }); + } + else { // Long format + str = str.replace(/\n|\r|\t/g, function(ch) { + switch (ch) { + case "\n": + return options.lineTerminator + indent(indentDepth + 1, options); + case "\r": + return ""; + case "\t": + return "\\t"; + } + }); + } + reply = '"' + str + '"'; + } + + // function + else if (typeof data == "function") { + reply = "function"; + } + + // Array + else if (data && data.join) { + if (showLevels == 0) { + // Short format (don't show items) + reply = (data.length > 0) ? "[...]" : "[]"; + } + else { + // Long format (show items) + var strarr = []; + strarr.push("["); + var count = 0; + for (var i = 0; i < data.length; i++) { + if (! (i in data)) { + continue; + } + var itemvalue = data[i]; + if (count > 0) { + strarr.push(", "); + } + if (isShown(itemvalue)) { + strarr.push(",^^^"); + continue; + } + if (showLevels === 1) { // One-line format + if (count == options.oneLineMaxItems) { + strarr.push("..."); + break; + } + } + else if (data.length === 1) { + strarr.push(" "); + } + else { // Multi-line format + strarr.push(options.lineTerminator + indent(indentDepth+1, options)); + } + if (i != count) { + strarr.push(i); + strarr.push(":"); + } + strarr.push(recursive(itemvalue, showLevels-1, indentDepth+1, options)); + count++; + } + if (data.length === 1) { + strarr.push(" "); + } + else if (showLevels > 1) { + strarr.push(options.lineTerminator + indent(indentDepth, options)); + } + strarr.push("]"); + reply = strarr.join(""); + } + } + + // Objects except Date + else if ((data && typeof data == "object") && !((data && data.toUTCString) ? true : false)) { + if (showLevels == 0) { // Short format (don't show properties) + reply = _detailedTypeOf(data); + } + else { // Long format (show properties) + var strarr = []; + if (_detailedTypeOf(data) != "Object") { + strarr.push(_detailedTypeOf(data)); + if (data.valueOf && typeof data.valueOf() != "object") { + strarr.push(":"); + strarr.push(recursive(data.valueOf(), 1, indentDepth, options)); + } + } + strarr.push("{"); + var isDomObject = (data !== null && typeof data === "object" && data.nodeName !== null); + var count = 0; + var props = []; + for (var prop in data) { + props.push(prop); + } + props.sort(); + var skipped = false; + for (var i = 0; i < props.length; i++) { + var prop = props[i]; + try { + var propvalue = data[prop]; + if (isDomObject) { + if (typeof propvalue == "function") { + skipped = true; + continue; + } + if (skipDomProperties[prop]) { + skipped = true; + continue; + } + if (prop.toUpperCase() == prop) { + skipped = true; + continue; + } + } + + if (count === 0) { + strarr.push(" "); + } + if (count > 0) { + strarr.push(", "); + } + + if (showLevels === 1 || props.length === 1) { + // One-line format + if (count == options.oneLineMaxItems) { + strarr.push("..."); + break; + } + } + else { + // Multi-line format + strarr.push(options.lineTerminator + indent(indentDepth+1, options)); + } + + strarr.push(prop.length > options.propertyNameMaxLength ? prop.substring(0, options.propertyNameMaxLength-3) + "..." : prop); + strarr.push(":"); + if (isShown(propvalue)) { + strarr.push(",^^^"); + continue; + } + strarr.push(recursive(propvalue, showLevels-1, indentDepth+1, options)); + count++; + } + catch (ex) { + if (count > 0) { + strarr.push(", "); + } + strarr.push(prop + ":err"); + } + } + if (skipped) { + strarr.push(",..."); + } + if (props.length === 1) { + strarr.push(" "); + } + else if (count > 0) { + if (showLevels > 1) { + strarr.push(options.lineTerminator + indent(indentDepth, options)); + } + else { + strarr.push(" "); + } + } + strarr.push("}"); + reply = strarr.join(""); + } + } + + // undefined, null, number, boolean, Date + else { + reply = "" + data; + } + + return reply; + } + catch(err) { + return (err.message ? err.message : "" + err); + } + } + + function indent(count, options) { + var strarr = []; + strarr.push(options.baseIndent); + for (var i=0; i<count; i++) { + strarr.push(options.childIndent); + } + return strarr.join(""); + }; + + return recursive(data, showLevels, 0, opt); +}; + +var _detailedTypeOf = function(x) { + var reply = typeof x; + if (reply == "object") { + reply = Object.prototype.toString.apply(x); // Returns "[object class]" + reply = reply.substring(8, reply.length-1); // Just get the class bit + } + return reply; +}; diff -r c5f35032f22d browser/base/jar.mn --- a/browser/base/jar.mn Fri Jul 30 11:07:42 2010 +0200 +++ b/browser/base/jar.mn Fri Jul 30 16:07:43 2010 +0100 @@ -27,6 +27,7 @@ * content/browser/browser-tabPreviews.xml (content/browser-tabPreviews.xml) * content/browser/credits.xhtml (content/credits.xhtml) * content/browser/fullscreen-video.xhtml (content/fullscreen-video.xhtml) +* content/browser/style2.html (content/style2.html) * content/browser/pageinfo/pageInfo.xul (content/pageinfo/pageInfo.xul) * content/browser/pageinfo/pageInfo.js (content/pageinfo/pageInfo.js) * content/browser/pageinfo/pageInfo.css (content/pageinfo/pageInfo.css) diff -r c5f35032f22d browser/locales/en-US/chrome/browser/browser.dtd --- a/browser/locales/en-US/chrome/browser/browser.dtd Fri Jul 30 11:07:42 2010 +0200 +++ b/browser/locales/en-US/chrome/browser/browser.dtd Fri Jul 30 16:07:43 2010 +0100 @@ -184,6 +184,10 @@ <!ENTITY inspectStyleButton.label "Style"> <!ENTITY inspectStyleButton.accesskey "S"> <!ENTITY inspectStylePanelTitle.label "Style"> +<!ENTITY inspectStyle2Button.label "Style2"> +<!ENTITY inspectStyle2Button.accesskey "S"> +<!ENTITY inspectStyle2Button.commandkey ";"> +<!ENTITY inspectStyle2PanelTitle.label "Style2"> <!ENTITY fileMenu.label "File"> <!ENTITY fileMenu.accesskey "F">

The sorting is not currently correct. Needs fixing

while this looks cool, I'm a little concerned about the time to reimplement. We have a XUL tree implementation which is coming along and nearly functional, and would allow us to easily edit individual properties. Do you think you can have the HTML rewrite complete in a week?

I'm requesting blocking status for this bug. I view this as a key feature that is really useful in providing good information to users and will differentiate the new Firefox Inspector from the similar features out there.

I looked through the code and here's my feedback and suggestions: - Every listed property shows the number of rules on the right side. When I highlight an element I think it would be nicer to update that number and show the number of matched rules. So, when I go for digging deeper into the style panel view, I know into which properties to dig sooner rather than later. The current view makes all properties equal at a glance, irrespective of the highlighted element. I'd like to be able to glance over the properties like now and see "5/10 rules" (5 matched out of 10 rules), or "5 matched rules". The view does not need to change otherwise. Just this tiny detail of the "N rules". I agree that the developer needs to see why a property did *not* match. Show "0 matched rules", and when he/she expands the property, the list of unmatched rules will show. No problem. Just the title change is what I am suggesting. ;) (My suggestion comes as a user of such tools! I'd really like this tiny change.) - Some web site layouts have 100+ rules for a single common property like color. Don't show all of the unmatched rules. Show only the first 10-20 rules, and add the option to "show all unmatched rules". - I'd like to see the computed style value for each property as well, somehow in the GUI. Perhaps add it to the table, as "Computed style → value". - There should be an option to show shorthand properties for margin-*, padding-*, and for other properties, when possible. It makes glancing over the styles much quicker. - In inspector.js, the new InspectorUI.htmlTree property name is confusing. To a newcomer this might seem to refer to an HTML representation of the web page DOM tree. I would suggest renaming the property to CssHtmlTree. - In csslogic.js the this.viewedDocument object is unwrapped, and you access properties that are unsafe. Isn't this a security issue? Example in CssLogin.matchesQuery(): let doc = XPCNativeWrapper.unwrap(element.ownerDocument); let matches = doc.querySelectorAll(query); I believe that's not safe. A malicious developer can overwrite that document method. - Nit-picking: style3.html should be renamed. It has that feeling of "unnamed01.html". :) - Similarly, the <title>Style3</title> should be changed to something more sensible. - The <style> tags should have type=text/css for correctness. - Links to javascript:null break the style panel when clicked. The style panel shows a blank page with a "null" string inside. Should be changed to something else, I presume? - In browser.xul, #inspector-style-browser ... I don't think you need the onclick, just handle the clicks for the elements you want inside the HTML page. Also, the CssHtmlTree.onClick event handler is empty. - The CssLogic.highlight() and CssHtmlTree.highlight() methods both check for element == this.viewedElement. While that is fine, to avoid checking twice for the same element, you should add an option to force the refresh of the CssLogic and CssHtmlTree object states. This is needed by DOM mutations event handler which may call these methods to refresh the panel (and other panels). - There's an error in the CssHtmlTree.highlight() method: // ... this.viewedElement = element; this.viewedDocument = XPCNativeWrapper.unwrap(element.ownerDocument); if (this.viewedElement) { this._populateMainTemplates(); } // ... You assume that element exists when you set viewedDocument, but then you're not sure - you call _populateMainTemplates() only if the viewedElement evaluates to true. I think you should check that element does not evaluate to false at the beginning of the method and return void before any action is taken. - In style3.html and style3.js, the #templateProperties template: <div class="temp-loop" foreach="property in ${group.properties}"> let data = { group: this }; this.tree._template(this.tree.templateProperties, this.element, data); Why not use data = { properties: this.properties} directly? It doesn't use other stuff from the StyleGroupView. - In style3.js the PropertyView object has a getter with an argument: get value(property) { return this.tree.cssLogic.getCssInfo(property).value; } I think you mean: get value() { return this.tree.cssLogic.getCssInfo(this.name).value; } - If I am not mistaken, the above getter is never used. The #templateRules takes the property values from each rule of CssInfo. The computedStyle value that is returned by the above getter is never displayed. - The click to expand properties fails to work after some time - when I change the selected element, while not inspecting. In the end, the style panel becomes unusable, because I cannot see any rules for any property. Is this a known bug? - The default style panel width should be larger: style groups have their titles wrapped. - The code needs internationalization work, for example see the style3.html hard-coded strings and style3.js. Overall, I really like the code. It's really clean and easy to understand. Great work!

Great feedback, thanks. I've fixed many of them already, I've got some bugs to raise, and a few to think about. - "5/10 rules": Is a very nice idea, but it's tricky. To know that there are 5 rules that apply we only need to dig through the stylesheets, we don't need to investigate the element itself, which can take time. I need to spend some more time checking the performance implications of this, but I agree that we should find a way to do it. - 100+ rules for a single property: Agreed, and I think it's important. - see the computed style value for each property as well: It's only missing due to the property.value bug. - shorthand properties for margin: Agreed, we've got work in general to do here. Investigation needed - s/InspectorUI.htmlTree/InspectorUI.cssHtmlTree/g -> done - Check use of unwrapping -> On my list - <title>Style3</title> is now <title>Style</title> - <style> tags should have type=text/css -> done - I saw the Links to javascript:null as part of 585566, but I probably should have made it clearer. I have now (see bug comment). - if (this.viewedElement) check in CssHtmlTree.highlight -> fixed - Using group.properties in template rather than group -> I'm 51/49 on this, arguing both ways, but I've come down on your side, and made the change. - PropertyView.value broken -> good spot, fixed, and it was used - see the computedStyle point above - click to expand properties fails -> Added to my list to investigate - style panel width should be larger -> Yeah - we need to get this sorted. On my list - i18n -> Funny I thought there was a separate bug for this, but I was confused with 585567. Will raise one. - CssHtmlTree.onClick -> removed - style3.html should be renamed -> Will do - Double check for element == this.viewedElement -> Added to my list to look at. Thanks.

(In reply to comment #24) > > - 100+ rules for a single property: Agreed, and I think it's important. I use a max-height to limit the height if there is a lot of output. See this screenshot: https://bug573103.bugzilla.mozilla.org/attachment.cgi?id=464927. This allows the developer to see all of the properties if he wants to but it doesn't blow up the entire panel because there is a lot of output.

Includes msucans fixes for 585572

Fixes for many of the comments raised by msucan

the beta 4 train is leaving without this one, but we'll want to be sure to get it reviewed and landed early in beta 5.

I'd prefer browser/base/content/csshtmltree.html to be xhtml and to be localized via a DTD as much as possible. Also, some parts of the CSS in there make me wonder about RTL, Ehsan, can you have a look?

HTML tree patch rebased to trunk.

(In reply to comment #33) > Also, some parts of the CSS in there make me wonder about RTL, Ehsan, can you > have a look? This is bad. Things like text-align: left should be changed to text-align: start, margin-left to -moz-margin-start, padding-right to -moz-padding-end (I might have missed some stuff), and float properties need to be overriden in RTL mode (possibly by a chromedir like trick).

Ehsan and Axel: thank you for your suggestions for improving the internationalization of the Style panel. I'd like to make the changes you have requested, but I still have some questions: 1. Do I reuse an existing DTD or do I create a new one? If I have to create a new one, should I put it in the same location as browser.dtd? If not, to which DTD should I add the new strings? 2. As you can see in csshtmltree.html, the HTML templates have almost no actual strings, they mostly have placeholders like ${variable}. Those are generated dynamically by the csshtmltree.js script. Some of those variables are strings that need localization. Do I use inspector.properties for those, or do I ensure from the JS that I pass entity names that are added into the HTML? Thank you!

Adding the updated patches, ready for review. Details regarding internationalization: - I have added more strings that I initially left out by mistake (in CssLogic). - Axel: As agreed with Robert Campbell we are sticking to string bundles (inspector.properties) due to the situation we have. The style panel is not a typical XUL/XHTML panel that has strings. The entire UI is built from JavaScript where we can best use string bundles. Once again, thank you for your feedback for l10n matters! If there are any other changes we have to make, please let us know! Patch changes: - Removed debug code. - Renamed csshtmltree.html to csshtmltree.xhtml as requested by Axel Hecht. - Comments adjusted to include type information for arguments and returns. - Renamed arguments to use the aArgumentName coding style. - Gave names to anonymous functions/methods, as per coding style guide lines. - Switched from single quotes to double quotes, as per coding style guide lines. - Cleaned-up TODOs and added bug numbers for remaining TODOs. - Fix indentation differences in CssHtmlTree.js. - Fixed a bug in the Templater: it did not remove the captureEventName attribute after using it, from the node that is processed. See Templater.processNode(). - Made the changes Ehsan recommended for better RTL support. This needs testing. - Fixed an infinite loop in CssLogic.getShortName(). - Renamed CssSheet.source to CssSheet.shortSource, as was requested in a TODO. - Made the changes suggested in bug 586980 - added missing properties. - Integrated the patch from bug 587147 which fixes group view toggling. - Fixed bug 585575 - the style panel takes into account properties attached to the element itself (element.style). - Fixed a bug which caused PropertyView.ruleTitle to execute after the Inspector style panel was closed, which in turn ended up in throwing an exception. - Fixed a bug which causes the Inspector code to throw when there is no selected node. - Made a fix for browser_inspector_initilization.js test file which failed sometimes. - Changed browser_inspector_stylePanel.js to work with the new Style panel. (this list includes the changes I did in the CssHtmlTree patch that I'll attach in a few moments)

The CssHtmlTree patch. This needs the CssLogic patch.

This is a screenshot of the latest Style panel patches.

Rebased patch. There have been some important merges into the default m-c branch. So, these patches need rebase. Other changes: - included a fix for bug 588726 - dropped CssLogic.matchesQuery() in favor to element.mozMatchesSelector() - thanks to dbaron for the tip!

Rebased patch.

rebased patch based on the tree panel and editor from bug 572038 and bug 575234.

test results from mochitest browser chrome with above patches installed: Browser Chrome Test Summary Passed: 6874 Failed: 19 Todo: 5 *** End BrowserChrome Test Results *** TEST-START | Shutdown INFO | automation.py | Application ran for: 0:07:25.072400 INFO | automation.py | Reading PID log: /var/folders/Bv/BvxnJzazGzeR6Yy4QRUEFU+++TI/-Tmp-/tmpIN7vmOpidlog WARNING | automationutils.processLeakLog() | refcount logging is off, so leaks can't be detected! INFO | runtests.py | Running tests: end. mochitest-browser-chrome failed: TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_inspector_cssinfo_order.js | Inspector Tree Panel is open TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_inspector_cssinfo_order.js | Timed out TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_inspector_cssinfo_order.js | Found a tab after previous test timed out: data:text/html,<!DOCTYPE%20html><html><head><style>%20.test,%20#test%20{%20color:%20green%20}%20div%20{%20color:%20blue%20}%20</style></head><body><div%20class='test'>cssInfo.rules%20order%20test%20for%20Inspector</div></body></html> TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_inspector_domPanel.js | Exception thrown at chrome://mochikit/content/browser/browser/base/content/test/browser_inspector_domPanel.js:89 - SyntaxError: missing : after property id TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_inspector_iframeTest.js | Timed out TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_inspector_iframeTest.js | Found a tab after previous test timed out: data:text/html,iframe%20tests%20for%20inspector TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_inspector_scrolling.js | Timed out TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_inspector_scrolling.js | Found a tab after previous test timed out: data:text/html,mouse%20scrolling%20test%20for%20inspector TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_inspector_stylepanel_groupview.js | Inspector Tree Panel is open TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_inspector_stylepanel_groupview.js | Timed out TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_inspector_stylepanel_groupview.js | Found a tab after previous test timed out: data:text/html,<!DOCTYPE%20html><html><head><style>%20.test,%20#test%20{%20color:%20green%20}%20div%20{%20color:%20blue%20}%20</style></head><body><div%20class='test'>cssInfo.rules%20order%20test%20for%20Inspector</div></body></html> TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | the network nodes are hidden when the corresponding filter is switched off - Got 5, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | the network nodes are hidden when the search string is set to "hxxp" - Got 5, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | the network nodes are hidden when the search string is set to " zzzz zzzz " - Got 5, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | the network nodes are hidden when searching for weasels - Got 5, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | the network nodes are hidden when searching for the bell character - Got 5, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | the network nodes are hidden when searching for the string "foo" - Got 5, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | the network nodes are hidden when searching for the string 'foo' - Got 5, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | the network nodes are hidden when searching for the string "foo"bar'baz"boo'" - Got 5, expected 0

Updating the patches, once again. It has been decided that: - bug 587125 gets merged. - and this bug will depend on bug 572038 from now. Sorry for this high-traffic of patch updates.

Changes: - rebased patch on top of bug 572038. - merged bug 587125. - more test code fixes - those that relate to the stylePanel. Other WebConsole and Inspector tests still fail, but they are unrelated.

This would be an _awesome_ improvement. However, not going to hold the release for it. If you get a well-tested patch ready in time, ask for approval.

I agree that this patch is awesome, but I think it goes beyond that. A couple weeks ago, I put this together to solidify my thinking about the value of the Inspector: http://hg.mozilla.org/users/kdangoor_mozilla.com/dtplanning/raw-file/tip/fx4/Inspector.html The top item is this style panel. This is the feature that differentiates this Inspector from those of WebKit and Firebug. It's the first view of styles that I've seen that directly exposes to the user how the browser figures out what an element should look like rather than just dumping out a whole bunch of information and then letting the user make sense of it all. I really want to see this land. Dolske had planned to get to the review late last week (but hasn't yet done so). I'm worried that the blocker "-" will mean that this won't make it, thus removing a ton of value from the Inspector. Obviously, there are a lot of strings in this patch so I fear not landing this in b5 means not landing it at all. It's also worth noting that Joe, Mihai, Rob and Patrick have all to one degree or another been doing work around this patch. So it has had an uncommon amount of attention in our group. I'm re-requesting blocking status for this.

Thanks for the context. If this change is what ultimately differentiates this feature, than it should block the release (or the shipping of the Inspector) - blocking+.

Making this betaN+ sounds far fetched, it has l10n impact, and it's arguably a feature, too. I'd argue that this needs to make feature and l10n freeze if it's supposed to be in, which is beta5+, right?

Yes, Axel's correct, and Kevin is aware of that as the deadline for this change (per comment 47) -> blocking b5+

Comment on attachment 467471 [details] [diff] [review] Split 1c: CssLogic A few immediate comments, haven't looked at the guts in detail yet: >+++ b/browser/base/content/browser.js ... >+#include csslogic.js Why browser.js? I think this is only intended for use in the Inspector, so it ought to be stuck in there. I would say just plop csslogic.js directly into inspector.js. Or make it a .jsm and Cu.import() it. >+CssLogic.STATUS = { >+ BEST: 3, >+ MATCHED: 2, >+ PARENT_MATCH: 1, >+ UNMATCHED: 0, >+ UNKNOWN: -1, >+}; Set these directly in the prototype? >+CssLogic.STATUS_NAMES = [ >+ //"Unmatched", "Parent Match", "Matched", "Best Match" >+]; Ditto. >+CssLogic.cacheStatusNames = function CssLogic_cacheStatusNames(aStrings) >+{ Not sure why this 1 function isn't a method on the prototype? >+CssLogic.prototype = { ... >+ if (domSheet.disabled) { >+ continue; >+ } General nit: 1-line blocks should omit braces (unless the other block of an if/else pair is multiline, in which both should be braced). This will be a huge win for CssSpecificity.compareTo(). :)

Thanks for the comments. We totally agree about csslogic being best as a .jsm. We're want to do some performance work on it, and we're planning to make it a .jsm at that point. I put things like .STATUS etc on the constructor rather than on the prototype so you could access them easily without creating an instance (or using .prototype.) Point taken about brackets - will fix. I will also have a patch update that merges some changes from elsewhere to make things easier for you.

(In reply to comment #51) > Comment on attachment 467471 [details] [diff] [review] > Split 1c: CssLogic > > A few immediate comments, haven't looked at the guts in detail yet: > > >+++ b/browser/base/content/browser.js > ... > >+#include csslogic.js > > Why browser.js? I think this is only intended for use in the Inspector, so it > ought to be stuck in there. I would say just plop csslogic.js directly into > inspector.js. Or make it a .jsm and Cu.import() it. Agreed. This has been discussed and we'll work on this soon. Not sure what priority is assigned to this change, but we have it mind, hehe. > >+CssLogic.STATUS = { > >+ BEST: 3, > >+ MATCHED: 2, > >+ PARENT_MATCH: 1, > >+ UNMATCHED: 0, > >+ UNKNOWN: -1, > >+}; > > Set these directly in the prototype? > > >+CssLogic.STATUS_NAMES = [ > >+ //"Unmatched", "Parent Match", "Matched", "Best Match" > >+]; > > Ditto. Thought of this, but these are static properties, unrelated to any object instance. > >+CssLogic.cacheStatusNames = function CssLogic_cacheStatusNames(aStrings) > >+{ > > Not sure why this 1 function isn't a method on the prototype? Same as above. > >+CssLogic.prototype = { > ... > >+ if (domSheet.disabled) { > >+ continue; > >+ } > > General nit: 1-line blocks should omit braces (unless the other block of an > if/else pair is multiline, in which both should be braced). > > This will be a huge win for CssSpecificity.compareTo(). :) True, but I followed the coding style from MDC, which very clearly states: "Always brace controlled statements, even a single-line consequent of an if else else. This is redundant typically, but it avoids dangling else bugs, so it's safer at scale than fine-tuning." See https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Control_Structures Most of all code I saw uses the braces for one-liners. Also, I'd add that I wanted to keep the same coding style as throughout the Inspector and the WebConsole. I'd favor keeping the braces, however I leave the final decision to you. Thank you very much for your review!

Merging in patches from other sources, and some clean-up.

Latest version incorporating patches from other sources

Previous upload had some changes to csslogic in csshtmltree. Fixed

Fixing bizarre cut-off from patch

Rebased patch on top of the latest tree panel patches.

More patch merging.

More patch merging

Apologies for the numerous patch updates. This set of patch updates comes with fixes for regressions: CssLogic failed to find the best match. This issue is now fixed in this patch.

Patch updates: - regression fix: property view failed to expand when there were more than 5 unmatched rules. - regression fix: the browser_inspector_stylePanel.js test file changes went missing in the last patch update, which meant that the test failed to execute with the new Style Panel code. Updated the patch to include the required changes, such that all tests pass. - the "Show rule score" input failed to remember its state when you highlighted a new node. - merged the "Text" and "Fonts and Color" property groups, as Limi suggested. - other minor changes. Thanks!

(In reply to comment #53) > > >+#include csslogic.js > > > > Why browser.js? I think this is only intended for use in the Inspector, so it > > ought to be stuck in there. I would say just plop csslogic.js directly into > > inspector.js. Or make it a .jsm and Cu.import() it. > > Agreed. This has been discussed and we'll work on this soon. Not sure what > priority is assigned to this change, but we have it mind, hehe. Good you've thought of it, but this does need to change for the current patch. If not making it a JSM now, it should be #included in inspector.js instead. > True, but I followed the coding style from MDC, which very clearly states: Sigh. That guide has never really reflected current practices for JS code, especially in /browser. I do think changing at least CssSpecificity.compareTo() would be worthwhile, though it's not a huge problem.

- rebased on top of the latest tree panel patches (bug 572038) and on top of the latest default mozilla-central branch (as of today). - merged changes requested by dolske - fixed some misleading comments - made a fix for media=all. On ubuntu.com the style panel did not show any rule, from any stylesheet. - made a fix for tab switching - the "show rule score" lost its state across tab switches.

Comment on attachment 469169 [details] [diff] [review] Split 1h: CssLogic I was about 3/4 of the way through reviewing 1h before you posted the updated 1i. So, it's possible some of these comments no longer apply. Update 1i with the below and we'll see where things are at then. >+/** >+ * Other bits of CSS display logic work simply by digging into the actually "actual". >+function CssLogic(aStylePanel, aStrings) >+{ It's weird to be passing in a string bundle here. Just make this a memoizing getter in the prototype. (Or if you end up making it a JSM now, it can be a XPCOMUtils.defineLazyGetter in the JSM's scope, to avoid it being per-instance). >+CssLogic.prototype = { ... >+ highlight: function CssLogic_highlight(aViewedElement) ... >+ this.viewedElement = aViewedElement; >+ let doc = this.viewedElement.ownerDocument; >+ if (doc != this.viewedDocument) { >+ // New document: clear/rebuild the cache. >+ this.viewedDocument = doc; >+ >+ this.sheets = []; Hmm, so, you're retaining references to elements and documents, but I don't see them explicitly released anywhere. Have to looked at the lifetimes of your objects, to ensure we're not keeping junk alive longer than it needs to be? >+ // Hunt down top level stylesheets, and cache them. >+ let domSheets = this.viewedDocument.styleSheets; What if a page dynamically adds stylesheets after you've created this static cache? >+ if (domSheet.disabled) { >+ continue; >+ } ...or if a page toggles .disabled? Or uses .insertRule() / .deleteRule()? >+ try { >+ let win = this.viewedDocument.defaultView; >+ this._computedStyle = win.getComputedStyle(this.viewedElement, ""); >+ } catch (ex) { >+ Services.console.logStringMessage("Warning getComputedStyle errored " + >+ "for " + this.viewedElement + ". " + ex); >+ } When does this actually throw? >+ _cacheSheet: function CssLogic_cacheSheet(aDomSheet) ... >+ // If it is an @import rule, it will have a styleSheet property. >+ if (domRule.styleSheet) { if (domRule.type == CSSRule.IMPORT_RULE) >+ this.sheets.push(new CssSheet(this.viewedDocument, aDomSheet)); Can't the CssSheet just derive the document by way of aDomSheet.ownerNode.ownerDocument? That would be a slightly simpler API that's harder to accidentally screw up / get into an undefined state. CSS2.1 says @imports must occur before everything else (except @charset), so it would be a nice perf boost to bail out of the loop here when the first non-@import, non-@charset is encountered. >+CssLogic.getShortName = function CssLogic_getShortName(aElement) >+{ ... >+ return aElement.tagName + "[" + priorSiblings + "]"; Seems like this might be a little confusing. Makes me think of XPath, but not quite. >+CssLogic.getShortNamePath = function CssLogic_getShortNamePath(aElement) >+{ >+ let doc = aElement.ownerDocument; >+ let reply = []; >+ >+ // Work up to document.body, but don't include it. >+ while (aElement && aElement != doc.body && aElement != doc.documentElement) { >+ reply.unshift({ >+ display: CssLogic.getShortName(aElement), >+ element: aElement >+ }); >+ aElement = aElement.parentNode; >+ } >+ >+ // If we don't have anything then include document.body or whatever element >+ // is. >+ if (aElement && !reply.length) { >+ reply.unshift({ >+ display: CssLogic.getShortName(aElement), >+ element: aElement >+ }); >+ } Rephrased, this is "always include aElement, and then include any interesting path above it if present". So this should be simpler as: if (!aElement) return []; // impossible? do { reply.unshift({ display: CssLogic.getShortName(aElement), element: aElement }); aElement = aElement.parentNode; } while(aElement && aElement != doc.body && aElement != doc.documentElement) >+CssSheet.prototype = { ... >+ get href() >+ { >+ if (!this._href) { >+ try { When can this throw? This and the other generic try/catch blocks should be avoided unless they're actually needed (or it's likely that the code inside is at risk of throwing). >+ this._href = this._domSheet.href; >+ if (!this._href) { >+ this._href = this._document.defaultView.location.href; this._href = this._document.location; >+ get shortSource() ... >+ // Get rid of any query string. There are times when this might be the >+ // wrong thing to do, but getting rid of it seems like the simplest option >+ // so we should do that until we have a good reason to do otherwise. >+ let queryPos = this._source.indexOf("?"); >+ if (queryPos != -1) { Consider http://site.com/?makeSomeCSS Maybe not worth worrying about for now? Followup bug? Might consider converting into a nsIURI [Services.io.newURI(this._source, null, null)], and accessing just the parts you want that way? >+ get domRules() >+ { >+ if (!this._domRules) { >+ // Cross domain sheets give NS_ERROR_DOM_SECURITY_ERR Really? You're running as chrome, no? Seems like that shouldn't happen. >+ /** >+ * Dig through all the sheets looking for matches for <tt>property</tt> and >+ * adding the results to <tt>rules</tt>. This method also finds the properties >+ * that are defined inline (element.style). >+ */ >+ _findRules: function CssInfo_findRules() I think the comment it wrong, _findRules doesn't seem to look at element.style at all (but findElementRules does). >+ _markMatches: function CssInfo_markMatches() >+ { It might be worthwhile to have someone like dbaron take a look at this function, since you're sorta reimplementing part of CSS here. Would be good for catching any quirky corner cases... >+ // domUtils.getRuleLine fails for @import rules. File a bug for this (and note it here), seems like it shouldn't! >+ this.line = -1; >+ if (aCssLogic.stylePanel && this.selector != "@element.style") { >+ try { >+ this.line = aCssLogic.stylePanel.domUtils.getRuleLine(aDomRule); If sheet's an inline <style>, is the line number here relative to the document source, or start of the <style> block? >+function CssSpecificity(aSelector, aIndex, aPriority) >+{ This looks like another good chunk of code to ask dbaron for feedback on. >+ aSelector.split(/[ >+]/).forEach(function(aSimple) { Shouldn't "~" should be here too? >+ // The regex leaves empty nodes combinators like ' > ' >+ if (!aSimple) { >+ return; >+ } >+ // See section 6.4.3 -> http://www.w3.org/TR/CSS2/cascade.html#specificity Should this instead reference (and implement) http://www.w3.org/TR/css3-selectors/#specificity >+ this.ids += (aSimple.match(/#/g) || []).length; /^#/ to force matching at the beginning? (consider ".weird#class"?) I'm not sure if I should go through the CSS grammar to looks for edge cases here and the rest of the function. Have you done so? >+CssSpecificity.prototype = { ... >+ compareTo: function CssSpecificity_compareTo(aThat) >+ { Thrice for having a CSS expert look at this function.

It's late (or should that be early) so I'm going to do a quick fly-through of your points. Anything I don't mention, I agree with and we'll work on it. You are correct about there being a general issue with caching. In the short-term, we need the performance boost, but we've got a plan optimizing our lookups, and I think this will alleviate the need for the caching. Is it OK for us to fix these issues with bug 589849? > CSS2.1 says @imports must occur before everything else (except @charset), > so it would be a nice perf boost to bail out of the loop here when the first > non-@import, non-@charset is encountered. It looks to me that the browser doesn't actually ignore @imports that come later on. So we might we be technically correct, but practically wrong by doing this? > >+ return aElement.tagName + "[" + priorSiblings + "]"; > Seems like this might be a little confusing. Makes me think of XPath, > but not quite. You are right, however we are really stuck for space here. We are waiting for limi to report on UI issues. Perhaps he should have some input. > > _markMatches() > > It might be worthwhile to have someone like dbaron take a look at this > function, since you're sorta reimplementing part of CSS here. Would be good > for catching any quirky corner cases... Good point. I think this function will be cut in half by our performance work, so if it's OK with you, I'd like to postpone this until we've done that. > >+function CssSpecificity(aSelector, aIndex, aPriority) > > This looks like another good chunk of code to ask dbaron for feedback on. We already know of a bug in this function, and Mihai has done some great work in bug 590090 fixing it. We should get dbaron's feedback when were more sure that it is bug free. Specifically I think you've probably found some issues for us to look at. I'd like to raise some bugs for some of these issues so we can fix them properly rather than rushing something wrong in. Is that OK? Thanks for the review. Hugely useful. Joe.

(In reply to comment #68) > You are correct about there being a general issue with caching. ... > Is it OK for us to fix these issues with bug 589849? Yes. > It looks to me that the browser doesn't actually ignore @imports that come > later on. Hmpf, ok, ignore me then! > > > _markMatches() > > > > It might be worthwhile to have someone like dbaron take a look at this > > function, since you're sorta reimplementing part of CSS here. Would be good > > for catching any quirky corner cases... > > Good point. I think this function will be cut in half by our performance work, > so if it's OK with you, I'd like to postpone this until we've done that. That's fine, but file a followup bug so we don't lose track of the issue. It should, at worst, be mostly-working as is, so that's fine for initial user feedback. But as a quality developer tool, we should obviously make sure it's not doing things wrong and giving people misleading info. > I'd like to raise some bugs for some of these issues so we can fix them > properly rather than rushing something wrong in. Is that OK? Yes, I'd be fine with filing followup bugs for the things I've noted so far (though for the safe, minor stuff it's probably easier to just fix them, but your call). Please mark them as blocking this bug.

Patch rebased to trunk.

Patch rebased to trunk.

(In reply to comment #36) > Ehsan and Axel: thank you for your suggestions for improving the > internationalization of the Style panel. > > I'd like to make the changes you have requested, but I still have some > questions: > > 1. Do I reuse an existing DTD or do I create a new one? If I have to create a > new one, should I put it in the same location as browser.dtd? If not, to which > DTD should I add the new strings? A new DTD file would be good, next to inspector.properties. > 2. As you can see in csshtmltree.html, the HTML templates have almost no actual > strings, they mostly have placeholders like ${variable}. Those are generated > dynamically by the csshtmltree.js script. Some of those variables are strings > that need localization. Do I use inspector.properties for those, or do I ensure > from the JS that I pass entity names that are added into the HTML? inspector.properties sounds right for those. A few more comments: Why is style.property.important localizable? Isn't that just CSS? MDC is MDN now, I think. Nit on localization notes, # LOCALIZATION NOTE (style.rule.sourceUnknown,style.rule.sourceElement,style.rule.sourceInline): I didn't get what style.rule.specificity is about, fwiw. What are those 4 variables?

reducing the depends list, because this bug does not truly depend on those followups.

I'm just going through things making fixes, and raising bugs for stuff that we're not going to fix today. Anything I've missed out is fixed. > >+CssLogic.prototype = { > ... > >+ highlight: function CssLogic_highlight(aViewedElement) > ... > >+ this.viewedElement = aViewedElement; > >+ this.viewedDocument = doc; > >+ this.sheets = []; > > Hmm so you're retaining references to elements and documents, but I don't see > them explicitly released anywhere. Have to looked at the lifetimes of your > objects, to ensure we're not keeping junk alive longer than it needs to be? The idea is that inspector.js calls highlight(null) which releases them. I think this is done correctly, but we should check. I have raised bug 591211 to ensure that we do this. > >+ // Hunt down top level stylesheets, and cache them. > >+ let domSheets = this.viewedDocument.styleSheets; > > What if a page dynamically adds stylesheets after you've created this static > cache? > > >+ if (domSheet.disabled) { > >+ continue; > >+ } > > ...or if a page toggles .disabled? Or uses .insertRule() / .deleteRule()? I have raised bug 591212 which will make sure that we do this as part of the performance work > >+ get shortSource() > ... > >+ // Get rid of any query string. There are times when this might be the > >+ // wrong thing to do, but getting rid of it seems like the simplest option > >+ // so we should do that until we have a good reason to do otherwise. > >+ let queryPos = this._source.indexOf("?"); > >+ if (queryPos != -1) { > > Consider http://site.com/?makeSomeCSS > > Maybe not worth worrying about for now? Followup bug? Simple fix: "if (queryPos > 0)" ensures that the string has substance. > >+ // domUtils.getRuleLine fails for @import rules. > > File a bug for this (and note it here), seems like it shouldn't! See bug 591303 > >+ this.line = -1; > >+ if (aCssLogic.stylePanel && this.selector != "@element.style") { > >+ try { > >+ this.line = aCssLogic.stylePanel.domUtils.getRuleLine(aDomRule); > > If sheet's an inline <style>, is the line number here relative to the document > source, or start of the <style> block? line numer in the HTML document, which is what we want. > >+ aSelector.split(/[ >+]/).forEach(function(aSimple) { > > Shouldn't "~" should be here too? Yes, but it's not that simple because of selectors like E[foo~="bar"] This will be fixed as part of bug 590090. > >+ this.ids += (aSimple.match(/#/g) || []).length; > > /^#/ to force matching at the beginning? (consider ".weird#class"?) > > I'm not sure if I should go through the CSS grammar to looks for edge cases > here and the rest of the function. Have you done so? We think that irrespective of what the spec says, the browser looks up elements with class="weird" and id="class" here, but we'll check. We will also double check with the spec as part of this. See bug 591308

Fixes as requested by dolske + a rebase.

Rebase and minor change (due to parameter change) to keep up with review changes to csslogic.

(In reply to comment #72) > (In reply to comment #36) > > Ehsan and Axel: thank you for your suggestions for improving the > > internationalization of the Style panel. > > > > I'd like to make the changes you have requested, but I still have some > > questions: > > > > 1. Do I reuse an existing DTD or do I create a new one? If I have to create a > > new one, should I put it in the same location as browser.dtd? If not, to which > > DTD should I add the new strings? > > A new DTD file would be good, next to inspector.properties. > > > 2. As you can see in csshtmltree.html, the HTML templates have almost no actual > > strings, they mostly have placeholders like ${variable}. Those are generated > > dynamically by the csshtmltree.js script. Some of those variables are strings > > that need localization. Do I use inspector.properties for those, or do I ensure > > from the JS that I pass entity names that are added into the HTML? > > inspector.properties sounds right for those. > > A few more comments: > > Why is style.property.important localizable? Isn't that just CSS? > MDC is MDN now, I think. > Nit on localization notes, > # LOCALIZATION NOTE > (style.rule.sourceUnknown,style.rule.sourceElement,style.rule.sourceInline): > I didn't get what style.rule.specificity is about, fwiw. What are those 4 > variables? Thanks for your reply, Axel! The Style panel uses all of its strings from JavaScript, and as you said it's appropriate for us to use inspector.properties. Therefore, at the moment we do not need a new DTD. I have made the other changes to the internationalization code, as you have suggested back then. Joe will change the string from MDC to MDN in the upcoming CssHtmlTree patch. With regards to the four localization strings (the four sources): I tried to explain it in the notes... Maybe I was unsuccessful - sorry. The idea is that we show style properties that are each grouped into style rules. Rules are given by selectors - they are rules on how to match elements in the web page (HTML). Developers can put such style rules with properties in multiple locations - inside the web page, and external to the page, in CSS files. Those style.rule.source* strings refer to different types of such sources. When the external source can be determined - a file or some URL - we show it, instead of any localized string. Maybe Joe can improve the localization note for the four strings. I am not a native English speaker and I already tried, hehe. :) I did add the style.property.important string because the idea is not to display the CSS source which has "!important", but to display a human-readable string that tells the developer the style property is marked as important, inside the style rule. While I agree that in English this is not really needed - CSS itself is English - I think that people from other countries won't mind seeing this localized. If you consider that string is unnecessary, we can remove it. Thanks again!

Comment on attachment 469894 [details] [diff] [review] Split 2l: the CssHtmlTree (Partial review, posting what I have done so far. More coming.) >+++ b/browser/base/content/browser.xul ... >- <listbox id="inspector-style-listbox" flex="1"/> >+ <browser id="inspector-style-browser" Why a <browser>? Can you just use an <iframe> here? Seems like that's all that's really needed. >+++ b/browser/base/content/csshtmltree.js ... >+function CssHtmlTree(aStyleWin, aCssLogic, aStrings) >+{ As in the other file, just make a this.strings getter, instead of passing it in through the constructor. >+ // 'window' in that it contains a document TBH, I don't think that any of the comments in the constructor here really add value. I'd just mention csshtmltree.xhtml once and nuke all the others, that way it's easy to tell it's just a big block of getElementById() calls. >+ _template: function CssHtmlTree_template(aTemplate, aDestination, aData) >+ { ... >+ (new Templater()).processNode(duplicated, aData); Seems like all of Templater could just be made static (this, just |Templater.processNode(x,y)| here)? >+ createStyleGroupViews: function CssHtmlTree_createStyleGroupViews() >+ { >+ // Take care if you want to change the names of the group titles, these >+ // strings are used in internationalization, so the changes need to be >+ // followed through to the nsIStringBundle of the Inspector >+ // (see inspector.properties). >+ this.styleGroups = [ >+ new StyleGroupView(this, "Text, Fonts and Color", [ Pass in the actual ID here (something like "textFontColorGroup"), instead of the an english string that then gets magically mangled into an ID. >+ new StyleGroupView(this, "Effects and Other", [ For a followup bug: I think you really need a way to dynamically get a list of all known-to-Gecko CSS properties (which I believe are common to all elements?), group the ones this code knows about, and then plop everything else into this "Other" group. Alternatively, write a test that looks at these tables and fails when an new (unknown) property is added. The problem here is that no one is going to remember to modify this code when new CSS properties are added in the future. So, either a test is needed to ensure people know to do that, or new properties should automatically show up somewhere generic (at which point users would presumably notice and file a bug to clean up the UI). >+StyleGroupView.prototype = { >+ /** >+ * The click event handler for the title of the style group view. >+ */ >+ click: function StyleGroupView_click() >+ { ... >+ if (!this.closed) { >+ this.element.style.display = "none"; >+ this.toggle.innerHTML = "&#x25B6;"; Eww. This should just be done with CSS. Set an attribute (|open="true"|, like XUL), and have 1 structural rule that controls .display, and 1 theme rule that controls the twisty widget appearance. We've got native-looking twisties for Windows in toolkit/themes/winstripe/global/tree/, not sure where the OS X ones are coming from. There's also toolkit/themes/*/global/arrow/ which is uglier but on all platforms. Maybe just use those for now, and file a theme bug to sort out where to get nice arrows for any platform. >+function PropertyView(aTree, aGroup, aName) >+{ ... >+ this.link = "https://developer.mozilla.org/en/CSS/" + aName; This needs to either be localized, or use a language-agnostic link (which the site would presumably redirect to as appropriate). Not sure how to do this on DevMo; https://developer.mozilla.org/en/CSS/color works bug https://developer.mozilla.org/CSS/color just redirects to the toplevel page. File a followup to sort this out? >+PropertyView.prototype = { ... >+ click: function PropertyView_click(aEvent) >+ { ... >+ if (!this.closed) { >+ this.element.style.display = "none"; >+ if (this.toggle) { >+ this.toggle.innerHTML = "&#x25B6;"; >+ } Ditto for using CSS and chrome:// images here. >+ _populateTemplate: function PropertyView_populateTemplate(showAll) Checkpoint: I'm going to skip the rest of this file for the moment, and jump ahead to some other stuff. >+++ b/browser/base/content/csshtmltree.xhtml >@@ -0,0 +1,284 @@ >+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN" >+ "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd"> HTML5 is the new XHTML! >+<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en"> The xml:lang is going to be wrong for most locales! >+ <!-- The title is localized when the Style panel is open by inspector.js --> >+ <title>Style</title> Just leave it blank, then, or use a string entity if you want a fallback string. >+ <style type="text/css"> >+ body { >+ font-family: Lucida Grande, sans-serif; All this should go into separate CSS files. browser/base/content/foo.css for structural stuff, and browser/themes/*/browser/bar.css for presentational stuff (ie, stuff that themes would want to control). >+ /* Take away these two :visited rules to get a core dumper */ ??? >+ <div id="templateHeader"> >+ <p class="path"> >+ <label>${l10n('lookingAtLabel')}</label> >+ <ol> >+ <li foreach="item in ${pathElements}"> >+ <a href="#" onclick="${pathClick}" Templated JS makes me afraid of security issues. Can this just be made a constant |onclick="foo(this)"|, where foo(aElement) looks at aElement / aElement.getAttribute("something") to get the data it needs? >+ <input type="checkbox" onchange="${toggleSpecificity}" Ditto. >+ <a class="link" href="https://developer.mozilla.org/en/CSS/Specificity" Also needs to deal with localization. I generally wonder if inventing a new template format is worthwhile here, seems like it might be more straightforward to just have the code look for the specific sections (<div> IDs, say) and use DOM createElement brute force to create what's needed. >+++ b/browser/locales/en-US/chrome/browser/inspector.properties ... >+style.property.numberOfRulesSingular=%1$S rule >+style.property.numberOfRulesPlural=%1$S rules Singular and Plural? Oh, if only it was so simple! See https://developer.mozilla.org/en/Localization_and_Plurals You'll be wanting to fix this in a followup. :)

> This should just be done with CSS. Set an attribute (|open="true"|, like XUL), > and have 1 structural rule that controls .display, and 1 theme rule that > controls the twisty widget appearance. > > We've got native-looking twisties for Windows in > toolkit/themes/winstripe/global/tree/, not sure where the OS X ones are coming > from. There's also toolkit/themes/*/global/arrow/ which is uglier but on all > platforms. Maybe just use those for now, and file a theme bug to sort out where > to get nice arrows for any platform. gavin suggested | -moz-appearance: treetwistyopen | for mac and linux and it seems to work fine. I've had mixed results with the toolkit/themes/winstripe/global/tree graphics but they should do.

This is the list of things from comment 78 that we're going to try fix quickly - Use <iframe> rather than <browser> if we can - Do l10n through a getter rather than passing a strings blob in - Reduce clutter in the constructor - Pass an titleID into the StyleGroupView ctor rather than en_US to be mangled - Use CSS rather than hacking innerHTML (twice) - Replace xhtml with html5 (check with l10n team!) - Don't specify the lang in the <html> element (or localize it!) - Leave the <title> blank at the start - Move the CSS into a separate file

This is the list of things from comment 78 that we have opened bugs for: > Seems like all of Templater could just be made static (this, just > |Templater.processNode(x,y)| here)? and: > I generally wonder if inventing a new template format is worthwhile here, > seems like it might be more straightforward to just have the code look for > the specific sections (<div> IDs, say) and use DOM createElement brute force > to create what's needed. We have an open issue as to the best way to do templating in general. This template engine was taken from Bespin, because we wanted something simple but functional. You are right that this could be static, but there didn't seem to be much point in making the change when we're probably going to make bigger changes down the line. As to the issue of whether we need a template engine at all: Some of the complexity is perhaps masked by the fact that we've got 3 levels of nested templates. Converting them all to straight DOM would be a significant bloat, and I'm convinced that if we have a template engine, that it should be used here. Standing alone it may not be worth templating, but I think there is a bigger question-mark to be answered. Bug 591580 > I think you really need a way to dynamically get a list of all known-to-Gecko > CSS properties (which I believe are common to all elements?), group the ones > this code knows about, and then plop everything else into this "Other" group. > Alternatively, write a test that looks at these tables and fails when an new > (unknown) property is added. > > The problem here is that no one is going to remember to modify this code when > new CSS properties are added in the future. So, either a test is needed to > ensure people know to do that, or new properties should automatically show up > somewhere generic (at which point users would presumably notice and file a bug > to clean up the UI). Bug 591582 > >+ this.link = "https://developer.mozilla.org/en/CSS/" + aName; > > This needs to either be localized, or use a language-agnostic link (which the > site would presumably redirect to as appropriate). Bug 591584 > >+ /* Take away these two :visited rules to get a core dumper */ We did have a core dumper, I will be checking to see if it still exists and raising a bug if it does. > Singular and Plural? Oh, if only it was so simple! > > See https://developer.mozilla.org/en/Localization_and_Plurals Bug 591590

One other thing to understand better: > >+ <div id="templateHeader"> > >+ <p class="path"> > >+ <label>${l10n('lookingAtLabel')}</label> > >+ <ol> > >+ <li foreach="item in ${pathElements}"> > >+ <a href="#" onclick="${pathClick}" > > Templated JS makes me afraid of security issues. Can this just be made a > constant |onclick="foo(this)"|, where foo(aElement) looks at aElement / > aElement.getAttribute("something") to get the data it needs? > > >+ <input type="checkbox" onchange="${toggleSpecificity}" > > Ditto. Could you clarify what the security issues could be? Thanks,

I should add: thanks for the review. I've just been through it quickly so we can get started. More comments as we learn more.

PS: There's no l10n technology for HTML5, I don't think it's up to the task of writing localizable applications.

(In reply to comment #84) > PS: There's no l10n technology for HTML5, I don't think it's up to the task of > writing localizable applications. We do have localization - it's provided by the templating, so I think this is a bit of a bikeshed issue. I lean slightly towards HTML5 because it's the hip new thing, but really I'd like to get everyone to consensus, and the patch in.

I previously put this down to do quickly: - "Use CSS rather than hacking innerHTML (twice)" On reflection, we are going to do them in a separate bug because it's not as easy as I thought, and because we've yet to hear finally from limi on this issue. Bug 591651

(In reply to comment #84) > PS: There's no l10n technology for HTML5, I don't think it's up to the task of > writing localizable applications. From a technical perspective localization in HTML5 versus XHTML is the same. I have switched the code from .html to .xhtml as was recommended by you. Dolske and Joe: I would suggest we keep csshtmltree.xhtml and not rename back to .html. I will modify the DOCTYPE to be that of HTML5: <!DOCTYPE html>. Doctypes have no relevance other than triggering the standards-mode rendering.

I'll obviously need to open a thread on .platform on this.

(In reply to comment #78) > Comment on attachment 469894 [details] [diff] [review] > Split 2l: the CssHtmlTree > > (Partial review, posting what I have done so far. More coming.) > > >+++ b/browser/base/content/browser.xul > ... > >- <listbox id="inspector-style-listbox" flex="1"/> > >+ <browser id="inspector-style-browser" > > Why a <browser>? Can you just use an <iframe> here? Seems like that's all > that's really needed. Good point. Using an <iframe> now. Works fine with that as well. > >+++ b/browser/base/content/csshtmltree.js > ... > >+function CssHtmlTree(aStyleWin, aCssLogic, aStrings) > >+{ > > As in the other file, just make a this.strings getter, instead of passing it in > through the constructor. Good point. Implemented the same solution as was done by Joe in csslogic.js. > >+ // 'window' in that it contains a document > > TBH, I don't think that any of the comments in the constructor here really add > value. I'd just mention csshtmltree.xhtml once and nuke all the others, that > way it's easy to tell it's just a big block of getElementById() calls. Correct. Changed the constructor now. > >+ _template: function CssHtmlTree_template(aTemplate, aDestination, aData) > >+ { > ... > >+ (new Templater()).processNode(duplicated, aData); > > Seems like all of Templater could just be made static (this, just > |Templater.processNode(x,y)| here)? Good point. The Templater comes from the Bespin project. I have changed it now to use a static object. Simpler and nicer. > >+ createStyleGroupViews: function CssHtmlTree_createStyleGroupViews() > >+ { > >+ // Take care if you want to change the names of the group titles, these > >+ // strings are used in internationalization, so the changes need to be > >+ // followed through to the nsIStringBundle of the Inspector > >+ // (see inspector.properties). > >+ this.styleGroups = [ > >+ new StyleGroupView(this, "Text, Fonts and Color", [ > > Pass in the actual ID here (something like "textFontColorGroup"), instead of > the an english string that then gets magically mangled into an ID. Good point. Fixed. > >+ new StyleGroupView(this, "Effects and Other", [ > > For a followup bug: > > I think you really need a way to dynamically get a list of all known-to-Gecko > CSS properties (which I believe are common to all elements?), group the ones > this code knows about, and then plop everything else into this "Other" group. > Alternatively, write a test that looks at these tables and fails when an new > (unknown) property is added. > > The problem here is that no one is going to remember to modify this code when > new CSS properties are added in the future. So, either a test is needed to > ensure people know to do that, or new properties should automatically show up > somewhere generic (at which point users would presumably notice and file a bug > to clean up the UI). Good point. This is for a follow-up. See bug 591582. Not sure if we have an API for this kind of stuff, or we should auto-magically infer such information using the DOM 2 Style API? > >+StyleGroupView.prototype = { > >+ /** > >+ * The click event handler for the title of the style group view. > >+ */ > >+ click: function StyleGroupView_click() > >+ { > ... > >+ if (!this.closed) { > >+ this.element.style.display = "none"; > >+ this.toggle.innerHTML = "&#x25B6;"; > > Eww. > > This should just be done with CSS. Set an attribute (|open="true"|, like XUL), > and have 1 structural rule that controls .display, and 1 theme rule that > controls the twisty widget appearance. > > We've got native-looking twisties for Windows in > toolkit/themes/winstripe/global/tree/, not sure where the OS X ones are coming > from. There's also toolkit/themes/*/global/arrow/ which is uglier but on all > platforms. Maybe just use those for now, and file a theme bug to sort out where > to get nice arrows for any platform. Hehe, good point. I changed the code to use open=true|false, and to change the .display property based on that attribute from CSS - no longer JS for that. Agreed it was ugly. I haven't switched to XUL twisties and the likes because that kind of work, IIANM, require more cross-platform testing. For the moment, what I did is, I hope, sufficient to go past review. Nonetheless, Joe has reported bug 591651 as a follow-up, if we further need to make improvements to the code. > >+function PropertyView(aTree, aGroup, aName) > >+{ > ... > >+ this.link = "https://developer.mozilla.org/en/CSS/" + aName; > > This needs to either be localized, or use a language-agnostic link (which the > site would presumably redirect to as appropriate). > > Not sure how to do this on DevMo; https://developer.mozilla.org/en/CSS/color > works bug https://developer.mozilla.org/CSS/color just redirects to the > toplevel page. File a followup to sort this out? Yeah, Joe has filled bug 591584. Currently, I cannot do anything about this issue. We can't link to supposedly-localized pages for each CSS property, until MDN properly redirects to the English version. > >+++ b/browser/base/content/csshtmltree.xhtml > >@@ -0,0 +1,284 @@ > >+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN" > >+ "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd"> > > HTML5 is the new XHTML! Hehe, yeah. I switched to the HTML5 DOCTYPE: <!DOCTYPE html>. However, I kept the file as XHTML because this was asked by Axel - first iterations of the Style panel code used an .html file. > >+<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en"> > > The xml:lang is going to be wrong for most locales! Agreed. Removed. > >+ <!-- The title is localized when the Style panel is open by inspector.js --> > >+ <title>Style</title> > > Just leave it blank, then, or use a string entity if you want a fallback > string. Left it blank. It's dynamically localized by the Inspector code. > >+ <style type="text/css"> > >+ body { > >+ font-family: Lucida Grande, sans-serif; > > All this should go into separate CSS files. browser/base/content/foo.css for > structural stuff, and browser/themes/*/browser/bar.css for presentational stuff > (ie, stuff that themes would want to control). Agreed. I moved the style into browser/base/content/csshtmltree.css. There will be, perhaps, substantial changes post-beta5 for the Style panel UI, based on feedback from Limi. I have not made the separation between theme and content style, because I am not even sure how to do it, and it's all very much subject to change. Additionally, I think the style will need a review, a cleanup after beta5 - once we implement Limi's UI changes. > >+ <div id="templateHeader"> > >+ <p class="path"> > >+ <label>${l10n('lookingAtLabel')}</label> > >+ <ol> > >+ <li foreach="item in ${pathElements}"> > >+ <a href="#" onclick="${pathClick}" > > Templated JS makes me afraid of security issues. Can this just be made a > constant |onclick="foo(this)"|, where foo(aElement) looks at aElement / > aElement.getAttribute("something") to get the data it needs? True, but here we do not execute any user-code at all. I don't see any potential security issue. Please expand. > I generally wonder if inventing a new template format is worthwhile here, seems > like it might be more straightforward to just have the code look for the > specific sections (<div> IDs, say) and use DOM createElement brute force to > create what's needed. Agreed. This has been previously discussed within the devtools team. Please see bug 591580. > >+++ b/browser/locales/en-US/chrome/browser/inspector.properties > ... > >+style.property.numberOfRulesSingular=%1$S rule > >+style.property.numberOfRulesPlural=%1$S rules > > Singular and Plural? Oh, if only it was so simple! > > See https://developer.mozilla.org/en/Localization_and_Plurals > > You'll be wanting to fix this in a followup. :) Agreed. Joe has filed bug 591590. However, today I fixed this issue - I hope it's fine now. Thank you very much for your time to review these style panel patches! (will attach the updated patch in a few moments)

FTR, we're feature and string frozen for B5, this should land for B6. I still disagree with the l10n aspects of this code, but I don't have the cycles to reverse engineer the whole thing.

Updated patch with changes requested by Dolske in his latest review. (see previous comment) Other minor changes: - Renamed the Bullets style group to Lists. - Moved the Lists group to be the second group displayed in the style panel. (these two changes were suggested by Limi) Also note that plurals are now used for more than the "numberOfRules" l10n string. We had other places where we could've made use of plurals. Thanks again for your review!

(In reply to comment #87) > From a technical perspective localization in HTML5 versus XHTML is the same. That's completely wrong.

(In reply to comment #92) > (In reply to comment #87) > > From a technical perspective localization in HTML5 versus XHTML is the same. > > That's completely wrong. Oops. Sorry. Then I am not aware of how localization works, but I am happy to learn. I thought only from the perspective of entities: we can use entities both in HTML and XHTML. (or maybe I am wrong with this as well!) With regards to the localization of the style panel: please let us know what further changes we need to make. Thank you very much!

We only parse DTDs for xhtml, not for html files, so we can't use entities in html. As for figuring out how to move content from .properties to .dtd, strings that are dynamically generated should probably be in the .properties, whereas strings that are conditionally shown can be in the DTD and be switched on and off via CSS. The most extreme example of such a code would be netError.xhtml, http://mxr.mozilla.org/mozilla-central/source/docshell/resources/content/netError.xhtml.

(In reply to comment #90) > FTR, we're feature and string frozen for B5, this should land for B6. > > I still disagree with the l10n aspects of this code, but I don't have the > cycles to reverse engineer the whole thing. It looks like string freeze is September 10, shipped with beta 6 not beta 5. https://wiki.mozilla.org/Firefox/4/Beta (In reply to comment #94) Is there something in the handling of .properties and .dtd which means we should prefer localization via .dtd?

We have string and feature freezes for B5, which are done. And for Firefox proper, which have been moved to B6. My comment was merely to make sure we don't have more feature work landing over the weekend. Re dtd vs properties, yes, DTDs are in general preferred as it's easier to find how and why which strings show up where in the UI. With .properties, you basically have to reverse engineer the code every time.

Re-requesting blocking status because we need to incorporate Axel's feedback into this feature and land it as soon as possible in b6.

Updated patch. Moved the strings that we could move into a new inspector.dtd file (from the inspector.properties file), as recommended by Axel (thank you!). The XHTML file now includes the inspector.dtd entities. Axel: please check and let us know if there are further internationalization changes needed. Thanks a lot!

Comment on attachment 470313 [details] [diff] [review] Split 2n: the CssHtmlTree Noting a review request on myself, I'll have more comments tomorrow.

(In reply to comment #96) > Re dtd vs properties, yes, DTDs are in general preferred as it's easier to find > how and why which strings show up where in the UI. With .properties, you > basically have to reverse engineer the code every time. I understand that we need to write code for the .properties case, but from your point of view, to work out how it is used, doesn't it come down to a 'grep -R' in both cases: $ time grep -Rn 'inspectStylePanelTitle.label' . ./base/content/browser.xul:296: ... ./locales/en-US/chrome/browser/browser.dtd:193: ... grep -Rn 'inspectStylePanelTitle.label' . 0.03s user 0.03s system 93% cpu 0.065 $ time grep -Rn 'specificityHelpLink' . ./base/content/csshtmltree.xhtml:208: ... ./locales/en-US/chrome/browser/inspector.properties:28: ... ./locales/en-US/chrome/browser/inspector.properties:31: ... grep -Rn 'specificityHelpLink' . 0.03s user 0.03s system 89% cpu 0.068 total We've done the work, so this isn't about that, I'm trying to understand why this is important. Thanks,

For example, style.rule.sourceUnknown won't find a call site. I don't even find sourceUnknown outside of the properties files. OK, are they actually used? Trying an example where I can come up with something, style.rule.status.BEST. That's a partial match in cacheStatusNames, and depends on CssLogic.STATUS. That's likely in csslogic.js, which I can't find. This turns out to be tricky to explain, cause I don't find the call sites for the strings I pick.

Ahh, csslogic is in the other patch in this bug. You probably want a comment cross referencing the STATUS array to the localized names, to make sure people understand that the two need to be consistent. Strings that are not yet used but are planned to be added in follow ups should be added in those patches. Localization Notes should reference the IDs they refer to as single explicit list, comma separated. So instead of # LOCALIZATION NOTE (style.rule.sourceUnknown): # LOCALIZATION NOTE (style.rule.sourceElement): # LOCALIZATION NOTE (style.rule.sourceInline): make that # LOCALIZATION NOTE (style.rule.sourceUnknown, style.rule.sourceElement, style.rule.sourceInline): (Though I expect these strings to move to another patch) style.rule.specificity.* entries with Plurals should each get a PluralForm comment. I wish the path from style.group. to an <h1> was simpler. I wonder if those could be all in <span>s with a class and CSS to switch one of them on. Like <h1 onclick=${group.click}" class="${tbd}"> <span class="Text_Fonts_and_Color">&style.group.Text_Fonts_and_Color;</span> <span class="Background">&style.group.Background;</span> ... </h1>

Updated patch. Only changed the comment for the CssLogic.STATUS array, as requested by Axel.

Updated patch. Changes: - removed unused strings. This means I'll update bug 585565 accordingly. - made changes to LOCALIZATION NOTES as requested (including mentioning of PluralForms). - made changes to groups, such that the group titles show up in the csshtmltree.xhtml file (and in the new inspector.dtd). This required a bit of changes in csshtmltree.js, but nothing dramatic / not too much. Thanks for your review Axel! Please let us know if further changes are required.

Comment on attachment 470493 [details] [diff] [review] Split 2o: the CssHtmlTree That looks pretty cool, thanks. I'd have one nit, could you replicate the plurals comment for each entity? I'm having an upcoming patch to compare-locales that will read those, and it'll be much easier if I can just do that per entity.

Comment on attachment 470493 [details] [diff] [review] Split 2o: the CssHtmlTree PS: Canceling the review request in favor of the feedback flag.

If you are having trouble applying the csshtmltree patch then it's because it needs rebasing on top of the html tree panel (bug 572038). I've had some difficulty getting those patches to work correctly, so I'm hesitating before I upload a rebased patch. If you find a reject with inspector.js, to do with highlight(null), then try this patchlet instead: diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js --- a/browser/base/content/inspector.js +++ b/browser/base/content/inspector.js @@ -782,6 +782,9 @@ if (this.isStylePanelOpen) { this.stylePanel.hidePopup(); } + this.cssLogic.highlight(null); + this.cssHtmlTree.highlight(null); + if (this.domPanel) { this.domPanel.hidePopup(); this.domBox = null; Any confirmation of the issue, and I'll upload a rebased patch.

Comment on attachment 470489 [details] [diff] [review] Split 1k: CssLogic First, apologies for taking so long to get back to this. I have a few tiny comments on the current patch, but I'm going to go ahead an mark it r+. I think this code is adequate to land, although I still have some significant concerns about its correctness. Specifically: * Handling dynamic page style updates (comment 67) -- bug 591212 * Behavior of CssSpecificity / markMatches() / smaller nits can differ from what the browser's style system actually does. -- bug 592743 * Getting a consult from a resident CSS expect for any other corner cases I've missed. The second worries me the most (a broken tool can be far more frustrating than no tool at all). But I think this is an excellent start, you've already recognized these weak areas, and we'll get important feedback from this as-is while improving it. >+CssLogic.l10n = function CssLogic_l10n(aName) >+{ >+ if (!CssLogic._strings) { >+ CssLogic._strings = Services.strings.createBundle( >+ "chrome://browser/locale/inspector.properties"); >+ } >+ return CssLogic._strings.GetStringFromName(aName); >+}; You could just use XPCOMUtils to handle the memoization... XPCOMUtils.defineLazyGetter(CssLogic, "_strings", function() { return Services.strings.createBundle(...); }); CssLogic.l10n = function CssLogic_l10n(aName) { return CssLogic._strings.GetStringFromName(aName); }; >+ get shortSource() >+ { >+ if (!this._shortSource) { >+ this._shortSource = this.domSheet.href; >+ if (this._shortSource) { >+ this._shortSource = this._shortSource.split("/"); >+ this._shortSource = this._shortSource[this._shortSource.length - 1]; nsIURL might help here, and avoiding the need to parse off any query... let url = Services.ios.newURI(this._shortSource, null, null); url.QueryInterface(Ci.nsIURL); if (url.filename) this._shortSource = uri.filename; >+ get ruleCount() >+ { >+ if (!this._rules) { >+ this._findRules(); >+ } >+ if (!this._localRules) { >+ this._findElementRules(); This pattern happens in 2 other places [along with ._matched / ._markMatches()]. Might help to roll these all into a single setup() / init() call. Is the ruleCount() special case really worth it? Seems like it's just used in one place... > >+ _findRules: function CssInfo_findRules() >+ { ... >+ if (!domRule.selectorText) { >+ // See https://bugzilla.mozilla.org/show_bug.cgi?id=591349 "bug 591349" is plenty. :) >+ // jlog('no selectorText in', domRule); Nit, nuke.

Comment on attachment 470493 [details] [diff] [review] Split 2o: the CssHtmlTree A few brief comments before I crash... Will finish up tomorrow, this is top of my list still. >+++ b/browser/base/content/browser.xul ... >- <listbox id="inspector-style-listbox" flex="1"/> >+ <iframe id="inspector-style-browser" flex="1" tooltip="aHTMLTooltip" >+ src="chrome://browser/content/csshtmltree.xhtml" /> Hmm, you might need to leave out |src| here, and instead set it at runtime (eg, in an onpanelshowing handler), otherwise this might cause a Ts/Txul regression due to overhead from loading the iframe. Maybe having it in a <panel> avoids the problem, but I'm not sure. Pushing it to try and looking at the perf numbers would answer that. Or it might be easier to just change it. :) >+++ b/browser/base/content/csshtmltree.css >+ * The Initial Developer of the Original Code is >+ * Netscape Communications Corporation. >+ * Portions created by the Initial Developer are Copyright (C) 1998-1999 Still hoping to ship in Netscape 4.0? :) Please update. >+body { >+ font-family: Lucida Grande, sans-serif; >+ font-size: 11px; >+ background: #EEE; >+} Style rules like these (fonts, colors, gradients, images, etc) should go into platform-specific files under browser/themes/*/browser/, fine to make them identical across platforms for now. >+.property-view[open="false"] > table { >+ display: none; >+} Functional/structural things like this should stay in this file, though. Things in between can be a bit fuzzy, the general guideline is that is a 3rd party theme could reasonably be expected to want to do things differently, it should go under browser/themes/*. Style that would break functionality if missing or don't make sense to change stays in browser/base/content. >+/* Take away these two :visited rules to get a core dumper */ Remove or file, lest this live on forever. >+.property-view[open="false"] .rule-count:after { >+ content: " â¶"; >+} >+.property-view[open="true"] .rule-count:after { >+ content: " â¼"; >+} See comment 79.

(In reply to comment #81) > This is the list of things from comment 78 that we have opened bugs for: > > I generally wonder if inventing a new template format is worthwhile here, > > seems like it might be more straightforward to just have the code look for > > the specific sections (<div> IDs, say) and use DOM createElement brute force > > to create what's needed. > > We have an open issue as to the best way to do templating in general. This > template engine was taken from Bespin, because we wanted something simple but > functional. Fair enough. Good that this wasn't freshly invented, at least. :) I'll look again with that in mind. (In reply to comment #82) > > >+ <li foreach="item in ${pathElements}"> > > >+ <a href="#" onclick="${pathClick}" > > > > Templated JS makes me afraid of security issues. > > Could you clarify what the security issues could be? Basically, all the typical XSS type of concerns. EG, if a page can inject a value like |" onmouseover="doEvilStuff()| here, the user is pwned. This code it all running with chrome privileges, so it needs to be extra sure not to allow that to happen. (It might be interesting to look at running this in a content-type iframe, with some chrome helpers, so that an XSS-style flow wouldn't give access to anything more than a normal webpage would have.) (In reply to comment #94) > We only parse DTDs for xhtml, not for html files, so we can't use entities in > html. Really? That's surprising, I would have expected that to work. (ISTR the spec says a UA can, but that authors shouldn't assume a UA will). Haven't tried it, but in any case the HTML vs XHTML thing isn't a big deal.

(In reply to comment #108) > Comment on attachment 470489 [details] [diff] [review] > Split 1k: CssLogic > > First, apologies for taking so long to get back to this. > > I have a few tiny comments on the current patch, but I'm going to go ahead an > mark it r+. > > I think this code is adequate to land, although I still have some significant > concerns about its correctness. [snip] Thanks. We share your concerns for its correctness, and we are attempting to get expert review. There is a test tool which intrigues me - it does a tree walk on a random page comparing what we predict for a property, and what the computed style is. I'll look into how feasible this is. > >+CssLogic.l10n = function CssLogic_l10n(aName) > >+{ > >+ if (!CssLogic._strings) { > >+ CssLogic._strings = Services.strings.createBundle( > >+ "chrome://browser/locale/inspector.properties"); > >+ } > >+ return CssLogic._strings.GetStringFromName(aName); > >+}; > > You could just use XPCOMUtils to handle the memoization... > > XPCOMUtils.defineLazyGetter(CssLogic, "_strings", function() { > return Services.strings.createBundle(...); > }); > > CssLogic.l10n = function CssLogic_l10n(aName) > { > return CssLogic._strings.GetStringFromName(aName); > }; I did it this way because I liked that it could all be done in once place with less dependencies, but I'm happy to change it if needs be. > >+ get shortSource() > >+ { > >+ if (!this._shortSource) { > >+ this._shortSource = this.domSheet.href; > >+ if (this._shortSource) { > >+ this._shortSource = this._shortSource.split("/"); > >+ this._shortSource = this._shortSource[this._shortSource.length - 1]; > > nsIURL might help here, and avoiding the need to parse off any query... > > let url = Services.ios.newURI(this._shortSource, null, null); > url.QueryInterface(Ci.nsIURL); > if (url.filename) > this._shortSource = uri.filename; Done. > >+ get ruleCount() > >+ { > >+ if (!this._rules) { > >+ this._findRules(); > >+ } > >+ if (!this._localRules) { > >+ this._findElementRules(); > > This pattern happens in 2 other places [along with ._matched / > ._markMatches()]. Might help to roll these all into a single setup() / init() > call. > > Is the ruleCount() special case really worth it? Seems like it's just used in > one place... This was all to do with optimizing things for the template engine. However we've got a better way of doing things now as part of the performance work. See the patch in bug 589849 for more details. (This patch has not been internally reviewed right now) > >+ _findRules: function CssInfo_findRules() > >+ { > ... > >+ if (!domRule.selectorText) { > >+ // See https://bugzilla.mozilla.org/show_bug.cgi?id=591349 > > "bug 591349" is plenty. :) Done > >+ // jlog('no selectorText in', domRule); > > Nit, nuke. Done. Thanks.

(In reply to comment #110) > (In reply to comment #82) > > > >+ <li foreach="item in ${pathElements}"> > > > >+ <a href="#" onclick="${pathClick}" > > > > > > Templated JS makes me afraid of security issues. > > > > Could you clarify what the security issues could be? > > Basically, all the typical XSS type of concerns. EG, if a page can inject a > value like |" onmouseover="doEvilStuff()| here, the user is pwned. This code > is all running with chrome privileges, so it needs to be extra sure not to > allow that to happen. (It might be interesting to look at running this in a > content-type iframe, with some chrome helpers, so that an XSS-style flow > wouldn't give access to anything more than a normal webpage would have.) We are better off here than is obvious because we're not doing conventional string templating - this is DOM templating - we're manipulating the DOM rather than strings. Clearly, calling setAttribute('" onmouseover="doEvilStuff()') should not cause a security problem.

This patch fixes the minor issues raised by dolske. It retains it's r+ status.

fixing a merge issue. no content change

Inspector feature postponed. Removing blocking flags from Inspector bugs.

Removing items from kd4b6.

Fixed regression. The CssSheet.shortSource getter failed to work - there were some typos (Services.ios versus Services.io, uri versus url, etc).

Reprioritizing bugs. You can filter the mail on the word TEABAGS.

Comment on attachment 470493 [details] [diff] [review] Split 2o: the CssHtmlTree >+++ b/browser/base/content/browser.xul ... >- <listbox id="inspector-style-listbox" flex="1"/> >+ <iframe id="inspector-style-browser" flex="1" tooltip="aHTMLTooltip" >+ src="chrome://browser/content/csshtmltree.xhtml" /> Still wary about this possibly causing a perf regression, as a minimum this needs to run through try server and have the perf numbers examined. >+++ b/browser/base/content/csshtmltree.css ... >+body { >+ font-family: Lucida Grande, sans-serif; >+ font-size: 11px; >+ background: #EEE; >+} Split structural/presentational CSS.... Oh, this is the same patch version as I started reviewing in comment 109. So I'll just finish up with the parts I didn't look at there! >+++ b/browser/base/content/csshtmltree.js ... >+/** >+ * A quick job scheduler to allow us to do lots of work without choking the >+ * main thread. >+ * TODO: There must be a better way to not choke the main thread. >+ * @see bug 587760. >+ */ This is kind of sadmaking, we really should look at what the slow parts are here and see if we can those faster. Especially because this limits you to 10 jobs/second, so that could lead to problems with the user thinking the UI is broken, only to have things pop into view seconds later. >+Templater.processNode = function Templater_processNode(aNode, aData) >+{ Sigh. Templates. I'm really not keen on this at all, for the aforementioned security risks, and its a blob of relatively complex code unlike anything else we use in the browser. I suspect it's break-even, code wise, with just doing plain old JS-driven DOM creation without any templates (and loses in the long run due to maintenance burden). And then there's documentation and tests... Might be better to just use XUL <tree>s here, since it's an existing UI widget that does most of what you need. I'd like Gavin to weigh in here. [Since this is going to be pref'd off for Fx4, I suppose there is the option of landing as-is (more or less), as long as we can all agree on what the final architecture should looks like and when it needs to be done...] >+/** >+ * Like eval, but that creates a context of the variables in >+ * <tt>aEnvironment</tt> in which the script is evaluated. >+ * WARNING: This script uses 'with' which is generally regarded to be evil. >+ * The alternative is to create a Function at runtime that takes X parameters >+ * according to the X keys in the aEnvironment object, and then call that >+ * function using the values in the aEnvironment object. This is likely to be >+ * slow, but workable. Blah. Does the new ES5 Function.bind() help out here? http://whereswalden.com/2010/09/07/now-in-spidermonkey-and-firefox-es5s-function-prototype-bind/

(In reply to comment #119) > Comment on attachment 470493 [details] [diff] [review] > >+++ b/browser/base/content/csshtmltree.js > ... > >+/** > >+ * A quick job scheduler to allow us to do lots of work without choking the > >+ * main thread. > >+ * TODO: There must be a better way to not choke the main thread. > >+ * @see bug 587760. > >+ */ > > This is kind of sadmaking, we really should look at what the slow parts are > here and see if we can those faster. Especially because this limits you to 10 > jobs/second, so that could lead to problems with the user thinking the UI is > broken, only to have things pop into view seconds later. Thanks to Mihai, we already have a patch for this. See bugs 589849 and 587760 > >+Templater.processNode = function Templater_processNode(aNode, aData) > >+{ > > Sigh. Templates. > > I'm really not keen on this at all, for the aforementioned security risks, See my earlier comment: This isn't string templating, it doesn't have the risks associated with string templating. Instead it has similar risks to long-hand DOM coding. > and > its a blob of relatively complex code unlike anything else we use in the > browser. I suspect it's break-even, code wise, with just doing plain old > JS-driven DOM creation without any templates (and loses in the long run due to > maintenance burden). I think that template systems have a valid use in the developers toolbox, and there is more than just this code that wants to use a template engine. As previously stated, this might not be it though. > Might be better to just use XUL <tree>s here, since it's an existing UI > widget that does most of what you need. We don't think so. We had a DOM tree implementation previously. It was much more code and was very ugly from a UI perspective. > >+/** > >+ * Like eval, but that creates a context of the variables in > >+ * <tt>aEnvironment</tt> in which the script is evaluated. > >+ * WARNING: This script uses 'with' which is generally regarded to be evil. > >+ * The alternative is to create a Function at runtime that takes X params > >+ * according to the X keys in the aEnvironment object, and then call that > >+ * function using the values in the aEnvironment object. This is likely to > >+ * be slow, but workable. > > Blah. Does the new ES5 Function.bind() help out here? I don't see how it can. Could you elucidate? As far as I can see the arguments against using 'with' don't apply here, and it's much easier to understand than the alternatives like 'new Function'. Thanks

(In reply to comment #119) > Comment on attachment 470493 [details] [diff] [review] > Split 2o: the CssHtmlTree > > >+++ b/browser/base/content/browser.xul > ... > >- <listbox id="inspector-style-listbox" flex="1"/> > >+ <iframe id="inspector-style-browser" flex="1" tooltip="aHTMLTooltip" > >+ src="chrome://browser/content/csshtmltree.xhtml" /> > > Still wary about this possibly causing a perf regression, as a minimum this > needs to run through try server and have the perf numbers examined. I've been wrangling with a 1-3% (depending on noise and error) Txul regression from the tree-panel. We're pretty sure it's a result of embedding the iframe in browser.xul. My most-recent patch pulled out the iframe entirely and builds it up after startup using js. see bug 592525 for details.

Merge of several patches into one

Merge of several patches into one

Here is an updated patch for review.

Updated patch

Comment on attachment 529448 [details] [diff] [review] CSSLogic + HtmlTree Upload 17 Thanks for agreeing to review this Mihai.

Comment on attachment 529450 [details] [diff] [review] Test only patch that fully implements jlog and jprofile (upload 3) Thanks for agreeing to do this review Mihai.

Noticed a couple of bugs prior to review, fixed