714071 - The Show Statistics setting is not preserved when toggling the full screen mode

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[(no longer active)]

STR:

1. Play a video and right click on it and hit Show Statistics.
2. Click the full-screen button.  The statistics disappear.
3. Right click on the video.  The Hide Statistics menu item is displayed.  Clicking on it doesn't do anything.

The same thing can also be observed when going back to normal mode from full-screen mode.

Comment 1

[Josh Matthews [:jdm]]

Jared, is this something you could mentor?

Comment 2

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

I'm not sure exactly why this is happening. It could either be due to the styling changes that are introduced when an element goes full screen (position:fixed, maybe a z-index change), or it is caused by the code that changes the controls based on the size of the video.

The files in question are located at:
/toolkit/content/widgets/videocontrols.xml
/toolkit/content/widgets/videocontrols.css

Comment 4

[Diogo Golovanevsky Monteiro [:diogogmt]]

If nobody is currently working on this bug I would like to give it a try.

Comment 5

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

Thanks Diogo for taking this.

I have narrowed down the cause of this. The bindings for the video controls are detached and reattached when the video enters and exits fullscreen (bug 718107). This is causing part of the state of the controls to be lost.

To fix this bug, we'll need to workaround bug 718107 and add a call to the |this.showStatistics()| function, passing |this.video.mozMediaStatisticsShowing| as the argument. This function will need to be called within either |init()| or |setupInitialState()|.

Only JS changes are necessary, I don't think any changes to CSS are required here.

Comment 6

[Diogo Golovanevsky Monteiro [:diogogmt]]

Attachment: v1

Added a check to preserve the state of the video statistics when toggling fullscreen.

Comment 7

[Diogo Golovanevsky Monteiro [:diogogmt]]

Attachment: V2

Attached wrong patch before.

This one does the check.

Comment 8

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

Comment on attachment 594069 [details] [diff] [review]
V2

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

This looks good and works great. Just fix the two nits below and then request review from [e-mail].

::: toolkit/content/widgets/videocontrols.xml
@@ +448,5 @@
>                      this.controlBar.hidden = true;
>                      this.adjustControlSize();
> +                    
> +                    // Preserve Statistics when toggling fullscreen mode
> +                    // Bug 714071

nit: Please change this to one line and add a period, such as |// Preserve statistics when toggling fullscreen mode due to bug 714071.|

@@ +451,5 @@
> +                    // Preserve Statistics when toggling fullscreen mode
> +                    // Bug 714071
> +                    if (this.video.mozMediaStatisticsShowing) {
> +                      this.showStatistics(true);
> +                    }      

nit: Can you remove this trailing whitespace?

Comment 9

[Diogo Golovanevsky Monteiro [:diogogmt]]

Attachment: V3

Fixed format issues.

Comment 10

[[e-mail]]

Comment on attachment 594143 [details] [diff] [review]
V3

>+                    if (this.video.mozMediaStatisticsShowing) {
>+                      this.showStatistics(true);
>+                    }
Two nits:
1. File style is not to brace single statements
2. File style is to have 4-space indent for some reason
r=me with those fixed.

Comment 11

[Diogo Golovanevsky Monteiro [:diogogmt]]

Attachment: V4

Comment 12

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

Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/2ec69945ec11

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/2ec69945ec11

Comment 14

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20120423 Firefox/13.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120424 Firefox/13.0a2
Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20120423 Firefox/13.0a2

Verified the fix on latest aurora13 builds:  Statistics overlay is preserved when toggling the full screen mode.