Table headers are not removed when selecting an empty storage

VERIFIED FIXED in Firefox 68

Status

defect
P2
normal
VERIFIED FIXED
3 years ago
6 days ago

People

(Reporter: sebo, Assigned: avi.mathur.engg+github, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug})

Trunk
Firefox 68
Dependency tree / graph

Firefox Tracking Flags

(firefox51 wontfix, firefox68 verified)

Details

(Whiteboard: [todo-mr][t1][lang=js])

Attachments

(3 attachments)

Reporter

Description

3 years ago
As of bug 1264582:

The table headers of the previously selected storage are still shown when selecting a storage, which doesn't contain any items.

Steps to reproduce:
1. Open the Storage Inspector on this page
2. Expand 'Cookies' and select 'bugzilla.mozilla.org'
   => Some cookies will be shown in a table with the headers 'Name', 'Path', ..., 'isHttpOnly'
3. Select 'Local Storage'

=> The headers of the 'Cookies' table are still shown.

You'd expect that the table headers are hidden when there are no entries.

Sebastian
Reporter

Updated

3 years ago
See Also: → 1309824
Whiteboard: [papercut-mr]
Filter on Brobdingnagian.
Whiteboard: [papercut-mr] → [todo-mr]
Filter on HOTFROG.
Whiteboard: [todo-mr] → [todo-mr][t1]
Reporter

Comment 3

2 years ago
I assume that should be easy to fix. So, adding 'good-first-bug'.

Sebastian
Keywords: good-first-bug
Reporter

Comment 4

2 years ago
Mike, I've added you as mentor, if that's ok. I may tackle this bug myself at some point if no one else steps up.

Sebastian
Mentor: mratcliffe
Whiteboard: [todo-mr][t1] → [todo-mr][t1][lang=js]
Not seeing this on Nightly 57.0a1 64-bit on linux.
Reporter

Comment 8

2 years ago
Your second screenshot shows the error, the headers of the 'Cookies' table ('Name', 'Domain', 'Path', etc.) are shown at the top of the panel.

Sebastian
Oh, sorry, only on my second look-through do I see the word "headers." I'm not sure that I agree they shouldn't show, but I'll take a look at figuring out a patch if that's the consensus.
Also of note: The + button still appears when the panel is first opened. Should I open a new bug for this?
Why do we allow clicking on the host at all? If it never displays any data, what's the point? I think the better solution would be to make clicking the host either a) select the first subitem or b) expand/collapse the element. Thoughts?

Anyway, I was trying to figure out how to hide the tree headers, and I can't seem to figure it out. I think it needs to be added in ui.js at line 606 [1], surrounded in `if (howManyNodesIn <= 1) { ... }`. I have tried `this.table.clear()`.

[1] https://searchfox.org/mozilla-central/source/devtools/client/storage/ui.js#606
Flags: needinfo?(mratcliffe)
(In reply to Christopher Phonis-Phine from comment #11)
> Why do we allow clicking on the host at all? If it never displays any data,
> what's the point? I think the better solution would be to make clicking the
> host either a) select the first subitem or b) expand/collapse the element.
> Thoughts?
> 
> Anyway, I was trying to figure out how to hide the tree headers, and I can't
> seem to figure it out. I think it needs to be added in ui.js at line 606
> [1], surrounded in `if (howManyNodesIn <= 1) { ... }`. I have tried
> `this.table.clear()`.
> 
> [1]
> https://searchfox.org/mozilla-central/source/devtools/client/storage/ui.
> js#606

If there is no data then clicking on the host should really show a panel saying "There is no data for the selected host."

The host is selectable because in the future we plan on showing data relating to the storage type when it is clicked e.g. when an indexedDB host is selected we can show the number of databases, tables etc. along with their sizes and remaining capacities.
Flags: needinfo?(mratcliffe)
Reporter

Comment 13

2 years ago
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #12)
> (In reply to Christopher Phonis-Phine from comment #11)
> > Why do we allow clicking on the host at all? If it never displays any data,
> > what's the point? I think the better solution would be to make clicking the
> > host either a) select the first subitem or b) expand/collapse the element.
> > Thoughts?
> 
> If there is no data then clicking on the host should really show a panel
> saying "There is no data for the selected host."
> 
> The host is selectable because in the future we plan on showing data
> relating to the storage type when it is clicked e.g. when an indexedDB host
> is selected we can show the number of databases, tables etc. along with
> their sizes and remaining capacities.

