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?

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.

I've redone the patch based on the steps that you provided. I like your approach much better. Thanks!

dolske: review ping?

Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/4d2189c89d37

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!

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

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.

Pushed to mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/eba22b452eb0

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