1250741 - Update video player spec

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: spec_videocont_mock5.png

We have bug 1065076 tracking SVG asset changes for our current video player. But I think the controls need to be cleaned up overall. 

Here is the design spec for the full player that would be attached under the video rather than centered on top.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Is the control overflows on top of the video (like the current YouTube player) or itself would take some space?

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I don't think web authors expect a control taking spaces -- so a design like that would probably force us to shrink the size of the video.

Comment 3

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

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #1)
> Is the control overflows on top of the video (like the current YouTube
> player) or itself would take some space?

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #2)
> I don't think web authors expect a control taking spaces -- so a design like
> that would probably force us to shrink the size of the video.

These should overlap the video.

Comment 4

[:Margaret Leibovic]

This would be about updating our built-in video controls, which are found here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml
http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/touchcontrols.css

Those overlap the video, so these should overlap the video as well.

Comment 5

[Ray Lin[:ralin]]

Hi, Anthony

I found the equation: px = dp * (dpi / 160), and I think it is reasonable to change icon size accordingly for different devices with different DPI. 

For instance, in Z3C which dpi is 320, the size of icon will be 96px 96px, but it is 4 times larger than we discussed in Bug 1065076. Could you help me to clarify the spec and please correct me if any problem in my understanding? Thanks :)

Comment 6

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

(In reply to Ray Lin[:ralin] from comment #5)
> Hi, Anthony
> 
> I found the equation: px = dp * (dpi / 160), and I think it is reasonable to
> change icon size accordingly for different devices with different DPI. 

hi Ray!

The numbers seem to make sense. 

I originally spec'd this out in dp. So, you should be able to take the values and multiply them to fit different devices' DPI.

> For instance, in Z3C which dpi is 320, the size of icon will be 96px 96px,
> but it is 4 times larger than we discussed in Bug 1065076. Could you help me
> to clarify the spec and please correct me if any problem in my
> understanding? Thanks :)

I think the best way to do this is to go through screenshots or builds. Could I get one to test?

Thanks!

Comment 7

[Ray Lin[:ralin]]

Attachment: appendAll.png

Thank Anthony for your information.

I was trying to figure out how web page affects the size of video and icons.
This attachment is WIP, and still has many detail to be adjusted, the height of the control bar now is 24px with 2px padding around. 

Please feel free to tell me any blemishes on current work.

And could you provide me new scrubber SVG that shown on spec? Thanks!!

Comment 8

[:Margaret Leibovic]

Anthony is PTO this week and will be returning March 30, but maybe he will still comment with feedback, because he's bad at getting away from work :)

Feel free to upload a WIP patch as well if you want any feedback on the code.

Comment 9

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

