Click to play overlay doesn't fade out if the video control bar play button is used

VERIFIED FIXED in Firefox 12

Status

()

defect
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 unaffected, firefox12+ verified)

Details

(Whiteboard: [qa!])

Attachments

(1 attachment, 1 obsolete attachment)

Posted patch Patch for bug (obsolete) — Splinter Review
STR:
1) Open http://www.mozilla.org/projects/firefox/prerelease.html
2) Press the play button on the control bar

Expected:
3) The overlay play button should fade out

Actual:
3) The overlay play button stays on top of the video


I'd also like to add a test here for this but I think the native anonymous content makes a test like this impossible, unless we do a reftest? Thoughts?
Attachment #592650 - Flags: review?(dolske)
The overlay play button disappears if you click on full screen mode during play.
Comment on attachment 592650 [details] [diff] [review]
Patch for bug

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

::: toolkit/content/widgets/videocontrols.xml
@@ +490,5 @@
>                              this.setupStatusFader();
>                              if (!this._triggeredByControls && this.dynamicControls && this.videocontrols.isTouchControl)
>                                  this.startFadeOut(this.controlBar);
> +                            if (!this.clickToPlay.hasAttribute("fadeout"))
> +                                this.handleClickToPlay();

Hmm, this seems a little strange for cause-effect.

How about just adding a |this.clickToPlay.hidden = true;| to togglePause()?
(In reply to Jared Wein [:jaws] from comment #0)

> I'd also like to add a test here for this but I think the native anonymous
> content makes a test like this impossible, unless we do a reftest? Thoughts?

Yay tests! \o/

Yeah, this has bugged me for a while. It used to be possible to use getBoxObject() to sneakily get into it, but that's removed now.

roc/bz might have a better idea, but one thought is to add a chrome-only API to some utility interface (nsIDOMWindowUtils, or something like that).

That way tests can do things like:

  var utils = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
                    .getInterface(Components.interfaces.nsIDOMWindowUtils);
  // or var utils = SpecialPowers.getDOMWindowUtils(window);
  var binding = utils.getMideoControlsFor(someVideoElement);

A someVideoElement.controlBinding property (callable and visible only to chrome) would be nicer, but I'm not sure if that's possible.
(In reply to Justin Dolske [:Dolske] from comment #2)
> Comment on attachment 592650 [details] [diff] [review]
> Patch for bug
> 
> Review of attachment 592650 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/widgets/videocontrols.xml
> @@ +490,5 @@
> >                              this.setupStatusFader();
> >                              if (!this._triggeredByControls && this.dynamicControls && this.videocontrols.isTouchControl)
> >                                  this.startFadeOut(this.controlBar);
> > +                            if (!this.clickToPlay.hasAttribute("fadeout"))
> > +                                this.handleClickToPlay();
> 
> Hmm, this seems a little strange for cause-effect.
> 
> How about just adding a |this.clickToPlay.hidden = true;| to togglePause()?

We can't do that because it would break the animation on the play button.
(In reply to Jared Wein [:jaws] from comment #4)

> > How about just adding a |this.clickToPlay.hidden = true;| to togglePause()?
> 
> We can't do that because it would break the animation on the play button.

Oh, derp.

This still feels odd to me, though. So, "how about" take 2...

* (starting from scratch)
* rename handleClickToPlay() to hideClickToPlay().
* remove togglePause() call from it
* make togglePause() call hideClickToPlay() [if it's calling .play()]
* make clickToPlayClickHandler() call togglePause() instead of handleClickToPlay()
* [optional] make the click listener for this.clickToPlay.addEventListener() and this.controlsSpacer.addEventListener() be the same callback function, since they're the same code now.

Seems like this makes what's happening a little clearer? Now all of our togglePause() callers will end up invoking hideClickToPlay(), without having to wait for the "play" event to be dispatched and indirectly hide the CTP button.
Duplicate of this bug: 724767
Duplicate of this bug: 725087
I've redone the patch based on the steps that you provided. I like your approach much better. Thanks!
Attachment #592650 - Attachment is obsolete: true
Attachment #592650 - Flags: review?(dolske)
Attachment #597188 - Flags: review?(dolske)
dolske: review ping?
Attachment #597188 - Flags: review?(dolske) → review+
When using the nightly I also noticed that the big play button would persist when the video is activated via tabbing to it and pressing space to play, so it may still be an issue for people using accessibility controls or similar.
(In reply to Lozzy from comment #11)
> When using the nightly I also noticed that the big play button would persist
> when the video is activated via tabbing to it and pressing space to play, so
> it may still be an issue for people using accessibility controls or similar.

Lozzy, the patch that was written to fix this bug should also fix the bug that you have mentioned. Can you please test to make sure that this was fixed the day after this gets merged to mozilla-central? If it's not fixed, please file a bug and CC me to it. Thanks!
https://hg.mozilla.org/mozilla-central/rev/4d2189c89d37
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla13
Forgot to check! Anyway, this fixed all the issues I had noticed. Though I expected it would; the patched code looked much more reliable. Thanks all!
Comment on attachment 597188 [details] [diff] [review]
Patch for bug v2

[Approval Request Comment]
Regression caused by (bug #): bug 666306
User impact if declined: play button won't hide if the smaller play button is used.
Testing completed (on m-c, etc.): been on m-c since 2/27, no bugs reported on it
Risk to taking this patch (and alternatives if risky): no risk expected
String changes made by this patch: none
Attachment #597188 - Flags: approval-mozilla-aurora?
Comment on attachment 597188 [details] [diff] [review]
Patch for bug v2

[Triage Comment]
Low risk fix in support of a recent change - approved for Aurora 12.
Attachment #597188 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa+]
Verified as fixed with the steps from comment 0 on:
Mozilla/5.0 (Windows NT 5.1; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0) Gecko/20100101 Firefox/12.0
20120403211507
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.