Closed Bug 1065076 Opened 5 years ago Closed 4 years ago

Update icons for video controls

Categories

(Firefox for Android :: Screencasting, defect)

x86
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- verified
fennec + ---

People

(Reporter: antlam, Assigned: ralin, Mentored)

References

Details

(Whiteboard: [lang=js][lang=css][good first bug])

Attachments

(5 files, 4 obsolete files)

Attached image prev_controls_mock1.png (obsolete) —
Not as big of a scope as bug 704229, simply about updating the icons and moving them around to be better spaced.
Flags: needinfo?(blassey.bugs)
Component: Theme and Visual Design → Screencasting
^assets for controls in bug 704229 :) don't want to post in multiple places in case/when updates happen there'd be one less version to "make obsolete"
I'm assuming the NI was to track this
tracking-fennec: --- → ?
Flags: needinfo?(blassey.bugs)
Flags: needinfo?(wjohnston)
Assignee: nobody → wjohnston
tracking-fennec: ? → 35+
I just provided a screenshot for the Marketing team for our new site that contains this UI in it, would be nice if the new UI were in it instead.. :)
This will not happen in 35.
tracking-fennec: 35+ → ?
Flags: needinfo?(wjohnston)
Blocks: 1098285
Anthony, is this just an icon swap, or is there more?
Flags: needinfo?(alam)
If it is an icon swap please provide the needed files for this to be an actionable bug.
Attached file icon_pbar_mob.zip (obsolete) —
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #5)
> Anthony, is this just an icon swap, or is there more?

Yes, this would just be a basic icon swap.

(In reply to Kevin Brosnan [:kbrosnan] from comment #6)
> If it is an icon swap please provide the needed files for this to be an
> actionable bug.

I filed the assets in bug 704229, but here they are again! :)
Flags: needinfo?(alam)
^
Flags: needinfo?(snorp)
tracking-fennec: ? → 37+
Flags: needinfo?(snorp)
Summary: Basic clean up to our basic video controls → Update icons for video controls
Wes this seems reasonable to do in 37 if it's just an icon swap. If you disagree please renom I guess :)
Assignee: wjohnston → nobody
Mentor: wjohnston, mark.finkle
tracking-fennec: 37+ → +
Whiteboard: [lang=js][lang=css][good-first-bug]
Whiteboard: [lang=js][lang=css][good-first-bug] → [lang=js][lang=css][good first bug]
Hello, I'm new to open source and looking for the first bug, so I would like to take this one :)
Awesome! Thanks Pavel.

NI-ing Mark and Wesj here to help you get started
Flags: needinfo?(wjohnston)
Flags: needinfo?(mark.finkle)
Hi, Pavel! First of all, you'll want to set up a Firefox for Android build environment. You can find instructions for that here:
https://wiki.mozilla.org/Mobile/Fennec/Android

Please join us in #mobile on irc.mozilla.org to ask questions if you run into any issues getting a build set up. Also feel free to say hi to introduce yourself!

After that, to fix this bug you are going to take the attached ZIP file and replace the existing images with the new images. To make this harder than it could be, the images in the ZIP file *do not* have the same name s the image file you need to replace. For example, the unmute image is found here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/images/unmute-hdpi.png

it is 42px by 42px, and there is only one size in the source code since this is an image displayed in web content.

The replace ZIP has many different sizes of an image named "icon_pbar_volx_XXX.png" where the XXX is different DPI sizes, and none of those sizes matches 42px by 42px exactly. The "XHDPI" size comes close, start with it.

I also see these images in the source: play-hdpi.png, pause-hdpi.png, mute-hdpi.png, cast-ready-hdpi.png, exitfullscreen-hdpi.png, fullscreen-hdpi.png and scrubber-hdpi.png

You need to replace all of those with suitable replacement images. Make a build and see that the new images are being used.

