609372 - Better icons and color palette for the Web Console

Status: VERIFIED FIXED

(DevTools :: General, defect, P1)

Description

[Kevin Dangoor]

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.

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

Switching this over to Stephen since he's our visual design guru

Comment 2

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 3

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 4

[Patrick Walton (:pcwalton)]

We also need this icon:

 * a blue (rgb(0, 182, 240)) 8x8 /!\ warning triangle

Comment 5

[Kevin Dangoor]

Is there an ETA on this?

Comment 6

[Stephen Horlander [:shorlander]]

(In reply to comment #5)
> Is there an ETA on this?

End of this week or beginning of next.

Comment 7

[Stephen Horlander [:shorlander]]

Attachment: Updated Web Console Icons

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 8

[Stephen Horlander [:shorlander]]

Attachment: Updated Icons Patch - i01

Patch built on top of attachment 497824 [details] [diff] [review].

Comment 9

[Stephen Horlander [:shorlander]]

Attachment: Screenshot

Comment 10

[Patrick Walton (:pcwalton)]

Comment on attachment 499304 [details] [diff] [review]
Updated Icons Patch - i01

Feedback+, thanks for this!

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 499304 [details] [diff] [review]
Updated Icons Patch - i01

patch has bitrotted

Comment 12

[Rob Campbell [:rc] (:robcee)]

Attachment: Updated and rebased Icons Patch

rebased

Comment 13

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 505154 [details] [diff] [review]
Updated and rebased Icons Patch

resubmitting for review.

Comment 14

[Rob Campbell [:rc] (:robcee)]

Really hoping we can get this reviewed as it adds some extra polish to the icons. The console looks much better with it applied.

Comment 15

[Dão Gottwald [:dao]]

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

Comment 16

[Rob Campbell [:rc] (:robcee)]

Attachment: Updated Web Console Icons, v1.2

fixed mismerge

Comment 17

[Rob Campbell [:rc] (:robcee)]

Attachment: Updated Web Console Icons, v1.3

Found the -moz-image-rect was applied to the wrong section. Fixed and updated the patch. Sorry about that!

Comment 18

[Dão Gottwald [: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?

Comment 19

[Rob Campbell [:rc] (:robcee)]

You'd have to ask shorlander about that, but I'm guessing it's because the icons are 8x8 only.

Comment 20

[Rob Campbell [:rc] (:robcee)]

Attachment: Icons, v1.4

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.

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 507540 [details] [diff] [review]
Icons, v1.4

What are the arrows in the bottom row of webconsole.png used for?

Comment 22

[Rob Campbell [:rc] (:robcee)]

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.

Comment 23

[Patrick Walton (:pcwalton)]

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

Comment 24

[Rob Campbell [:rc] (:robcee)]

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.

Comment 25

[Rob Campbell [:rc] (:robcee)]

Attachment: Icons, v1.5

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.

Comment 26

[Rob Campbell [:rc] (:robcee)]

Attachment: Icons, v1.6

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.

Comment 27

[Rob Campbell [:rc] (:robcee)]

Attachment: Icons, v1.6

and the correct file...

Comment 28

[Dão Gottwald [: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.

Comment 29

[Rob Campbell [:rc] (:robcee)]

Attachment: Icons, v1.7

yeah that works well. updated!

Comment 30

[Dão Gottwald [: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

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 507904 [details] [diff] [review]
Icons, v1.7

see comment 30

Comment 32

[Rob Campbell [:rc] (:robcee)]

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.

Comment 33

[Rob Campbell [:rc] (:robcee)]

Attachment: Icons, v1.8

v.1.8, comments from c#30 addressed.

Comment 34

[Rob Campbell [:rc] (:robcee)]

Attachment: icons for checkin

updated, exported patch suitable for checking in.

Comment 35

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/d545aa98feb1

Comment 36

[(no longer active)]

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!

Comment 37

[Dão Gottwald [:dao]]

 .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.

Comment 38

[Rob Campbell [:rc] (:robcee)]

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?)

Comment 39

[Rob Campbell [:rc] (:robcee)]

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.

Comment 40

[Rob Campbell [:rc] (:robcee)]

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?

Comment 41

[Rob Campbell [:rc] (:robcee)]

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=[e-mail]

curious to see if this fixes the problem. In any case, the icons appear more vertically-centered (on OS X).

Comment 42

[Dão Gottwald [:dao]]

Maintaining the height+margin sum would mean to use margin: 3px 4px;

Comment 43

[Rob Campbell [:rc] (:robcee)]

(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.

Comment 44

[Rob Campbell [:rc] (:robcee)]

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.

Comment 45

[Rob Campbell [:rc] (:robcee)]

Attachment: screen shot, margin: 3px 4px

Comment 46

[Rob Campbell [:rc] (:robcee)]

Attachment: [checked-in] Icons, margin: 3px

Comment 47

[Dão Gottwald [:dao]]

(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.

Comment 48

[Rob Campbell [:rc] (:robcee)]

(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.

Comment 49

[(no longer active)]

(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.

Comment 50

[(no longer active)]

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?

Comment 51

[Rob Campbell [:rc] (:robcee)]

(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.

Comment 52

[Justin Dolske [:Dolske]]

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?

Comment 53

[Rob Campbell [:rc] (:robcee)]

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 54

[Rob Campbell [:rc] (:robcee)]

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

http://hg.mozilla.org/mozilla-central/rev/dc8abaadc629

Comment 55

[Maniac Vlad Florin (:vladmaniac)]

Verified fixed on 

Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110206 Firefox/4.0b12pre