1055239 - SVG icons and theming for SocialAPI/Loop

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect, P1)

Description

[Andrei Oprea [mention me on [e-mail]]]

We got visual designs for Loop along with accompanying SVG assets from mmaslaney over in bug 1016087.  Mike de Boer implemented the theming shown in the designs and the graphics as well, replacing the current SocialAPI bitmaps with SVG assets in the process.

There is still one known bug with the chatbox camera looking squashed that needs to be fixed before this can go up for review, there may be others as well.

Comment 1

[Andrei Oprea [mention me on [e-mail]]]

Attachment: SVG icons and theming for SocialAPI/Loop

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

The squished camera can be fixed by setting the following:

> .chat-toolbarbutton {
>   padding-top: 0;
>   padding-bottom: 0;
>   width: 16px;
>   height: 16px;
> }

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Ok, comment #2 wasn't entirely correct, because doing so would make the minimize, expand, and close icons too big.

The camera icon is naturally 16x16, whereas these other three icons are 16x10 (and need their 3px of internal vertical padding).

So the simplest fix would be to remove the padding-top and padding-bottom from the camera icon, and set the width and height of that icon to 16px.

The (maybe?) better fix would be to make the minimize, expand, and close icons include their vertical padding so they are also 16x16.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch with fixed camera icon (Mike's patch with my CSS fix)

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8477550 [details] [diff] [review]
Patch with fixed camera icon (Mike's patch with my CSS fix)

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

::: browser/components/loop/MozLoopService.jsm
@@ +417,5 @@
>        if (chatbox.contentWindow.navigator.mozLoop) {
>          return;
>        }
>  
> +      chatbox.setAttribute("dark", true);

Note that the popout window doesn't seem to inherit this dark attribute. It also gets lost when popping back into the window.

::: browser/themes/shared/social/chat-icons.svg
@@ +5,5 @@
> +<svg xmlns="http://www.w3.org/2000/svg"
> +     xmlns:xlink="http://www.w3.org/1999/xlink"
> +     x="0px" y="0px"
> +     viewBox="0 0 10 10"
> +     enable-background="new 0 0 10 10"

+     viewBox="-3 -3 16 16"
+     enable-background="new 0 0 16 16"

The above will make this 16x16 with 3px padding on each side.

@@ +24,5 @@
> +use[id$="-disabled"] {
> +  fill: #c1c1c1;
> +}
> +</style>
> +<defs style="display:none">

Nit: I don't think this inline style is needed as I thought the point of <defs> is for it to be hidden.

@@ +28,5 @@
> +<defs style="display:none">
> +  <polygon id="close-shape" fill-rule="evenodd" clip-rule="evenodd" points="10,1.717 8.336,0.049 5.024,3.369 1.663,0 0,1.668 
> +    3.36,5.037 0.098,8.307 1.762,9.975 5.025,6.705 8.311,10 9.975,8.332 6.688,5.037"/>
> +  <path id="dropdown-shape" fill-rule="evenodd" clip-rule="evenodd" d="M9,3L4.984,7L1,3H9z"/>
> +  <polygon id="expand-shape" fill-rule="evenodd" clip-rule="evenodd" points="10,0 4.838,0 6.506,1.669 0,8.175 1.825,10 8.331,3.494 

Nit: trailing whitespace on two lines

::: browser/themes/shared/social/chat.inc.css
@@ +63,5 @@
>  .chat-toolbarbutton > .toolbarbutton-text {
>    display: none;
>  }
>  
> +.chat-toolbarbutton:not(.webRTC-sharingDevices-notification-icon) > .toolbarbutton-icon {

I don't think this social file should be mentioning webrtc-specific stuff at all.

I think we should do this suggestion:
"The (maybe?) better fix would be to make the minimize, expand, and close icons include their vertical padding so they are also 16x16."

then the icon sizes don't change and are consistent with each other. See my comment in chat-icons.svg for how to easily change the SVG size.

@@ +145,5 @@
>  
> +chatbox[dark=true] > .chat-titlebar {
> +  height: 30px;
> +  min-height: 30px;
> +  padding: 7px 12px;

Changing the dimensions and padding for "dark" chatboxes only seems weird. Any reason we shouldn't do this for all?

::: browser/themes/windows/jar.mn
@@ +181,5 @@
>          skin/classic/browser/preferences/applications.css            (preferences/applications.css)
>          skin/classic/browser/preferences/aboutPermissions.css        (preferences/aboutPermissions.css)
>          skin/classic/browser/social/services-16.png                  (social/services-16.png)
>          skin/classic/browser/social/services-64.png                  (social/services-64.png)
> +*       skin/classic/browser/social/chat-icons.svg                   (../shared/social/chat-icons.svg)

Why does this file need to be pre-processed? I don't see anything that needs it.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

I adjusted the padding on the titlebar to be symmetrical. It looks a lot better this way, with the close button in the corner like it should be (it was previously offset by an extra 5 pixels).

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8479343 [details] [diff] [review]
Patch v2

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

(In reply to Matthew N. [:MattN] from comment #5)
> ::: browser/components/loop/MozLoopService.jsm
> @@ +417,5 @@
> >        if (chatbox.contentWindow.navigator.mozLoop) {
> >          return;
> >        }
> >  
> > +      chatbox.setAttribute("dark", true);
> 
> Note that the popout window doesn't seem to inherit this dark attribute. It
> also gets lost when popping back into the window.

Jared, were you able to reproduce this issue? It doesn't seem like the new patch addresses it.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Oh, sorry, I forgot to address that issue.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2.1

I addressed the issue of the "dark" attribute getting lost when the chat box was detached.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8482103 [details] [diff] [review]
Patch v2.1

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

Thanks

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/756308489990

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/756308489990

Comment 13

[u279076]

All icons look fine to me in the latest Nightly on my Retina Macbook Pro. Paul, can you please take a look at Linux and Windows for me? Please check with a couple of different Firefox and OS themes, including dark themes.

Comment 14

[Paul Silaghi, QA [:pauly]]

35.0a1 (2014-09-15), Win 7 x64
Couple of issues, say which to file or already known:
1. switching aero with basic themes on Win 7 with FF open makes the Loop panel larger on every theme change
http://i.imgur.com/QTE2ZgP.png
2. camera icon on the start page is blurry
http://i.imgur.com/ro3hTb6.png
3. client icons are blurry compared to the caller icons
http://i.imgur.com/FjsLyxs.png
4. mic icon in the client window on incoming call is blurry
5. no space between 'Accept' and the camera icon
http://i.imgur.com/XlOL2nY.png

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

The patch in this bug only made the drop-down, close, minimize, and expand icons use SVG. It didn't change any of the other icons.

1. file, but i doubt this is a regression from this bug
2. known issue, bug 1060590
3. file, but not a regression from this bug
4. file, but not a regression form this bug
5. file, but not a regression from this bug

Comment 16

[Paul Silaghi, QA [:pauly]]

Thanks Jared.
Filed bug 1068592, bug 1068593, bug 1068594, none of them are regression of this bug.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15)
> The patch in this bug only made the drop-down, close, minimize, and expand
> icons use SVG. It didn't change any of the other icons.
Marking this bug as verified fixed on 35.0a1 (2014-09-16) Win 7, Ubuntu 13.04

Comment 17

[u279076]

Untracking for verification before uplift.

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8482103 [details] [diff] [review]
Patch v2.1

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ce61b95701c5

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8482103 [details] [diff] [review]
Patch v2.1

Already landed per previous offline approval for jesup. Cleaning up the bug. Aurora+

Comment 21

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 34b10 Win 7 x64.