672744 - Add search filter to style inspector

Status: VERIFIED FIXED

(DevTools :: General, defect, P3)

Description

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

We should add a search filter textbox to the style inspector

Comment 1

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

Attachment: Wireframe

Comment 2

[Kevin Dangoor]

That wireframe doesn't show the search filter textbox at all, does it?

Comment 3

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

Attachment: mockup

I added it to a bunch of bugs, I guess this one needs the mockup, not the wireframe

Comment 5

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

My understanding is that this was a minotaur bug, especially because it is easy.

Comment 6

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

Attachment: Style Inspector search filter

Comment 7

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

Attachment: Style Inspector search filter 2, the sequel

Rebased

Comment 8

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

Attachment: Style Inspector search filter 3, the sequel to the sequel

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 559436 [details] [diff] [review]
Style Inspector search filter 3, the sequel to the sequel

Review of attachment 559436 [details] [diff] [review]:
-----------------------------------------------------------------

This is an awesome improvement to the style inspector! Great work! Thanks!

General comment: the StyleInspector.jsm and browser.css changes are not related to this bug. They seem to be something for the new UI. Still, since they are minimal changes, they are acceptable here.

Giving the patch r+ because it works as advertised and the comments below are easy to address. Looking forward to see the updated patch!

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +235,5 @@
>      }
>    },
>  
>    /**
> +   * Called when the user enters a search term

Nit: period at the end. :)

@@ +243,5 @@
> +  filterChanged: function CssHtmlTree_filterChanged(aEvent)
> +  {
> +    let win = this.styleWin.contentWindow;
> +
> +    if (this.filterChanged.timeout) {

Please use this.filterChangeTimeout.

@@ +248,5 @@
> +      win.clearTimeout(this.filterChanged.timeout);
> +    }
> +
> +    this.filterChanged.timeout = win.setTimeout(function() {
> +      this.refreshPanel();

Please also set this.filterChangeTimeout = null.

@@ +249,5 @@
> +    }
> +
> +    this.filterChanged.timeout = win.setTimeout(function() {
> +      this.refreshPanel();
> +    }.bind(this), 300);

Magic number. Please define a constant at the beginning of the file.

@@ +313,5 @@
>  
>    this.templateMatchedSelectors = aTree.styleDocument.getElementById("templateMatchedSelectors");
>    this.templateUnmatchedSelectors = aTree.styleDocument.getElementById("templateUnmatchedSelectors");
>    this.defaultStylesCheckbox = aTree.styleDocument.getElementById("defaultStyles");
> +  this.searchbar = aTree.styleDocument.getElementById("searchbar");

This line is not needed. See one of the comments below (in csshtmltree.xhtml).

@@ +397,5 @@
>      }
>  
> +    let searchTerm = this.searchbar.value;
> +    if (searchTerm) {
> +      if (this.name.indexOf(searchTerm) == -1 && this.value.indexOf(searchTerm) == -1) {

Please do case insensitive matching.

Also, you can combine the two ifs.

if (searchTerm && this.name....) {
  visible = false;
}

::: browser/devtools/styleinspector/csshtmltree.xhtml
@@ +85,5 @@
>    <div id="templateRoot">
>      <div class="path">
>        <input id="defaultStyles" type="checkbox" onchange="${refreshPanel}"/>
>        <label id="defaultStylesLabel" dir="${getRTLAttr}">&defaultStylesLabel;</label>
> +      <input id="searchbar" type="text" oninput="${filterChanged}"/>

The DOM will end up having two elements with the same ID. Please do not use IDs. Add save="searchField" (rename from searchbar to searchField).

::: browser/devtools/styleinspector/test/browser/browser_styleinspector_bug_672744_search_filter.js
@@ +1,5 @@
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Tests that the checkbox to include the UA stylesheet works properly.

This is a copy paste from the previous bug you worked on. :) Please update the comment.

@@ +12,5 @@
> +  doc.body.innerHTML = '<style type="text/css"> ' +
> +    '.matches {color: #F00;}</style>' +
> +    '<span id="matches" class="matches">Some styled text</span>' +
> +    '</div>';
> +  doc.title = "Style Inspector Default Styles Test";

Also update the title.

@@ +52,5 @@
> +  info("checking default styles checkbox");
> +  let iframe = stylePanel.querySelector("iframe");
> +  let checkbox = iframe.contentDocument.querySelector("#defaultStyles");
> +  Services.obs.addObserver(SI_AddFilterText, "StyleInspector-populated", false);
> +  EventUtils.sendMouseEvent({ type: "click" }, checkbox);

This test doesn't need to test the default styles checkbox. Please "clean up" and focus only on the filter box testing.

@@ +69,5 @@
> +  EventUtils.synthesizeKey("c", {}, iframe.contentWindow);
> +  EventUtils.synthesizeKey("o", {}, iframe.contentWindow);
> +  EventUtils.synthesizeKey("l", {}, iframe.contentWindow);
> +  EventUtils.synthesizeKey("o", {}, iframe.contentWindow);
> +  EventUtils.synthesizeKey("r", {}, iframe.contentWindow);

Maybe you can coalesce these calls. searchbar.value = "color" ? Does that fire the |input| event? If not, just do .value = "colo" and synthesizeKey("r").

@@ +86,5 @@
> +        "span " + name + " property is visible");
> +    } else {
> +      is(propertyVisible(name), false,
> +        "span " + name + " property is hidden");
> +    }

You can change the entire if() block to something simpler:

is(propView.visible, name.indexOf("color") > -1,
   "span " + name + " property visibility check");

(you don't need the propertyVisible() function in this test)

Comment 10

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

Attachment: Style Inspector search filter 4

(In reply to Mihai Sucan [:msucan] from comment #9)
> Comment on attachment 559436 [details] [diff] [review]
> Style Inspector search filter 3, the sequel to the sequel
> 
> Review of attachment 559436 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is an awesome improvement to the style inspector! Great work! Thanks!
> 
> General comment: the StyleInspector.jsm and browser.css changes are not
> related to this bug. They seem to be something for the new UI. Still, since
> they are minimal changes, they are acceptable here.
> 
> Giving the patch r+ because it works as advertised and the comments below
> are easy to address. Looking forward to see the updated patch!
> 
> ::: browser/devtools/styleinspector/CssHtmlTree.jsm
> @@ +235,5 @@
> >      }
> >    },
> >  
> >    /**
> > +   * Called when the user enters a search term
> 
> Nit: period at the end. :)
> 

Done

> @@ +243,5 @@
> > +  filterChanged: function CssHtmlTree_filterChanged(aEvent)
> > +  {
> > +    let win = this.styleWin.contentWindow;
> > +
> > +    if (this.filterChanged.timeout) {
> 
> Please use this.filterChangeTimeout.
> 

I am not sure why you want this to be a public property as opposed to a static property of the function but ... done.

> @@ +248,5 @@
> > +      win.clearTimeout(this.filterChanged.timeout);
> > +    }
> > +
> > +    this.filterChanged.timeout = win.setTimeout(function() {
> > +      this.refreshPanel();
> 
> Please also set this.filterChangeTimeout = null.
> 

Done

> @@ +249,5 @@
> > +    }
> > +
> > +    this.filterChanged.timeout = win.setTimeout(function() {
> > +      this.refreshPanel();
> > +    }.bind(this), 300);
> 
> Magic number. Please define a constant at the beginning of the file.
> 

Done

> @@ +313,5 @@
> >  
> >    this.templateMatchedSelectors = aTree.styleDocument.getElementById("templateMatchedSelectors");
> >    this.templateUnmatchedSelectors = aTree.styleDocument.getElementById("templateUnmatchedSelectors");
> >    this.defaultStylesCheckbox = aTree.styleDocument.getElementById("defaultStyles");
> > +  this.searchbar = aTree.styleDocument.getElementById("searchbar");
> 
> This line is not needed. See one of the comments below (in
> csshtmltree.xhtml).
> 

Removed defaultStylesCheckbox & searchbar.

> @@ +397,5 @@
> >      }
> >  
> > +    let searchTerm = this.searchbar.value;
> > +    if (searchTerm) {
> > +      if (this.name.indexOf(searchTerm) == -1 && this.value.indexOf(searchTerm) == -1) {
> 
> Please do case insensitive matching.
> 
> Also, you can combine the two ifs.
> 
> if (searchTerm && this.name....) {
>   visible = false;
> }
> 

Of course ... done.

> ::: browser/devtools/styleinspector/csshtmltree.xhtml
> @@ +85,5 @@
> >    <div id="templateRoot">
> >      <div class="path">
> >        <input id="defaultStyles" type="checkbox" onchange="${refreshPanel}"/>
> >        <label id="defaultStylesLabel" dir="${getRTLAttr}">&defaultStylesLabel;</label>
> > +      <input id="searchbar" type="text" oninput="${filterChanged}"/>
> 
> The DOM will end up having two elements with the same ID. Please do not use
> IDs. Add save="searchField" (rename from searchbar to searchField).
> 

Done

> :::
> browser/devtools/styleinspector/test/browser/
> browser_styleinspector_bug_672744_search_filter.js
> @@ +1,5 @@
> > +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> > +/* Any copyright is dedicated to the Public Domain.
> > +   http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +// Tests that the checkbox to include the UA stylesheet works properly.
> 
> This is a copy paste from the previous bug you worked on. :) Please update
> the comment.
> 

Done

> @@ +12,5 @@
> > +  doc.body.innerHTML = '<style type="text/css"> ' +
> > +    '.matches {color: #F00;}</style>' +
> > +    '<span id="matches" class="matches">Some styled text</span>' +
> > +    '</div>';
> > +  doc.title = "Style Inspector Default Styles Test";
> 
> Also update the title.
> 

Done

> @@ +52,5 @@
> > +  info("checking default styles checkbox");
> > +  let iframe = stylePanel.querySelector("iframe");
> > +  let checkbox = iframe.contentDocument.querySelector("#defaultStyles");
> > +  Services.obs.addObserver(SI_AddFilterText, "StyleInspector-populated", false);
> > +  EventUtils.sendMouseEvent({ type: "click" }, checkbox);
> 
> This test doesn't need to test the default styles checkbox. Please "clean
> up" and focus only on the filter box testing.
> 

Actually, we need to uncheck this box to make all styles visible ... it is not extraneous at all.

> @@ +69,5 @@
> > +  EventUtils.synthesizeKey("c", {}, iframe.contentWindow);
> > +  EventUtils.synthesizeKey("o", {}, iframe.contentWindow);
> > +  EventUtils.synthesizeKey("l", {}, iframe.contentWindow);
> > +  EventUtils.synthesizeKey("o", {}, iframe.contentWindow);
> > +  EventUtils.synthesizeKey("r", {}, iframe.contentWindow);
> 
> Maybe you can coalesce these calls. searchbar.value = "color" ? Does that
> fire the |input| event? If not, just do .value = "colo" and
> synthesizeKey("r").
> 

Setting the value fails to fire the input event ... setting the value and synthesizeKey("r") does, but doing it that way 

> @@ +86,5 @@
> > +        "span " + name + " property is visible");
> > +    } else {
> > +      is(propertyVisible(name), false,
> > +        "span " + name + " property is hidden");
> > +    }
> 
> You can change the entire if() block to something simpler:
> 
> is(propView.visible, name.indexOf("color") > -1,
>    "span " + name + " property visibility check");
> 
> (you don't need the propertyVisible() function in this test)

Done

Comment 11

[Mihai Sucan [:msucan]]

Patch is fine. Thanks for your update!

I am glad to see that it already addresses some of my comments from bug 672746 comment 28 - I like what you did for onlyUserStylesCheckbox.

Also, this patch doesn't apply cleanly on top of the latest patch bug 672746. It needs a rebase. Still, this didn't prevent me from reviewing the code changes.

Comment 12

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

Attachment: [in-fx-team] Style Inspector search filter 5

(In reply to Mihai Sucan [:msucan] from comment #11)

> Also, this patch doesn't apply cleanly on top of the latest patch bug
> 672746. It needs a rebase. Still, this didn't prevent me from reviewing the
> code changes.

It does now.

Comment 13

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

Comment on attachment 561154 [details] [diff] [review]
[in-fx-team] Style Inspector search filter 5

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

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/eff517413fc1

Comment 15

[Florin Strugariu [:Bebe]]

verified fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:9.0a2) Gecko/20111011 Firefox/9.0a2
and
Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111011 Firefox/10.0a1