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

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: mihaelav, Assigned: DChen)

Tracking

12 Branch
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 10 obsolete attachments)

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
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
Whiteboard: [good first bug][mentor=jwein][lang=js]
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).
Posted patch Bug 729111: Patch-1 (obsolete) — Splinter Review
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.)
Attachment #604825 - Flags: review?(jwein)
Assignee: nobody → charles.wh.chan
Status: NEW → ASSIGNED
Posted patch Bug 729111: Patch-2 (obsolete) — Splinter Review
Resubmit the patch with 8 lines of context.
Attachment #604825 - Attachment is obsolete: true
Attachment #605280 - Flags: review?(jwein)
Attachment #604825 - Flags: review?(jwein)
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 || ...
Attachment #605280 - Flags: review?(jwein) → feedback+
Posted patch Bug 729111: Patch-3 (obsolete) — Splinter Review
(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.
Attachment #605280 - Attachment is obsolete: true
Attachment #605290 - Flags: review?(jwein)
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 ||
Attachment #605290 - Flags: review?(jwein) → feedback+
Posted patch Bug 729111: Patch-4 (obsolete) — Splinter Review
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. ;)
Attachment #605290 - Attachment is obsolete: true
Attachment #605301 - Flags: review?(jwein)
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.
Attachment #605301 - Flags: review?(jwein) → feedback+
(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 ...)
Posted patch Bug 729111: Patch-5 (obsolete) — Splinter Review
One more time.
Attachment #605301 - Attachment is obsolete: true
Attachment #605303 - Flags: review?(jwein)
(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
Attachment #605303 - Flags: review?(jwein)
Attachment #605303 - Flags: review?(dolske)
Attachment #605303 - Flags: feedback?(jwein)
Comment on attachment 605303 [details] [diff] [review]
Bug 729111: Patch-5

rs=me, assuming Jared's ok with it.
Attachment #605303 - Flags: review?(dolske) → review+
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 :)
Attachment #605303 - Flags: feedback?(jwein) → feedback+
(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.
(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.
I removed an extra whitespace character from the patch and pushed it to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/c7e4db80d280
Keywords: checkin-needed
Whiteboard: [good first bug][mentor=jwein][lang=js] → [good first bug][mentor=jwein][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c7e4db80d280
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=jwein][lang=js][fixed-in-fx-team] → [good first bug][mentor=jwein][lang=js]
Target Milestone: --- → mozilla13
Depends on: 735574
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.
> 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.
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.
Sure. (I will also learn more about the mechanics of those automated tests.)
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
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.
Sounds good; I'll re-land.
No longer depends on: 735574
Does the target milestone need to be updated due to the backout/relanding?
(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.
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
Attachment #608435 - Flags: review?(dolske)
Attachment #608435 - Flags: approval-mozilla-aurora?
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.
Attachment #608435 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #608435 - Flags: review?(dolske) → review+
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.
Backed out on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e4ac6159f8
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla14 → ---
Whiteboard: [good first bug][mentor=jwein][lang=js] → [good first bug][mentor=jaws][lang=js]
Posted patch Patch 6 (obsolete) — Splinter Review
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.
Assignee: charles.wh.chan → owen
Attachment #705187 - Flags: feedback?(jaws)
Posted patch Patch 6.1 (obsolete) — Splinter Review
Uploaded wrong patch file.
Attachment #705187 - Attachment is obsolete: true
Attachment #705187 - Flags: feedback?(jaws)
Attachment #705188 - Flags: feedback?(jaws)
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.
Attachment #705188 - Flags: feedback?(jaws) → feedback+
Owen, the patch for bug 876426 added a "resizevideocontrols" event that we can use here. Can you pick this bug back up?
Flags: needinfo?(owen)
Assignee: owen → nobody
Status: REOPENED → NEW
Flags: needinfo?(owen)
Posted patch Bug 729111: Patch 7 (obsolete) — Splinter Review
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.
Attachment #8398855 - Flags: review?(jaws)
Assignee: nobody → dannychen210
Status: NEW → ASSIGNED
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)`
Attachment #8398855 - Flags: review?(jaws) → feedback+
Posted patch Patch 8 (obsolete) — Splinter Review
Okay, I've made the change. I didn't realize that attribute existed.
Attachment #8398855 - Attachment is obsolete: true
Attachment #8399604 - Flags: feedback?(jaws)
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 :)
Attachment #8399604 - Flags: feedback?(jaws) → review+
Keywords: checkin-needed
Attachment #605303 - Attachment is obsolete: true
Attachment #608435 - Attachment is obsolete: true
Attachment #705188 - Attachment is obsolete: true
Attachment #8399604 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c23e2ff6c65e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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?
Attachment #608435 - Flags: approval-mozilla-aurora+
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
Depends on: 996122
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.