When an element has no user styles, or when we search for a string that doesn't exist, there should be some sort of placeholder text rather than a blank style inspector.

True, very true

Comment on attachment 564617 [details] [diff] [review] Patch 1 Review of attachment 564617 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks good. Tests pass! Please address the comments below. Thank you! ::: browser/devtools/styleinspector/CssHtmlTree.jsm @@ +230,5 @@ > this.gotoMdnIcon.addEventListener("click", function() { > this.win.open(PropertyView.link, "MDNWindow"); > }.bind(this)); > + this.noResults.style.display = this.numVisibleProperties ? > + "none" : "block"; Please use noResults.hidden. @@ +245,5 @@ > */ > refreshPanel: function CssHtmlTree_refreshPanel() > { > this.win.clearTimeout(this._panelRefreshTimeout); > + this.noResults.style.display = "none"; Same as above. @@ +269,5 @@ > // display the next batch of 15. > this._panelRefreshTimeout = this.win.setTimeout(refreshView.bind(this), 0); > } else { > + this.noResults.style.display = this.numVisibleProperties ? > + "none" : "block"; Same as above. @@ +501,5 @@ > { > if (!this.visible) { > return; > } > + this.tree.numVisibleProperties++; This should be in PropertyView.refresh(). ::: browser/devtools/styleinspector/test/browser/browser_styleinspector_bug_689759_no_results_placeholder.js @@ +11,5 @@ > +{ > + doc.body.innerHTML = '<style type="text/css"> ' + > + '.matches {color: #F00;}</style>' + > + '<span id="matches" class="matches">Some styled text</span>' + > + '</div>'; There's no open tag for <div>. @@ +27,5 @@ > + > + ok(stylePanel.isOpen(), "style inspector is open"); > + > + Services.obs.addObserver(SI_AddFilterText, "StyleInspector-populated", false); > + SI_inspectNode(); You can merge SI_inspectNode() into runStyleInspectorTests(). @@ +32,5 @@ > +} > + > +function SI_inspectNode() > +{ > + var span = doc.querySelector("#matches"); s/var/let/ @@ +49,5 @@ > +{ > + Services.obs.removeObserver(SI_AddFilterText, "StyleInspector-populated", false); > + > + let iframe = stylePanel.querySelector("iframe"); > + let searchbar = iframe.contentDocument.querySelector(".searchfield"); You can do: let searchbar = stylePanel.cssHtmlTree.searchField; @@ +58,5 @@ > + EventUtils.synthesizeKey("x", {}, iframe.contentWindow); > + EventUtils.synthesizeKey("x", {}, iframe.contentWindow); > + EventUtils.synthesizeKey("x", {}, iframe.contentWindow); > + EventUtils.synthesizeKey("x", {}, iframe.contentWindow); > + EventUtils.synthesizeKey("x", {}, iframe.contentWindow); You can do: for each (let c in "xxxxxx") { EventUtils.synthesizeKey(c, {}, iframe.contentWindow); } @@ +66,5 @@ > +{ > + Services.obs.removeObserver(SI_checkPlaceholderVisible, "StyleInspector-populated", false); > +info("SI_checkPlaceholderVisible called"); > + let iframe = stylePanel.querySelector("iframe"); > + let placeholder = iframe.contentDocument.querySelector("#noResults"); let placehoder = stylePanel.cssHtmlTree.noResults; @@ +77,5 @@ > + > +function SI_ClearFilterText() > +{ > + let iframe = stylePanel.querySelector("iframe"); > + let searchbar = iframe.contentDocument.querySelector(".searchfield"); Same as above. @@ +89,5 @@ > + > +function SI_checkPlaceholderHidden() > +{ > + Services.obs.removeObserver(SI_checkPlaceholderHidden, "StyleInspector-populated", false); > + let iframe = stylePanel.querySelector("iframe"); Wrong indentation in this function. @@ +90,5 @@ > +function SI_checkPlaceholderHidden() > +{ > + Services.obs.removeObserver(SI_checkPlaceholderHidden, "StyleInspector-populated", false); > + let iframe = stylePanel.querySelector("iframe"); > + let placeholder = iframe.contentDocument.querySelector("#noResults"); Same as above. ::: browser/locales/en-US/chrome/browser/styleinspector.dtd @@ +21,5 @@ > + > +<!-- LOCALIZATION NOTE (noPropertiesFound): In the case where there are no CSS > + - properties to display e.g. due to search criteria this message is > + - displayed. --> > +<!ENTITY noPropertiesFound "No CSS properties found"> Should probably have a period at the end. ::: browser/themes/gnomestripe/browser/devtools/csshtmltree.css @@ +223,5 @@ > + > +#noResults { > + display: none; > + font-weight: bold; > + margin: 5px; I would've expected an increased font size and centered text. Please ping shorlander about this.

(In reply to Mihai Sucan [:msucan] from comment #3) > Comment on attachment 564617 [details] [diff] [review] [diff] [details] [review] > Patch 1 > > Review of attachment 564617 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > Patch looks good. Tests pass! > > Please address the comments below. Thank you! > > ::: browser/devtools/styleinspector/CssHtmlTree.jsm > @@ +230,5 @@ > > this.gotoMdnIcon.addEventListener("click", function() { > > this.win.open(PropertyView.link, "MDNWindow"); > > }.bind(this)); > > + this.noResults.style.display = this.numVisibleProperties ? > > + "none" : "block"; > > Please use noResults.hidden. > Done ... I had not realized that you can use the hidden="" attribute and then toggle using element.hidden, awesome. > @@ +245,5 @@ > > */ > > refreshPanel: function CssHtmlTree_refreshPanel() > > { > > this.win.clearTimeout(this._panelRefreshTimeout); > > + this.noResults.style.display = "none"; > > Same as above. > Done > @@ +269,5 @@ > > // display the next batch of 15. > > this._panelRefreshTimeout = this.win.setTimeout(refreshView.bind(this), 0); > > } else { > > + this.noResults.style.display = this.numVisibleProperties ? > > + "none" : "block"; > > Same as above. > Done > @@ +501,5 @@ > > { > > if (!this.visible) { > > return; > > } > > + this.tree.numVisibleProperties++; > > This should be in PropertyView.refresh(). > Done > ::: > browser/devtools/styleinspector/test/browser/ > browser_styleinspector_bug_689759_no_results_placeholder.js > @@ +11,5 @@ > > +{ > > + doc.body.innerHTML = '<style type="text/css"> ' + > > + '.matches {color: #F00;}</style>' + > > + '<span id="matches" class="matches">Some styled text</span>' + > > + '</div>'; > > There's no open tag for <div>. > Removed > @@ +27,5 @@ > > + > > + ok(stylePanel.isOpen(), "style inspector is open"); > > + > > + Services.obs.addObserver(SI_AddFilterText, "StyleInspector-populated", false); > > + SI_inspectNode(); > > You can merge SI_inspectNode() into runStyleInspectorTests(). > Done > @@ +32,5 @@ > > +} > > + > > +function SI_inspectNode() > > +{ > > + var span = doc.querySelector("#matches"); > > s/var/let/ > Done > @@ +49,5 @@ > > +{ > > + Services.obs.removeObserver(SI_AddFilterText, "StyleInspector-populated", false); > > + > > + let iframe = stylePanel.querySelector("iframe"); > > + let searchbar = iframe.contentDocument.querySelector(".searchfield"); > > You can do: > let searchbar = stylePanel.cssHtmlTree.searchField; > Done. > @@ +58,5 @@ > > + EventUtils.synthesizeKey("x", {}, iframe.contentWindow); > > + EventUtils.synthesizeKey("x", {}, iframe.contentWindow); > > + EventUtils.synthesizeKey("x", {}, iframe.contentWindow); > > + EventUtils.synthesizeKey("x", {}, iframe.contentWindow); > > + EventUtils.synthesizeKey("x", {}, iframe.contentWindow); > > You can do: > > for each (let c in "xxxxxx") { > EventUtils.synthesizeKey(c, {}, iframe.contentWindow); > } > Done > @@ +66,5 @@ > > +{ > > + Services.obs.removeObserver(SI_checkPlaceholderVisible, "StyleInspector-populated", false); > > +info("SI_checkPlaceholderVisible called"); > > + let iframe = stylePanel.querySelector("iframe"); > > + let placeholder = iframe.contentDocument.querySelector("#noResults"); > > let placehoder = stylePanel.cssHtmlTree.noResults; > Done > @@ +77,5 @@ > > + > > +function SI_ClearFilterText() > > +{ > > + let iframe = stylePanel.querySelector("iframe"); > > + let searchbar = iframe.contentDocument.querySelector(".searchfield"); > > Same as above. > Done > @@ +89,5 @@ > > + > > +function SI_checkPlaceholderHidden() > > +{ > > + Services.obs.removeObserver(SI_checkPlaceholderHidden, "StyleInspector-populated", false); > > + let iframe = stylePanel.querySelector("iframe"); > > Wrong indentation in this function. > Fixed > @@ +90,5 @@ > > +function SI_checkPlaceholderHidden() > > +{ > > + Services.obs.removeObserver(SI_checkPlaceholderHidden, "StyleInspector-populated", false); > > + let iframe = stylePanel.querySelector("iframe"); > > + let placeholder = iframe.contentDocument.querySelector("#noResults"); > > Same as above. > Fixed > ::: browser/locales/en-US/chrome/browser/styleinspector.dtd > @@ +21,5 @@ > > + > > +<!-- LOCALIZATION NOTE (noPropertiesFound): In the case where there are no CSS > > + - properties to display e.g. due to search criteria this message is > > + - displayed. --> > > +<!ENTITY noPropertiesFound "No CSS properties found"> > > Should probably have a period at the end. > Done > ::: browser/themes/gnomestripe/browser/devtools/csshtmltree.css > @@ +223,5 @@ > > + > > +#noResults { > > + display: none; > > + font-weight: bold; > > + margin: 5px; > > I would've expected an increased font size and centered text. > > Please ping shorlander about this. I want him to have a general look at the new UI anyhow so I plan on upping to try and then pinging him.

Try build: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mratcliffe@mozilla.com-499bdbfeabc0/

Comment on attachment 567028 [details] [diff] [review] Patch 2 Review of attachment 567028 [details] [diff] [review]: ----------------------------------------------------------------- Giving r+ as long as the patch passes all the try server runs. Please note the patch needs rebase - could not to test it locally. Thanks for the update! ::: browser/devtools/styleinspector/CssHtmlTree.jsm @@ +620,5 @@ > } > > + if (this.visible) { > + this.tree.numVisibleProperties++; > + } The if is not needed. This code path is not executed if this.visible is false. ::: browser/devtools/styleinspector/test/browser/browser_styleinspector_bug_689759_no_results_placeholder.js @@ +13,5 @@ > + '.matches {color: #F00;}</style>' + > + '<span id="matches" class="matches">Some styled text</span>'; > + doc.title = "Tests that the no results placeholder works properly"; > + ok(window.StyleInspector, "StyleInspector exists"); > + ok(StyleInspector.isEnabled, "style inspector preference is enabled"); This is not needed here.

We're doing developer tool prioritization, filter on 'brontozaur' to ignore the spam.

Rebased to land on top of bug 689759

Oops, I meant that I rebased it on top of bug 698762

backed out in: https://hg.mozilla.org/integration/fx-team/rev/90b03b0aa9ee

Another rebase. This patch now depends on bug 698762.