729111 - Centered play video button is not re-enabled after video is resized (large)

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (Windows NT 6.1; rv:12.0a2) Gecko/20120220 Firefox/12.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0a2) Gecko/20120220 Firefox/12.0a2
Mozilla/5.0 (X11; Linux i686; rv:12.0a2) Gecko/20120220 Firefox/12.0a2

STR:
1. Open a page that contains an HTML5 video (e.g. http://www.robwinters.co.uk/lab/html5/video/drag-and-resize.htm)
2. Resize the video to less than 64px width
3. Resize the video back to more than 64px width

Actual result:
The play button from the center of the video is not re-enabled

Expected result:
After step 3, the center play button should be displayed again

Comment 1

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

To fix this bug, in /toolkit/content/widgets/videocontrols.xml the adjustControlSize function will need to be updated to make visible the clickToPlay element if it is currently hidden and the dimensions of the video are large enough to display it.

The changes should be made in an |else if (!this.clickToPlay.hidden)| block after this check: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#1247

Comment 2

[Mihaela Velimiroviciu (:mihaelav)]

Based on bug 666306 comment 68:
The play button should not be displayed if video size is smaller than 85px height, since it won't match the video (control bar will overlap the button).

Comment 3

[Charles Chan]

Attachment: Bug 729111: Patch-1

This patch takes cares of both of the issues mentioned. Please check if this is the appropriate way to fix the bug. Thanks.

One issue I noticed, after the video is resized, it takes a while before the playback controls reappear. Perhaps a refresh issue? (I am testing on Ubuntu.)

Comment 4

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

Comment on attachment 604825 [details] [diff] [review]
Bug 729111: Patch-1

Can you reupload with 8 lines of context for the patch? See here for more details: https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 5

[Charles Chan]

Attachment: Bug 729111: Patch-2

Resubmit the patch with 8 lines of context.

Comment 6

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

Comment on attachment 605280 [details] [diff] [review]
Bug 729111: Patch-2

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

Thanks for the extra context.

::: toolkit/content/widgets/videocontrols.xml
@@ +1251,5 @@
>                  _durationLabelWidth : 0,
>                  _muteButtonWidth : 0,
>                  _fullscreenButtonWidth : 0,
>                  _controlBarHeight : 0,
> +                _overlayPlayButtonHeight : 85,

Please leave this at 64. We can use the above _controlBarHeight to get our height of 85.

@@ +1260,5 @@
>  
>                      let videoHeight = this.video.clientHeight;
>                      let videoWidth = this.video.clientWidth;
>  
>                      if (this._overlayPlayButtonHeight > videoHeight || this._overlayPlayButtonWidth > videoWidth)

At this line we would just do:
> (this._overlayPlayButtonHeight + this._controlBarHeight) > videoHeight || ...

Comment 7

[Charles Chan]

Attachment: Bug 729111: Patch-3

(In reply to Jared Wein [:jaws] from comment #6)
> Please leave this at 64. We can use the above _controlBarHeight to get our
> height of 85.

Fixed, please review again. Thanks.

Comment 8

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

Comment on attachment 605290 [details] [diff] [review]
Bug 729111: Patch-3

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

::: toolkit/content/widgets/videocontrols.xml
@@ +1250,5 @@
>                  _playButtonWidth : 0,
>                  _durationLabelWidth : 0,
>                  _muteButtonWidth : 0,
>                  _fullscreenButtonWidth : 0,
> +                _controlBarHeight : 21,

We shouldn't set the height here. It's already set at https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#447. Just assume that it has already been set.

@@ +1260,5 @@
>  
>                      let videoHeight = this.video.clientHeight;
>                      let videoWidth = this.video.clientWidth;
>  
> +                    if ((this._overlayPlayButtonHeight + this._controlBarHeight) > videoHeight || this._overlayPlayButtonWidth > videoWidth)

nitpick: Lines should be < 80 characters long. Can you reformat this conditional to have a line break after the ||

Comment 9

[Charles Chan]

Attachment: Bug 729111: Patch-4

1) Line reformatted. 

2) Regarding the feedback:
> We shouldn't set the height here. It's already set at 
> https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#447. 
> Just assume that it has already been set.

Not sure if I completely understood clearly. Hopefully I interpreted it correctly. ;)

Comment 10

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

Comment on attachment 605301 [details] [diff] [review]
Bug 729111: Patch-4

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

Thank you for sticking with this.

::: toolkit/content/widgets/videocontrols.xml
@@ -1250,5 @@
>                  _playButtonWidth : 0,
>                  _durationLabelWidth : 0,
>                  _muteButtonWidth : 0,
>                  _fullscreenButtonWidth : 0,
> -                _controlBarHeight : 0,

Sorry, my wording before was pretty confusing. Basically, just leave this line as it was before. In other words, this line should be unchanged.

