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

RESOLVED FIXED in Firefox 63

Status

defect
P1
normal
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: fvsch, Assigned: fvsch)

Tracking

(Blocks 1 bug)

Trunk
Firefox 63
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [boogaloo-reserve])

Attachments

(7 attachments, 4 obsolete attachments)

83.97 KB, image/png
Details
177.14 KB, image/png
Details
639.30 KB, image/png
Details
46 bytes, text/x-phabricator-request
nchevobbe
: review+
Details | Review
46 bytes, text/x-phabricator-request
nchevobbe
: review+
Details | Review
285.98 KB, image/png
Details
294.09 KB, image/png
Details
(Assignee)

Description

9 months ago
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)
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.
Victoria for r? on the mockup. I personally like the improved alignment!
Flags: needinfo?(victoria)
Great idea and mockup! Thanks Florens! (agreed re: keeping the blue icon on focus)
Flags: needinfo?(victoria)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Whiteboard: [boogaloo-reserve]
(Assignee)

Comment 4

9 months ago
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.
Attachment #8997866 - Flags: review?(nchevobbe)
Assignee: nobody → florens
Status: NEW → ASSIGNED
Priority: P3 → P1
(Assignee)

Comment 5

9 months ago
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).
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.
(Assignee)

Comment 7

8 months ago
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.
Attachment #8999929 - Flags: ui-review?(victoria)
(Assignee)

Comment 8

8 months ago
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.
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.
(Assignee)

Comment 10

8 months ago
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;
}
(Assignee)

Comment 11

8 months ago
Opened bug 1483782 to follow up on removing the old webconsole.svg sprite.
(Assignee)

Updated

8 months ago
Blocks: 1483782
(Assignee)

Comment 12

8 months ago
Attachment #9001313 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8997866 - Attachment is obsolete: true
Attachment #8997866 - Flags: review?(nchevobbe)
(Assignee)

Updated

8 months ago
Attachment #9001531 - Attachment is obsolete: true
(Assignee)

Comment 15

8 months ago
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
(Assignee)

Comment 17

8 months ago
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
(Assignee)

Comment 18

8 months ago
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.
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).
(Assignee)

Updated

8 months ago
Attachment #8999929 - Flags: ui-review?(victoria)
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.
(Assignee)

Comment 21

8 months ago
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?
(Assignee)

Comment 23

8 months ago
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.
Attachment #9002493 - Attachment is obsolete: true
(Assignee)

Comment 24

8 months ago
Vertical alignment and error background issues fixed in:
https://phabricator.services.mozilla.com/D3483
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.
Attachment #9001533 - Attachment description: Bug 1479750 Part 1 - Tweak clear icon and devtools-separator style; r=nchevobbe → Bug 1479750 - Part 1: Tweak clear icon and devtools-separator style; r=nchevobbe
Attachment #9001534 - Attachment description: Bug 1479750 Part 2 - Improve console alignment, visual consistency and icons; r=nchevobbe → Bug 1479750 - Part 2: Improve console alignment, visual consistency and icons; r=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.
Attachment #9001534 - Flags: review+
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.
Attachment #9001533 - Flags: review+
(Assignee)

Comment 29

8 months ago
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

8 months ago
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/707789fe9986
Part 1: Tweak clear icon and devtools-separator style; r=nchevobbe

Comment 31

8 months ago
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ce26d250b22d
Part 2: Improve console alignment, visual consistency and icons; r=nchevobbe

Comment 32

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/707789fe9986
https://hg.mozilla.org/mozilla-central/rev/ce26d250b22d
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.