Closed Bug 1113825 Opened 9 years ago Closed 8 years ago

console.table() column overflows when a cell contains a long string

Categories

(DevTools :: Console, defect)

36 Branch
x86
macOS
defect
Not set
normal

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: lspeciale, Assigned: gl)

References

Details

(Keywords: DevAdvocacy, Whiteboard: [polish-backlog][DevRel:P3])

Attachments

(6 files, 3 obsolete files)

Attached image consoletable.png
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20141219004003

Steps to reproduce:

If you console.table() an array with a string containing spaces the column will overflow the table


Actual results:

The column overflows the table


Expected results:

The table should expand
Component: Untriaged → Developer Tools: Console
Whiteboard: [polish-backlog]
Assignee: nobody → jfong
Attached patch Bug1113825.patch (obsolete) — Splinter Review
I think those were the only two instances (.console-string and .cm-number) but let me know if I missed one or more.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6d92887649d
Attachment #8672077 - Flags: review?(bgrinstead)
Attached image table-overflow.png
I don't think this is the right solution.  For instance, if I run:

console.table(["Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed enim velit, venenatis ac laoreet vel, scelerisque vel est. Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac turpis egestas. Pellentesque tincidunt nisi sit amet tortor sodales, vel euismod magna aliquet. Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Fusce at elementum nibh. Mauris lacinia tortor quam, vel maximus tortor tempus sit amet. Curabitur eros purus, tincidunt at lectus a, commodo vehicula urna. Duis pretium sem magna, eleifend eleifend mauris tristique ac."]);

Then without the patch applied (top) it has the overflowing issue, but with the patch applied (bottom) there is a worse problem, that all of the console output becomes scrollable to the right, including any long input lines that previously wrapped.  I'd have to look closer but I think ideally we could switch the layout to use display:table or similar to make sure that the cell's height expands.
Attached image two-rows.png
Screenshot of an even weirder problem (current bug without the patch applied) by running

console.table(["Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed enim velit, venenatis ac laoreet vel, scelerisque vel est. Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac turpis egestas. Pellentesque tincidunt nisi sit amet tortor sodales, vel euismod magna aliquet. Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Fusce at elementum nibh. Mauris lacinia tortor quam, vel maximus tortor tempus sit amet. Curabitur eros purus, tincidunt at lectus a, commodo vehicula urna. Duis pretium sem magna, eleifend eleifend mauris tristique ac.", "hi"]);
Comment on attachment 8672077 [details] [diff] [review]
Bug1113825.patch

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

See comment 2 - even though this does fix the height issue, I don't think having the console output area scroll horizontally is ideal
Attachment #8672077 - Flags: review?(bgrinstead) → review-
Attached patch Bug1113825.patch (obsolete) — Splinter Review
So I realized after playing around that "flex" in xul is not really similar to flexbox in CSS.

This is one variation I managed to get working based on a few situations I came across (updated patch and updated-overflow-hidden.png):

- I wasn't able to get display: table and the related parts (table-row, table-cell) to work since these are being listed in code vertically rather than horizontally, where the splitter separates each part. If we match the height, I assume they have to be displayed horizontally - so unless we rewrite the way it is displayed in the widget or we use JS to detect a change in height, I'm not sure of any other way to make that happen.

- In Chrome devtools, they never change the height, only overflow: hidden long text and add text ellipsis. I wasn't able to get text ellipsis working since white-space: nowrap on the text breaks the xul flex layout - do you know if there is some rule where I can force a xul element to not allow a child element's nowrap to override its width?
Attachment #8672077 - Attachment is obsolete: true
Attachment #8673379 - Flags: feedback?(bgrinstead)
Turns out that we're doing a new table widget so that might be better with flexbox instead of intermixing xul flex and flexbox. I'll just wait until that gets updated first.
Depends on: 1214323
Attachment #8673379 - Flags: feedback?(bgrinstead)
Could we land Jen's tiny patch as temporary fix instead of hard blocking on the much larger Bug 1214312?