(In reply to Ray Lin[:ralin] from comment #7)
> Created attachment 8733828 [details]
> appendAll.png
> 
> Thank Anthony for your information.
> 
> I was trying to figure out how web page affects the size of video and icons.
> This attachment is WIP, and still has many detail to be adjusted, the height
> of the control bar now is 24px with 2px padding around. 
>
> Please feel free to tell me any blemishes on current work.

Awesome! this is looking great, Ray. But I think the bar itself should be 48px tall. Can we try that? That should give the icons enough padding.

In the design, I've centered everything (including the start times) inside a 48dp x 48dp square (the red squares in the spec). So, that should convert to 48px x 48px here. 

The start/end video times, are those Roboto reg 16px? 

> And could you provide me new scrubber SVG that shown on spec? Thanks!!

Sure!

Comment 10

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

Attachment: icon_scrubber2.svg

try this Ray!

Comment 11

[Ray Lin[:ralin]]

Attachment: Bug-1250741-Update-video-player-to-new-spec.patch

This is a WIP, could you give me some feedback? Thanks!

Comment 12

[:Margaret Leibovic]

Comment on attachment 8735355 [details] [diff] [review]
Bug-1250741-Update-video-player-to-new-spec.patch

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

Nice, this is looking good! Once we get Anthony's approval for any tweaks, I will be happy to r+ this.

::: mobile/android/themes/core/touchcontrols.css
@@ +20,5 @@
>  
> +.controlsSpacer {
> +  display: none;
> +  -moz-box-flex: 0;
> +}

If this is hidden, could we just remove it from the markup?

@@ +166,5 @@
>    width: 32px;
>  }
>  
> +.positionLabel, .durationLabel {
> +  font-family: 'Roboto';

Should we include a back-up font name in case Roboto isn't available?

::: toolkit/content/widgets/videocontrols.xml
@@ +1587,5 @@
> +                            <progressmeter class="bufferBar"/>
> +                            <progressmeter class="progressBar" max="10000"/>
> +                            <scale class="scrubber" movetoclick="true"/>
> +                        </stack>
> +                        <label class="durationLabel" role="presentation"/>

This mark-up is also used by b2g. Do we also need to update the b2g touchcontrols.css?

Comment 14

[Ray Lin[:ralin]]

Attachment: wp.png

I've changed them into 48px square with padding, and Roboto to 16px.
Anthony, could you please give me some feedback? Thanks.


Thank for reviewing, Margaret.
1. For .controlsSpacer, I think it has function to adapt different size of video. 
2. I agree we need some back-up font names, maybe Helvetica or Arial? 
3. The b2g's touchcontrols.css is now quite different from andriod's, still I will update it after tweaks.

Comment 15

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

Comment on attachment 8737062 [details]
wp.png

This is looking REALLY good - great work Ray!

I think after we clean up the padding around each of the icons, and time (text labels), this will be finished!

Comments:
 - What size is the "scrubber"? I think we can make it a little bit smaller. Is it 18px right now? Perhaps we can try 12px?

 - Can we make the padding around each element the same? E.g. the padding on the right and left side of the "full screen" button or the "play/pause" button are not equal. The same should be true for all padding along the bar.

Comment 16

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

Attachment: Screen Shot 2016-04-01 at 11.03.01 AM.png

Attaching visual of my comment about padding ^

Let me know if this makes sense :)

Comment 17

[Ray Lin[:ralin]]

Attachment: ss.png

Sorry for the late response.

The visualized comment is very helpful, thanks, and it does make sense to have equal padding like so.

