Update icons for video controls

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: antlam, Assigned: ralin, Mentored)

Tracking

unspecified
Firefox 48
x86
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 verified, fennec+)

Details

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

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8486670 [details]
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.
Flags: needinfo?(blassey.bugs)
(Reporter)

Updated

4 years ago
Component: Theme and Visual Design → Screencasting
(Reporter)

Comment 1

4 years ago
^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)
(Reporter)

Updated

4 years ago
Flags: needinfo?(wjohnston)
Assignee: nobody → wjohnston
tracking-fennec: ? → 35+
(Reporter)

Comment 3

4 years ago
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)

Updated

4 years ago
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.
(Reporter)

Comment 7

4 years ago
Created attachment 8522410 [details]
icon_pbar_mob.zip

(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)
(Reporter)

Comment 8

4 years ago
^
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]

Updated

4 years ago
Whiteboard: [lang=js][lang=css][good-first-bug] → [lang=js][lang=css][good first bug]

Comment 10

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

Comment 11

4 years ago
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)
(Reporter)

Comment 13

4 years ago
Thanks for picking this up pavel! NI-ing you here just in case you missed mfinkle's comments ^
Flags: needinfo?(prdik91)

Comment 14

4 years ago
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)

Comment 16

3 years ago
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.
(Reporter)

Comment 17

3 years ago
Created attachment 8693006 [details]
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)
(Reporter)

Comment 18

3 years ago
(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)

Updated

3 years ago
Mentor: wjohnston2000 → margaret.leibovic

Comment 19

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

Comment 20

3 years ago
(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

3 years ago
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.
Attachment #8706033 - Flags: review?(margaret.leibovic)

Comment 22

3 years ago
(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)

Comment 23

3 years ago
(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 24

3 years ago
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)

Comment 25

3 years ago
(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

3 years ago
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)
(Reporter)

Comment 27

3 years ago
(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)

Comment 28

3 years ago
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)

Comment 29

3 years ago
(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)
(Assignee)

Comment 30

3 years ago
Created attachment 8730585 [details] [diff] [review]
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
Flags: needinfo?(margaret.leibovic)
Assignee: nobody → ralin
Status: NEW → ASSIGNED

Comment 31

3 years ago
(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)

Updated

3 years ago
Attachment #8706033 - Attachment is obsolete: true

Comment 32

3 years ago
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
(Reporter)

Comment 33

3 years ago
(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)
(Assignee)

Comment 34

3 years ago
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)
(Assignee)

Comment 35

3 years ago
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)
(Reporter)

Comment 36

3 years ago
(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)
(Reporter)

Comment 37

3 years ago
(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

3 years ago
(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.
(Assignee)

Comment 39

3 years ago
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)
(Reporter)

Comment 40

3 years ago
(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)
(Reporter)

Comment 41

3 years ago
Created attachment 8731754 [details]
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?
(Assignee)

Comment 42

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

Thanks!
Flags: needinfo?(ralin) → needinfo?(alam)
(Assignee)

Updated

3 years ago
Attachment #8731996 - Attachment description: adjust-svg-distortion → adjust-svg-distortion under is new version.
(Reporter)

Comment 43

3 years ago
(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)
(Assignee)

Comment 44

3 years ago
Created attachment 8732090 [details] [diff] [review]
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!!
Attachment #8730585 - Attachment is obsolete: true
Attachment #8732090 - Flags: review?(margaret.leibovic)

Comment 45

3 years ago
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+

Updated

3 years ago
Keywords: checkin-needed

Comment 47

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7bc4feca89f4
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Created attachment 8732827 [details]
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)
status-firefox48: fixed → verified
You need to log in before you can comment on or make changes to this bug.