689759 - Style Inspector needs a no-results placeholder

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Dave Camp (:dcamp)]

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.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

True, very true

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch 1

Comment 3

[Mihai Sucan [:msucan]]

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.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch 2

(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.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Try build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-499bdbfeabc0/

Comment 6

[Mihai Sucan [:msucan]]

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.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Rebased

Comment 8

[Dave Camp (:dcamp)]

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

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Rebased

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Rebased

Rebased to land on top of bug 689759

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Comment 12

[Rob Campbell [:rc] (:robcee)]

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

Comment 13

[Rob Campbell [:rc] (:robcee)]

backed out in:

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

Comment 14

[Mihai Sucan [:msucan]]

Attachment: another rebase

Another rebase.

This patch now depends on bug 698762.

Comment 15

[Dave Camp (:dcamp)]

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

Comment 16

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/100b93342d7c