1224115 - Allow to search within Storage Inspector

Status: VERIFIED FIXED

(DevTools :: Storage Inspector, enhancement)

Description

[Sebastian Zartner [:sebo]]

The Storage panel should offer a way to search within it.

Sebastian

Comment 1

[Tim Nguyen :ntim]

Attachment: Patch

I'd love to get feedback on the code, and on the approach in general. I feel like I'm over-complicating things right now.

Helen, right now, all the rows where at least one column contains the query are shown. I'm not sure on whether it should work that way or not. It may be more natural to only take in account the key and the value columns. What do you think ?

Comment 2

[Tim Nguyen :ntim]

Also, Michael, I sometimes get a strange error when I'm typing in the search box. I'm not sure when, how or why it happens, since the search functionality works fine, even with this error:

 
TypeError: this.cells is null: Column.prototype.onTableFiltered@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/TableWidget.js:643:21
EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:147:11
filterItems@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/TableWidget.js:476:5
filterItems@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/storage/ui.js:655:5
EventListener.handleEvent*StorageUI@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/storage/ui.js:86:3
StoragePanel.prototype.open/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/storage/panel.js:54:17
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:454:5
StoragePanel.prototype.open@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/storage/panel.js:50:12
Toolbox.prototype.loadTool/onLoad@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:1253:19

Comment 3

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

Comment on attachment 8724447 [details] [diff] [review]
Patch

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