See also bug 1102716 and bug 1309824.

Sebastian
I thing bug 1102716 makes the most sense, and either way when either of those are resolved this bug becomes irrelevant.

Comment 15

2 years ago
Hey is it okay if i take care of this bug? Im a student in an open-source class and im looking to start off in this community
(In reply to Parth from comment #15)
> Hey is it okay if i take care of this bug? I'm a student in an open-source
> class and im looking to start off in this community

Sure, just ping me if you need any help.

I have assigned you to this bug.
Assignee: nobody → ppatel221
Reporter

Updated

2 years ago
Status: NEW → ASSIGNED

Comment 17

Last year
Hi, this bug has already been assigned, but it's been 5 months and I'm not sure if it's been fixed. I am new here and would like to work on this!
Reporter

Comment 18

Last year
As Parth was not active on Bugzilla for half a year, I think it is fine to assign this bug to you, Brian.
Thank you for picking this up!

Sebastian
Assignee: ppatel221 → brianf.luk

Updated

Last year
Product: Firefox → DevTools

This bug has not been updated in the last 6 months. Resetting the assignee field.
Please, feel free to pick it up again and add a comment outlining your plans for it if you do still intend to work on it.
This is just trying to clean our backlog of bugs and make bugs available for people.

Assignee: brianf.luk → nobody
Status: ASSIGNED → NEW

hi, I would like to work on this bug. I am new to this code base and I don't know where to start. Can someone help me to work on this bug?

Assignee

Comment 21

4 months ago

Hello, I would also like to help with this. If some one can advice where to look for.

Assignee

Comment 22

4 months ago

Mike I have fixed this bug, please advice me how to proceed ?

Flags: needinfo?(mratcliffe)
Assignee: nobody → avi.mathur.engg+github
Flags: needinfo?(mratcliffe)

(In reply to avi.mathur.engg+github from comment #22)

Mike I have fixed this bug, please advice me how to proceed ?

Hi, so you have fixed the bug and have a patch ready for review? That's great!

Please work through the instructions here to get set up to request a code review.
https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

In a notshell ():

  • Go to https://phabricator.services.mozilla.com/
  • Click Log in
  • Register and log in.
  • Install arc and generate a certificate using arc install-certificate
  • Install moz-phab
  • Ensure your patch commit message looks like this: Bug 1291427 - Table headers are not removed when selecting an empty storage r?mratcliffe
  • moz-phab submit <revisionNumber>

If you need any more help you are best to ping me in the #general channel of https://devtools-html.slack.com/

Assignee

Comment 24

4 months ago

Before this change clicking on Storage Type use to show table headers from previous selection. Now clicking on Storage Type will reset table headers also in case empty table headers will get cleared.

Assignee

Updated

3 months ago
Status: NEW → ASSIGNED

Comment 25

3 months ago
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c8a4f2586fc0
Table headers are not removed when selecting an empty storage r=miker

Comment 26

3 months ago

Backed out changeset c8a4f2586fc0 (bug 1291427) for devtools on browser_storage_cookies_edit.js on a CLOSED TREE

Backout: https://hg.mozilla.org/integration/autoland/rev/d3d719ba069153ae29a99f60ec537133345c7ff0

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=superseded%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&selectedJob=232670539&searchStr=linux%2Cx64%2Cdebug%2Cmochitests%2Cwith%2Ce10s%2Ctest-linux64%2Fdebug-mochitest-devtools-chrome-e10s-3%2Cm-e10s%28dt3%29&revision=c8a4f2586fc01b1e13303f55eaa096c306674416

Failure log:[task 2019-03-08T15:11:58.750Z] 15:11:58 INFO - TEST-PASS | devtools/client/storage/test/browser_storage_cookies_edit.js | value column has the right value for newTest3{9d414cc5-8319-0a04-0586-c0a6ae01670a}test1.example.org{9d414cc5-8319-0a04-0586-c0a6ae01670a}/ -
[task 2019-03-08T15:11:58.751Z] 15:11:58 INFO - Typing true
[task 2019-03-08T15:11:58.752Z] 15:11:58 INFO - Pressing KEY_Enter
[task 2019-03-08T15:11:58.753Z] 15:11:58 INFO - Validating results... waiting for ROW_EDIT event.
[task 2019-03-08T15:11:58.755Z] 15:11:58 INFO - Buffered messages finished
[task 2019-03-08T15:11:58.756Z] 15:11:58 INFO - TEST-UNEXPECTED-FAIL | devtools/client/storage/test/browser_storage_cookies_edit.js | Test timed out -
[task 2019-03-08T15:11:58.759Z] 15:11:58 INFO - GECKO(3030) | ++DOMWINDOW == 16 (0x7f69eec5e400) [pid = 3030] [serial = 78] [outer = 0x7f69ef50c3e0]
[task 2019-03-08T15:11:58.760Z] 15:11:58 INFO - GECKO(3030) | console.warn: "Error while detaching the thread front: 'detach' request packet to 'server1.conn10.child1/context22' can't be sent as the connection is closed."
[task 2019-03-08T15:11:59.192Z] 15:11:59 INFO - Removing tab.
[task 2019-03-08T15:11:59.193Z] 15:11:59 INFO - Waiting for event: 'TabClose' on [object XULElement].
[task 2019-03-08T15:11:59.254Z] 15:11:59 INFO - Got event: 'TabClose' on [object XULElement].
[task 2019-03-08T15:11:59.290Z] 15:11:59 INFO - Tab removed and finished closing
[task 2019-03-08T15:11:59.372Z] 15:11:59 INFO - GECKO(3030) | MEMORY STAT | vsize 2049MB | residentFast 323MB | heapAllocated 103MB
[task 2019-03-08T15:11:59.376Z] 15:11:59 INFO - TEST-OK | devtools/client/storage/test/browser_storage_cookies_edit.js | took 91507ms
[task 2019-03-08T15:11:59.392Z] 15:11:59 INFO - GECKO(3030) | ++DOCSHELL 0x7fcebe53c800 == 1 [pid = 3187] [id = {d3742d67-5344-49e6-b7f1-f059b8a7eb9b}]
[task 2019-03-08T15:11:59.396Z] 15:11:59 INFO - GECKO(3030) | ++DOMWINDOW == 1 (0x7fcebe9d9d40) [pid = 3187] [serial = 17] [outer = (nil)]
[task 2019-03-08T15:11:59.460Z] 15:11:59 INFO - GECKO(3030) | ++DOMWINDOW == 2 (0x7fcebe583400) [pid = 3187] [serial = 18] [outer = 0x7fcebe9d9d40]
[task 2019-03-08T15:11:59.488Z] 15:11:59 INFO - checking window state

Flags: needinfo?(avi.mathur.engg+github)

This didn't stay landed for very long, never mind, it happens.

Can you go to Phabricator and address my comment?

Assignee

Comment 28

3 months ago

Done. Is this okay ?

Flags: needinfo?(avi.mathur.engg+github)
Assignee

Comment 29

3 months ago

Hello Mike, I have made those changes. Please review.
Tried contacting you over IRC however I can't seem to find you. That's why commenting here.
The slack channels you have provided only allows mozilla.com accounts let me know if I can create one to join the slack channel.

Flags: needinfo?(mratcliffe)
Attachment #9048896 - Attachment description: Bug 1291427 - Table headers are not removed when selecting an empty storage r?mratcliffe → Bug 1291427 - Remove table headers when selecting an empty storage type r?mratcliffe
Flags: needinfo?(mratcliffe)

Comment 31

3 months ago
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae39470fc216
Remove table headers when selecting an empty storage type r=miker

Comment 32

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Reporter

Comment 33

3 months ago

Looks good to me on Nightly 68.0a1 (2019-03-22). Thank you for fixing this!

Sebastian

Status: RESOLVED → VERIFIED
Flags: qe-verify+

I have reproduced this issue using Firefox 51.0a1 Win 10 x64.
I can confirm this issue is fixed, I verified using Firefox 68.0b7 on Win 10 x64, macOS 10.13.6 and Ubuntu 18.04 x64.

Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.