Without the patch, console.table is unusable for values that contain multi-word strings.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jfong)
Flags: needinfo?(bgrinstead)
Keywords: DevAdvocacy
Here's a quick one-liner, so you don't have to go looking up Lorem Ipsum. :-)

    x = { foo: 'foo '.repeat(75), bar: 'bar' }; console.table([x, x, x])
So with Jen's latest patch applied it fixes the horizontal scrolling issue from the first patch and also the original reported issue.  The downside to this patch is that we don't ever ellipses text but it seems better than the current bug.  We'd want to update the CSS to not even try to ellipses and add a comment explaining, but that seems OK to me
Flags: needinfo?(bgrinstead)
Flags: needinfo?(jfong)
Attachment #8673379 - Flags: review?(bgrinstead)
Comment on attachment 8673379 [details] [diff] [review]
Bug1113825.patch

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

::: devtools/client/themes/webconsole.css
@@ +458,5 @@
>  .consoletable {
>    margin: 5px 0 0 0;
>  }
>  
> +.consoletable .table-widget-cell span {

This needs to be `.consoletable .table-widget-cell > span` so DOM Nodes and other custom previewers are supported

@@ +461,5 @@
>  
> +.consoletable .table-widget-cell span {
> +  overflow: hidden;
> +  display: flex;
> +  text-overflow: ellipsis;

This doesn't seem to do anything, it can be removed

@@ +462,5 @@
> +.consoletable .table-widget-cell span {
> +  overflow: hidden;
> +  display: flex;
> +  text-overflow: ellipsis;
> +  height: 1.25em;

1) I think we also need to set the line height to 1.25em to smooth over differences between system fonts.

2) Where did 1.25em come from?  Seems like something that might not match across platforms due to default font sizes on the 'index' column at  x = { foo: 'foo '.repeat(505), bar: 'bar' }; console.table([x, x, x]).

I have an idea for this in which we could pass _index in as a span element instead of an int which would make this style to apply to it also.  Unfortunately this looks like it'd require refactoring the TableWidget to support objects in the uniqueId field (it renders properly but you can't select any rows because it'd be storing the span in a data-attribute on the element).

I will attach a patch that does something like that at least to show our options.
Attachment #8673379 - Flags: review?(bgrinstead)
Attached patch console-table-wrapping-1.patch (obsolete) — Splinter Review
Adds an option so that a tablewidget will never set the 'value' attribute on a label.  There's no test coverage partly because I wanted to upload something before I am away for a week but mostly because of the impending change in Bug 1214312. 

This option is needed to make sure that we can consistently style the index cells alongside their contents (see Comment 12).  Another patch with Jen's styling changes + some modifications is incoming.  Gabe, please take a look at this whenever you have a chance - it's possible I'm missing an obvious solution to this problem but we've been back and forth on trying to get this styled properly and this seems to hit the intersection of normal flex + XUL flex where nothing works like you'd think.
Attachment #8701289 - Flags: review?(gabriel.luong)
Jen's patch + my review comments + using the option introduced in part 1.

Dan, can you test this out and see what you think?  Here's an ongoing try push with the changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2eccab44c719
Attachment #8673379 - Attachment is obsolete: true
Flags: needinfo?(dan.callahan)
Comment on attachment 8701289 [details] [diff] [review]
console-table-wrapping-1.patch

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

::: devtools/client/shared/widgets/TableWidget.js
@@ +947,5 @@
>  
> +    if (this.wrapTextInElements && !(value instanceof Ci.nsIDOMNode)) {
> +      let span = this.label.ownerDocument.createElementNS(HTML_NS, "span");
> +      span.textContent = value;
> +      value = span;

Didn't that cause some width issue as seen in bug 1194196 ?
At a first glance, the Try build looks good to me. Not ideal (per Bug 1214312), but much more usable than the status quo without that patch.
Flags: needinfo?(dan.callahan)
Assignee: jfong → gl
Depends on: 1247065
Whiteboard: [polish-backlog] → [polish-backlog][DevRel:P3]
Attachment #8701289 - Flags: review?(gl) → review+
Attachment #8701290 - Flags: review+
Attachment #8701289 - Attachment is obsolete: true
Attachment #8772968 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d8384a5bd4df
https://hg.mozilla.org/mozilla-central/rev/c050984325e3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: