*** Pulled out of bug 518008 *** - When autoplay is off, a play button should display in the center of the video and the entire content area should be a clickable play button. The video should fade partially to black. This should work the same in fullscreen mode as standard play mode, as well as only if controls are enabled.

Boriss: Can you please provide graphics and feedback on the progress of this patch so far? Thanks!

(In reply to comment #1) > Created attachment 543309 [details] [diff] [review] [review] > Patch for bug 666306 > > Boriss: Can you please provide graphics and feedback on the progress of this > patch so far? Thanks! Please note that the patch-so-far only implements this feature for Windows.

WiP of this bug. Waiting on final graphics and how the pretty should be handled.

Stephen, can you please provide a graphic for the overlay play button? I have tried making one myself but I don't think it is high enough quality to ship ;) The button currently uses a lot of the same styling as the toolbar buttons (with the blue glow on hover and inset shadows in the active state.

Fixed an issue where the overlay would not disappear if the user: 1. entered full screen while the overlay was visible 2. paused the video in full screen mode 3. exit full screen

This patch uses SVG to draw the play triangle (as opposed to a static graphic).

Comment on attachment 544931 [details] [diff] [review] Patch for bug 666306 - SVG implementation Stephen, can you provide a simple play graphic for this? Thanks!

Comment on attachment 544557 [details] Screenshot of current play button graphic We should redo all our icons with Paint!

This patch is dependent upon the patch for bug 462117, and uses the "Play Button Overlay SVG" graphic that shorlander has supplied. The interactions seen are such that: - If the video is large enough, the play button will expand and fade out upon being clicked. - If the video is not large enough for the expand/fadeout animation, the play button will simply disappear. - If the video is not large enough to fit the 64x64 pixel graphic, we will not show the play graphic.

Comment on attachment 553594 [details] [diff] [review] Patch for bug 666306 v2 Review of attachment 553594 [details] [diff] [review]: ----------------------------------------------------------------- - Hover and the animation look great! - Loading animation for videos is visible behind the play overlay but it is off-center - It's a little difficult to see the play overlay on light videos, so we could either tweak the graphic or use a dark overlay until the video is played

Jared, if you could unbitrot this patch, I'd be glad to provide feedback on it.

This patch fixes the feedback issues that shorlander pointed out earlier. Please note that this patch is dependent upon the patch for bug 462117.

Comment on attachment 555253 [details] [diff] [review] Patch for bug 666306 v3 Review of attachment 555253 [details] [diff] [review]: ----------------------------------------------------------------- Looks great!

Updating the patch to apply cleanly on top of the update patch for bug 462117. Carrying forward the r+ from shorlander.

I meant to say that it carried forward the ui-review+ from shorlander.

Comment on attachment 555855 [details] [diff] [review] Patch for bug 666306 v4 >+ <image class="playButtonOverlay" src="chrome://global/skin/media/playButtonOverlay.svg"/> The image source belongs in the stylesheets, using list-style-image.

Moved the image source to the CSS files and fixed some issues with the conditionals that were guarding whether to show the control bar. NB: This patch is to be applied on top of the patch for bug 462117.

Made a typo in my fix for the conditionals in the previous patch. Carrying forward the ui-r+ from shorlander.

Removing the old screenshot from this bug so people can see what it looks like now :)

Try run for f3928bd3a5cc is complete. Detailed breakdown of the results available here: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=f3928bd3a5cc Results (out of 12 total builds): success: 7 warnings: 5 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jwein@mozilla.com-f3928bd3a5cc

Added a fix for mozNoDynamicControls. Carrying forward the ui-review+ from shorlander.

Try run for 5fedba05d890 is complete. Detailed breakdown of the results available here: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=5fedba05d890 Results (out of 2 total builds): exception: 2 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jwein@mozilla.com-5fedba05d890

Try run for 5f8190d4bbed is complete. Detailed breakdown of the results available here: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=5f8190d4bbed Results (out of 27 total builds): success: 24 warnings: 3 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jwein@mozilla.com-5f8190d4bbed

Sigh... there were still a couple test failures in my previous patch. This patch should fix those test failures. Pushed to try: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=198b943ef6c1 NB: Carrying forward ui-r+ from shorlander, and please note that this patch needs to be applied on top of the patch from bug 462117.

Try run for 198b943ef6c1 is complete. Detailed breakdown of the results available here: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=198b943ef6c1 Results (out of 27 total builds): success: 25 warnings: 2 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jwein@mozilla.com-198b943ef6c1

The assertions generated from this patch have been logged as bug 688044.

These patches triggers the assertion: ###!!! ASSERTION: must have binding parent when in native anonymous subtree with a parent node: '!IsInNativeAnonymousSubtree() || GetBindingParent() || !GetParent()', file ../../../dist/include/nsIContent.h, line 271 for "testcase 2" in bug 571959: https://bugzilla.mozilla.org/attachment.cgi?id=492043 I don't see the above assertion without these patches (on Linux). Bug 688044 does fix the "ASSERTION: Why is the root in mDirtyRoots already?" but not the "must have binding parent" one.

Unbitrotted based on the updates for bug 462117.