If this seems complicated, it is, but if you have questions go to #mobile on our IRC server. Anyone of the team members can help you. Wes and I are on vacation this week, so you'll need to get someone else to help.
Flags: needinfo?(wjohnston)
Flags: needinfo?(mark.finkle)
Thanks for picking this up pavel! NI-ing you here just in case you missed mfinkle's comments ^
Flags: needinfo?(prdik91)
Thanks for the instructions... getting right on it.
Flags: needinfo?(prdik91)
Anthony - Should we try using SVG images here? Also, see comment 12:
1. Current images are 42x42, but the new images have a 45x45 (XXHDPI) which we can try to use, but just a heads up.
2. The ZIP file is missing exit-fullscreen, scrubber and casting images
Flags: needinfo?(alam)
Hello, I am new to open source and I would like to work on this bug. I have downloaded and built the firefox  already. Is it okay  if I continue with this bug.
Attached file video_player_SVG.zip
(In reply to Mark Finkle (:mfinkle) from comment #15)
> Anthony - Should we try using SVG images here? Also, see comment 12:
> 1. Current images are 42x42, but the new images have a 45x45 (XXHDPI) which
> we can try to use, but just a heads up.
> 2. The ZIP file is missing exit-fullscreen, scrubber and casting images

I'd love to try SVG! 

So, I've made this pack of SVG icons that we can use. They should be complete now.

Each icon is 48dp x 48dp as per our guidelines (and Google's) so we might have to resize them for the scope of this bug. 

I didn't want to make them at 42 x 42 because these were SVG anyway and after we update these icons, I'd look to stick with them as we clean up the rest of our video controls in the scope of bug 704229 (better UX, layout & spacing, etc)
Attachment #8486670 - Attachment is obsolete: true
Attachment #8522410 - Attachment is obsolete: true
Flags: needinfo?(alam)
(In reply to Babatunde Salaam from comment #16)
> Hello, I am new to open source and I would like to work on this bug. I have
> downloaded and built the firefox  already. Is it okay  if I continue with
> this bug.

Yep, I think so. 

As per comment 17, give these assets a try :) Afterwards, it'd be great if you could post a screenshot and we can clean it up from there.

Thanks!
Flags: needinfo?(salaamb)
Mentor: wjohnston2000 → margaret.leibovic
Hi, I am new to fixing bugs and I would like to work on the bug please.
(In reply to Pas from comment #19)
> Hi, I am new to fixing bugs and I would like to work on the bug please.

Hi there. It looks like other people have expressed interest in this bug, but nobody has made any progress.

Feel free to go ahead and work on a patch, and attach it to the bug when you're ready for feedback. See comment 12 for instructions.
Hi, this is still a work in progress and I'm looking forward to receive your feedback.
Attachment #8706033 - Flags: review?(margaret.leibovic)
(In reply to Prudhvi Rampey from comment #21)
> Created attachment 8706033 [details] [diff] [review]
> bug1065076_swappedvideoicons.diff
> 
> Hi, this is still a work in progress and I'm looking forward to receive your
> feedback.

Sorry for the delayed response, but looking at this patch, I don't actually see any changes... was there a problem generating a diff?
Flags: needinfo?(salaamb) → needinfo?(prudhvi.rampey)
(In reply to :Margaret Leibovic from comment #22)
> (In reply to Prudhvi Rampey from comment #21)
> > Created attachment 8706033 [details] [diff] [review]
> > bug1065076_swappedvideoicons.diff
> > 
> > Hi, this is still a work in progress and I'm looking forward to receive your
> > feedback.
> 
> Sorry for the delayed response, but looking at this patch, I don't actually
> see any changes... was there a problem generating a diff?

Nevermind, I see the binary changes in the raw diff, I'll take a look.
Flags: needinfo?(prudhvi.rampey)
Comment on attachment 8706033 [details] [diff] [review]
bug1065076_swappedvideoicons.diff

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

This seems like it's on the right track, but when I tested it out, the images don't look like they're the right size. Were you able to make a build and test this out yourself? I also don't see the play button appear.

You should attach screenshots of your build for Anthony to look at. You should also ask him for updated images if these ones aren't the size you need.
Attachment #8706033 - Flags: review?(margaret.leibovic)
(In reply to :Margaret Leibovic from comment #24)
> Comment on attachment 8706033 [details] [diff] [review]
> bug1065076_swappedvideoicons.diff
> 
> Review of attachment 8706033 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems like it's on the right track, but when I tested it out, the
> images don't look like they're the right size. Were you able to make a build
> and test this out yourself? I also don't see the play button appear.
> 
> You should attach screenshots of your build for Anthony to look at. You
> should also ask him for updated images if these ones aren't the size you
> need.

Sorry for the delay. Yes, I was able to test it out. That could be a design problem where the play button might be too big. Okay, will do that.
Hi Anthony. Here are the links to the screenshots:
https://www.dropbox.com/s/xm95fx38g33nvgs/Screenshot_2016-02-02-10-19-03.png?dl=0
https://www.dropbox.com/s/ljvdl97n5dyi8v1/Screenshot_2016-01-22-11-49-06.png?dl=0

One is with the play button (which is not being shown) and the other, with the pause button. They seem to be a bit oversized, probably because of the 45x45px resolution. 
I need 42x42px resolution pngs for the correct size.
Flags: needinfo?(alam)
(In reply to Prudhvi Rampey from comment #26)
> Hi Anthony. Here are the links to the screenshots:
> https://www.dropbox.com/s/xm95fx38g33nvgs/Screenshot_2016-02-02-10-19-03.
> png?dl=0
> https://www.dropbox.com/s/ljvdl97n5dyi8v1/Screenshot_2016-01-22-11-49-06.
> png?dl=0
> 
> One is with the play button (which is not being shown) and the other, with
> the pause button. They seem to be a bit oversized, probably because of the
> 45x45px resolution. 
> I need 42x42px resolution pngs for the correct size.

Thanks 

As per comment 15 and comment 17, we're looking to use SVG here rather than PNGs. Could you try those and scale it down to 42 x 42 in the code? That would help us a lot here especially since I wouldn't have to create specific images for each resolution.
Flags: needinfo?(alam) → needinfo?(prudhvi.rampey)
Hi Margaret. I was wondering if I should downsize the existing svgs to 42 x 42  pngs and simply replace them or modify the code to handle the svg files.

Thanks.
Flags: needinfo?(prudhvi.rampey) → needinfo?(margaret.leibovic)
(In reply to Prudhvi Rampey from comment #28)
> Hi Margaret. I was wondering if I should downsize the existing svgs to 42 x
> 42  pngs and simply replace them or modify the code to handle the svg files.

Yous should modify the code to handle the svg files.

Thanks! These controls can be a bit of a pain to debug, so let us know if you need any help. Feel free to ask questions in #mobile on IRC as well (irc.mozilla.org).
Flags: needinfo?(margaret.leibovic)
Attached patch uncommited-1065076.patch (obsolete) — Splinter Review
Hi Margaret.

I replaced png with resized svg, but it looks like a little bit oversized in original space. Do we need to adjust it in this bug? or leave it to bug 1250741. Thanks.

screenshots:
https://www.dropbox.com/s/jjeq3rd8gv1t5cn/Screenshot_2016-03-15-07-42-19.png?dl=0
https://www.dropbox.com/s/3u6hgtyn6wpfn6h/Screenshot_2016-03-15-07-45-47.png?dl=0
Flags: needinfo?(margaret.leibovic)
Assignee: nobody → ralin
Status: NEW → ASSIGNED
(In reply to Ray Lin[:ralin] from comment #30)
 
> I replaced png with resized svg, but it looks like a little bit oversized in
> original space. Do we need to adjust it in this bug? or leave it to bug
> 1250741. Thanks.

Thanks for the patch! I agree it looks a bit oversized. I think we should adjust it in this bug, since we don't want to have these in an incorrect state until bug 1250741 lands.

Anthony (our UX designer) should be able to advise on the dimensions here.
Flags: needinfo?(ralin)
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(alam)
Attachment #8706033 - Attachment is obsolete: true
Comment on attachment 8730585 [details] [diff] [review]
uncommited-1065076.patch

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

::: mobile/android/themes/core/images/cast-active.svg
@@ +2,5 @@
> +<svg width="66px" height="54px" viewBox="0 0 66 54" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:sketch="http://www.bohemiancoding.com/sketch/ns">
> +    <!-- Generator: Sketch 3.4 (15575) - http://www.bohemiancoding.com/sketch -->
> +    <title>!Cast</title>
> +    <desc>Created with Sketch.</desc>
> +    <defs></defs>

We should remove the unused tags and attributes that we auto-generated by sketch. Here's a good MDN page with guidelines about cleaning up SVG files: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines
(In reply to :Margaret Leibovic from comment #31)
> (In reply to Ray Lin[:ralin] from comment #30)
>  
> > I replaced png with resized svg, but it looks like a little bit oversized in
> > original space. Do we need to adjust it in this bug? or leave it to bug
> > 1250741. Thanks.
> 
> Thanks for the patch! I agree it looks a bit oversized. I think we should
> adjust it in this bug, since we don't want to have these in an incorrect
> state until bug 1250741 lands.
> 
> Anthony (our UX designer) should be able to advise on the dimensions here.

Awesome Ray!

What size are they right now (in your screenshot)? The "play button" also looks a bit distorted.

Since the icons are not all perfect squares, maybe it'd be easier to wrap them inside a square and then simply size that? I have specs over in bug 1250741 that work as such.

Either way, let me know what size these currently are in the screenshot so I can help size them down.

Thanks!
Flags: needinfo?(alam)
Thank Margaret, I'll keep update this patch.

Hi, Anthony. Thanks for your advice! Their size right now are 42px x 42px.
Flags: needinfo?(ralin)
Hi, Anthony!

Since I noticed the unit in spec is "dp", I have some updates.

I cloud not find unit corresponding to "dp" in CSS, so I converted it into "pt" (like 48dp will be 21.60pt). Is it correct to convert unit like so? 


It is a WIP build, and the icons are wrapped in 21.60pt size boxes. 
https://www.dropbox.com/s/maew6shmfvi0o82/Screenshot_2016-03-16-10-24-51.png?dl=0
https://www.dropbox.com/s/53rkxyh9w8o5kyc/Screenshot_2016-03-16-10-24-59.png?dl=0

Thanks!
Flags: needinfo?(alam)
(In reply to Ray Lin[:ralin] from comment #34)
> Thank Margaret, I'll keep update this patch.
> 
> Hi, Anthony. Thanks for your advice! Their size right now are 42px x 42px.

Awesome! I think 24px should be good. Can I see a screenshot?
Flags: needinfo?(alam) → needinfo?(ralin)
(In reply to Ray Lin[:ralin] from comment #35)
> Hi, Anthony!
> 
> Since I noticed the unit in spec is "dp", I have some updates.
> 
> I cloud not find unit corresponding to "dp" in CSS, so I converted it into
> "pt" (like 48dp will be 21.60pt). Is it correct to convert unit like so? 
> 
> 
> It is a WIP build, and the icons are wrapped in 21.60pt size boxes. 
> https://www.dropbox.com/s/maew6shmfvi0o82/Screenshot_2016-03-16-10-24-51.
> png?dl=0
> https://www.dropbox.com/s/53rkxyh9w8o5kyc/Screenshot_2016-03-16-10-24-59.
> png?dl=0
> 
> Thanks!

Neat! these screenshots seem to show more than just the icon switch that's scoped out for this bug. 

Are these screenshots for this bug or bug 1250741? :)
(In reply to Anthony Lam (:antlam) from comment #37)

> Are these screenshots for this bug or bug 1250741? :)

If we're doing all of these changes at once, it would be fine to dupe one of these bugs to the other and write the code all in the same place.
Hi, Anthony. 

These screenshots are for bug 1250741, I thought to demonstrate the size look like wherein more close to spec, sorry about that.

I put these icons in 24px height/width boxes with no padding, could you help me to check if it is good? Would it be better to add padding? Thanks.
https://www.dropbox.com/s/hvctmcrcvolxj8y/Screenshot_2016-03-17-02-19-27.png?dl=0
https://www.dropbox.com/s/v09lvi8i3qpd6eq/Screenshot_2016-03-17-02-19-38.png?dl=0

Thank you Margaret! I think I would finish icon switching before bug 1250741.
Flags: needinfo?(ralin) → needinfo?(alam)
(In reply to Ray Lin[:ralin] from comment #39)
> Hi, Anthony. 
> 
> These screenshots are for bug 1250741, I thought to demonstrate the size
> look like wherein more close to spec, sorry about that.

No problem :)

> I put these icons in 24px height/width boxes with no padding, could you help
> me to check if it is good? 

Can you confirm that the icons are not distorted? The play and pause icon in particular looks like it might be being stretched too wide. I'll attach a side-by-side comparison :).

> Would it be better to add padding? Thanks.

I don't think we need that right now.

Thanks Ray!
Flags: needinfo?(alam) → needinfo?(ralin)
Here's a comparison Ray.

Can you see the distortion I'm referring to?
Hi, Anthony

I see the distortion, thank you for providing clearer comparison picture.

Instead of set fixed size of width/height to svg, I restrict their max width/height to keep them in correct ratio. Could you help me to check if there still has distortion? 

Thanks!
Flags: needinfo?(ralin) → needinfo?(alam)
Attachment #8731996 - Attachment description: adjust-svg-distortion → adjust-svg-distortion under is new version.
(In reply to Ray Lin[:ralin] from comment #42)
> Created attachment 8731996 [details]
> adjust-svg-distortion
> 
> under is new version.
> 
> Hi, Anthony
> 
> I see the distortion, thank you for providing clearer comparison picture.
> 
> Instead of set fixed size of width/height to svg, I restrict their max
> width/height to keep them in correct ratio. 

This sounds like the right idea. Since the icons aren't perfect squares, setting a height and width will distort them. We need to make sure we maintain the original aspect ratio :)

> Could you help me to check if
> there still has distortion? 
> 
> Thanks!

This looks better! thanks!

I think the scrubber (circle icon that the user drags to move to different video positions) is a little big but we can worry about that later in bug 1250741
Flags: needinfo?(alam)
Attached patch 1065076.patchSplinter Review
Hi, Margaret.

In this patch:
1. replace png icons with new svg icons
2. cleanup unused attrs and tags in svg files
3. adjust icon size: attachment 8731996 [details]

Could you help me to review it?

Thanks!!
Attachment #8730585 - Attachment is obsolete: true
Attachment #8732090 - Flags: review?(margaret.leibovic)
Comment on attachment 8732090 [details] [diff] [review]
1065076.patch

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

This is great, thanks for the patch!
Attachment #8732090 - Flags: review?(margaret.leibovic) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7bc4feca89f4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Verified as fixed using:
Device: One A2001 (Android 5.1.1)
Build: Firefox for Android 48.0a1 (2016-03-20)
You need to log in before you can comment on or make changes to this bug.