@@ +1260,5 @@
>                      let videoHeight = this.video.clientHeight;
>                      let videoWidth = this.video.clientWidth;
>  
> +                    if ((this._overlayPlayButtonHeight + this._controlBarHeight) > videoHeight || 
> +                        this._overlayPlayButtonWidth > videoWidth) {

nit: The style in this file only uses curly brackets on if-statements if there is more than one line in the if-block. Remove the curly brackets in this if-elseif.

Comment 11

[Charles Chan]

(In reply to Jared Wein [:jaws] from comment #10)
> > -                _controlBarHeight : 0,
> 
> Sorry, my wording before was pretty confusing. Basically, just leave this
> line as it was before. In other words, this line should be unchanged.

That was my other guess, but I couldn't figure out what would happen. 
By leaving the line as it was before, wouldn't it set the height to 0 and override whatever was set in line 447?

> nit: The style in this file only uses curly brackets on if-statements if there 
> is more than one line in the if-block. Remove the curly brackets in this if-
> elseif.

Ouch. I was trying to find a coding standing and simply following the style established here: https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#1004

Thanks for your help! (Working on the patch now ...)

Comment 12

[Charles Chan]

Attachment: Bug 729111: Patch-5

One more time.

Comment 13

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

(In reply to Charles Chan from comment #11)
> (In reply to Jared Wein [:jaws] from comment #10)
> > > -                _controlBarHeight : 0,
> > 
> > Sorry, my wording before was pretty confusing. Basically, just leave this
> > line as it was before. In other words, this line should be unchanged.
> 
> That was my other guess, but I couldn't figure out what would happen. 
> By leaving the line as it was before, wouldn't it set the height to 0 and
> override whatever was set in line 447?

It's created with 0, but line 447 will happen during initialization of the controls (and as such, will be after _controlBarHeight is initialized to 0).

> > nit: The style in this file only uses curly brackets on if-statements if there 
> > is more than one line in the if-block. Remove the curly brackets in this if-
> > elseif.
> 
> Ouch. I was trying to find a coding standing and simply following the style
> established here:
> https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/
> videocontrols.xml#1004

Good catch :) This file is odd in that it uses four spaces for indentation, whereas most files in the codebase use two spaces. Below is a link to the general coding style in case you are interested, but 99% of the time we just ask for consistency with the surrounding code.

https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

Comment 14

[Justin Dolske [:Dolske]]

Comment on attachment 605303 [details] [diff] [review]
Bug 729111: Patch-5

rs=me, assuming Jared's ok with it.

Comment 15

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

Comment on attachment 605303 [details] [diff] [review]
Bug 729111: Patch-5

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

Looks good! Thanks for the patch Charles :)

Comment 16

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

(In reply to Charles Chan from comment #3)
> One issue I noticed, after the video is resized, it takes a while before the
> playback controls reappear. Perhaps a refresh issue? (I am testing on
> Ubuntu.)

We only update the controls on mouseout (bug 462117) because we don't have the onresize event for elements yet (bug 227495). Sorry, I didn't see this question earlier.

Comment 17

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

(In reply to Jared Wein [:jaws] from comment #16)
> (In reply to Charles Chan from comment #3)
> > One issue I noticed, after the video is resized, it takes a while before the
> > playback controls reappear. Perhaps a refresh issue? (I am testing on
> > Ubuntu.)
> 
> We only update the controls on mouseout (bug 462117) because we don't have
> the onresize event for elements yet (bug 227495). Sorry, I didn't see this
> question earlier.

I forgot to add that the resize event for media controls is tracked by bug 696593.

Comment 18

[Frank Yan (:fryn)]

I removed an extra whitespace character from the patch and pushed it to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/c7e4db80d280

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 20

[Charles Chan]

Hello Phil, I have a quick glance at bug 735574, but not really sure how the (simple) changes in this ticket could cause a memory leak. Could you help? Basically, should I worry about it and possibly revert the changes for this ticket?

Thanks.

Comment 21

[Justin Lebar (not reading bugmail)]

> Basically, should I worry about it and possibly revert the changes for this ticket?

The patch seems innocuous to me, to be sure, we're going to back it out and see if the leak goes away.  But this patch needs to be merged to mozilla-inbound, first.

Comment 22

[Justin Lebar (not reading bugmail)]

Backed out on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/114c4148c6f3

Charles, it would be helpful if you'd watch bug 735574 and let us know in this bug whether you still see the test failure on mozilla-inbound after this backout.

Comment 23

[Charles Chan]

Sure. (I will also learn more about the mechanics of those automated tests.)

Comment 24

[Charles Chan]

It looks like the build was still failing after the changes were backed out:
114c4148c6f3 Justin Lebar – Back out bug 729111 (rev c7e4db80d280) due to suspected randomorange (bug 735574).
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=114c4148c6f3

But the next build seems to pass:
d067c50e01dc Ehsan Akhgari – Backout changeset ea6be5f60c42 (bug 722946) for breaking Windows builds
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d067c50e01dc

Comment 25

[Charles Chan]

Update: Based on the comment in bug 735574 (https://bugzilla.mozilla.org/show_bug.cgi?id=735574#c17), the issue is not related to this change.

Comment 26

[Justin Lebar (not reading bugmail)]

Sounds good; I'll re-land.

Comment 27

[Justin Lebar (not reading bugmail)]

Re-landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/672cda014328

Comment 28

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/672cda014328

Comment 29

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

Does the target milestone need to be updated due to the backout/relanding?

Comment 30

[Justin Lebar (not reading bugmail)]

(In reply to Jared Wein [:jaws] from comment #29)
> Does the target milestone need to be updated due to the backout/relanding?

No (although I thought we were using status-firefox-n, not target milestones?) because it made it in while trunk was still FF13 and was never backed out of FF13.  We backed out and re-landed on the FF14 branch only.

Comment 31

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

Attachment: Backout of patch from Fx13 Aurora

I'd like to back this out of Aurora. We still have time to fix the regressions on Nightly though.

[Approval Request Comment]
Regression caused by (bug #): bug 729111
User impact if declined: bug 737774 and bug 738401 have been reported so far
Testing completed (on m-c, etc.): n/a
Risk to taking this patch (and alternatives if risky): this will undo the fix for bug 729111, but regressions caused by this patch aren't worth the bug fix.
String changes made by this patch: none

Comment 32

[Alex Keybl [:akeybl]]

Comment on attachment 608435 [details] [diff] [review]
Backout of patch from Fx13 Aurora

[Triage Comment]
Once this has an r+, a=akeybl for this backout. Agreed that the new regression is worse than the previous.

Comment 33

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

Backed out on mozilla-aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/15fb08cbc70d

Comment 34

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

Due to lack of activity on this bug and the regressions it introduced, we should back out this patch on Nightly14 until a better patch comes along that doesn't carry with it these regressions.

Comment 35

[Chris Pearce [:cpearce (Not reading bugmail)]]

Agreed.

Comment 36

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

Backed out on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e4ac6159f8

Comment 37

[(no longer active)]

Backout merged: https://hg.mozilla.org/mozilla-central/rev/f1e4ac6159f8

Comment 38

[Owen Coutts [:tallOwen]]

Attachment: Patch 6

I tried fixing this bug. This patch only seems to work if you move your mouse over the video. To properly fix it we would need to some how listen for when the video element changes sizes. As I mentioned in bug 696593 I'm not sure there is a good way to do that.

Comment 39

[Owen Coutts [:tallOwen]]

Attachment: Patch 6.1

Uploaded wrong patch file.

Comment 40

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

Comment on attachment 705188 [details] [diff] [review]
Patch 6.1

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

This seems fine, but I still think we'll have to wait until we have better resize notifications.

Comment 41

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

Owen, the patch for bug 876426 added a "resizevideocontrols" event that we can use here. Can you pick this bug back up?

Comment 42

[Danny Chen [:DChen]]

Attachment: Bug 729111: Patch 7

Hi, this is my first time contributing to the Mozilla codebase.

This patch should avoid the control bar height issue mentioned in Comment 2. I haven't experienced the need to mouse-over the video as in Comment 38.

Comment 43

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

Comment on attachment 8398855 [details] [diff] [review]
Bug 729111: Patch 7

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

::: toolkit/content/widgets/videocontrols.xml
@@ +1390,3 @@
>                          this.clickToPlay.hidden = true;
> +                    else if (this.clickToPlay.hidden && this.video.currentTime === 0)
> +                        this.clickToPlay.hidden = false;

This works good, but it has a couple issues.

1. If the user starts playing the video, returns it back to time=0, and then resizes the video, the click-to-play button will reappear.
2. We allow the user to drag the scrubber of the video before playing, and the click to play button should stay visible. If the user resizes the video to small, then to large, the play button won't reappear.
3. Similar to (2), the video can have a non-zero starting time, and we still want to have the same behaviors.

You can check this.video.played.length to see if the video has been played. With that, this else-if condition can be changed to:

`else if (this.clickToPlay.hidden && !this.video.played.length)`

Comment 44

[Danny Chen [:DChen]]

Attachment: Patch 8

Okay, I've made the change. I didn't realize that attribute existed.

Comment 45

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

Comment on attachment 8399604 [details] [diff] [review]
Patch 8

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

Yeah, it's a pretty cool attribute and I think it's relatively new. There is more information about the API here, http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#media-elements

Congrats on your first patch. You can now reupload this patch with r=jaws at the end of the commit message and then set the "checkin-needed" keyword on the bug :)

Comment 46

[Danny Chen [:DChen]]

Attachment: Patch 8 with updated commit message

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c23e2ff6c65e

Comment 48

[Ryan VanderMeulen [:RyanVM]]

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

Comment 49

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

Danny, now that this patch has landed in the mozilla-central repository, it should appear in tomorrow's (4/3) Nightly builds. Thanks for your help on this bug, do you have another bug that you are interested in helping out on?

Comment 50

[Firefox Commit Bot]

The following changeset is now in Firefox Nightly:

> c23e2ff6c65e Bug 729111 - Redisplay centered play video button after video is resized. r=jaws

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central