Comment on attachment 561931 [details] [diff] [review] Patch for bug 666306 Review of attachment 561931 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/widgets/videocontrols.xml @@ -340,5 @@ > this.statusIcon.setAttribute("type", "error"); > this.setupStatusFader(true); > - } else { > - this.statusIcon.setAttribute("type", "throbber"); > - this.setupStatusFader(); Eh? Accidental removal? @@ +394,5 @@ > switch (aEvent.type) { > case "play": > this.setPlayButtonState(false); > + if (this.clickToPlayOverlay.getAttribute("hidden") == "true") > + this.setupStatusFader(); Hmm. I wonder if we should use a <deck> instead. Or maybe control with CSS? @@ +991,5 @@ > var self = this; > this.muteButton.addEventListener("command", function() { self.toggleMute(); }, false); > this.playButton.addEventListener("command", function() { self.togglePause(); }, false); > this.controlsSpacer.addEventListener("click", function(e) { if (e.button == 0) { self.togglePause(); } }, false); > + this.clickToPlayOverlay.addEventListener("click", function(e) { This inline function is kinda long, more to Utils (ala toggleMute / togglePause). @@ +997,5 @@ > + let videoHeight = self.video.clientHeight; > + let videoWidth = self.video.clientWidth; > + let overlayPlayButtonHeight = self.clickToPlay.clientHeight; > + let overlayPlayButtonWidth = self.clickToPlay.clientWidth; > + let animationScale = 3; Add a comment here about what this is for. I noticed this from testing, wherein a reasonably sized video wasn't showing the effect but a slightly larger one was. A value less than the actual final scale factor would be fine, since it's largely transparent by that point and a little cropping wouldn't be noticed. @@ +1008,5 @@ > + self.clickToPlayOverlay.setAttribute("fadeout", "true"); > + self.togglePause(); > + } > + }, false); > + this.playButtonOverlay.addEventListener("load", function (e) { e.stopPropagation(); }, false); Eww. Is this still firing a load even with list-style-image? We should really avoid this if possible (eg via background-image). Note that a capturing listener will still see the event, I believe. ::: toolkit/themes/winstripe/global/media/videocontrols.css @@ +199,5 @@ > + height: 64px; > +} > + > +.clickToPlayOverlay { > + background-image: url(chrome://global/skin/icons/tabprompts-bgtexture.png); Hmm, this source image actually looks wrong on Windows. I think the original was scaled incorrectly? The top row and column are blank, and the grid-like texture is 2x2 in the center but 2x1 towards the edges...

(In reply to Justin Dolske [:Dolske] from comment #30) > ::: toolkit/themes/winstripe/global/media/videocontrols.css > @@ +199,5 @@ > > + height: 64px; > > +} > > + > > +.clickToPlayOverlay { > > + background-image: url(chrome://global/skin/icons/tabprompts-bgtexture.png); > > Hmm, this source image actually looks wrong on Windows. I think the original > was scaled incorrectly? The top row and column are blank, and the grid-like > texture is 2x2 in the center but 2x1 towards the edges... Filed: https://bugzilla.mozilla.org/show_bug.cgi?id=688918

(In reply to Justin Dolske [:Dolske] from comment #30) > Comment on attachment 561931 [details] [diff] [review] [diff] [details] [review] > Patch for bug 666306 > > Review of attachment 561931 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > ::: toolkit/content/widgets/videocontrols.xml > @@ -340,5 @@ > > this.statusIcon.setAttribute("type", "error"); > > this.setupStatusFader(true); > > - } else { > > - this.statusIcon.setAttribute("type", "throbber"); > > - this.setupStatusFader(); > > Eh? Accidental removal? Since the play button will be showing, we don't want to show the throbber behind it. The throbber here will only show if needed after the play button has been pressed. > > @@ +394,5 @@ > > switch (aEvent.type) { > > case "play": > > this.setPlayButtonState(false); > > + if (this.clickToPlayOverlay.getAttribute("hidden") == "true") > > + this.setupStatusFader(); > > Hmm. I wonder if we should use a <deck> instead. Or maybe control with CSS? We use |hidden| a couple places for transitions with video controls. I'm not sure how animations would work with <deck> > > @@ +991,5 @@ > > var self = this; > > this.muteButton.addEventListener("command", function() { self.toggleMute(); }, false); > > this.playButton.addEventListener("command", function() { self.togglePause(); }, false); > > this.controlsSpacer.addEventListener("click", function(e) { if (e.button == 0) { self.togglePause(); } }, false); > > + this.clickToPlayOverlay.addEventListener("click", function(e) { > > This inline function is kinda long, more to Utils (ala toggleMute / > togglePause). Moved. > @@ +997,5 @@ > > + let videoHeight = self.video.clientHeight; > > + let videoWidth = self.video.clientWidth; > > + let overlayPlayButtonHeight = self.clickToPlay.clientHeight; > > + let overlayPlayButtonWidth = self.clickToPlay.clientWidth; > > + let animationScale = 3; > > Add a comment here about what this is for. > > I noticed this from testing, wherein a reasonably sized video wasn't showing > the effect but a slightly larger one was. A value less than the actual final > scale factor would be fine, since it's largely transparent by that point and > a little cropping wouldn't be noticed. Good points. I've changed the scaling factor to |2|. > @@ +1008,5 @@ > > + self.clickToPlayOverlay.setAttribute("fadeout", "true"); > > + self.togglePause(); > > + } > > + }, false); > > + this.playButtonOverlay.addEventListener("load", function (e) { e.stopPropagation(); }, false); > > Eww. Is this still firing a load even with list-style-image? We should > really avoid this if possible (eg via background-image). Note that a > capturing listener will still see the event, I believe. Yeah, kinda nasty. I've switched to |background-image| and pushed to try to make sure that it doesn't break any tests related to the |load| event.

Comment on attachment 562203 [details] [diff] [review] Patch for bug 666306 Review of attachment 562203 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/widgets/videocontrols.xml @@ +226,5 @@ > </vbox> > + > + <vbox class="clickToPlayOverlay" flex="1" hidden="true"> > + <box class="clickToPlay"> > + <image class="playButtonOverlay"/> I think the clickToPlay box is superfluous, you should be able to just make the <image> a direct child? Nit: s/playButtonOverlay/clickToPlayButton/, "overlay" should just be used to refer to entire sheets. @@ +394,5 @@ > switch (aEvent.type) { > case "play": > this.setPlayButtonState(false); > + if (this.clickToPlayOverlay.getAttribute("hidden") == "true") > + this.setupStatusFader(); I see why you want the check now. I think it would be preferable to move the hidden-check into setupStatusFader() itself, so callers don't have to know about this detail. Or, better yet, use CSS + an attribute on the the <stack> (or mediaControlsFrame) so that we can just control which overlay is shown (either via visibility:hidden or opacity:0 if you need to crossfade between 'em). @@ +464,5 @@ > + // In the scenario where a user enters Full Screen from the context menu while > + // the |clickToPlayOverlay| is visible, the overlay will need to be hidden > + // when the user exits Full Screen. > + this.clickToPlayOverlay.setAttribute("fadeout", "true"); > + I don't understand this comment. Why should entering/leaving FS change the overlay? Would be nice to avoid setting an attribute on every timeupdate, since there are a lot of those... But not worth worrying about this perf detail too much for the moment, since post bust 545812 we'll be changing our full-screen stuff soon enough (and there's a CSS pseudoclass for knowing when we're full-screen to leverage). @@ +1009,1 @@ > Nit: instead of |if(!a) b else c|, do |if (a) c else b|. Less double-negative thinking. :) But I'm not sure I understand why dynamicControls is involved in this check. Shouldn't this just be a check to see if, when shouldShow == true, we should actually show the clickToPlay overlay instead? ::: toolkit/themes/pinstripe/global/jar.mn @@ +149,5 @@ > skin/classic/global/media/error.png (media/error.png) > skin/classic/global/media/throbber.png (media/throbber.png) > skin/classic/global/media/stalled.png (media/stalled.png) > skin/classic/global/media/volumeThumb.png (media/volumeThumb.png) > + skin/classic/global/media/playButtonOverlay.svg (media/playButtonOverlay.svg) Looking at the SVG, I wonder if performance is going to be a concern... There's a lot of gradients and blurs to recompute when we're scaling it. Fine for now, but let's do a followup bug to consider just using a PNG. To make it look decent while zooming, we could just display it normally at 1/2 size. Pixelization shouldn't be noticeable as it zooms to 3x, since it's mostly transparent by then anyway. (How good/bad the < 100% scaling looks is a different matter... Anyway, followup). ::: toolkit/themes/pinstripe/global/media/playButtonOverlay.svg @@ +4,5 @@ > + xmlns:svg="http://www.w3.org/2000/svg" > + xmlns="http://www.w3.org/2000/svg" > + xmlns:xlink="http://www.w3.org/1999/xlink" > + version="1.1" > + id="videoPlayButtonOverlay" No "overlay". Ditto for filename, want to emphasize the "click to play" part more than just "play" anyway.

> These patches triggers the assertion: With what stack, on what node? Separate bug?

Try run for c167e750ba4d is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=c167e750ba4d Results (out of 109 total builds): exception: 1 success: 104 warnings: 3 failure: 1 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jwein@mozilla.com-c167e750ba4d

Updated patch based on review comments.

Noticed some lines were accidentally commented out and the calculation for the size of the play button was being done incorrectly.

Unbitrotted the patch.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=445a50cc239e

Comment on attachment 565381 [details] [diff] [review] Patch for bug 666306 (rebased) Review of attachment 565381 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/widgets/videocontrols.xml @@ +280,5 @@ > </hbox> > </vbox> > + > + <vbox class="clickToPlayOverlay" flex="1" hidden="true"> > + <image class="clickToPlayButton"/> s/image/box just for consistency with the status overlay @@ +332,5 @@ > isAudioOnly : false, > > setupStatusFader : function(immediate) { > + if (this.clickToPlayOverlay.getAttribute("hidden") != "true") > + return; Use .hidden instead of .getAttribute()? We've tried, at least in the past, to initially show the throbber until the video has loaded enough to actually play without quickly stalling... I sorta wonder if we should have some kind of inactive/loading state for this button, so the big happy button doesn't tempt the user to click, only to immediatelly stall because it was still loading data. OTOH, that leads to 2 UI transitions per video, which would be kind of blinky as the page loads. I guess we'll try it as-is and see how it feels. @@ +402,5 @@ > this.bufferBar.setAttribute("value", 0); > > // Set the current status icon. > if (this.video.error || this.video.networkState == this.video.NETWORK_NO_SOURCE) { > + this.clickToPlayOverlay.setAttribute("hidden", "true"); also .hidden @@ +409,5 @@ > } > > // An event handler for |onresize| should be added when bug 227495 is fixed. > + this._overlayPlayButtonHeight = parseInt(window.getComputedStyle(this.clickToPlayButton).height, 10); > + this._overlayPlayButtonWidth = parseInt(window.getComputedStyle(this.clickToPlayButton).width, 10); Use .clientHeight / .clientWidth? The other code here is... @@ +523,5 @@ > + // In the scenario where a user enters Full Screen from the context menu while > + // the |clickToPlayOverlay| is visible, the overlay will need to be hidden > + // when the user exits Full Screen. > + if (!this.clickToPlayOverlay.hasAttribute("fadeout")) > + this.clickToPlayOverlay.setAttribute("fadeout", "true"); I still don't quite get this... why do this? @@ +762,2 @@ > if (!isMouseOver) > this.adjustControlSize(); Should we adjust the control size before bailing out? I guess this raises the question of if the controls should always stay present, fade back in, or why. EG, if after clicking the play button I want to adjust volume, I think with this patch I'd have to mouseout and then mouseover again? @@ +835,5 @@ > // controlling volume. > }, > > + handleClickToPlay : function () { > + if (this.clickToPlayOverlay.getAttribute("hidden") != "true") { Seems like this is a useless check, how else would this code get called? What might make more sense here is a flag to indicate if it's been clicked once, since I suspect it's possible to click while animating (before it's actually hidden), in which case I'm not sure what happens :) @@ +850,5 @@ > + } else { > + this.clickToPlayOverlay.removeAttribute("immediate"); > + } > + this.clickToPlayOverlay.setAttribute("fadeout", "true"); > + this.togglePause(); Ah, this reminds me. Need to make sure that if the page itself calls play(), the CTP overlay should go away. Probably just an unconditional hiding of it in the |play| handler? @@ +1094,4 @@ > adjustControlSize : function adjustControlSize() { > + if (this.isAudioOnly) { > + this.clickToPlayOverlay.setAttribute("hidden", "true"); > + return; This isn't really control-size related... This should go into 2 spots: the |loadedmetadata| handler, and setupInitialState(). Both of these are where we decide is isAudioOnly should be true. @@ +1178,5 @@ > + // otherwise we want to just show the |controlBar| at all times. > + if (this.dynamicControls) > + this.startFade(this.clickToPlayOverlay, shouldShow, true); > + else > + this.startFade(this.controlBar, shouldShow, true); From above, maybe we want to show the control bar always? @@ +1203,1 @@ > this.videocontrols.addEventListener("transitionend", function(e) { self.onTransitionEnd(e); }, false); Doesn't |transitionend| bubble? The existing listener should catch this without needing a second listener. @@ +1329,5 @@ > this.videocontrols.addEventListener("transitionend", this, false); > }, > > handleEvent : function (aEvent) { > + if (aEvent.type == "transitionend" && aEvent.originalTarget == this.videocontrols) { This is only for the touchcontrols, seems like this is not needed?

Noticed that I forgot to fix the adjusting of controls on mousein/mouseout.

(In reply to Justin Dolske [:Dolske] from comment #40) > Comment on attachment 565381 [details] [diff] [review] [diff] [details] [review] > Patch for bug 666306 (rebased) > > Review of attachment 565381 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > ::: toolkit/content/widgets/videocontrols.xml > @@ +280,5 @@ > > </hbox> > > </vbox> > > + > > + <vbox class="clickToPlayOverlay" flex="1" hidden="true"> > > + <image class="clickToPlayButton"/> > > s/image/box just for consistency with the status overlay > > @@ +332,5 @@ > > isAudioOnly : false, > > > > setupStatusFader : function(immediate) { > > + if (this.clickToPlayOverlay.getAttribute("hidden") != "true") > > + return; > > Use .hidden instead of .getAttribute()? > > We've tried, at least in the past, to initially show the throbber until the > video has loaded enough to actually play without quickly stalling... I sorta > wonder if we should have some kind of inactive/loading state for this > button, so the big happy button doesn't tempt the user to click, only to > immediatelly stall because it was still loading data. > > OTOH, that leads to 2 UI transitions per video, which would be kind of > blinky as the page loads. I guess we'll try it as-is and see how it feels. > > @@ +402,5 @@ > > this.bufferBar.setAttribute("value", 0); > > > > // Set the current status icon. > > if (this.video.error || this.video.networkState == this.video.NETWORK_NO_SOURCE) { > > + this.clickToPlayOverlay.setAttribute("hidden", "true"); > > also .hidden > > @@ +409,5 @@ > > } > > > > // An event handler for |onresize| should be added when bug 227495 is fixed. > > + this._overlayPlayButtonHeight = parseInt(window.getComputedStyle(this.clickToPlayButton).height, 10); > > + this._overlayPlayButtonWidth = parseInt(window.getComputedStyle(this.clickToPlayButton).width, 10); > > Use .clientHeight / .clientWidth? The other code here is... .clientHeight/.clientWidth can only be used if the clickToPlayButton is not hidden. We do this quickly with the control bar, but the control bar is much smaller and would be harder to notice a flicker. > > @@ +523,5 @@ > > + // In the scenario where a user enters Full Screen from the context menu while > > + // the |clickToPlayOverlay| is visible, the overlay will need to be hidden > > + // when the user exits Full Screen. > > + if (!this.clickToPlayOverlay.hasAttribute("fadeout")) > > + this.clickToPlayOverlay.setAttribute("fadeout", "true"); > > I still don't quite get this... why do this? I've tried to make the comment easier to read. Here is the updated comment: // If a user enters Full Screen from the context menu while // the |clickToPlayOverlay| is visible and pauses the video before // exiting, the |clickToPlayOverlay| is still visible when the user // exits full screen since a separate instance of the video player // is used to display the full screen video. > @@ +762,2 @@ > > if (!isMouseOver) > > this.adjustControlSize(); > > Should we adjust the control size before bailing out? Yeah we should. I've fixed that. > I guess this raises the question of if the controls should always stay > present, fade back in, or why. EG, if after clicking the play button I want > to adjust volume, I think with this patch I'd have to mouseout and then > mouseover again? With the previous patch, you wouldn't have to mouseout/mouseover. On the next mouseover event the controls would be shown. But anyways, this behavior has been changed to always show the control bar if the clickToPlayOverlay is being shown. This was discussed during a UX meeting and we felt that it also helps users who want to mute the video before it begins playing. > @@ +835,5 @@ > > // controlling volume. > > }, > > > > + handleClickToPlay : function () { > > + if (this.clickToPlayOverlay.getAttribute("hidden") != "true") { > > Seems like this is a useless check, how else would this code get called? > > What might make more sense here is a flag to indicate if it's been clicked > once, since I suspect it's possible to click while animating (before it's > actually hidden), in which case I'm not sure what happens :) A very fast double-click results in the video being paused. YouTube's embedded player seems to have this same issue. I think maybe we could do a follow-up bug for "double-click on clickToPlayOverlay should only start playing the video, not play followed quickly by pause". > @@ +850,5 @@ > > + } else { > > + this.clickToPlayOverlay.removeAttribute("immediate"); > > + } > > + this.clickToPlayOverlay.setAttribute("fadeout", "true"); > > + this.togglePause(); > > Ah, this reminds me. Need to make sure that if the page itself calls play(), > the CTP overlay should go away. Probably just an unconditional hiding of it > in the |play| handler? Done. But needed to check that the play wasn't called through our own controls or the animation will get short-circuited. > @@ +1094,4 @@ > > adjustControlSize : function adjustControlSize() { > > + if (this.isAudioOnly) { > > + this.clickToPlayOverlay.setAttribute("hidden", "true"); > > + return; > > This isn't really control-size related... This should go into 2 spots: the > |loadedmetadata| handler, and setupInitialState(). Both of these are where > we decide is isAudioOnly should be true. Done. > @@ +1178,5 @@ > > + // otherwise we want to just show the |controlBar| at all times. > > + if (this.dynamicControls) > > + this.startFade(this.clickToPlayOverlay, shouldShow, true); > > + else > > + this.startFade(this.controlBar, shouldShow, true); > > From above, maybe we want to show the control bar always? Yeah, that's a better solution. I've made that change. > @@ +1203,1 @@ > > this.videocontrols.addEventListener("transitionend", function(e) { self.onTransitionEnd(e); }, false); > > Doesn't |transitionend| bubble? The existing listener should catch this > without needing a second listener. Good catch. Thanks. > @@ +1329,5 @@ > > this.videocontrols.addEventListener("transitionend", this, false); > > }, > > > > handleEvent : function (aEvent) { > > + if (aEvent.type == "transitionend" && aEvent.originalTarget == this.videocontrols) { > > This is only for the touchcontrols, seems like this is not needed? Yeah, not needed. Thanks.

Comment on attachment 569719 [details] [diff] [review] Patch for bug 666306 v2.01 Review of attachment 569719 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/widgets/videocontrols.xml @@ +394,5 @@ > if (this.video.readyState >= this.video.HAVE_METADATA) { > if (this.video instanceof HTMLVideoElement && > (this.video.videoWidth == 0 || this.videoHeight == 0)) > this.isAudioOnly = true; > + this.clickToPlayOverlay.hidden = true; Either the indentation or the logic is wrong here. I think the latter. feedback- for this. @@ +431,5 @@ > this._durationLabelWidth = this.durationLabel.clientWidth; > this._muteButtonWidth = this.muteButton.clientWidth; > this._controlBarHeight = this.controlBar.clientHeight; > if (controlBarWasHidden) > this.controlBar.setAttribute("hidden", "true"); If we can get the property instead of the attribute, can we also set the property instead? ::: toolkit/themes/pinstripe/global/media/videoClickToPlayButton.svg @@ +12,5 @@ > + <linearGradient id="whiteGradientStops"> > + <stop id="whiteGradientStop01" style="stop-color:#ffffff;stop-opacity:.95" offset="0" /> > + <stop id="whiteGradientStop02" style="stop-color:#ffffff;stop-opacity:.75" offset=".45" /> > + <stop id="whiteGradientStop03" style="stop-color:#ffffff;stop-opacity:.72" offset=".55" /> > + <stop id="whiteGradientStop04" style="stop-color:#ffffff;stop-opacity:.65" offset="1" /> NIT: only 1 space needed before offset attribute for each of these. ::: toolkit/themes/winstripe/global/media/videoClickToPlayButton.svg @@ +12,5 @@ > + <linearGradient id="whiteGradientStops"> > + <stop id="whiteGradientStop01" style="stop-color:#ffffff;stop-opacity:.95" offset="0" /> > + <stop id="whiteGradientStop02" style="stop-color:#ffffff;stop-opacity:.75" offset=".45" /> > + <stop id="whiteGradientStop03" style="stop-color:#ffffff;stop-opacity:.72" offset=".55" /> > + <stop id="whiteGradientStop04" style="stop-color:#ffffff;stop-opacity:.65" offset="1" /> Same nit for this file as for pinstripe. It seems silly that we have duplicate files for this. There should be a better way, but I don't know of one off the top of my head. :\

Thanks for the feedback Frank! Good catch on the logic bug there. I've addressed your feedback comments in this latest patch.

Comment on attachment 572578 [details] [diff] [review] Patch for bug 666306 v2.1 The large play button only appears at the beginning. After clicking to play, it doesn't appear when clicking to pause. Is that intended? The animation looks fantastic if you wait long enough for the play button's hover animation to finish before clicking. If you click before the play button's hover animation finishes, the animation jerks in opacity before fading. I'd like to see this latter issue resolved, but feedback+ nonetheless! :)

(In reply to Frank Yan (:fryn) from comment #46) > Comment on attachment 572578 [details] [diff] [review] [diff] [details] [review] > Patch for bug 666306 v2.1 > > The large play button only appears at the beginning. After clicking to play, > it doesn't appear when clicking to pause. Is that intended? Yes, that is working as intended. > The animation looks fantastic if you wait long enough for the play button's > hover animation to finish before clicking. If you click before the play > button's hover animation finishes, the animation jerks in opacity before > fading. I'd like to see this latter issue resolved, but feedback+ > nonetheless! :) Maybe we could remove the transition on the opacity change from hover, but I'm not sure which is the better tradeoff.

I have unbitrotted the previous patch.

I have unbitrotted the patch again. Dolske: Do you think you will have time to review this or would you recommend that I request review from someone else?

Comment on attachment 583330 [details] [diff] [review] Patch for bug 666306 v2.1 (unbitrotted again) As usual, don't use tabprompts-bgtexture.png if you don't have a tabprompt.

Removed the reference of tabprompts-bgtexture.png and replaced it with the data URI of the same image (cleaned with pnggauntlet) that was provided in comment #80 of bug 376997 by Alfred Kayser: https://bugzilla.mozilla.org/show_bug.cgi?id=376997#c80

Review ping?

Comment on attachment 583348 [details] [diff] [review] Patch for bug 666306 v2.2 Review of attachment 583348 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay here. :( ::: toolkit/content/widgets/videocontrols.xml @@ +274,5 @@ > + <spacer class="controlsSpacer" flex="1"/> > + <vbox flex="1" class="clickToPlayOverlay" hidden="true"> > + <box class="clickToPlayButton"/> > + </vbox> > + </stack> Hmm. This seems more complicated than it needs to be. I'd suggest either effectively no change (just change the spacer to a box, make the button a background-image on it, control it with CSS + attribute), or a simple <stack> <spacer/> <box class="clicktoplay"/> </stack> The controlSpacer and button are semantically the same, so they might as well be the same thing... @@ +350,5 @@ > maxCurrentTimeSeen : 0, > isAudioOnly : false, > > setupStatusFader : function(immediate) { > + if (!this.clickToPlayOverlay.hidden) Add a short comment wrt why we do this. @@ +419,1 @@ > } I don't think you need to nest the logic like this. Should be fine as: if (this.video.readyState etc etc original conditions) this.isAudioOnly = true; if (this.isAudioOnly) this.CTP.hidden = true; @@ +448,1 @@ > this.controlBar.removeAttribute("hidden"); We could unhide |clickToPlayButton| here, just like the control bar, so that .clientHeight/Width should work... Actually, WTF are we already doing this for? This code it only called in the init() path, and it should always be hidden at this point (init may unhide it when its call to this function returns, though). IE, not sure how controlBarWasHidden could ever be false here. Did this code become deadwood at some point, or was it always this way? @@ +565,5 @@ > + // exiting, the |clickToPlayOverlay| is still visible when the user > + // exits full screen since a separate instance of the video player > + // is used to display the full screen video. > + if (!this.clickToPlayOverlay.hasAttribute("fadeout")) > + this.clickToPlayOverlay.setAttribute("fadeout", "true"); I think this is no longer relevant, since we're using the DOM FS stuff? Hopefully, since it's kind of hacky to have to do this in the timeupdate path. @@ +624,5 @@ > // any of the child elements for playback during resource selection. > if (this.hasError()) { > + this.clickToPlayOverlay.hidden = true; > + this.statusIcon.setAttribute("type", "error"); > + this.setupStatusFader(true); Err, looks like the call to this.updateErrorText() got dropped? @@ +1252,5 @@ > let videoHeight = this.video.clientHeight; > let videoWidth = this.video.clientWidth; > > + if (this._overlayPlayButtonHeight > videoHeight || this._overlayPlayButtonWidth > videoWidth) > + this.clickToPlayOverlay.hidden = true; The button height should be compared to (videoHeight - controlBarHeight). There's also sorta the hypothetical case of having a button that's smaller than the control bar, such that we have space for the button but not the bar (or prefer to kill the bar before the button). I don't think we should worry about this, though. ::: toolkit/themes/pinstripe/global/media/videocontrols.css @@ +209,5 @@ > + opacity: 0.7; > + background-image: url(chrome://global/skin/media/videoClickToPlayButton.svg); > +} > +.clickToPlayOverlay { > + background-image: url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABMAAAATCAMAAABFjsb+AAAAhFBMVEXl5eXN1+XI0+PE0N/G0eHW2+XDzt7d4OW9yNi8x9fK1eWdqLi0wNCtuMjS2eW4w9Ortsazvs5/ipqRnKyirb3h4uWPmqq/ytrb3+XBzNzY3eWyvc3AzNuptMSmscHj4+WSna3f4eWUn6+6xdWhrLyfqrrQ2OW5xNR+iZmNmaicp7dteIiLLzq/AAAALHRSTlMaGhoaGhoaGhoaGhoaGhoaGhoaGhoaGhoaGhoaGhoaGhoaGhoaGhoaGhoaGtsI/XQAAADdSURBVHheXZDlagRhEARrPpV131OXyPu/X7glJJf8GxqGri6UVqJ0SsaktJxK0G8zzQj9FboNvnWgZhPEum1Zbp2VYGZ1QQjvUGSQFTBGpEU1wqqyPgRvq9VBGgV6tFQfUNeQVaw7DQlXWGJ+GvKILRxpT2KbefId7HJ8NiFHTF8S6hN5w1AHyt6AuT6zgeaZxSX793ubkPNPx/DSoTtLlf1lUZtv5vhkRjYKhDAelm23323eRFm7qSwnt5ZovHqg25mmg75/ceUWp2KMLE6duiNcWtgf4byH9sH98wtOlQ5jxZmREQAAAABJRU5ErkJggg==); Let's not dump images into CSS as data: URIs. :( Either make a copy of the tab-prompts texture, or file a quick bug to just move the existing file up a level or two so that multiple things can reference it without surprises.

Comment on attachment 583348 [details] [diff] [review] Patch for bug 666306 v2.2 > // any of the child elements for playback during resource selection. > if (this.hasError()) { >- this.statusIcon.setAttribute("type", "error"); >- this.updateErrorText(); >- this.setupStatusFader(true); >- // If video hasn't shown anything yet, disable the controls. >- if (!this.firstFrameShown) >- this.startFadeOut(this.controlBar); >+ this.clickToPlayOverlay.hidden = true; >+ this.statusIcon.setAttribute("type", "error"); >+ this.setupStatusFader(true); >+ // If video hasn't shown anything yet, disable the controls. >+ if (!this.firstFrameShown) >+ this.startFadeOut(this.controlBar); > } Grr. My "Err, looks like the call to this.updateErrorText() got dropped?" comment makes more sense with the full diff.

I've addressed the changes as requested and responded to the questions below. (In reply to Justin Dolske [:Dolske] from comment #53) > Comment on attachment 583348 [details] [diff] [review] > Patch for bug 666306 v2.2 > > Review of attachment 583348 [details] [diff] [review]: > ----------------------------------------------------------------- > We could unhide |clickToPlayButton| here, just like the control bar, so that > .clientHeight/Width should work... > > Actually, WTF are we already doing this for? This code it only called in the > init() path, and it should always be hidden at this point (init may unhide > it when its call to this function returns, though). IE, not sure how > controlBarWasHidden could ever be false here. > > Did this code become deadwood at some point, or was it always this way? This code was originally authored this way in bug 462117 (by myself). It was done so from just being over-cautious and trying to be defensive. I've removed the unnecessary parts of the code here. AFAIK, since we are using a single <box> for clickToPlay now, we can't measure the clientHeight/clientWidth of just one of the background-images. I've hardcoded the height/width of the play button and it removes some complexity from this code as well. > @@ +565,5 @@ > > + // exiting, the |clickToPlayOverlay| is still visible when the user > > + // exits full screen since a separate instance of the video player > > + // is used to display the full screen video. > > + if (!this.clickToPlayOverlay.hasAttribute("fadeout")) > > + this.clickToPlayOverlay.setAttribute("fadeout", "true"); > > I think this is no longer relevant, since we're using the DOM FS stuff? > > Hopefully, since it's kind of hacky to have to do this in the timeupdate > path. > Yeah, this is irrelevant now. I've removed it.

The play button shows back up when entering and exiting fullscreen. I see the XBL binding is getting reset 22 times when enter then exit fullscreen.

(In reply to Jared Wein [:jwein and :jaws] from comment #56) > The play button shows back up when entering and exiting fullscreen. I see > the XBL binding is getting reset 22 times when enter then exit fullscreen. I think this is only noticed now since the timeupdate handler was removed in the latest version of this patch. I'll hold off landing this until bug 718107 is fixed.

I have worked around the issue in bug 718107. Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/8d11d8bc8091

Is there still the plan to replace the .svg with a .png?

Comment on attachment 552590 [details] Play Button Overlay SVG > <path > d="M32,3C16.536,3,4,15.536,4,31s12.536,28,28,28s28-12.536,28-28S47.464,3,32,3z M47.285,32.013L23.75,45.762C22.797,46.319,22,45.863,22,44.751v-27.5c0-0.697,0.32-1.137,0.781-1.232 c0.277-0.058,0.612,0.012,0.969,0.221l23.535,13.751C48.238,30.546,48.238,31.458,47.285,32.013z" > id="playButton" > style="fill:url(#whiteGradient);fill-opacity:1;stroke:none" /> Are graphics packages incapable of understanding the concept of circles?

Comment on attachment 592015 [details] [diff] [review] Patch for checkin >+ background-color: hsla(0,0%,10%,.5); Do we have any style guidelines on when to use hsld, rgba, or anything else?

(In reply to Jared Wein [:jaws] from comment #10) > - If the video is large enough, the play button will expand and fade out > upon being clicked. > - If the video is not large enough for the expand/fadeout animation, the > play button will simply disappear. > - If the video is not large enough to fit the 64x64 pixel graphic, we will > not show the play graphic. Are there any specs for video size values for which the fade out animation is enabled or not? Thanks

(In reply to Mihaela Velimiroviciu [QA] from comment #64) > Are there any specs for video size values for which the fade out animation > is enabled or not? > Thanks The animation shouldn't occur if the video width is less than or equal to 128px or if the video height is less than or equal to 100px (128px - 28px for the control bar).

Mozilla/5.0 (Windows NT 6.1; rv:12.0a2) Gecko/20120220 Firefox/12.0a2 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0a2) Gecko/20120220 Firefox/12.0a2 Mozilla/5.0 (X11; Linux i686; rv:12.0a2) Gecko/20120220 Firefox/12.0a2 Verified the following: 1. Play button is displayed in the center of the video and the entire content area is a clickable play button; the video should fades partially to black. 2. In full screen mode, the play button is displayed in the center of the video and the entire content area is a clickable play button; the video should fade partially to black 3. When controls are hidden, no play button should is displayed in the center of the video; the content area doesn't act like a clickable play button; the video doesn't fade partially to black. 4. There is no large play button if the video is paused 5. The play button expands and fades out upon being clicked. (In reply to Jared Wein [:jaws] from comment #65) > (In reply to Mihaela Velimiroviciu [QA] from comment #64) > > Are there any specs for video size values for which the fade out animation > > is enabled or not? > > Thanks > > The animation shouldn't occur if the video width is less than or equal to > 128px or if the video height is less than or equal to 100px (128px - 28px > for the control bar). The animation doesn't show for videos larger than 128px width/100px height ( 276x155 or less), either... Should I raise another bug for this? Also, IMO the play button should not be displayed if video size is smaller than 85px height, since it won't match the video (control bar will overlap the button). Another issue here is that the play button won't be displayed if the video is resized to less than 64px width and then back to more than 64px, but that was addressed in bug #729111

(In reply to Mihaela Velimiroviciu [QA] from comment #66) > The animation doesn't show for videos larger than 128px width/100px height ( > 276x155 or less), either... Should I raise another bug for this? I'm not sure why that doesn't work, but I don't think it deserves a bug if it doesn't look awkward. I'll leave it to your judgement here. > Also, IMO the play button should not be displayed if video size is smaller > than 85px height, since it won't match the video (control bar will overlap > the button). I think this could be added to bug 729111 and that bug could be updated to be geared towards fixing these two issues since they are in the same section of the code.

(In reply to Jared Wein [:jaws] from comment #68) > (In reply to Mihaela Velimiroviciu [QA] from comment #66) > > The animation doesn't show for videos larger than 128px width/100px height ( > > 276x155 or less), either... Should I raise another bug for this? > > I'm not sure why that doesn't work, but I don't think it deserves a bug if > it doesn't look awkward. I'll leave it to your judgement here. I addressed this as an enhancement: bug #737357. Having the rest verified in comment #66, I'm marking this VERIFIED