Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Add search filter to style inspector

VERIFIED FIXED in Firefox 9

Status

()

Firefox
Developer Tools
P3
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: miker, Assigned: miker)

Tracking

unspecified
Firefox 9
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [styleinspector][minotaur][review+][fixed-in-fx-team])

Attachments

(2 attachments, 5 obsolete attachments)

We should add a search filter textbox to the style inspector
Depends on: 582596
Depends on: 672743
Created attachment 547012 [details]
Wireframe
Whiteboard: [hydra]

Comment 2

6 years ago
That wireframe doesn't show the search filter textbox at all, does it?
Created attachment 547090 [details]
mockup

I added it to a bunch of bugs, I guess this one needs the mockup, not the wireframe
Attachment #547012 - Attachment is obsolete: true
Whiteboard: [hydra] → [styleinspector]

Updated

6 years ago
Duplicate of this bug: 677927
Depends on: 674856
My understanding is that this was a minotaur bug, especially because it is easy.
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [styleinspector] → [styleinspector][minotaur]
Depends on: 672746
No longer depends on: 674856
Created attachment 559400 [details] [diff] [review]
Style Inspector search filter
Attachment #559400 - Flags: review?(mihai.sucan)
Created attachment 559423 [details] [diff] [review]
Style Inspector search filter 2, the sequel

Rebased
Attachment #559400 - Attachment is obsolete: true
Attachment #559423 - Flags: review?(mihai.sucan)
Attachment #559400 - Flags: review?(mihai.sucan)
Created attachment 559436 [details] [diff] [review]
Style Inspector search filter 3, the sequel to the sequel
Attachment #559423 - Attachment is obsolete: true
Attachment #559436 - Flags: review?(mihai.sucan)
Attachment #559423 - Flags: review?(mihai.sucan)
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)
Attachment #559436 - Flags: review?(mihai.sucan) → review+
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
Attachment #559436 - Attachment is obsolete: true
Whiteboard: [styleinspector][minotaur] → [styleinspector][minotaur][review+]
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.
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.
Attachment #560219 - Attachment is obsolete: true
Comment on attachment 561154 [details] [diff] [review]
[in-fx-team] Style Inspector search filter 5

https://hg.mozilla.org/integration/fx-team/rev/eff517413fc1
Attachment #561154 - Attachment description: Style Inspector search filter 5 → [in-fx-team] Style Inspector search filter 5
Whiteboard: [styleinspector][minotaur][review+] → [styleinspector][minotaur][review+][fixed-in-fx-team]

Updated

6 years ago
Priority: P2 → P3
https://hg.mozilla.org/mozilla-central/rev/eff517413fc1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.