Closed
Bug 1113825
Opened 10 years ago
Closed 8 years ago
console.table() column overflows when a cell contains a long string
Categories
(DevTools :: Console, defect)
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)
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
Updated•10 years ago
|
Component: Untriaged → Developer Tools: Console
Updated•9 years ago
|
Whiteboard: [polish-backlog]
Updated•9 years ago
|
Assignee: nobody → jfong
Comment 1•9 years ago
|
||
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)
Comment 2•9 years ago
|
||
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.
Comment 3•9 years ago
|
||
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 4•9 years ago
|
||
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-
Comment 5•9 years ago
|
||
Comment 6•9 years ago
|
||
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)
Comment 7•9 years ago
|
||
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
Updated•9 years ago
|
Attachment #8673379 -
Flags: feedback?(bgrinstead)
Comment 9•9 years ago
|
||
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
Comment 10•9 years ago
|
||
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])
Comment 11•9 years ago
|
||
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)
Updated•9 years ago
|
Flags: needinfo?(jfong)
Attachment #8673379 -
Flags: review?(bgrinstead)
Comment 12•9 years ago
|
||
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)
Comment 13•9 years ago
|
||
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)
Comment 14•9 years ago
|
||
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 15•9 years ago
|
||
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 ?
Comment 16•9 years ago
|
||
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)
Updated•9 years ago
|
Assignee: jfong → gl
Updated•9 years ago
|
Whiteboard: [polish-backlog] → [polish-backlog][DevRel:P3]
Assignee | ||
Updated•8 years ago
|
Attachment #8701289 -
Flags: review?(gl) → review+
Assignee | ||
Updated•8 years ago
|
Attachment #8701290 -
Flags: review+
Assignee | ||
Comment 17•8 years ago
|
||
Attachment #8701289 -
Attachment is obsolete: true
Attachment #8772968 -
Flags: review+
Assignee | ||
Comment 18•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d8384a5bd4dfd6371e3d0d17fa0a91367a20dc7a Bug 1113825 - Provide option to always wrap table contents into elements;r=gl https://hg.mozilla.org/integration/fx-team/rev/c050984325e379286c1456a261b0cdc92dab89c1 Bug 1113825 - Nowrap content in console.table(). r=bgrins
Comment 19•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d8384a5bd4df https://hg.mozilla.org/mozilla-central/rev/c050984325e3
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•