Closed Bug 1399897 Opened 3 years ago Closed 3 years ago

Restrict the height of console.table

Categories

(DevTools :: Console, enhancement, P1)

enhancement

Tracking

(firefox57 wontfix, firefox58 wontfix, firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: nchevobbe, Assigned: Towkir, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [newconsole-mvp])

Attachments

(3 files, 3 obsolete files)

Attached image 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.
Priority: -- → P3
Whiteboard: [reserve-console-html]
Priority: P3 → P2
Whiteboard: [reserve-console-html] → [newconsole-mvp]
Mentor: nchevobbe
Severity: normal → enhancement
Keywords: good-first-bug
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)
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)
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)
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??
(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
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)
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
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)
Attached patch new-consoletable-approach.diff (obsolete) — Splinter Review
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)
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+
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)
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 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+
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`
Attachment #8934408 - Attachment is obsolete: true
Attachment #8935050 - Attachment is obsolete: true
hey Ahmed, do you need help to debug this test failure ?
Flags: needinfo?(3ugzilla)
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+
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 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
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.
https://hg.mozilla.org/mozilla-central/rev/914fce4a03e3
https://hg.mozilla.org/mozilla-central/rev/83acad9bf47b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.