Restrict the height of console.table

RESOLVED FIXED in Firefox 59

Status

P1
enhancement
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: nchevobbe, Assigned: Towkir, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 59
good-first-bug

Firefox Tracking Flags

(firefox57 wontfix, firefox58 wontfix, firefox59 fixed)

Details

(Whiteboard: [newconsole-mvp])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
If you pass a large dataset to console.table, the table itself can be pretty tall and take the whole vertical space of the console (see attachment).
It would be nice if we restrict the maximum height of the table, like we do for the HTTPinspector in the console, still providing a way to scroll the table and see the whole dataset of course.

Either the body only could be scrollable, or the table itself if we apply sticky to the thead.
(Reporter)

Updated

2 years ago
Priority: -- → P3
(Reporter)

Updated

2 years ago
Whiteboard: [reserve-console-html]
Priority: P3 → P2
Whiteboard: [reserve-console-html] → [newconsole-mvp]
(Reporter)

Updated

a year ago
Mentor: nchevobbe
Severity: normal → enhancement
Keywords: good-first-bug
(Assignee)

Comment 1

a year ago
Hi Nicolas, 
Looks like this is a good first one.
I would like to take this and...
Could you please suggest some steps to do this, though you already suggested some ideas, still how exactly you would like to get this done.
Beside, also suggest an way to load a long data on the console in order to reproduce the state as the attached screenshot, I think I can then figure out the rest.
Thanks
Flags: needinfo?(nchevobbe)
(Reporter)

Comment 2

a year ago
Hello Ahmed, 

To have a long table, you can open the console in this page, and evaluate `console.table($$("*"))` (populate a table with all the element of the page). It should be long enough :)

So ideas, yes, first thing, you can simply put a `max-height` in webconsole.css, like we do for the netmonitor https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/devtools/client/themes/webconsole.css#1021

Something like this would work: 
```
max-height: 250px;
overflow-y: auto;
display: block;
```

Bonus point if you can get the table thead to always be visible (see if position:sticky could work here).

Thanks !
Flags: needinfo?(nchevobbe)
(Assignee)

Comment 3

a year ago
Hi Nicolas, 
I tried setting your rules at [1] and the fixed height is applied, but looks like position sticky does not work on thead elements or other table elements, 
Do you want me to figure something else ? or do you have anything specific in mind ?

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/webconsole.css#1083
Flags: needinfo?(nchevobbe)
(Reporter)

Comment 4

a year ago
Hello Ahmed, thanks for working on this.

Yeah, I read about non-sticky thead, that's a bummer.
Could you post a patch to review so I can play with it, see if it's fine like that ?

The solution to our issue would be to handle everything with divs and CSS grid.

The markup would need to be transformed to something like this : 
```
<div class="console-table"  role="grid" style="grid-template-columns: repeat(3, 1fr)">
  <div class="table-header" role="columnheader">Header 1</div>
  <div class="table-header" role="columnheader">Header 2</div>
  <div class="table-header" role="columnheader">Header 3</div>

  <div class="table-cell" role="columnheader">row 1 - column 1</div>
  <div class="table-cell" role="columnheader">row 1 - column 2</div>
  <div class="table-cell" role="columnheader">row 1 - column 3</div>

  <div class="table-cell" role="columnheader">row 2 - column 1</div>
  <div class="table-cell" role="columnheader">row 2 - column 2</div>
  <div class="table-cell" role="columnheader">row 2 - column 3</div>
</div>
```

It means we'd have to change the ConsoleMessage [1] component to reflect this markup, removing thead tbody and tr , and migrating th and td to divs.

Note the `grid-template-columns: repeat(3, 1fr)`, here the `3` should be dynamic and reflect the actual number of columns we have.


I believe this can be done in react,  in the ConsoleTable component [1] with
```
  dom.div({
    className: "console-table", 
    style: {
      gridTemplateColumns: `repeat(${columns.length}, 1fr)`
    }
  })
```