(In reply to Tim Nguyen [:ntim] from comment #2)
> Also, Michael, I sometimes get a strange error when I'm typing in the search
> box. I'm not sure when, how or why it happens, since the search
> functionality works fine, even with this error:
> 
>  
> TypeError: this.cells is null:
> Column.prototype.onTableFiltered@resource://gre/modules/commonjs/toolkit/
> loader.js -> resource://devtools/client/shared/widgets/TableWidget.js:643:21
> EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://devtools/shared/event-emitter.js:147:11
> filterItems@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://devtools/client/shared/widgets/TableWidget.js:476:5
> filterItems@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://devtools/client/storage/ui.js:655:5
> EventListener.handleEvent*StorageUI@resource://gre/modules/commonjs/toolkit/
> loader.js -> resource://devtools/client/storage/ui.js:86:3
> StoragePanel.prototype.open/<@resource://gre/modules/commonjs/toolkit/loader.
> js -> resource://devtools/client/storage/panel.js:54:17
> Handler.prototype.process@resource://gre/modules/Promise.jsm ->
> resource://gre/modules/Promise-backend.js:937:23
> this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm ->
> resource://gre/modules/Promise-backend.js:816:7
> Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.
> jsm -> resource://gre/modules/Promise-backend.js:747:11
> this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm ->
> resource://gre/modules/Promise-backend.js:779:7
> Promise.prototype.then@resource://gre/modules/Promise.jsm ->
> resource://gre/modules/Promise-backend.js:454:5
> StoragePanel.prototype.open@resource://gre/modules/commonjs/toolkit/loader.
> js -> resource://devtools/client/storage/panel.js:50:12
> Toolbox.prototype.loadTool/onLoad@resource://gre/modules/commonjs/toolkit/
> loader.js -> resource://devtools/client/framework/toolbox.js:1253:19

From what I can make out, if you have something in the searchbox and switch to another table e.g. local storage then it will throw this error. I have left a comment below explaining what should be done about that.

I am fairly certain that you don't need the CSS rule.

One thing that we will want to address at some point would be to make it work with long lists of items. At the moment I think we show 50 items and, as the user scrolls down, we show the next 50 (endless scrolling). Making the search work with this kind of thing is a much harder so it should be a future bug.

How does your filtering work when columns are hidden?

Your approach is fine by the way... this is exactly the way filtering should be done.

::: devtools/client/shared/widgets/TableWidget.js
@@ +447,5 @@
> +   * @param {String} value: The filter value
> +   * @param {Array} ignoreProps: Props to ignore while filtering
> +   */
> +  filterItems(value, ignoreProps = []) {
> +    if (this.filteredValue == value) {

if (this.filteredValue == value || !this.cells) {

@@ +557,5 @@
>    this.onRowUpdated = this.onRowUpdated.bind(this);
>    this.table.on(EVENTS.ROW_UPDATED, this.onRowUpdated);
>  
> +  this.onTableFiltered = this.onTableFiltered.bind(this);
> +  this.table.on(EVENTS.TABLE_FILTERED, this.onTableFiltered);

In destructor:
this.table.off(EVENTS.TABLE_FILTERED, this.onTableFiltered);

::: devtools/client/storage/ui.js
@@ +82,5 @@
>                                  GENERIC_VARIABLES_VIEW_SETTINGS);
>  
> +  this.searchBox = this._panelDoc.getElementById("storage-searchbox");
> +  this.filterItems = this.filterItems.bind(this);
> +  this.searchBox.addEventListener("input", this.filterItems);

In destructor:
this.searchBox.removeEventListener("input", this.filterItems);
this.searchBox = null;

@@ +651,5 @@
>  
>    /**
> +   * Handles filtering the table
> +   */
> +  filterItems() {

Surprised that works... it should be:
filterItems: function() {

::: devtools/client/themes/widgets.css
@@ +1240,5 @@
>    -moz-user-focus: normal;
>    color: var(--theme-body-color);
>  }
>  
> +.table-widget-cell[hidden] {

I don't think you need this because there is already a hidden attribute in XUL that hides stuff.

Comment 4

[Tim Nguyen :ntim]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #3)
> Comment on attachment 8724447 [details] [diff] [review]
> Patch
> 
> Review of attachment 8724447 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Tim Nguyen [:ntim] from comment #2)
> > Also, Michael, I sometimes get a strange error when I'm typing in the search
> > box. I'm not sure when, how or why it happens, since the search
> > functionality works fine, even with this error:
> > 
> >  
> > TypeError: this.cells is null:
> > Column.prototype.onTableFiltered@resource://gre/modules/commonjs/toolkit/
> > loader.js -> resource://devtools/client/shared/widgets/TableWidget.js:643:21
> > EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js ->
> > resource://devtools/shared/event-emitter.js:147:11
> > filterItems@resource://gre/modules/commonjs/toolkit/loader.js ->
> > resource://devtools/client/shared/widgets/TableWidget.js:476:5
> > filterItems@resource://gre/modules/commonjs/toolkit/loader.js ->
> > resource://devtools/client/storage/ui.js:655:5
> > EventListener.handleEvent*StorageUI@resource://gre/modules/commonjs/toolkit/
> > loader.js -> resource://devtools/client/storage/ui.js:86:3
> > StoragePanel.prototype.open/<@resource://gre/modules/commonjs/toolkit/loader.
> > js -> resource://devtools/client/storage/panel.js:54:17
> > Handler.prototype.process@resource://gre/modules/Promise.jsm ->
> > resource://gre/modules/Promise-backend.js:937:23
> > this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm ->
> > resource://gre/modules/Promise-backend.js:816:7
> > Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.
> > jsm -> resource://gre/modules/Promise-backend.js:747:11
> > this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm ->
> > resource://gre/modules/Promise-backend.js:779:7
> > Promise.prototype.then@resource://gre/modules/Promise.jsm ->
> > resource://gre/modules/Promise-backend.js:454:5
> > StoragePanel.prototype.open@resource://gre/modules/commonjs/toolkit/loader.
> > js -> resource://devtools/client/storage/panel.js:50:12
> > Toolbox.prototype.loadTool/onLoad@resource://gre/modules/commonjs/toolkit/
> > loader.js -> resource://devtools/client/framework/toolbox.js:1253:19
> 
> From what I can make out, if you have something in the searchbox and switch
> to another table e.g. local storage then it will throw this error. I have
> left a comment below explaining what should be done about that.
Thanks!

> One thing that we will want to address at some point would be to make it
> work with long lists of items. At the moment I think we show 50 items and,
> as the user scrolls down, we show the next 50 (endless scrolling). Making
> the search work with this kind of thing is a much harder so it should be a
> future bug.
Will file a follow up.

> How does your filtering work when columns are hidden?
It actually takes in account the hidden columns, which is actually odd, I will make sure to exclude the hidden columns in my next iterations.

> @@ +651,5 @@
> >  
> >    /**
> > +   * Handles filtering the table
> > +   */
> > +  filterItems() {
> 
> Surprised that works... it should be:
> filterItems: function() {
It's ES6 notation. Let me know if you'd like me to use the full notation.

> ::: devtools/client/themes/widgets.css
> @@ +1240,5 @@
> >    -moz-user-focus: normal;
> >    color: var(--theme-body-color);
> >  }
> >  
> > +.table-widget-cell[hidden] {
> 
> I don't think you need this because there is already a hidden attribute in
> XUL that hides stuff.
Without this rule the cells don't hide at all.

Comment 5

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8724447 [details] [diff] [review]
Patch

(In reply to Tim Nguyen [:ntim] from comment #1)
> Helen, right now, all the rows where at least one column contains the query
> are shown. I'm not sure on whether it should work that way or not. It may be
> more natural to only take in account the key and the value columns. What do
> you think ?

I _think_ the way you've done it makes sense—if a string is captured by the filter query, it should show up, no?

I have a few thoughts on the table styling, just since playing with the filter makes gaps more obvious:

- I think the table might look nicer if the top labels have that same gray-border underneath them. Right now they float a bit: http://cl.ly/0m452J1a1O3A
- If you happen to filter for what the last row is, it has a border-bottom. If not, it does. It's just an odd discrepancy—I think it might be safe visually to do away with that bottom-border all together for consistency. http://cl.ly/3w1N1s2R470r vs. http://cl.ly/1Q211I1R1N0l (looks odd with only one search result, but I imagine most people playing with this don't have only four things they randomly generated off of an HTML5 localStorage-demo thing).

Comment 6

[Tim Nguyen :ntim]

Attachment: Part 1 - Basic search functionality

Comment 7

[Tim Nguyen :ntim]

Attachment: Part 2 - Make sure the "flash-out" class is removed once the row has flashed

This fixes the following case:
- Manually add a storage entry after having open the storage tool
- See the item flash
- Filter the table and make sure your filter hides the flashed item
- clear the filter
- see the item flash again (it shouldn't happen)

Comment 8

[Tim Nguyen :ntim]

Attachment: Part 3 - Fix keyboard navigation for filtered tables

Comment 9

[Tim Nguyen :ntim]

These parts are coming soon:
- Part 4 - Address Helen's UX comments
- Part 5 - Tests

Comment 10

[Tim Nguyen :ntim]

Attachment: Part 4 - Address UX comments

Comment 11

[Tim Nguyen :ntim]

Comment on attachment 8724837 [details] [diff] [review]
Part 4 - Address UX comments

Helen, you only need to apply Part 1 and Part 4. Part 2 and 3 are not necessary (they do improve things a bit though)

Comment 12

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

Comment on attachment 8724714 [details] [diff] [review]
Part 1 - Basic search functionality

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

r+ assuming that you localize the textbox placeholder.

::: devtools/client/storage/storage.xul
@@ +22,5 @@
>      <splitter class="devtools-side-splitter"/>
> +    <vbox flex="1">
> +      <hbox id="storage-toolbar" class="devtools-toolbar">
> +        <!-- TODO: localize "Filter items" -->
> +        <textbox id="storage-searchbox" class="devtools-searchinput" type="search" placeholder="Filter items"/>

Still needs localizing.

Comment 13

[Tim Nguyen :ntim]

Attachment: Part 1 - Basic search functionality

With localized placeholder.

Comment 14

[Tim Nguyen :ntim]

Attachment: Part 5 - Test

Comment 15

[Tim Nguyen :ntim]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=684fc70a3609

Comment 16

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8724837 [details] [diff] [review]
Part 4 - Address UX comments

I have one UI comment and the rest is just stuff I noticed that felt odd:

— I notice when you receive multiple results after filtering that we lose our white-gray-white-gray backgrounds. Since that's helpful for readability it'd be nice to make sure that we keep those styles on filter: http://cl.ly/300f0U3T2B3k
— If I fiddle too much with the filter bar, sometimes (seemingly) random results will flash orange. There doesn't seem to be any rhyme or reason to it and I have no idea if it's related to your patches: http://cl.ly/2N190u0a2n3m (wait for it....) This could be intended functionality that I just don't understand (which, arguably, is a different problem, but if not related to this work let's file a different bug).
— If I jump around between categories on the left (say, localStorage > Cookies > localStorage) and I had a filter applied in localStorage, it remains in the filter area but without filtering anything. It might be best to clear it: http://cl.ly/3o0M1h382P0M

I definitely feel like we could make some nice updates to the tables in general (and with filtering) but I'll try to do something more formal in bug 1170617 at a later point.

Comment 17

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

Regarding Helen's comments about Zebra Striping.

We have two choices:
1. Add classes to each row as you want them to appear.
2. Remove each "row" from the DOM and keep a cache that can reattach them in the correct place.

I suspect that 1 is our best option here.

I really wish this was an HTML table.

Comment 18

[Tim Nguyen :ntim]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #16)
> — I notice when you receive multiple results after filtering that we lose
> our white-gray-white-gray backgrounds. Since that's helpful for readability
> it'd be nice to make sure that we keep those styles on filter:
> http://cl.ly/300f0U3T2B3k
Will fix it by applying classes.

> — If I fiddle too much with the filter bar, sometimes (seemingly) random
> results will flash orange. There doesn't seem to be any rhyme or reason to
> it and I have no idea if it's related to your patches:
> http://cl.ly/2N190u0a2n3m (wait for it....) This could be intended
> functionality that I just don't understand (which, arguably, is a different
> problem, but if not related to this work let's file a different bug).
Part 2 fixes this issue.

> — If I jump around between categories on the left (say, localStorage >
> Cookies > localStorage) and I had a filter applied in localStorage, it
> remains in the filter area but without filtering anything. It might be best
> to clear it: http://cl.ly/3o0M1h382P0M
The current patch should re-filter the table, but I'll make sure to clear the filter in my next patch

Comment 19

[Tim Nguyen :ntim]

Attachment: Part 6 - Properly update zebra stripes

Comment 20

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

Comment on attachment 8725225 [details] [diff] [review]
Part 6 - Properly update zebra stripes

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

Looking good!

Comment 21

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Tim Nguyen [:ntim] from comment #18)
> > — If I fiddle too much with the filter bar, sometimes (seemingly) random
> > results will flash orange. There doesn't seem to be any rhyme or reason to
> > it and I have no idea if it's related to your patches:
> > http://cl.ly/2N190u0a2n3m (wait for it....) This could be intended
> > functionality that I just don't understand (which, arguably, is a different
> > problem, but if not related to this work let's file a different bug).
> Part 2 fixes this issue.
I had part 2 applied and was still seeing this. Might be something local?

Comment 22

[Tim Nguyen :ntim]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #21)
> (In reply to Tim Nguyen [:ntim] from comment #18)
> > > — If I fiddle too much with the filter bar, sometimes (seemingly) random
> > > results will flash orange. There doesn't seem to be any rhyme or reason to
> > > it and I have no idea if it's related to your patches:
> > > http://cl.ly/2N190u0a2n3m (wait for it....) This could be intended
> > > functionality that I just don't understand (which, arguably, is a different
> > > problem, but if not related to this work let's file a different bug).
> > Part 2 fixes this issue.
> I had part 2 applied and was still seeing this. Might be something local?

Looking at the screencast more closely, it seems like CNN is setting the value twice, once on the load time, and once afterwards. The flash only reflects that the row has been set/updated.

Comment 23

[Tim Nguyen :ntim]

Attachment: Part 1 - Basic search functionality

Now clears the input on host change.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a603b4bd2d3f
https://hg.mozilla.org/integration/fx-team/rev/355c766e71dd
https://hg.mozilla.org/integration/fx-team/rev/3e7920e7ba9b
https://hg.mozilla.org/integration/fx-team/rev/b8ef51241262
https://hg.mozilla.org/integration/fx-team/rev/bf4ae14158c3
https://hg.mozilla.org/integration/fx-team/rev/ad9e05f2447a

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a603b4bd2d3f
https://hg.mozilla.org/mozilla-central/rev/355c766e71dd
https://hg.mozilla.org/mozilla-central/rev/3e7920e7ba9b
https://hg.mozilla.org/mozilla-central/rev/b8ef51241262
https://hg.mozilla.org/mozilla-central/rev/bf4ae14158c3
https://hg.mozilla.org/mozilla-central/rev/ad9e05f2447a

Comment 26

[Will Bamberg [:wbamberg]]

docs -> https://developer.mozilla.org/en-US/docs/Tools/Storage_Inspector#Table_Widget

Please let me know if this covers it.

Comment 4 says that search excludes hidden columns, but I am not seeing that.

Comment 27

[Tim Nguyen :ntim]

(In reply to Will Bamberg [:wbamberg] from comment #26)
> docs ->
> https://developer.mozilla.org/en-US/docs/Tools/Storage_Inspector#Table_Widget
> 
> Please let me know if this covers it.
Thanks, perfectly covers it.

> Comment 4 says that search excludes hidden columns, but I am not seeing that.
Totally forgot to address that, filed bug 1257603

Comment 28

[Sebastian Zartner [:sebo]]

Works fine for me (disregarding bug 1257603). Thank you!

Sebastian