Closed Bug 1455645 Opened Last year Closed Last year

console.err call with long string without whitespace chars are displayed in a weird way

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

Details

(Whiteboard: [newconsole-mvp])

Attachments

(2 files)

**Steps to reproduce**
1. Open the console
2. Evaluate `console.error("fuuu".repeat(100))`


**Actual results**

An horizontal scrollbar is shown on the console, and the arrow icon is shrunk (see attachment)
Priority: -- → P2
Whiteboard: [newconsole-mvp]
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8971567 [details]
Bug 1455645 - Ensure we don't show an horizontal scrollbar in the console; .

https://reviewboard.mozilla.org/r/240328/#review246172

::: devtools/client/themes/webconsole.css:112
(Diff revision 1)
>  .message-location {
> -  max-width: 40%;
> +  max-width: 40vw;
> +  grid-column: -1 / -2;
> +  text-overflow: ellipsis;
> +  overflow: hidden;
> +  direction: rtl;

This can cause weird behavior with text directionality (moving characters from the back to the front of a string, for instance). I'd prefer to use something like float if that achieves the same effect (`float: right` or `float: inline-end` if we show the location on the left in RTL locales)
Attachment #8971567 - Flags: review?(bgrinstead)
Comment on attachment 8971567 [details]
Bug 1455645 - Ensure we don't show an horizontal scrollbar in the console; .

https://reviewboard.mozilla.org/r/240328/#review246172

> This can cause weird behavior with text directionality (moving characters from the back to the front of a string, for instance). I'd prefer to use something like float if that achieves the same effect (`float: right` or `float: inline-end` if we show the location on the left in RTL locales)

This is already something we do (as well as the netmonitor iirc), for example in https://searchfox.org/mozilla-central/rev/795575287259a370d00518098472eaa5b362bfa7/devtools/client/themes/webconsole.css#117-120 .
I guess float would help with some issues (text flowing around the location), but we could not have the same "ellipsis at the beginning" effect.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)
> Comment on attachment 8971567 [details]
> Bug 1455645 - Ensure we don't show an horizontal scrollbar in the console; .
> 
> https://reviewboard.mozilla.org/r/240328/#review246172
> 
> > This can cause weird behavior with text directionality (moving characters from the back to the front of a string, for instance). I'd prefer to use something like float if that achieves the same effect (`float: right` or `float: inline-end` if we show the location on the left in RTL locales)
> 
> This is already something we do (as well as the netmonitor iirc), for
> example in
> https://searchfox.org/mozilla-central/rev/
> 795575287259a370d00518098472eaa5b362bfa7/devtools/client/themes/webconsole.
> css#117-120 .
> I guess float would help with some issues (text flowing around the
> location), but we could not have the same "ellipsis at the beginning" effect.

Oh, right - forgot that we have to crop at the start of the string. I'll take another look.
Attachment #8971567 - Flags: review?(bgrinstead)
Duplicate of this bug: 1457807
I also checked with `--setpref intl.uidirection=1` , but the console content is still left-to-right aligned.
This is probably because of https://searchfox.org/mozilla-central/rev/ce9ff94ffed34dc17ec0bfa406156d489eaa8ee1/devtools/client/themes/webconsole.css#871 , which I'm not sure why it is set ?
Comment on attachment 8971567 [details]
Bug 1455645 - Ensure we don't show an horizontal scrollbar in the console; .

https://reviewboard.mozilla.org/r/240328/#review247304

