Closed Bug 609372 Opened 9 years ago Closed 9 years ago

Better icons and color palette for the Web Console

Categories

(DevTools :: General, defect, P1)

defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 4.0b12

People

(Reporter: dangoor, Assigned: rcampbell)

References

Details

(Keywords: polish)

Attachments

(3 files, 12 obsolete files)

149.96 KB, image/png
Details
46.06 KB, image/png
Details
19.22 KB, patch
Details | Diff | Splinter Review
From bug 605621:

We'll need a few icons to be designed for this:

 * a blue (rgb(0, 182, 240)) 8x8 "X"
 * an orange (rgb(251, 149, 0)) 8x8 "X"
 * an orange (rgb(251, 149, 0)) 8x8 /!\ warning triangle
 * a light gray (rgb(203, 203, 203)) 8x8 X
 * a light gray (rgb(203, 203, 203)) 8x8 /!\ warning triangle
 * a light gray (rgb(203, 203, 203)) 8x8 (i) info icon

We also may want a gray (#808080) 8x8 "left arrow" icon to represent user
input, and a gray (#808080) 8x8 "right arrow" icon to represent the
corresponding output.

See toolkit/themes/foostripe/global/icons/webconsole.png for placeholder icons.
Switching this over to Stephen since he's our visual design guru
Assignee: limi → shorlander
Summary: Better icons for the Web Console → Better icons and color pallet for the Web Console
mass change: filter on PRIORITYSETTING
Priority: -- → P1
mass change: filter on PRIORITYSETTING
Summary: Better icons and color pallet for the Web Console → Better icons and color palette for the Web Console
We also need this icon:

 * a blue (rgb(0, 182, 240)) 8x8 /!\ warning triangle
Is there an ETA on this?
(In reply to comment #5)
> Is there an ETA on this?

End of this week or beginning of next.
Attached image Updated Web Console Icons (obsolete) —
New icons for the Web Console.

>  * a light gray (rgb(203, 203, 203)) 8x8 /!\ warning triangle

This seemed too light to me. I used rgb(170, 170, 170) instead. I also left the arrow on the end but I didn't see anywhere that it was actually being used.
Comment on attachment 499304 [details] [diff] [review]
Updated Icons Patch - i01

Feedback+, thanks for this!
Attachment #499304 - Flags: feedback+
Comment on attachment 499304 [details] [diff] [review]
Updated Icons Patch - i01

patch has bitrotted
Attachment #499304 - Flags: review?(dao)
Attached patch Updated and rebased Icons Patch (obsolete) — Splinter Review
rebased
Attachment #499304 - Attachment is obsolete: true
Whiteboard: [rebased-2011-01-19]
Comment on attachment 505154 [details] [diff] [review]
Updated and rebased Icons Patch

resubmitting for review.
Attachment #505154 - Flags: review?(dao)
Blocks: 627819
Really hoping we can get this reviewed as it adds some extra polish to the icons. The console looks much better with it applied.
Keywords: polish
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [rebased-2011-01-19] → [rebased-2011-01-19][in-review]
Comment on attachment 505154 [details] [diff] [review]
Updated and rebased Icons Patch

>--- a/toolkit/themes/gnomestripe/global/webConsole.css
>+++ b/toolkit/themes/gnomestripe/global/webConsole.css

> .webconsole-msg-network > .webconsole-msg-icon-container {
>-  -moz-border-start: solid #000 6px;
>+  -moz-image-region: rect(0, 16px, 8px, 8px);
> }

looks like a bad merge
Attachment #505154 - Flags: review?(dao) → review-
Attached patch Updated Web Console Icons, v1.2 (obsolete) — Splinter Review
fixed mismerge
Attachment #499302 - Attachment is obsolete: true
Attachment #505154 - Attachment is obsolete: true
Attachment #506754 - Flags: review?
Attached patch Updated Web Console Icons, v1.3 (obsolete) — Splinter Review
Found the -moz-image-rect was applied to the wrong section. Fixed and updated the patch. Sorry about that!
Assignee: shorlander → rcampbell
Attachment #506754 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #506768 - Flags: review?
Attachment #506754 - Flags: review?
Attachment #506768 - Flags: review? → review?(dao)
Comment on attachment 506768 [details] [diff] [review]
Updated Web Console Icons, v1.3

> .webconsole-msg-icon {
>   list-style-image: url(chrome://global/skin/icons/webconsole.png);
>-  margin: 2px 3px;
>-  width: 10px;
>-  height: 10px;
>+  margin: 5px 4px;
>+  max-width: 8px;
>+  max-height: 8px;
> }

Why are you switching from width and height to max-width and max-height?
You'd have to ask shorlander about that, but I'm guessing it's because the icons are 8x8 only.
Attached patch Icons, v1.4 (obsolete) — Splinter Review
removed the max-/min- from the style sheets. After conferring with Mr. Horlander about the origin, it was determined that I hadn't rebased correctly.
Attachment #506768 - Attachment is obsolete: true
Attachment #507540 - Flags: review?(dao)
Attachment #506768 - Flags: review?(dao)
Comment on attachment 507540 [details] [diff] [review]
Icons, v1.4

What are the arrows in the bottom row of webconsole.png used for?
Stephen mentioned in Comment #7 that he wasn't sure what they were for and that they weren't being used anywhere but left them in. Not sure if they have a purpose or not, but they're currently unused.
If you're referring the down arrow at the right here [1], then that used to be used for the drop-downs, but it was changed to use native theme drop downs. It can be removed.

[1]: http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/pinstripe/global/icons/webconsole.png
talking with Patrick who did the original icons, there was only one black drop-down arrow which is no longer used and can be removed.

Stephen helpfully pointed me to comment #0 in this bug:

"We also may want a gray (#808080) 8x8 "left arrow" icon to represent user
input, and a gray (#808080) 8x8 "right arrow" icon to represent the
corresponding output."

I think we still require a little CSS to wire that up to the input and output lines, but the black drop-down is safe to remove.
Attached patch Icons, v1.5 (obsolete) — Splinter Review
Based on feedback, I've removed the unused down arrow and added rules to display the left and right arrows for console input and output.
Attachment #507540 - Attachment is obsolete: true
Attachment #507631 - Flags: review?(dao)
Attachment #507540 - Flags: review?(dao)
Attached patch Icons, v1.6 (obsolete) — Splinter Review
figured out why we needed the visibility: hidden. Explicitly added visibility: visible to the input and output nodes. Also removed an errant !important that had crept in during testing.
Attachment #507631 - Attachment is obsolete: true
Attachment #507883 - Flags: review?(dao)
Attachment #507631 - Flags: review?(dao)
Attached patch Icons, v1.6 (obsolete) — Splinter Review
and the correct file...
Attachment #507883 - Attachment is obsolete: true
Attachment #507885 - Flags: review?(dao)
Attachment #507883 - Flags: review?(dao)
Comment on attachment 507885 [details] [diff] [review]
Icons, v1.6

> .webconsole-msg-icon {
>   list-style-image: url(chrome://global/skin/icons/webconsole.png);
>-  margin: 2px 3px;
>-  width: 10px;
>-  height: 10px;
>+  margin: 5px 4px;
>+  width: 8px;
>+  height: 8px;
> }

I think you can add -moz-image-region: rect(0, 1px, 0, 0); here instead of the visibility:hidden/visible dance.
Attachment #507885 - Flags: review?(dao) → review-
Attached patch Icons, v1.7 (obsolete) — Splinter Review
yeah that works well. updated!
Attachment #507885 - Attachment is obsolete: true
Attachment #507904 - Flags: review?(dao)
Comment on attachment 507904 [details] [diff] [review]
Icons, v1.7

> .webconsole-msg-icon {
>   list-style-image: url(chrome://global/skin/icons/webconsole.png);
>-  margin: 2px 3px;
>-  width: 10px;
>-  height: 10px;
>+  margin: 5px 4px;
>+  width: 8px;
>+  height: 8px;
> }
> 
> .webconsole-msg-log > .webconsole-msg-icon-container > .webconsole-msg-icon {
>-  visibility: hidden;
>+  -moz-image-region: rect(0, 1px, 0, 0);
> }

-moz-image-region: rect(0, 1px, 0, 0); should be set on .webconsole-msg-icon, the second rule can go away. r=me with that change
Attachment #507904 - Flags: review?(dao) → review+
Attachment #507904 - Flags: approval2.0?
Comment on attachment 507904 [details] [diff] [review]
Icons, v1.7

see comment 30
Attachment #507904 - Flags: approval2.0?
yes, I saw that and was going to make the modification at checkin time. I didn't think you needed to see a patch to approve it.
Attached patch Icons, v1.8 (obsolete) — Splinter Review
v.1.8, comments from c#30 addressed.
Attachment #507904 - Attachment is obsolete: true
Attachment #508445 - Flags: review?(dao)
Attachment #508445 - Flags: approval2.0?
Attachment #508445 - Flags: review?(dao) → review+
Attachment #508445 - Flags: approval2.0? → approval2.0+
Attached patch icons for checkin (obsolete) — Splinter Review
updated, exported patch suitable for checking in.
Attachment #508445 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [rebased-2011-01-19][in-review] → [rebased-2011-01-19][checkin-needed]
Whiteboard: [rebased-2011-01-19][checkin-needed] → [rebased-2011-01-19][checkin-needed][will checkin feb 01 if not checked in sooner]
http://hg.mozilla.org/mozilla-central/rev/d545aa98feb1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [rebased-2011-01-19][checkin-needed][will checkin feb 01 if not checked in sooner]
Target Milestone: --- → Firefox 4.0b11
Backed out because of test failures:

http://hg.mozilla.org/mozilla-central/rev/6f5b3b1c6c78

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1296536708.1296540277.12347.gz

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_614793_jsterm_scroll.js | scroll location is the same - Got 2434, expected 2433

Seems like the test must be written more robustly.  We shouldn't be relying on pixel values like that!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
 .webconsole-msg-icon {
   list-style-image: url(chrome://global/skin/icons/webconsole.png);
-  margin: 2px 3px;
-  width: 10px;
-  height: 10px;
...
+  margin: 5px 4px;
+  width: 8px;
+  height: 8px;

Other than checking if the test is sane, you may also want to adjust the margin so that the sum of the height and the negative margin remains the same.
Keywords: uiwanted
augh. Sorry for the burn. Thanks for trying to land this, Ehsan.

Did that turn orange on all platforms or just a subset? (Mac 32bit by any chance?)
Target Milestone: Firefox 4.0b11 → ---
funny, I've been running with that patch applied locally for awhile now and haven't seen that test fail on my machine ever.

I'll try your adjustment, Dao and see if that helps on try, but I expect I'll need to do something similar to the "fix" in bug 627342.
yup, I verified on TBPL, appeared orange on OS X 32. What is it about 10.5 that causes us to have 1px variations on some of our layout?
sent to try with:

.webconsole-msg-icon {
  list-style-image: url(chrome://global/skin/icons/webconsole.png);
  -moz-image-region: rect(0, 1px, 0, 0);
  margin: 4px 4px;
  width: 8px;
  height: 8px;
}

http://tbpl.mozilla.org/?tree=MozillaTry&rev=50dde748c88b&pusher=rcampbell@mozilla.com

curious to see if this fixes the problem. In any case, the icons appear more vertically-centered (on OS X).
Maintaining the height+margin sum would mean to use margin: 3px 4px;
(In reply to comment #42)
> Maintaining the height+margin sum would mean to use margin: 3px 4px;

Remains the same as what? Sorry, I don't quite follow you.

Do you think it's worth sending through try server again with your dimensions or should I just use them? I had a successful run through OS X 32 with 4px 4px;

Let me know what you think. I'll do a little local testing in the meantime.

margin: 4px 4px actually looks centered now.
tried 3px 4px and it looks good too. I didn't take screencaps before (5 vs 4 vs 3) but I'll attach the 3px version and the reworked patch.
Attachment #508509 - Attachment is obsolete: true
(In reply to comment #43)
> (In reply to comment #42)
> > Maintaining the height+margin sum would mean to use margin: 3px 4px;
> 
> Remains the same as what? Sorry, I don't quite follow you.

The height used to be 10, the vertical margins 2. This adds up to 14.
The new height is 8. With 3 as the vertical margins, this adds up to 14 as well.
(In reply to comment #47)
> (In reply to comment #43)
> > (In reply to comment #42)
> > > Maintaining the height+margin sum would mean to use margin: 3px 4px;
> > 
> > Remains the same as what? Sorry, I don't quite follow you.
> 
> The height used to be 10, the vertical margins 2. This adds up to 14.
> The new height is 8. With 3 as the vertical margins, this adds up to 14 as
> well.

ah, ok. Thanks for the clarification.

I guess this is safe to land with the current changes. I'll give it a try when the tree turns a little greener.
(In reply to comment #38)
> augh. Sorry for the burn. Thanks for trying to land this, Ehsan.
> 
> Did that turn orange on all platforms or just a subset? (Mac 32bit by any
> chance?)

IIRC it's only that log link that I posted.
Why do I get the feeling that we're moving in the wrong direction, here?  Rob, do we really care about the pixel value in the test?  Why can't we just change the test to not rely on pixel values?
(In reply to comment #50)
> Why do I get the feeling that we're moving in the wrong direction, here?  Rob,
> do we really care about the pixel value in the test?  Why can't we just change
> the test to not rely on pixel values?

well, the updated margins fixed the problem on try, but that test isn't unique in its reliance on pixels see bug 627342.

And yes, it was only the one platform (OS X 32bit) that was causing the problem.
robcee: I was going to reland this as you had asked, but it's not clear to me what to land. The last patch here got backed out, perhaps you intended to attach a revised version?
The most-recent patch in here is the Icons, 3px patch. I'll double check that it includes what I need in it and hopefully land it now.

Thanks anyway!
Comment on attachment 508789 [details] [diff] [review]
[checked-in] Icons, margin: 3px

http://hg.mozilla.org/mozilla-central/rev/dc8abaadc629
Attachment #508789 - Attachment description: Icons, margin: 3px → [checked-in] Icons, margin: 3px
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0
Target Milestone: Firefox 4.0 → Firefox 4.0b12
Verified fixed on 

Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110206 Firefox/4.0b12pre
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.