Closed Bug 1328060 Opened 8 years ago Closed 7 years ago

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

Categories

(Toolkit :: Video/Audio Controls, defect)

Product:
Toolkit
Component:
Video/Audio Controls
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox52 --- unaffected
firefox53 + verified
firefox54 --- verified

People

(Reporter: arni2033, Assigned: ralin)

References

Details

(Keywords: regression)

Attachments

(3 files)

position and total time.png
180.41 KB, image/png
Details
Bug 1328060 - re-adjust controls when get metadata of video duration.
59 bytes, text/x-review-board-request
gchang
: approval-mozilla-aurora+
Details
Screenshot 2017-01-24 14.02.06.png
1.29 MB, image/png
Details 
>>>   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.
No longer blocks: 1277113
Component: Untriaged → Video/Audio Controls
Product: Firefox → Toolkit
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.
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
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.
See Also: → 1328062
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.
Flags: needinfo?(jaws)
(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?
Assignee: nobody → ralin
Status: NEW → ASSIGNED
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.
Flags: needinfo?(jaws)
(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
Flags: needinfo?(jaws)
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?
Attachment #8829250 - Flags: review+
Flags: needinfo?(jaws)
This was mentioned by SV as partially blocking "Video Play Visual Refresh" feature. Tracked for 53+
status-firefox53: --- → affected
tracking-firefox53: --- → +
(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'
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.
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.
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6661e2f65c07
re-adjust controls when get metadata of video duration. r=jaws
Keywords: checkin-needed
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=74249697&repo=autoland
Flags: needinfo?(ralin)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e2486eb5bb76
Backed out changeset 6661e2f65c07 for test failures in own test
(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 :)
Flags: needinfo?(ralin)
rebased and the android failure is fixed now. Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51a4eba2ccf183e842fc0aab942c509c2dff9ea1

Thanks.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0ea9dbb84c03
re-adjust controls when get metadata of video duration. r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0ea9dbb84c03
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora approval on this when you get a chance.
status-firefox52: --- → unaffected
Flags: qe-verify+
Flags: needinfo?(ralin)
Hi Hani,

Could you verify whether this issue is still? Thanks :)
Flags: needinfo?(ralin) → needinfo?(hani.yacoub19)
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.
status-firefox54: fixed → verified
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
Attachment #8829250 - Flags: approval-mozilla-aurora?
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+.
Attachment #8829250 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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)
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
Flags: needinfo?(hani.yacoub19)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: