Closed Bug 1055239 Opened 10 years ago Closed 10 years ago

SVG icons and theming for SocialAPI/Loop

Categories

(Firefox Graveyard :: SocialAPI, defect, P1)

defect

Tracking

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

VERIFIED FIXED
Firefox 35
Tracking Status
firefox32 --- unaffected
firefox33 --- unaffected
firefox34 --- verified
firefox35 --- verified

People

(Reporter: aoprea, Assigned: jaws)

References

Details

(Whiteboard: [fig:wontverify])

Attachments

(1 file, 3 obsolete files)

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.
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.
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-
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attached patch Patch v2.1Splinter Review
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+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in fx-team]
Target Milestone: --- → Firefox 35
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
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?
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.
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: