Console error and stack trace UI follow up

RESOLVED FIXED in Firefox 31

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: bgrins, Assigned: msucan)

Tracking

Trunk
Firefox 31
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

Assignee

Updated

5 years ago
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Reporter

Comment 1

5 years ago
Some notes from Bug 920116

1) Text in some lines seems off-center vertically
2) The background seems to extend a few px too far to the left (beyond the solid left border) and not quite far enough to the right (just barely covering the line #s) https://bugzilla.mozilla.org/attachment.cgi?id=8410470
3) We need to do something about the contrast in the dark theme.  As Darrin mentioned, we could
 a) Fine tune the orange/background combo as per bgrins' suggestion to get contrast compliance
 b) Re-evaluate the dark theme text colors in general, as even on black some of the colors lack strong clarity
Reporter

Comment 2

5 years ago
The vertical alignment seems to be caused by the margin-top in the following rule:

.message > .message-body-wrapper {
  flex: 1 1 100%;
  margin-top: 4px;
}

Could we change this to margin: 2px 0; to balance the vertical spacing?
Reporter

Comment 3

5 years ago
While we are making changes - in getClassNameForValueGrip, IMO we should set 'undefined' to cm-comment (which is a grey color in both themes).  This makes entering random commands in the console nicer since the return value from many of them is undefined, and right now it really stands out (as cm-atom).
Reporter

Comment 4

5 years ago
Here is the current state of things
Reporter

Comment 5

5 years ago
Posted image console-colors-green.png (obsolete) —
Using cm-number
Reporter

Comment 6

5 years ago
Posted image console-colors-green-newcolors.png (obsolete) —
Same as green, but using the new colors for green and red in dark theme. https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors
Assignee

Comment 8

5 years ago
Posted patch bug1000243-1.diff (obsolete) — Splinter Review
This is what I got for now.

background-clip worked, but getting a consistent background for all elements wasnt doable without ugly hacks/changes. As such, I abandoned the background-clip approach.

This patch *should* fix horizontal and vertical alignments. Please let me know if something doesnt work on your system.

The background color for severity=error and the text color for strings are the values you suggested in bug 920116 comment 24.

From the experiments you posted I like the green strings most of all. Which color values did you use? Should we switch to green?

Thank you!
Attachment #8411292 - Flags: review?(bgrinstead)
Assignee

Comment 9

5 years ago
Assignee

Comment 10

5 years ago
Darrin: this is showing the changes I did in the patch. What do you think? Please also check attachment 8411297 [details], and the experiments Brian did. Thanks!
Attachment #8411299 - Flags: ui-review?(dhenein)
Reporter

Comment 11

5 years ago
> From the experiments you posted I like the green strings most of all. Which
> color values did you use? Should we switch to green?

OK, so for Attachment 8411111 [details] I just replaced cm-string with cm-number in console-output.js and didn't make any CSS changes.

For Attachment 8411113 [details] I also replaced cm-string with cm-number, but in addition to this I changed the cm-number / theme-fg-color1 in dark-theme.css to #70bf53, and changed the severity background color to rgba(235, 83, 104, .2) or so.  The alpha may have been somewhere between .1 and .2.  These colors are from https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors.

We could also use that new red background color (rgba(235, 83, 104, X)) regardless of what foreground we use if it ends up looking better or having better contrast.

I'd like to hear Darrin's opinion on what to go with.  Darrin: I can send over more screenshots if you want to see different options also.
Reporter

Comment 12

5 years ago
Comment on attachment 8411292 [details] [diff] [review]
bug1000243-1.diff

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

Code looks good overall.  We've discussed a few small changes on IRC so I'll review the updated patch
Attachment #8411292 - Flags: review?(bgrinstead)
Reporter

Comment 13

5 years ago
Comment on attachment 8411299 [details]
screenshot 2 with patch

Darrin said this one was his favorite option on #devtools - we are going to go with this text color for string messages / background color for errors, and stick with the current colors for the light theme
Attachment #8411299 - Flags: ui-review?(dhenein) → ui-review+
Reporter

Updated

5 years ago
Attachment #8411111 - Attachment is obsolete: true
Reporter

Updated

5 years ago
Attachment #8411113 - Attachment is obsolete: true
Reporter

Updated

5 years ago
Attachment #8411117 - Attachment is obsolete: true
Assignee

Comment 14

5 years ago
Posted patch bug1000243-2.diff (obsolete) — Splinter Review
Updated the patch based on your comments and the IRC chat with you and Darrin.

I also included an attempt to better align the icon with the rest of visual elements.

Screenshot:
https://i.imgur.com/TStpObz.png

Please let me know if I missed anything.
Attachment #8411292 - Attachment is obsolete: true
Attachment #8412668 - Flags: review?(bgrinstead)
Reporter

Comment 15

5 years ago
Comment on attachment 8412668 [details] [diff] [review]
bug1000243-2.diff

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

This is good.  I've made one minor note about alignment on the icons in OSX.  The alignment thing is not a big deal, and I could pick it up in a follow up if you'd like to do it that way.  If my inline suggestion looks OK on your system, we could go with that extra top margin and make adjustments if needed in the future.

::: browser/themes/shared/devtools/webconsole.inc.css
@@ +41,3 @@
>    padding: 0 4px;
>    width: 8px;
> +  height: 1.2em;

This is causing icon misalignment on OSX.  It actually does work if we add 3px top margin on this element (which matches the 3px margin on message-body-wrapper
Attachment #8412668 - Flags: review?(bgrinstead) → review+
Assignee

Comment 16

5 years ago
Update to fix alignment on macs, based on the IRC chat.

Thanks for the review and help with testing.
Attachment #8412668 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/40abf689af04
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.