Last Comment Bug 672744 - Add search filter to style inspector
: Add search filter to style inspector
Status: VERIFIED FIXED
[styleinspector][minotaur][review+][f...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86 All
: P3 normal (vote)
: Firefox 9
Assigned To: Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
:
Mentors:
: 677927 (view as bug list)
Depends on: 582596 672743 672746
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-20 01:47 PDT by Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
Modified: 2011-10-12 04:33 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Wireframe (208.32 KB, image/png)
2011-07-20 02:00 PDT, Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
no flags Details
mockup (174.46 KB, image/png)
2011-07-20 07:43 PDT, Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
no flags Details
Style Inspector search filter (22.01 KB, patch)
2011-09-09 02:00 PDT, Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
no flags Details | Diff | Review
Style Inspector search filter 2, the sequel (21.91 KB, patch)
2011-09-09 04:55 PDT, Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
no flags Details | Diff | Review
Style Inspector search filter 3, the sequel to the sequel (21.91 KB, patch)
2011-09-09 06:24 PDT, Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
mihai.sucan: review+
Details | Diff | Review
Style Inspector search filter 4 (26.96 KB, patch)
2011-09-14 12:20 PDT, Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
no flags Details | Diff | Review
[in-fx-team] Style Inspector search filter 5 (24.08 KB, patch)
2011-09-20 03:03 PDT, Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
no flags Details | Diff | Review

Description Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-07-20 01:47:21 PDT
We should add a search filter textbox to the style inspector
Comment 1 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-07-20 02:00:45 PDT
Created attachment 547012 [details]
Wireframe
Comment 2 Kevin Dangoor 2011-07-20 07:01:33 PDT
That wireframe doesn't show the search filter textbox at all, does it?
Comment 3 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-07-20 07:43:18 PDT
Created attachment 547090 [details]
mockup

I added it to a bunch of bugs, I guess this one needs the mockup, not the wireframe
Comment 4 Mihai Sucan [:msucan] 2011-08-10 08:57:11 PDT
*** Bug 677927 has been marked as a duplicate of this bug. ***
Comment 5 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-09-07 05:11:28 PDT
My understanding is that this was a minotaur bug, especially because it is easy.
Comment 6 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-09-09 02:00:41 PDT
Created attachment 559400 [details] [diff] [review]
Style Inspector search filter
Comment 7 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-09-09 04:55:05 PDT
Created attachment 559423 [details] [diff] [review]
Style Inspector search filter 2, the sequel

Rebased
Comment 8 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-09-09 06:24:42 PDT
Created attachment 559436 [details] [diff] [review]
Style Inspector search filter 3, the sequel to the sequel
Comment 9 Mihai Sucan [:msucan] 2011-09-09 12:23:36 PDT
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 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-09-14 12:20:24 PDT
Created attachment 560219 [details] [diff] [review]
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] 2011-09-15 05:43:10 PDT
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 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-09-20 03:03:03 PDT
Created attachment 561154 [details] [diff] [review]
[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) 2011-09-20 08:04:53 PDT
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) 2011-09-21 04:55:12 PDT
https://hg.mozilla.org/mozilla-central/rev/eff517413fc1
Comment 15 Florin Strugariu [:Bebe] 2011-10-12 04:33:48 PDT
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

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