1479750 - Improve visual alignment and consistency of console icons (log lines and input line)

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Florens Verschelde :fvsch]

Attachment: console-horizontal-alignment.png

Here’s a mockup for style tweaks to the Console log and input line.

The idea is to achieve a slightly more polished look through:
1. aligning icons and the start edge of text
2. using the same "chevrons" icon for input as for the output lines (maybe keeping the blue color for input)

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Thanks a lot Florens, this does look neater.

I definitely think we should keep the blue icon as it's now the only indicator that the input is focused.

Comment 2

[:Harald Kirschner :digitarald]

Victoria for r? on the mockup. I personally like the improved alignment!

Comment 3

[Victoria Wang [:victoria]]

Great idea and mockup! Thanks Florens! (agreed re: keeping the blue icon on focus)

Comment 4

[Florens Verschelde :fvsch]

Attachment: bug-1479750-devtools-tweak-clear-icon-and-separator.patch

I’d like to work on those visual changes, and have started with some CSS tweaks that impact the console and toolbars in some other tools (mostly the netmonitor). I’ve separated them in a Part 1 patch, so that I can do a Part 2 with only the Console input/output style tweaks.

Contained in this patch:

1. Update the "clear.svg" (trash bin) icon so it's more visually centered. It was a 13px-wide icon on a 16px viewBox, with 0px on the left and blank pixels on the right. I changed it to have 1px blank on the left and 2px on the right. (Can't perfectly center it, or everything will be blurry on low-dpi screens.)
2. Changed the devtools-separator style to use a simpler border-color instead of a border-image. The current style had bugs where the separator was not always visually at the same distance from the top and bottom line: it was correct in the main tab bar, but incorrect in the Console an Netmonitor. Now the separator is styled to take a calc(100% - 8px) height and use 4px top/bottom margins. I’ve changed a few style overrides so that it works everywhere.
3. Also changed the separator style to use 1px horizontal margins by default. This means that the space between two buttons or between a button and a separator remains the same: 2px. There are overrides for the separators in the main tab bar, which use slightly different styles. The end result is also that the two separators now align in the Console.

Comment 5

[Florens Verschelde :fvsch]

Attachment: devtools-separator-before-after.png

This is what the Part 1 patch looks like.
Notice how the "clear" icon is better aligned and the separators line up.

One small downside is that the 1px separator margins eat up a few pixels in the netmonitor. In very narrow settings like in this screenshot, it can be a bit troublesome. But rather than removing the margins, I would fix this by maybe removing one or two separators in the netmonitor, or tweaking the padding from some netmonitor toolbar groups (some padding seems a bit too big IMO).

Comment 6

[Victoria Wang [:victoria]]

Very cool, I like where you're going with this! 

A couple quick things I noticed in the bottom Netmonitor screenshot: 
- filter placeholder is cut off
- Throttle label needs less left spacing (perhaps try matching the left spacing between the Persist Logs checkbox and its left separator) - that should fix the lack-of-space issue on the right end.

Comment 7

[Florens Verschelde :fvsch]

Attachment: console-line-height.png

Hi Victoria, I’m wondering if we should change the line-height for log content, or keep it as is.

Our base font-size is 11px. For the console input (bottom), we set a line-height of 16px. For log lines, we don't set a line-height, so it ends up depending on the actual font used (somewhere in the 12.5px–14px range depending on the font, I believe).

I did a quick test with 11/14 line-height (left) and 11/16 line-height (right).

Pros:
- Improves readability (IMO)
- Matches the input style (for multiline input)

Cons:
- Shows a bit less content
- This might make the separation between log items a bit less clear (when they use the same background)

Do you think it would be worth pursuing this change?

Some notes on line-heights for code in other tools:
- Inspector DOM tree: around 15.45px
- Inspector Rules: 14.25px
- Style Editor code: 13.75px
- Debugger code: 13.5px