This looks good to me. I'm a little worried about changing display values and some of the side effects that could have, but after doing some local testing it seems pretty solid any I'm not seeing any issues.
Attachment #8971567 - Flags: review?(bgrinstead) → review+
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.d_webidl hook is invalid: import of "mozhghooks.prevent_webidl_changes" failed
remote: (run with --traceback for stack trace)
abort: push failed on remote
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2df67c1f4980
Ensure we don't show an horizontal scrollbar in the console; r=bgrins.
https://hg.mozilla.org/mozilla-central/rev/2df67c1f4980
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
(In reply to Alexandre Poirot [:ochameau] from comment #13)
> This patch regressed the performance of bulklog and console open with cache
> tests:
>  
> https://treeherder.mozilla.org/perf.html#/
> comparesubtest?originalProject=try&originalRevision=2aff86fbdf9e9562d75e9c01c
> 3a529729c4aca64&newProject=try&newRevision=0efe4746859ecb688a72e5bcd42fa86a02
> eb30c8&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignatur
> e=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1

Looks like along with those regressions, it significantly improved complicated.webconsole.open.settle for some reason.
(In reply to Brian Grinstead [:bgrins] from comment #14)
> (In reply to Alexandre Poirot [:ochameau] from comment #13)
> > This patch regressed the performance of bulklog and console open with cache
> > tests:
> >  
> > https://treeherder.mozilla.org/perf.html#/
> > comparesubtest?originalProject=try&originalRevision=2aff86fbdf9e9562d75e9c01c
> > 3a529729c4aca64&newProject=try&newRevision=0efe4746859ecb688a72e5bcd42fa86a02
> > eb30c8&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignatur
> > e=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1
> 
> Looks like along with those regressions, it significantly improved
> complicated.webconsole.open.settle for some reason.

This may highlight that the new code better wait for async operations during opening.
But still the regression is concerning on console.openwithcache.open and console.openwithcache.open.settle where they both regress. Also, the regression on bulklog is small but is on a critical metric.

Nicolas, could you look into that? Tell me if you need some help.
Flags: needinfo?(nchevobbe)
I'll have a look. The main change here is a switch from a display flex to a grid one, so it's probably a layout/painting issue.
Flags: needinfo?(nchevobbe)
Product: Firefox → DevTools
Duplicate of this bug: 1327375
Brad, would you have an idea on why we are seeing a perf regression on this patch ? 
The patch was mainly switching from flex to grid https://hg.mozilla.org/mozilla-central/rev/2df67c1f4980 .
Flags: needinfo?(bwerth)
Nicholas, I'm not certain, but I am guessing that the performance slowdown could be coming from the 'max-content' in:

> grid-template-columns: 1fr auto max-content;

max-content is saying that the column width must be set to the largest inherent width of all the items that appear in that column. Seems like it would introduce a re-layout of the entire grid once those sizes are known. For a grid with many rows, that could be a sizable performance penalty. Is there a way for you to give this column a fixed size?

Mats, would you please confirm if max-content is dangerous for performance for large grids?
Flags: needinfo?(bwerth) → needinfo?(mats)
So I made the following change: 

# HG changeset patch
# User Nicolas Chevobbe <nchevobbe@mozilla.com>
# Date 1532937306 -7200
#      Mon Jul 30 09:55:06 2018 +0200
# Node ID 693706460e234fbb07aaaf52f10c5acafbf98404
# Parent  0be4463d29159905dded07f1dbddc5bb7dfaa336
Bug XXX - Swap max-content for auto

diff --git a/devtools/client/themes/webconsole.css b/devtools/client/themes/webconsole.css
--- a/devtools/client/themes/webconsole.css
+++ b/devtools/client/themes/webconsole.css
@@ -141,7 +141,7 @@ a {
 
 .message-flex-body {
   display: grid;
-  grid-template-columns: 1fr auto max-content;
+  grid-template-columns: 1fr auto auto;
   grid-gap: 5px;
 }
 

and there were no performance changes reported by DAMP.

The thing to know here also is that the grid used here probably only have one row. Not sure if it should have any impact.
Got it. I'm happy to dig deeper and give a better answer as to what is going on. Can you help me with steps to reproduce the performance regression case?
Flags: needinfo?(mats)
One of the case that is regressing is displaying 100 messages in the console.

So you can:
1. Go to data:text/html,<meta charset=utf8><script>for (var i = 0; i < 100; i++) {console.log('damp', i+1, document);}</script>
2. Open the console

and to see how it was before, you may try to apply this patch: 

diff --git a/devtools/client/themes/webconsole.css b/devtools/client/themes/webconsole.css
--- a/devtools/client/themes/webconsole.css
+++ b/devtools/client/themes/webconsole.css
@@ -105,9 +105,7 @@ a {
 }
 
 .message-location {
-  max-width: 40vw;
-  grid-column: -1 / -2;
-  color: var(--frame-link-source);
+  max-width: 40%;
 }
 
 .stack-trace {
@@ -116,8 +114,8 @@ a {
   white-space: normal;
 }
 
-.message-location,
-.stack-trace .frame-link-source {
+.stack-trace .frame-link-source,
+.message-location .frame-link-source {
   /* Makes the file name truncated (and ellipsis shown) on the left side */
   direction: rtl;
   white-space: nowrap;
@@ -140,9 +138,7 @@ a {
 }
 
 .message-flex-body {
-  display: grid;
-  grid-template-columns: 1fr auto max-content;
-  grid-gap: 5px;
+  display: flex;
 }
 
 .message-body {
@@ -649,7 +645,8 @@ a.learn-more-link.webconsole-learn-more-
 }
 
 .webconsole-output-wrapper .message-flex-body > .message-body {
-  overflow-x: auto;
+  display: inline-block;
+  max-width: 100%;
 }
 
 .webconsole-output-wrapper .message-body > * {
@@ -957,7 +954,6 @@ body #output-container {
  * Properties were copied from devtools/client/shared/components/reps/reps.css.
  */
 .webconsole-output-wrapper img.collapse-button.arrow {
-  flex: none;
   mask: url("chrome://devtools/skin/images/devtools-components/arrow.svg") no-repeat;
   mask-size: 100%;
   width: 9px;
Thanks for the reproduction case. These are my results running the perf-html profiler on a debug nightly build running the testcase with webconsole using flex (old) and grid (new) versions:

flex
styling: 1093ms
reflow: 634ms

grid
styling: 2133ms
reflow: 1044ms

So with grid the styling is taking about 2x the time taken by flex, and reflow is taking about 1.5x the time. I'm still learning how to use the profiler to understand this. For now focusing on the styling, it appears that the grid case hits a slow path in nsCSSFrameConstructor::ConstructScrollableBlockWithConstructor where the call to ConstructBlock, instead of being very fast, takes as long as the call to BeginBuildingScrollFrame. This is the symptom of the 2x time. I'll continue to explore this and see if I can determine what is making this path so slow.
New theory: the difference is the path taken in nsCSSFrameConstructor::FindDisplayData, specifically at https://searchfox.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#4550. The fast path is needScrollFrame=false, and the slow path is needScrollFrame=true. I'll see if I can modify the styling of the grid to prevent the need to create a scroll frame.
I've opened Bug 1479627 to address the performance regressions introduced by the patches for this bug.
You need to log in before you can comment on or make changes to this bug.