1399897 - Restrict the height of console.table

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

Attachment: console_table_tall.jpg

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.

Comment 1

[[:Towkir] Ahmed]

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

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

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 !

Comment 3

[[:Towkir] Ahmed]

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

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 5

[kumar.abhishek3031]

hey, i am new to open source, can i give it a go??

Comment 6

[[:Towkir] Ahmed]

(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

Comment 7

[[:Towkir] Ahmed]

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.

Comment 8

[[:Towkir] Ahmed]

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  |
+---------------------+-----------------------+

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

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)

Comment 10

[[:Towkir] Ahmed]

Attachment: new-consoletable-approach.diff

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

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

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"`

Comment 12

[[:Towkir] Ahmed]

Attachment: console-table-height-restricted.patch

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

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 14

[[:Towkir] Ahmed]

Attachment: Bug 1399897 - restricted height for the console table, reimplemented the table with divs;

Review commit: https://reviewboard.mozilla.org/r/206218/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206218/

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

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))
);

Comment 16

[[:Towkir] Ahmed]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206218/diff/1-2/

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

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`

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

hey Ahmed, do you need help to debug this test failure ?

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1399897 - Restrict the height of console table and add sticky headers;

Since table elements (th, tr) do not support sticky headers, the console table now
uses only sibling divs for the cells, with appropriate aria attributes and a CSS grid
to mimic an actual table.

Review commit: https://reviewboard.mozilla.org/r/210928/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210928/

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1399897 - Fix console_table mochitest; .

Now that we are using sibling divs for all the cells of the table,
we don't have the same notion of what is a row.
The console table test is adapted to the new component.

Review commit: https://reviewboard.mozilla.org/r/210930/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210930/

Comment 21

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

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.

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210928/diff/1-2/

Comment 25

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8940680 [details]
Bug 1399897 - Fix console_table mochitest; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210930/diff/1-2/

Comment 26

[Nicolas Chevobbe [:nchevobbe]]

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

[Pulsebot]

Pushed by [e-mail]:
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

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/914fce4a03e3
https://hg.mozilla.org/mozilla-central/rev/83acad9bf47b