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

VERIFIED FIXED in mozilla13

Status

()

defect
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: Ehsan, Assigned: diogogmt)

Tracking

Trunk
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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.
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
QA Contact: video.audio → video.audio
Jared, is this something you could mentor?
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
Whiteboard: [good first bug][mentor=jwein][lang=css][lang=js]
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Duplicate of this bug: 711612
If nobody is currently working on this bug I would like to give it a try.
Assignee: nobody → diogo.gmt
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.
Status: NEW → ASSIGNED
Whiteboard: [good first bug][mentor=jwein][lang=css][lang=js] → [good first bug][mentor=jwein][lang=js]
Posted patch v1 (obsolete) — Splinter Review
Added a check to preserve the state of the video statistics when toggling fullscreen.
Attachment #594067 - Flags: feedback?
Posted patch V2 (obsolete) — Splinter Review
Attached wrong patch before.

This one does the check.
Attachment #594067 - Attachment is obsolete: true
Attachment #594067 - Flags: feedback?
Attachment #594069 - Flags: feedback?
Attachment #594069 - Flags: feedback? → feedback?(jwein)
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 neil@parkwaycc.co.uk.

::: 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?
Attachment #594069 - Flags: feedback?(jwein) → feedback+
Posted patch V3 (obsolete) — Splinter Review
Fixed format issues.
Attachment #594069 - Attachment is obsolete: true
Attachment #594143 - Flags: review?(neil)
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.
Attachment #594143 - Flags: review?(neil) → review+
Posted patch V4Splinter Review
Attachment #594143 - Attachment is obsolete: true
Attachment #596491 - Flags: review?(neil)
Attachment #596491 - Flags: review?(neil) → review+
Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/2ec69945ec11
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/2ec69945ec11
Status: ASSIGNED → RESOLVED
Last Resolved: 7 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
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.