Last Comment Bug 689759 - Style Inspector needs a no-results placeholder
: Style Inspector needs a no-results placeholder
Status: RESOLVED FIXED
[minotaur][styleinspector][fixed-in-f...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86 All
: P2 normal (vote)
: Firefox 10
Assigned To: Michael Ratcliffe [:miker] [:mratcliffe]
:
Mentors:
Depends on: 698762
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-27 15:02 PDT by Dave Camp (:dcamp)
Modified: 2011-11-07 06:59 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1 (12.52 KB, patch)
2011-10-04 11:36 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
mihai.sucan: review+
Details | Diff | Review
Patch 2 (13.19 KB, patch)
2011-10-14 01:28 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
mihai.sucan: review+
Details | Diff | Review
Rebased (14.36 KB, patch)
2011-10-27 06:35 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
Rebased (14.33 KB, patch)
2011-11-04 07:00 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
Rebased (14.11 KB, patch)
2011-11-05 04:53 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
another rebase (14.14 KB, patch)
2011-11-06 02:44 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Review

Description Dave Camp (:dcamp) 2011-09-27 15:02:28 PDT
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 Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-28 01:15:34 PDT
True, very true
Comment 2 Michael Ratcliffe [:miker] [:mratcliffe] 2011-10-04 11:36:42 PDT
Created attachment 564617 [details] [diff] [review]
Patch 1
Comment 3 Mihai Sucan [:msucan] 2011-10-05 10:12:24 PDT
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 Michael Ratcliffe [:miker] [:mratcliffe] 2011-10-14 01:28:26 PDT
Created attachment 567028 [details] [diff] [review]
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 Michael Ratcliffe [:miker] [:mratcliffe] 2011-10-14 08:23:17 PDT
Try build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mratcliffe@mozilla.com-499bdbfeabc0/
Comment 6 Mihai Sucan [:msucan] 2011-10-14 14:02:01 PDT
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 Michael Ratcliffe [:miker] [:mratcliffe] 2011-10-27 06:35:37 PDT
Created attachment 569951 [details] [diff] [review]
Rebased
Comment 8 Dave Camp (:dcamp) 2011-10-27 08:53:54 PDT
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.
Comment 9 Michael Ratcliffe [:miker] [:mratcliffe] 2011-11-04 07:00:38 PDT
Created attachment 571959 [details] [diff] [review]
Rebased
Comment 10 Michael Ratcliffe [:miker] [:mratcliffe] 2011-11-05 04:53:58 PDT
Created attachment 572190 [details] [diff] [review]
Rebased

Rebased to land on top of bug 689759
Comment 11 Michael Ratcliffe [:miker] [:mratcliffe] 2011-11-05 04:54:58 PDT
Oops, I meant that I rebased it on top of bug 698762
Comment 12 Rob Campbell [:rc] (:robcee) 2011-11-05 10:46:02 PDT
https://hg.mozilla.org/integration/fx-team/rev/b73aa0bd2b4e
Comment 13 Rob Campbell [:rc] (:robcee) 2011-11-05 15:22:01 PDT
backed out in:

https://hg.mozilla.org/integration/fx-team/rev/90b03b0aa9ee
Comment 14 Mihai Sucan [:msucan] 2011-11-06 02:44:43 PST
Created attachment 572279 [details] [diff] [review]
another rebase

Another rebase.

This patch now depends on bug 698762.
Comment 16 Rob Campbell [:rc] (:robcee) 2011-11-07 06:59:52 PST
https://hg.mozilla.org/mozilla-central/rev/100b93342d7c

Note You need to log in before you can comment on or make changes to this bug.