I've made some changes in this screenshot:
- resize the scrubber to 12px. (previous's is 16px) 
- rewrite the padding of boxes and SVG scaling with more proper syntax.

Could you help me to check it again? Thank you!

Comment 18

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

(In reply to Ray Lin[:ralin] from comment #17)
> Created attachment 8738481 [details]
> ss.png
> 
> Sorry for the late response.
> 
> The visualized comment is very helpful, thanks, and it does make sense to
> have equal padding like so.
> 
> I've made some changes in this screenshot:
> - resize the scrubber to 12px. (previous's is 16px) 
> - rewrite the padding of boxes and SVG scaling with more proper syntax.
> 
> Could you help me to check it again? Thank you!

Looks AMAZING! thanks Ray!

Comment 19

[Ray Lin[:ralin]]

Attachment: Screen Shot 2016-04-08 at 17.36.04.png

It took me a while to build and test on B2G, sorry. This screenshot is captured from B2g emulator. 

Video widget is shared with B2G and mobile, therefore I think we have to fix it in this bug as well. I'd like not to change a lot styles on B2G, however it seems inevitable since there's rendering problem (as screenshot shown).

Margaret, do I need to replace B2G's icons to SVG as well? Would it scope out this bug too much? 

I am working on to make B2G looks like spec as possible. If anything I should or I shouldn't do, please tell me. Thanks!

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Ray Lin[:ralin] from comment #19)
> I am working on to make B2G looks like spec as possible. If anything I
> should or I shouldn't do, please tell me. Thanks!

You can make your own decision wisely. There is no obligation to maintain B2G anymore, so if it only takes < 20% of effort to update B2G style *while* update Fennec style, I would saw you could do it. If it's more than that, just file a Firefox OS :: General bug for others to pick it up the work, after the Fennec patch lands and breaks B2G.

Do the style sheet already marked with #ifdef defines? If so *maybe* use of a MOZ_B2G define would help. <-- Margret might disagree, though.


Thanks for your thoughtful comment!

Comment 21

[Ray Lin[:ralin]]

Attachment: Bug-1250741-Update-video-player-to-new-spec-v2.patch

Margaret could you help me review this patch? Thanks.

For B2G, thanks to Tim's advice, I fixed little to make layout not look too bad..., and maybe we could file another bug to polish it after landed.

Comment 22

[:Margaret Leibovic]

Comment on attachment 8739854 [details] [diff] [review]
Bug-1250741-Update-video-player-to-new-spec-v2.patch

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

Heh, I'm not too worried about b2g. I just wanted to mention it, since that is another consumer of videocontrols.xml.

Thanks for your work on this!

Comment 23

[:Margaret Leibovic]

Do you have mozilla-central commit access? If not, you can just mark the "checkin-needed" keyword on this bug.

Comment 24

[Ray Lin[:ralin]]

No problem Margaret, thanks!!

I don't have commit access and I'm not sure if it is appropriate to ask you to be my voucher(level 1)? but if okay, could you help me with it? I'd like to and will make more contribution to Firefox in the future, so I need at least "try access" to help my workflow.

Comment 25

[:Margaret Leibovic]

(In reply to Ray Lin[:ralin] from comment #24)
> No problem Margaret, thanks!!
> 
> I don't have commit access and I'm not sure if it is appropriate to ask you
> to be my voucher(level 1)? but if okay, could you help me with it? I'd like
> to and will make more contribution to Firefox in the future, so I need at
> least "try access" to help my workflow.

Yes, I will vouch! NI me on the level 1 commit access bug and I'll vouch.

Comment 26

[Pulsebot]

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

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd9ece951467

Comment 28

[Sorina Florean [:sflorean]]

Verified as fixed in build: Aurora 48.0a2 (2016-05-08);
Device: Asus ZenPad 8(Android 5.0.2).

Comment 29

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Sorina Florean {:Sorina} from comment #28)
> Verified as fixed in build: Aurora 48.0a2 (2016-05-08);
> Device: Asus ZenPad 8(Android 5.0.2).

This is strange. Are you not seeing bug 1267935 on Aurora 48.0a2 today? Could you reconfirm? Thanks!

Comment 30

[Ray Lin[:ralin]]

I just rebuild the latest aurora codebase, and found the problem is still. It seems that the issue is not easy to inspect on big screen devices like 8' ZenPad. Sorina, could you verify your build on different devices? Thanks!

Comment 31

[Sorina Florean [:sflorean]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #29)
> (In reply to Sorina Florean {:Sorina} from comment #28)
> > Verified as fixed in build: Aurora 48.0a2 (2016-05-08);
> > Device: Asus ZenPad 8(Android 5.0.2).
> 
> This is strange. Are you not seeing bug 1267935 on Aurora 48.0a2 today?
> Could you reconfirm? Thanks!

 
 (In reply to Ray Lin[:ralin] from comment #30)
> I just rebuild the latest aurora codebase, and found the problem is still.
> It seems that the issue is not easy to inspect on big screen devices like 8'
> ZenPad. Sorina, could you verify your build on different devices? Thanks!


On Aurora 48.0a2 I see the Bug 1267935, which is about video controls that are not correctly scaled for several video types. This bug is about update video player spec. If is  another bug logged for that issue I considered to mark this as verified, because video control spec are updated.

Tested on:
- LG G4 (Android 5.1);
- Nexus 5 (Android 6.0.1);
- Asus Transformer Pad (Android 4.2.1);
Video control spec are updated.