Ideally I’d love to have it all use the same line-height, whenever it makes sense. 11/14 would be the more conservative choice, but 11/15 or 11/16 might be more readable.

Comment 8

[Florens Verschelde :fvsch]

Attachment: bug-1479750-console-alignment-icons.patch

Part 2 patch, this one should have everything, plus a bit of work from bug 1479861 (still waiting for UI validation on some icon choices).

I haven't run tests yet, I wouldn't be too surprised if a few things fail. There are a lot of changes in webconsole.css. I also tried to be the brave soul I want to see in the world and remove some of the styles from the old console.

I also extracted icons from the webconsole.svg sprite, but kept it for now because it's sometimes reused in other tools. Ultimately it should be possible to remove those external uses, and remove this file.

Comment 9

[Victoria Wang [:victoria]]

Thanks for investigating all the line heights Florens! It's hilarious and disturbing that literally everything is different! (A bit of variation might make sense - E.g. trees with selectable rows may need more line-height?)

Makes a lot of sense to unify the console input and log line heights now that we have an inline input. Chrome's is totally seamless from input to log, and it looks nice.

Looks like Console logs in the default font-size are currently 13.5px in line height? Even 14px looks like a big improvement in polish. 16px does look more readable and modern in style, and is fitting for today's big monitors, though it's true that compact styling does makes sense for a tool as utilitarian as Console. I noticed Chrome's console appears to be 13px, Mac Terminal is 14px. I read up on this a bit and people usually use 1.2 - 1.5 em. Based on all of this, I'd suggest we go with the conservative 14px for now. Looks like the background areas on errors and warnings may need to be made a bit shorter to go along with this change.

Comment 10

[Florens Verschelde :fvsch]

Indeed 11/14 seems reasonable for now, it can always be revisited.

I used variables in webconsole.css to implement it (and use them in two places):

:root {
  /* 20px high log items: 3 + (11 * 1.2727) + 3 */
  --console-output-font-size: 11px;
  --console-output-line-height: 1.2727;
  --console-output-vertical-padding: 3px;
}

Comment 11

[Florens Verschelde :fvsch]

Opened bug 1483782 to follow up on removing the old webconsole.svg sprite.

Comment 12

[Florens Verschelde :fvsch]

Attachment: bug-1479750-part2-console-alignment-icons.patch

Comment 13

[Florens Verschelde :fvsch]

Attachment: Bug 1479750 - Part 1: Tweak clear icon and devtools-separator style; r=nchevobbe

Comment 14

[Florens Verschelde :fvsch]

Attachment: Bug 1479750 - Part 2: Improve console alignment, visual consistency and icons; r=nchevobbe

Depends on D3482

Comment 15

[Florens Verschelde :fvsch]

Pushed to Phabricator:

https://phabricator.services.mozilla.com/D3482
https://phabricator.services.mozilla.com/D3483

I ran mochitest on devtools/client/webconsole, and mochitest-browser tests run fine, but mochitest-chrome tests seem stuck and eventually time out.

mochitest-browser
~~~~~~~~~~~~~~~~~
Ran 3147 checks (200 tests, 2947 subtests)
Expected results: 3139
Skipped: 8 tests
OK

mochitest-chrome
~~~~~~~~~~~~~~~~
Ran 1 checks (1 tests)
Expected results: 0
OK

Comment 16

[Florens Verschelde :fvsch]

Try seems okay:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=38e19c226b2e6a378f6ed5988870eac2a8871386

Comment 17

[Florens Verschelde :fvsch]

I was wondering if using 4 individual icons with `context-fill` and `-moz-context-properties: fill` would create a perf regression. Looks like there is no issue in Talos damp-e10s, but maybe there are no tests that stress the Console with e.g. 500 logs.

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=2d67438a3d40&newProject=try&newRevision=c0f755bc4c5c&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=webconsole&framework=1

Comment 18

[Florens Verschelde :fvsch]

I don’t see a significant difference when logging 10,000 lines with:

function logalot(iterations=100) {
  const methods = ['log', 'warn', 'error', 'info'];
  for (let i = 0; i < iterations; i++) {
    console[methods[i % methods.length]]('TEST ' + i);
  }
}
logalot(10000);

On my machine it takes roughly 18-20 seconds, before and after the icon changes.
And if I just use console.log(i), so there are no icons shown, the performance is in the same ballpark. So icon/SVG rendering is not the bottleneck here.

By the way, with 10,000 line of log the CodeMirror performance becomes sluggish. There’s a lot of delay on each character input, suggestions are barely usable, etc. With 1,000 lines there’s a noticeable input delay too.

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

Thanks for checking the performance impact Florens. I'd also prefer to have distinct icons instead of the sprite we currently have.

> By the way, with 10,000 line of log the CodeMirror performance becomes sluggish. There’s a lot of delay on each character input, suggestions are barely usable, etc. With 1,000 lines there’s a noticeable input delay too.

Yes, this is a known bug (Bug 1482798).

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

Attachment: input-min-height icon alignment

Thanks a lot Florens, this looks really good !
I only have one visual comment: when the input is shrunk to it's minimum height, the icon appears not vertically centered, which is a bit odd.

Comment 21

[Florens Verschelde :fvsch]

Attachment: input-vertical-centering.png

I see. Changing the input's line-height from 16px to 14px (to match the output style) seems to have shifted the text 0.5px higher; perhaps up to 1px higher on low dpi screens.

I can try bringing the text and icon 1px lower and see how that works.

It also looks like I’ve regressed the error (and possibly warning) line background. I’ll fix that.

Is it okay if I do a Part 3 commit for these adjustments?

Comment 22

[Florens Verschelde :fvsch]

Attachment: Bug 1479750 Part 2 - Improve console alignment, visual consistency and icons; r=nchevobbe

Depends on D3482

Comment 23

[Florens Verschelde :fvsch]

Comment on attachment 9002493 [details]
Bug 1479750 Part 2 - Improve console alignment, visual consistency and icons; r=nchevobbe

Mark link to bad Phabricator revision as obsolete.

Comment 24

[Florens Verschelde :fvsch]

Vertical alignment and error background issues fixed in:
https://phabricator.services.mozilla.com/D3483

Comment 25

[Nicolas Chevobbe [:nchevobbe]]

Arf, sorry I missed it yesterday, but we should also have the new icon in the old jsterm (i.e. when code mirror is not enabled).
See https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/devtools/client/themes/webconsole.css#287

You can have the old jsterm by going to about:config and setting `devtools.webconsole.jsterm.codeMirror` to false. This is important because it's likely that we won't ship the new jsterm in 63 due to performance issue in the input.

Also, there are other known issue with the old input (the most visible one being an offset of the "completion text") which are going to be fixed in Bug 1484979.

Comment 26

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9001534 [details]
Bug 1479750 - Part 2: Improve console alignment, visual consistency and icons; r=nchevobbe

Nicolas Chevobbe [:nchevobbe] has approved the revision.

Comment 27

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9001533 [details]
Bug 1479750 - Part 1: Tweak clear icon and devtools-separator style; r=nchevobbe

Nicolas Chevobbe [:nchevobbe] has approved the revision.

Comment 28

[Nicolas Chevobbe [:nchevobbe]]

Pushed to TRY one last time to make sure everything is still fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=18104c08da12715d7ffabc7f12082edc249e1598

Comment 29

[Florens Verschelde :fvsch]

New TRY with the old `--theme-command-line-image` variables removed (using the new icon in the old debugger):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fa6cd1390cc860c6adf6fb27ffbcdb886034ef6

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/707789fe9986
Part 1: Tweak clear icon and devtools-separator style; r=nchevobbe

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce26d250b22d
Part 2: Improve console alignment, visual consistency and icons; r=nchevobbe

Comment 32

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/707789fe9986
https://hg.mozilla.org/mozilla-central/rev/ce26d250b22d