This way we could then apply stickyness to columnheader (see https://jsfiddle.net/tu25enz4/ for an example)

[1] https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/devtools/client/webconsole/new-console-output/components/ConsoleTable.js#100
Flags: needinfo?(nchevobbe)
hey, i am new to open source, can i give it a go??
(Assignee)

Comment 6

a year ago
(In reply to kumar.abhishek3031 from comment #5)
> hey, i am new to open source, can i give it a go??
Thanks for your interest Kumar, I am currently working on this.
You can find more good-first bugs here: 
https://www.joshmatthews.net/bugsahoy
http://bugs.firefox-dev.tools
(Assignee)

Comment 7

a year ago
Hi Nicolas,
The DOM structure you suggested would look like this:

+---------------------+-----------------------+
|   header-1   div-1  |    header-2  div-2    |
+---------------------+-----------------------+
|  row-1 col-1 div-3  |  row-1 col-2  div-4   |
+---------------------+-----------------------+
|  row-2 col-1 div-5  |  row-2 col-2  div-6   |
+---------------------+-----------------------+
|  row-2 col-1 div-7  |  row-2 col-2  div-8   |
+---------------------+-----------------------+
|  row-2 col-1 div-9  |  row-2 col-2  div-10  |
+---------------------+-----------------------+

Notice the div number, all the odd number div's are in the same column, in which case, we might have difficulties to apply the zebra styled colors on rows. I think wrapping each pair of div's into one div (row) would be better.

What do you say about it ?
If you allow me the freedom of modifying this, I think I can submit a nice solution as we have come out of the table element already.

One more thing, 
On the react ConsoleTable component,   gridTemplateColumns: `repeat(${columns.length}, 1fr)`   did not work, 
I am not that familiar with reactjs yet, I get a div with a blank style attribute  (style="")
Any idea why is that happening ?

I could submit a patch but it would not be enough yet to play with.
Once you provide feedback on those, I will submit one.
Meanwhile assigning this.
Assignee: nobody → 3ugzilla
Status: NEW → ASSIGNED
Flags: needinfo?(nchevobbe)
(Assignee)

Comment 8

a year ago
Ooops, My bad.
Ignore the previous wireframe, 
Have a look at here, this one has correct numbers on the cells:
+---------------------+-----------------------+
|   header-1   div-1  |    header-1  div-2    |
+---------------------+-----------------------+
|  row-1 col-1 div-3  |  row-1 col-2  div-4   |
+---------------------+-----------------------+
|  row-2 col-1 div-5  |  row-2 col-2  div-6   |
+---------------------+-----------------------+
|  row-3 col-1 div-7  |  row-3 col-2  div-8   |
+---------------------+-----------------------+
|  row-4 col-1 div-9  |  row-4 col-2  div-10  |
+---------------------+-----------------------+
Priority: P2 → P1
(Reporter)

Comment 9

a year ago
Hello Ahmed, 

Sadly we can't do this. In order for the cell to be managed by the css grid, they have to be direct children of the table container. But, we can put className attribute like "odd" and "even" on cells in order to style them properly.

I do think "style: {gridTemplateColumns: `repeat(${columns.length}, 1fr)`}" should work (see https://codepen.io/nchevobbe/pen/bYZEqq?editors=0110, even if it's jsx)
Flags: needinfo?(nchevobbe)
(Assignee)

Comment 10

a year ago
Hi Nicolas,
This is not a complete patch, submitting so that you can have a look, play with it and suggest something.
BTW, the odd and even class adding approach should be controlled from JS right ?
Could you show a trick to do that please ?

Thanks
Attachment #8934408 - Flags: feedback?(nchevobbe)
(Reporter)

Comment 11

a year ago
Comment on attachment 8934408 [details] [diff] [review]
new-consoletable-approach.diff

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

Thanks Ahmed, 
this still need some work, but it's going into the right direction. See my comments

::: devtools/client/themes/webconsole.css
@@ +1108,5 @@
>    border: 1px solid var(--consoletable-border);
> +  background-color: var(--theme-toolbar-background);
> +  color: var(--theme-body-color);
> +  margin: 0;
> +  padding: 8px 4px 3px;

the padding looks a bit weird
also, when you scroll the table cells arrow are visible behind the header http://prntscr.com/hjb9m4

we should also put a border on the root element

@@ +1116,5 @@
> +.new-consoletable-header + .new-consoletable-header {
> +  border-left: 0;
> +}
> +
> +.new-consoletable-rows {

we won`t have rows i think

@@ +1122,3 @@
>  }
>  
>  .new-consoletable tbody {

we can remove the table elements style, since we are not going to use them

::: devtools/client/webconsole/new-console-output/components/ConsoleTable.js
@@ +50,3 @@
>  
>    getHeaders(columns) {
>      let headerItems = [];

this is not related to your patch, but we could have something better : 

return columns.map(column => dom.div({ role: "columnheader" }, column))

@@ +64,1 @@
>        let cells = [];

same thing here, let's map instead of creating intermediate variables: 

return columns.map((_, index) => dom.div({
  role: "gridcell",
  className: (Math.sin(((i + 0.001) * Math.PI) / number) > 0 ? "even" : "odd"
}, GripMessageBody({…})));

note the use of Math.sin to get odd and even elements

@@ +99,5 @@
>      );
>  
>      return (
> +      dom.div({
> +        className: "new-consoletable",

we should also have `role: "grid"`
Attachment #8934408 - Flags: feedback?(nchevobbe) → feedback+
(Assignee)

Comment 12

a year ago
Hi,
Here is the patch, I left some commented codes both in JS and CSS.
Once you test out the rest of it, and make the map functions work, I will do a nit and submit a clean patch again, 
Hope this helps :) 

Thanks
Attachment #8935050 - Flags: review?(nchevobbe)
(Reporter)

Comment 13

a year ago
Comment on attachment 8935050 [details] [diff] [review]
console-table-height-restricted.patch

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

Thanks Ahmed, I have been playing with it and it works quite well.
I think we should have `repeat(${columns.size}, auto)` instead of 1fr, it looks better (columns expand as they need)

See my inline comments for small suggestions.
Also, could you have a look at pushing to MozReview next (http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html) time please ? It makes the whole workflow easier for both you and me :)

::: devtools/client/themes/webconsole.css
@@ +1094,5 @@
>    --consoletable-border: var(--theme-splitter-color);
>    margin-block-end: var(--attachment-margin-block-end);
>    color: var(--theme-body-color);
> +  display: grid;
> +  height: 250px;

i'd put this as the max-height, so it still can be smaller

@@ +1100,2 @@
>    border: 1px solid var(--consoletable-border);
> +  border-left: none;

add a comment before to say that the border will be put by the cells

@@ +1112,4 @@
>    background-color: var(--theme-toolbar-background);
>    color: var(--theme-body-color);
>    margin: 0;
> +  padding: 5px 4px;

can the padding stay the same as it was before ?

@@ +1118,3 @@
>  }
>  
> +div[role=gridcell] {

.new-consoletable [role=gridcell] would be slightly better imo

@@ +1121,5 @@
> +  background-color: var(--theme-body-background);
> +  padding: 3px 4px;
> +  min-width: 100px;
> +  color: var(--theme-body-color);
> +  /*height: 1.25em;*/

why are those commented?

@@ +1127,3 @@
>  }
>  
> +div[role=gridcell].even {

use the selector (+ "odd") than for the previous rule

::: devtools/client/webconsole/new-console-output/components/ConsoleTable.js
@@ +54,2 @@
>      return headerItems;
> +    // return columns.map(column => dom.div({ className: "new-consoletable-header", role: "columnheader" }, column))

let's remove this

@@ +67,5 @@
>          cells.push(
> +          dom.div(
> +            {
> +              role: "gridcell",
> +              className: (index % 2) ? "odd" : "even"

i think it's the opposite ? when the rest of / 2 is 0, we have an even number
Attachment #8935050 - Flags: review?(nchevobbe) → review-
Comment hidden (mozreview-request)
(Reporter)

Comment 15

a year ago
mozreview-review
Comment on attachment 8935318 [details]
Bug 1399897 - restricted height for the console table, reimplemented the table with divs;

https://reviewboard.mozilla.org/r/206218/#review211870

I only have a few comments, which should be easy to address. You can do so by amending your commit and push to mozreview again

::: devtools/client/themes/webconsole.css:1121
(Diff revision 1)
> -  padding: 5px 0 0;
> +  padding: 5px 4px;
>    font-weight: inherit;
> +  z-index: 1;
>  }
>  
> -.new-consoletable th:not(:last-of-type) {
> +.new-consoletable [role=gridcell] {

they should be direct children (.new-consoletable > [role=gridcell])

::: devtools/client/themes/webconsole.css
(Diff revision 1)
> -  vertical-align: top;
>  }

ditto

::: devtools/client/webconsole/new-console-output/components/ConsoleTable.js:53
(Diff revision 1)
>      dispatch(actions.messageTableDataGet(id, client, dataType));
>    }
>  
>    getHeaders(columns) {
>      let headerItems = [];
> -    columns.forEach((value, key) => headerItems.push(dom.th({}, value)));
> +    columns.forEach((value, key) => headerItems.push(dom.div({className: "new-consoletable-header", role: "columnheader"}, value)));

this line is too ling and will be seen as an error by eslint.
if you haven't yet, i encourage you to have a look at http://docs.firefox-dev.tools/contributing/eslint.html

in the meantime, we can probably split the line like 

columns.forEach((value, key) => headerItems.push(dom.div({
    className: "new-consoletable-header", 
    role: "columnheader"
  }, value))
);
Attachment #8935318 - Flags: review?(nchevobbe) → review+
Comment hidden (mozreview-request)
(Reporter)

Comment 17

a year ago
Looks like a test is failing https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d0c28c5fdcbdba504381e40384b5b364b6bcdc1&selectedJob=152167531 

You can run it with `./mach test devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_console_table.js`
(Reporter)

Updated

a year ago
Attachment #8934408 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8935050 - Attachment is obsolete: true
(Reporter)

Comment 18

a year ago
hey Ahmed, do you need help to debug this test failure ?
Flags: needinfo?(3ugzilla)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 21

a year ago
mozreview-review
Comment on attachment 8940679 [details]
Bug 1399897 - Restrict the height of console table and add sticky headers;

https://reviewboard.mozilla.org/r/210928/#review216666

Carrying r+ from https://bugzilla.mozilla.org/attachment.cgi?id=8935318
Attachment #8940679 - Flags: review?(nchevobbe) → review+
(Reporter)

Comment 22

a year ago
Comment on attachment 8935318 [details]
Bug 1399897 - restricted height for the console table, reimplemented the table with divs;

Marking the patch as obsolete since it is included in another review where I added a commit for fixing the failing tests.
Attachment #8935318 - Attachment is obsolete: true
Flags: needinfo?(3ugzilla)
Comment on attachment 8940680 [details]
Bug 1399897 - Fix console_table mochitest; .

https://reviewboard.mozilla.org/r/210930/#review217760

Looks good to me and passes localy on Win10

R+ assuming try is green and my inline comment is fixed.

Honza

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_console_table.js:164
(Diff revision 1)
>      JSON.stringify(testCase.expected.columns),
>      JSON.stringify(columns.map(column => column.textContent)),
>      "table has the expected columns"
>    );
>  
> -  is(testCase.expected.rows.length, rows.length,
> +  // We don't really have rows sonce we are using a CSS grid in order to have a sticky

typo: sonce -> since
Attachment #8940680 - Flags: review?(odvarko) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 26

a year ago
mozreview-review-reply
Comment on attachment 8940680 [details]
Bug 1399897 - Fix console_table mochitest; .

https://reviewboard.mozilla.org/r/210930/#review217760

> typo: sonce -> since

done. Thanks for the review Honza

Comment 27

a year ago
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/914fce4a03e3
Restrict the height of console table and add sticky headers; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/83acad9bf47b
Fix console_table mochitest; r=Honza.

Comment 28

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/914fce4a03e3
https://hg.mozilla.org/mozilla-central/rev/83acad9bf47b
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
status-firefox57: affected → wontfix
status-firefox58: --- → wontfix

Updated

9 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.