Closed Bug 1000243 Opened 6 years ago Closed 6 years ago

Console error and stack trace UI follow up

Categories

(DevTools :: Console, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: bgrins, Assigned: msucan)

References

Details

Attachments

(4 files, 5 obsolete files)

Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
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
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?
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).
Here is the current state of things
Attached image console-colors-green.png (obsolete) —
Using cm-number
Attached 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
Attached 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)
Attached image screenshot 1 with patch
Attached image screenshot 2 with patch
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)
> 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.
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)
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+
Attached 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)
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+
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/40abf689af04
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.