Closed Bug 1319301 Opened 8 years ago Closed 7 years ago

New video controls have leave a gray overlay over videos

Categories

(Toolkit :: Video/Audio Controls, defect)

Product:
Toolkit
Component:
Video/Audio Controls
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- verified

People

(Reporter: cpearce, Assigned: ralin)

References

Details

Attachments

(2 files)

Bug 1319301 - Part 1. fadeout grey spacer when play event is not triggered by controls.
58 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1319301 - Part 2. add a regression test for controlsSpacer.
58 bytes, text/x-review-board-request
jaws
: review+
Details 
If you open a video with the new builtin video controls enabled, the controls render a gray overlay over the video.

This must be a regression from Bug 1271765.

Steps to reproduce:
1. Open https://people-mozilla.org/~cpearce/mse-clearkey/ in Nightly (I have e10s enabled).
2. Observe that the letter boxing at the to and bottom of the video is gray, not black.

If you full screen the video, and then un-fullscreen, the letter boxing goes back to black as expected.
Flags: needinfo?(ralin)
Status: NEW → ASSIGNED
Flags: needinfo?(ralin)
(In reply to Chris Pearce (:cpearce) from comment #0)
> If you open a video with the new builtin video controls enabled, the
> controls render a gray overlay over the video.
> 
> This must be a regression from Bug 1271765.
> 
> Steps to reproduce:
> 1. Open https://people-mozilla.org/~cpearce/mse-clearkey/ in Nightly (I have
> e10s enabled).
> 2. Observe that the letter boxing at the to and bottom of the video is gray,
> not black.
> 
> If you full screen the video, and then un-fullscreen, the letter boxing goes
> back to black as expected.

Thank you :cpearce!!

One of the overlays still present even video is playing, and I didn't handle "play" event well on autoplay video.
New patch should fade out the spacer properly no matter "play" event is triggered by control or not.
Comment on attachment 8813010 [details]
Bug 1319301 - Part 1. fadeout grey spacer when play event is not triggered by controls.

https://reviewboard.mozilla.org/r/94528/#review94990

We should have a regression test for this. I'm pretty excited about the increased test coverage we have now for video controls and I'd like to keep that momentum moving. As we add regression tests, we will make sure that we don't have these types of bugs again when making changes and we can be more confident when writing the patches as well as reviewing that nothing unintended will change.
Attachment #8813010 - Flags: review?(jaws) → review-
Comment on attachment 8813010 [details]
Bug 1319301 - Part 1. fadeout grey spacer when play event is not triggered by controls.

https://reviewboard.mozilla.org/r/94528/#review94990

The case is that if `play` event is trigger by video.play() directly, controlsSpacer doesn't fadeout as expected. I added a test to cover this case, and the test result seems good :)
Comment on attachment 8813010 [details]
Bug 1319301 - Part 1. fadeout grey spacer when play event is not triggered by controls.

https://reviewboard.mozilla.org/r/94528/#review96050

::: toolkit/content/widgets/videocontrols.xml:560
(Diff revision 3)
>                              if (!this._triggeredByControls)
>                                  this.clickToPlay.hidden = true;
> +                                this.controlsSpacer.setAttribute("fadeout", "true");
>                              this._triggeredByControls = false;

This line is indented as if it should only be run if !this._triggeredByControls but the braces for the if-block are missing meaning that it will always run.
Attachment #8813010 - Flags: review?(jaws) → review-
Comment on attachment 8813936 [details]
Bug 1319301 - Part 2. add a regression test for controlsSpacer.

https://reviewboard.mozilla.org/r/95234/#review96054

::: toolkit/content/tests/widgets/test_bug1319301.html:50
(Diff revision 1)
> +  function getElementByAttribute(aName, aValue) {
> +    const videoControl = domUtils.getChildrenForNode(video, true)[1];
> +
> +    return SpecialPowers.wrap(document)
> +      .getAnonymousElementByAttribute(videoControl, aName, aValue);
> +  }

Please remove this and use the function added to head.js by bug 1311700.
Attachment #8813936 - Flags: review?(jaws) → review+
Depends on: 1311700
Comment on attachment 8813010 [details]
Bug 1319301 - Part 1. fadeout grey spacer when play event is not triggered by controls.

https://reviewboard.mozilla.org/r/94528/#review98724

::: toolkit/content/widgets/videocontrols.xml:2076
(Diff revisions 3 - 4)
>          </handler>
>      </handlers>
>  
>    </binding>
>  
> -  <binding id="touchControlsGonk" extends="chrome://global/content/bindings/videoControls.xml#touchControls">
> +  <binding id="touchControlsGonk" extends="chrome://global/content/bindings/videocontrols.xml#touchControls">

Did the different casing cause this to break for B2G?
Attachment #8813010 - Flags: review?(jaws) → review+
Comment on attachment 8813010 [details]
Bug 1319301 - Part 1. fadeout grey spacer when play event is not triggered by controls.

https://reviewboard.mozilla.org/r/94528/#review98724

> Did the different casing cause this to break for B2G?

I guess this might be a sort of rebase or mozreview problem, since I can't find any diff about this line in my patch.
Keywords: checkin-needed
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/04f20c35dcb7
Part 1. fadeout grey spacer when play event is not triggered by controls. r=jaws
https://hg.mozilla.org/integration/autoland/rev/9c9089213383
Part 2. add a regression test for controlsSpacer. r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/04f20c35dcb7
https://hg.mozilla.org/mozilla-central/rev/9c9089213383
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Tested this issue on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11 on Firefox Nightly 53.0a1 and I confirm that it's not reproducible anymore.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
OS: Unspecified → All
Hardware: Unspecified → All
Hey,

I am currently using the latest version of Firefox Dev Edition (v53) and this problem still persists and is not fixed.

This only happens for me, when Firefox uses its "new" Media Controls and not on sites like Twitch or Youtube, that provide their own media controls.
(In reply to LStudent from comment #17)
> Hey,
> 
> I am currently using the latest version of Firefox Dev Edition (v53) and
> this problem still persists and is not fixed.
> 
> This only happens for me, when Firefox uses its "new" Media Controls and not
> on sites like Twitch or Youtube, that provide their own media controls.

Thanks for your report and the discussion on IRC.

The issue is addressed by the other bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1332807, which just been uplifted hours ago, so you might need to wait to tomorrow to get a fine build.
(leaving some breadcrumb here)

The gray overlay have since been removed in bug 1374007 without its transition (and tests) removed. I will do that in bug 1449532.
See Also: → 1374007, 1449532
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: