Closed Bug 1224115 Opened 4 years ago Closed 4 years ago

Allow to search within Storage Inspector

Categories

(DevTools :: Storage Inspector, enhancement)

enhancement
Not set

Tracking

(firefox47 fixed)

VERIFIED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: sebo, Assigned: ntim)

References

Details

(Keywords: dev-doc-complete)

Attachments

(6 files, 3 obsolete files)

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

Sebastian
Assignee: nobody → ntim.bugs
Blocks: 1194190
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
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 ?
Attachment #8724447 - Flags: ui-review?
Attachment #8724447 - Flags: feedback?(mratcliffe)
Attachment #8724447 - Flags: ui-review? → ui-review?(hholmes)
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 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.
Attachment #8724447 - Flags: feedback?(mratcliffe) → feedback+
(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 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).
Attachment #8724447 - Flags: ui-review?(hholmes)
Attachment #8724447 - Attachment is obsolete: true
Attachment #8724714 - Flags: review?(mratcliffe)
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)
Attachment #8724717 - Flags: review?(mratcliffe)
Attachment #8724718 - Flags: review?(mratcliffe)
These parts are coming soon:
- Part 4 - Address Helen's UX comments
- Part 5 - Tests
Attachment #8724837 - Flags: review?(mratcliffe)
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)
Attachment #8724837 - Flags: ui-review?(hholmes)
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.
Attachment #8724714 - Flags: review?(mratcliffe) → review+
Attachment #8724717 - Flags: review?(mratcliffe) → review+
Attachment #8724718 - Flags: review?(mratcliffe) → review+
Attachment #8724837 - Flags: review?(mratcliffe) → review+
With localized placeholder.
Attachment #8724714 - Attachment is obsolete: true
Attachment #8724997 - Flags: review+
Attached patch Part 5 - TestSplinter Review
Attachment #8725023 - Flags: review?(mratcliffe)
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.
Attachment #8724837 - Flags: ui-review?(hholmes) → feedback+
Attachment #8725023 - Flags: review?(mratcliffe) → review+
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.
(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
Attachment #8725225 - Flags: review?(mratcliffe)
Comment on attachment 8725225 [details] [diff] [review]
Part 6 - Properly update zebra stripes

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

Looking good!
Attachment #8725225 - Flags: review?(mratcliffe) → review+
(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?
(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.
Now clears the input on host change.
Attachment #8724997 - Attachment is obsolete: true
Attachment #8725265 - Flags: review+
Keywords: dev-doc-needed
Depends on: 1253174
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.
Depends on: 1257603
(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
Works fine for me (disregarding bug 1257603). Thank you!

Sebastian
Status: RESOLVED → VERIFIED
Blocks: 1031189
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.