762903 - In Full screen "Hide Controls" is not applied on OGG videos

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20120607 Firefox/15.0a2

On Firefox 15.0a2, Hide Controls is not applied while in Full Screen.

Reproducible: always

Steps to reproduce:
1. Navigate to  www.getpersonas.com and play the video
2. Right click on the video and from the context menu select "Full Screen"
3. Right click on the video and select "Show Statistics"
4. Right click again and from the context menu select "Hide Control" 

Expected results:
The controls bar should be hidden.
Also, while the controls are hidden it shouldn't be possible to:
- pause the video by one click
- see the video statistics
 
Actual results:
The controls are not hidden. The video can be paused by clicking once and the video statistics are still visible.

Comment 1

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

After thinking about this for a bit, I think we should back out both bug 713608 and 760696. We can then just add the [controls] attribute to the video if the video was lacking the attribute when it entered fullscreen. This will allow the user to remove the controls via the context menu.

We could then store a bit to know if we had force-enabled the controls so that we can hide the controls when exiting fullscreen if the user never made an explicit change while in fullscreen.

Comment 2

[Alice0775 White]

(In reply to Jared Wein [:jaws] from comment #1)
> After thinking about this for a bit, I think we should back out both bug
> 713608 and 760696

I agree with you.

Comment 3

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

Diogo, can you take this?

Comment 4

[Diogo Golovanevsky Monteiro [:diogogmt]]

(In reply to Jared Wein [:jaws] from comment #3)
> Diogo, can you take this?

Definitely :D

Should we use a similar approach as the solution of the first patch on bug 713608 ?

Always displaying the controls when the video enters fullscreen from the context menu, and if the video didn't have the controls enabled before entering fullscreen then disable them when exiting?

Would this be acceptable?
this.video.setAttribute("controls", "true");

or could cause some problems if some scripts are listening for DOMMutationEvents as mentioned in the bug 713608 review.

Comment 5

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

Actually I think that would be acceptable. That is what is done when the "Show Controls" and "Hide Controls" in the context menu is used.

Comment 6

[Diogo Golovanevsky Monteiro [:diogogmt]]

Attachment: v1

This patch is similar to the one proposed on Bug 713608.

I ran a few manual tests and this are results:

1. Open http://mozilla.jp/firefox/videos/
2. Play the video
3. Right click on the video and choose "Full Screen"
Results:
The default controls are visible

4. Exit fullscreen
Results:
Controls are hidden, since they were not visible by default.


On the http://www.getpersonas.com/en-US/ I encountered a few problems.
When playing the video, the controls are hidden by default.
For some reason the check: | !this.video.getAttribute("controls"); | is evaluating to true when it should be false since the controls are enabled on the video.

On this page http://tinyvid.tv/ the videos behave as expected.

A couple of points about the code:
The maybeHideControls variable is set to -1 everytime the video is initialized.
When the video enters fullscreen, if the controls were visible, then maybeHideControls will be false, if not it will be set to true and the controls will be removed when the videos exists fullscreen.
The reason for setting maybeHideControls to -1 at the init function is because the video is being initialized twice. I think there's already a bug to fix the issue.



Would a mochitest be able to test this change? Is it possible to fire context menu changes from  a mochitest perspective? Or there is a better way to test this?

Comment 7

[Diogo Golovanevsky Monteiro [:diogogmt]]

Attachment: v1-fixed

Had the wrong bug number listed in some comments.

Comment 8

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

This is a little confusing as to dealing with the various bugs that are in transit here.

How about we do the following:
1) Backout the patches for bug 713608 and bug 760696 from mozilla-aurora and mozilla-central.
2a) Mark this bug as fixed due to the backout.
2b) Mark bug 713608 as REOPENED.
2c) Mark bug 760696 as RESOLVED-INVALID.
3) Move the attached patch to bug 713608.

Sound good? I can do the backouts.

Comment 9

[Diogo Golovanevsky Monteiro [:diogogmt]]

(In reply to Jared Wein [:jaws] from comment #8)
> This is a little confusing as to dealing with the various bugs that are in
> transit here.
> 
> How about we do the following:
> 1) Backout the patches for bug 713608 and bug 760696 from mozilla-aurora and
> mozilla-central.
> 2a) Mark this bug as fixed due to the backout.
> 2b) Mark bug 713608 as REOPENED.
> 2c) Mark bug 760696 as RESOLVED-INVALID.
> 3) Move the attached patch to bug 713608.
> 
> Sound good? I can do the backouts.

Sounds good to me.
I think Bug 713608 would address the problems that are happening with the OGG videos in this bug.

Comment 10

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

(In reply to Jared Wein [:jaws] from comment #8)
> This is a little confusing as to dealing with the various bugs that are in
> transit here.
> 
> How about we do the following:
> 1) Backout the patches for bug 713608 and bug 760696 from mozilla-aurora and
> mozilla-central.
> 2a) Mark this bug as fixed due to the backout.
> 2b) Mark bug 713608 as REOPENED.
> 2c) Mark bug 760696 as RESOLVED-INVALID.
> 3) Move the attached patch to bug 713608.
> 
> Sound good? I can do the backouts.


This does sound good. Please go ahead and do the backouts.

Comment 11

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

Comment on attachment 633949 [details] [diff] [review]
v1-fixed

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

::: toolkit/content/widgets/videocontrols.xml
@@ +401,5 @@
>                      this.setFullscreenButtonState();
>  
> +                    // Bug 762903. Show the default controls if video
> +                    // enters fullscreen mode via the context menu.
> +                    this.maybeHideControls = -1;

I think we'd be better to call this mustRemoveControlsOnExitFullscreen to make it clearer what its purpose is. Initialize it to false.

@@ +959,5 @@
>                  },
>  
> +                maybeShowControls: function () {
> +                    // Keep track if the default controls were enabled when entering fullscreen
> +                    if (this.maybeHideControls === -1) {

This will run whenever a document containing a video enters fullscreen. So if a user enters fullscreen in a document containing a video with controls, this function will run and initialize maybeHideControls. Then if the users disables controls and enters fullscreen on the video, this branch won't run again, and the controls won't get turned back on. That's not the behaviour we're aiming for here right?

@@ +965,5 @@
> +                    }
> +
> +                    // Toggle default controls when entering/exiting fullscreen
> +                    if (this.isVideoInFullScreen()) {
> +                        this.video.setAttribute("controls", "true");

Remove the "if (this.maybeHideControls === -1)" statement above, and set mustRemoveControlsOnExitFullscreen inside this branch, before the setAttribute call obviously. I think you should use hasAttribute() rather than getAttribute(), since when controls="false" we should actually still show the controls (it's a "boolean attribute", meaning if it's present it's considered true, regardless of its value, it's confusing I know).

Comment 12

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

Comment on attachment 633949 [details] [diff] [review]
v1-fixed

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

Also, we should change the context menu so that if the user removes and then adds the controls, we don't remove them when the user exits fullscreen. i.e. explicit user action should override what we're doing here. So in the context menu's remove controls handler, set mustRemoveControlsOnExitFullscreen to false.

Comment 13

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

This bug has been marked fixed as the erroneous patches for bug 713608 and bug 760696 have now been backed out. Those backout patches are also awaiting aurora-approval to remove them from Fx15.

All future work should on this bug should move to bug 713608 (which was reopened).