1065076 - Update icons for video controls

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Screencasting, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_controls_mock1.png

Not as big of a scope as bug 704229, simply about updating the icons and moving them around to be better spaced.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

^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"

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

I'm assuming the NI was to track this

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

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

Comment 4

[Wesley Johnston (:wesj)]

This will not happen in 35.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Anthony, is this just an icon swap, or is there more?

Comment 6

[Kevin Brosnan [Ex-Mozilla]]

If it is an icon swap please provide the needed files for this to be an actionable bug.

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: icon_pbar_mob.zip

(In reply to James Willcox (:snorp) ([e-mail]) 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! :)

Comment 8

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

^

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Wes this seems reasonable to do in 37 if it's just an icon swap. If you disagree please renom I guess :)

Comment 10

[Pavel Vitvera]

Hello, I'm new to open source and looking for the first bug, so I would like to take this one :)

Comment 11

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Awesome! Thanks Pavel.

NI-ing Mark and Wesj here to help you get started

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 13

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Thanks for picking this up pavel! NI-ing you here just in case you missed mfinkle's comments ^

Comment 14

[Pavel Vitvera]

Thanks for the instructions... getting right on it.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 16

[Babatunde Salaam]

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.

Comment 17

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: 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)

Comment 18

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

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

Comment 19

[Pas]

Hi, I am new to fixing bugs and I would like to work on the bug please.

Comment 20

[:Margaret Leibovic]

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

Comment 21

[Prudhvi Rampey]

Attachment: bug1065076_swappedvideoicons.diff

Hi, this is still a work in progress and I'm looking forward to receive your feedback.

Comment 22

[: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?

Comment 23

[:Margaret Leibovic]

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

Comment 24

[:Margaret Leibovic]

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.

Comment 25

[Prudhvi Rampey]

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

Comment 26

[Prudhvi Rampey]

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.

Comment 27

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

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

Comment 28

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

Comment 29

[: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).

Comment 30

[Ray Lin[:ralin]]

Attachment: uncommited-1065076.patch

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

Comment 31

[:Margaret Leibovic]

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

Comment 32

[:Margaret Leibovic]

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

Comment 33

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

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

Comment 34

[Ray Lin[:ralin]]

Thank Margaret, I'll keep update this patch.

Hi, Anthony. Thanks for your advice! Their size right now are 42px x 42px.

Comment 35

[Ray Lin[: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!

Comment 36

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

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

Comment 37

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

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

Comment 38

[:Margaret Leibovic]

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

Comment 39

[Ray Lin[:ralin]]

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.

Comment 40

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

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

Comment 41

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: Screen Shot 2016-03-17 at 9.28.18 AM.png

Here's a comparison Ray.

Can you see the distortion I'm referring to?

Comment 42

[Ray Lin[:ralin]]

Attachment: 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. Could you help me to check if there still has distortion? 

Thanks!

Comment 43

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(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

Comment 44

[Ray Lin[:ralin]]

Attachment: 1065076.patch

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

Comment 45

[: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!

Comment 46

[Pulsebot]

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

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7bc4feca89f4

Comment 48

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot from 2016-03-21 15:15:41.png

Verified as fixed using:
Device: One A2001 (Android 5.1.1)
Build: Firefox for Android 48.0a1 (2016-03-20)