1328060 - Total time is partially overlapped by mute button in some cases (video controls aren't responsive)

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 53, 32bit, ID 20161119030204 (2016-11-19)
STR_1:
1. Open http://ssyoutube.com/watch?v=s-UFPhz2nZ0 , click "Download", save the video
2. Open the file (saved in Step 1) in a new tab
3. Pause the video, seek it to the end (1:00:04)
4. Resize FF window to make last 2 digits of text label (total time) overlapped by mute button

AR:  2 digits of total time overlapped by mute button
ER:  Step 4 shouldn't be possible: no overlapping

This is regression from bug 1271765. Regression range:
> https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ec7fb4f14d3ec23ded7eea40ff49ebbcbec6bde1&tochange=8d2eecb7ea5a16e02862dd326ce4519082ce9901@ Ray Lin[:ralin]:
It seems that this is a regresion caused by your change. Please have a look.

Comment 1

[Hani Yacoub, Desktop QA]

Attachment: position and total time.png

Tested this issue on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11 on Firefox Nightly 53.0a1 (2017-01-10) and it's reproducible.
See screenshot attaced.

Comment 2

[Ray Lin[:ralin]]

The current implementation doesn't re-adjust the controls layout if the length of label changed. I have a idea about fixing the issue, will attach patch soon.

Thanks

Comment 3

[Ray Lin[:ralin]]

I was about to monitor the length variance whenever position label be updated, and call `adjustControlSize` if there's any. Unfortunately, the controls twitch while dragging forward and back on timeline.

Chrome is doing good about this as Chrome pre-allocates the space for the time label. e.g. 006:12 / 120:16 (in this case). We might need to consider similar way to prevent `adjustControlSize` frequently, perhaps allocating maximum possible width at beginning or changing the format of time label.

Comment 4

[Ray Lin[:ralin]]

Hi Jared,

I'm sorry to bother you many times in a day (so many NI? and feedback? in a day....)
Could you give me some advice about this bug? I guess we should prevent the length of scrubber from being changed(squeezed) while dragging or playing. Thanks.

Comment 5

[:Gijs (he/him)]

(In reply to Ray Lin[:ralin] from comment #4)
> Hi Jared,
> 
> I'm sorry to bother you many times in a day (so many NI? and feedback? in a
> day....)
> Could you give me some advice about this bug? I guess we should prevent the
> length of scrubber from being changed(squeezed) while dragging or playing.
> Thanks.

Not Jared, but it seems to me that it would make sense to pre-allocate enough space to display the maximum-required string for a particular video (once you get the video metadata). That would fix this problem, right?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Yeah, once we get the length of a video, we should calculate the number of digits that could be displayed at max and then set the width of the playback position/duration field to that size in 'ch' units.

If the video has a length of 1:00:13, then the field could show 1:00:13 / 1:00:13 and so we should set the width to 17ch.

Comment 7

[Ray Lin[:ralin]]

Attachment: Bug 1328060 - re-adjust controls when get metadata of video duration.

Review commit: https://reviewboard.mozilla.org/r/106372/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106372/

Comment 8

[Ray Lin[:ralin]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> Yeah, once we get the length of a video, we should calculate the number of
> digits that could be displayed at max and then set the width of the playback
> position/duration field to that size in 'ch' units.
> 
> If the video has a length of 1:00:13, then the field could show 1:00:13 /
> 1:00:13 and so we should set the width to 17ch.

I've tested some approaches to this issue, but none of them are perfectly work. It's fine to set 'ch' units to the container once 'loadedmetadata' triggered(per textNode's length), however, we need to update container's 'minWidth'[0] for `adjustControlSize()`.

Two conditions for updating correct 'minWidth':
1. 'loadedmetadata' has been triggered
2. duration span or position span should not be hidden (otherwise clientWidth would return 0)

It looks like there's no good timing that match both condition, as we could not make sure controlbar is still shown after 'loadedmetadata' event(e.g. in an autoplay video). I'm afraid that I don't have a proper way for this problem, but I came up with a workaround. Cloud you see the mozreview and give me feedback? Thanks :)


[0] https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#371-414

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8829250 [details]
Bug 1328060 - re-adjust controls when get metadata of video duration.

https://reviewboard.mozilla.org/r/106372/#review107562

So with this patch, we will always show the hour value in the position for media longer than or equal to 1 hour? For example: 0:13:42 / 1:32:45 ?

::: toolkit/content/tests/widgets/test_videocontrols.html:33
(Diff revision 1)
>  
>  const playButtonWidth = 30;
>  const playButtonHeight = 40;
>  const muteButtonWidth = 30;
>  const muteButtonHeight = 40;
> -const positionAndDurationWidth = 85;
> +const positionAndDurationWidth = 75;

Why did the positionAndDurationWidth decrease?

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

This was mentioned by SV as partially blocking "Video Play Visual Refresh" feature. Tracked for 53+

Comment 11

[Ray Lin[:ralin]]

Attachment: Screenshot 2017-01-24 14.02.06.png

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)

Yes, instead of 'ch', I tend to use max-required string format in the position label. As the screenshot shown, 17ch allocated slightly more space than the actual width of 'positionDurationBox' due to colon/slash/space character aren't as width as numeric character with non-monospace font.

I removed 'ch' in the patch, and found that the width around 75 reflects more precisely the real width of 'positionDurationBox'

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Okay thank you, the screenshot shows far too much space around the positionDuration box. We should make sure not to make the controls look bad in 99% of the cases just to fix the controls in 1% of the cases.

Comment 13

[Ray Lin[:ralin]]

Comment on attachment 8829250 [details]
Bug 1328060 - re-adjust controls when get metadata of video duration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106372/diff/1-2/

Comment 14

[Ray Lin[:ralin]]

sorry for removing r+ flag accidentally... I found the width 75 breaks `test_videocontrols.html` on Windows 8, there's should be a more sensible number that fit all platforms.

Comment 15

[Ray Lin[:ralin]]

Comment on attachment 8829250 [details]
Bug 1328060 - re-adjust controls when get metadata of video duration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106372/diff/2-3/

Comment 16

[Ray Lin[:ralin]]

Comment on attachment 8829250 [details]
Bug 1328060 - re-adjust controls when get metadata of video duration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106372/diff/3-4/

Comment 17

[Ray Lin[:ralin]]

Comment on attachment 8829250 [details]
Bug 1328060 - re-adjust controls when get metadata of video duration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106372/diff/4-5/

Comment 18

[Ray Lin[:ralin]]

latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7c6befc3dea

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6661e2f65c07
re-adjust controls when get metadata of video duration. r=jaws

Comment 20

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=74249697&repo=autoland

Comment 21

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2486eb5bb76
Backed out changeset 6661e2f65c07 for test failures in own test

Comment 22

[Ray Lin[:ralin]]

Comment on attachment 8829250 [details]
Bug 1328060 - re-adjust controls when get metadata of video duration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106372/diff/5-6/

Comment 23

[Ray Lin[:ralin]]

(In reply to Carsten Book [:Tomcat] from comment #20)
> sorry had to back this out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=74249697&repo=autoland

My fault that I didn't test the patch on Android, I should have been more careful.  The problem is that the main logic of video controls are still shared between Desktop and Fennec, but this patch is for Desktop only. 
Patch updated now, Thanks :)

Comment 24

[Ray Lin[:ralin]]

Comment on attachment 8829250 [details]
Bug 1328060 - re-adjust controls when get metadata of video duration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106372/diff/6-7/

Comment 25

[Ray Lin[:ralin]]

rebased and the android failure is fixed now. Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51a4eba2ccf183e842fc0aab942c509c2dff9ea1

Thanks.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ea9dbb84c03
re-adjust controls when get metadata of video duration. r=jaws

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0ea9dbb84c03

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 29

[Ray Lin[:ralin]]

Hi Hani,

Could you verify whether this issue is still? Thanks :)

Comment 30

[Hani Yacoub, Desktop QA]

Build ID: 20170212030213
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0

Verified as fixed on Windows 10 x 64, Mac OS X 10.11 and Ubuntu 16.04 x64 on Firefox Nightly 54.0a1.

Comment 31

[Ray Lin[:ralin]]

Comment on attachment 8829250 [details]
Bug 1328060 - re-adjust controls when get metadata of video duration.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1271765
[User impact if declined]: broken UI: some of controls would be overlapped by time slider
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: only update the size of time label once got metadata
[String changes made/needed]: none

thanks

Comment 32

[Gerry Chang [:gchang]]

Comment on attachment 8829250 [details]
Bug 1328060 - re-adjust controls when get metadata of video duration.

Fix a UI regression issue that total time icon is partially overlapped by mute button and was verified. Aurora53+.

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7f88a5494927

Comment 34

[Alexandru Simonca [:asimonca], Release Desktop QA]

Build ID: 20170307064827
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0

I have reproduced this issue on Windows 10 x64, Mac OS 10.11.6 and Ubuntu 16.04 x64 using Firefox Nightly 53.0a1 (id: 20161119030204). 
The issue is now VERIFIED FIXED on the same OS's on Firefox 53.0b1 (id: 20170307064827)