SVG icons and theming for SocialAPI/Loop

VERIFIED FIXED in Firefox 34

Status

()

Firefox
SocialAPI
P1
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: Andrei Oprea [mention me on andrei.br92@gmail.com], Assigned: jaws)

Tracking

unspecified
Firefox 35
Points:
3
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox32 unaffected, firefox33 unaffected, firefox34 verified, firefox35 verified)

Details

(Whiteboard: [fig:wontverify])

Attachments

(1 attachment, 3 obsolete attachments)

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.
Created attachment 8474765 [details] [diff] [review]
SVG icons and theming for SocialAPI/Loop
Priority: -- → P1
Target Milestone: --- → Firefox 34
The squished camera can be fixed by setting the following:

> .chat-toolbarbutton {
>   padding-top: 0;
>   padding-bottom: 0;
>   width: 16px;
>   height: 16px;
> }
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.
Created attachment 8477550 [details] [diff] [review]
Patch with fixed camera icon (Mike's patch with my CSS fix)
Attachment #8474765 - Attachment is obsolete: true
Attachment #8477550 - Flags: review?(mdeboer)
Attachment #8477550 - Attachment description: Patch with fixed camera icon → Patch with fixed camera icon (Mike's patch with my CSS fix)
Attachment #8477550 - Flags: review?(mdeboer) → review?(MattN+bmo)
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Points: --- → 1
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.
Attachment #8477550 - Flags: review?(MattN+bmo) → review-
Created attachment 8479343 [details] [diff] [review]
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).
Attachment #8477550 - Attachment is obsolete: true
Attachment #8479343 - Flags: review?(MattN+bmo)
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.
Attachment #8479343 - Flags: review?(MattN+bmo) → feedback+
Oh, sorry, I forgot to address that issue.
Created attachment 8482103 [details] [diff] [review]
Patch v2.1

I addressed the issue of the "dark" attribute getting lost when the chat box was detached.
Attachment #8479343 - Attachment is obsolete: true
Attachment #8482103 - Flags: review?(MattN+bmo)
Points: 1 → 3
Iteration: --- → 35.1
Flags: qe-verify+
Flags: firefox-backlog+
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: Firefox 34 → ---
Comment on attachment 8482103 [details] [diff] [review]
Patch v2.1

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

Thanks
Attachment #8482103 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/756308489990
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in fx-team]
Target Milestone: --- → Firefox 35

Updated

4 years ago
QA Contact: anthony.s.hughes
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.
Flags: needinfo?(paul.silaghi)
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
Flags: needinfo?(paul.silaghi) → needinfo?(jaws)
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
Flags: needinfo?(jaws)
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
Status: RESOLVED → VERIFIED
status-firefox35: --- → verified
Untracking for verification before uplift.
Whiteboard: [fig:wontverify]
Comment on attachment 8482103 [details] [diff] [review]
Patch v2.1

Approval Request Comment
Uplift request for patches staged and tested on Fig
Attachment #8482103 - Flags: approval-mozilla-aurora?

Updated

3 years ago
status-firefox34: --- → fixed
status-firefox32: --- → unaffected
status-firefox33: --- → unaffected
Comment on attachment 8482103 [details] [diff] [review]
Patch v2.1

Already landed per previous offline approval for jesup. Cleaning up the bug. Aurora+
Attachment #8482103 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed FF 34b10 Win 7 x64.
status-firefox34: fixed → verified
You need to log in before you can comment on or make changes to this bug.