Followup from bug 698940. VideoDocument.cpp (which audio files use) creates a <video> element even when viewing <audio>. <video> elements seem to have extra height to them by default. See these examples: data:text/html, <audio src="http://upload.wikimedia.org/wikipedia/commons/1/1d/Demo_chorus.ogg" controls /> data:text/html, <video src="http://upload.wikimedia.org/wikipedia/commons/1/1d/Demo_chorus.ogg" controls /> The default sizes for video elements is set here: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsVideoFrame.cpp#577 We may be able to set the intrinsic height and width on standalone <video> elements if we determine them to be audio only.

These changes can be made from the video controls within toolkit. In the file located at /toolkit/content/widgets/videocontrols.xml, at the point where we determine that the video is audio-only, we should set an intrinsic height and width on the video element. We should also store the previous intrinsic height and width in case we later determine this element to actually be a video. To check if the controls are being shown in a top-level video document, use the boolean document.mozSyntheticDocument. document.mozSyntheticDocument will be true if the video/audio file is being viewed in a top-level (standalone) document.

Martijn: Would you like to work on this bug?

Jared: Yes, looks like something I can handle.

(In reply to Jared Wein [:jwein and :jaws] from comment #1) > document.mozSyntheticDocument > will be true if the video/audio file is being viewed in a top-level > (standalone) document. I think there is an error in my previous statement. document.mozSyntheticDocument won't say if the document is top-level. We'll need to continue to check document.mozSyntheticDocument and also check if window == top to know if it is a top-level document.

Martijn: Is there anything that I can do to help you out here?

Updating the <video> size is going to be tricky since the bubble containing the current time position extends outside the control bar. Instead, we can add have videocontrols.xml automatically add a class to <video> itself if it is currently being run inside a synthetic document and we find out that it is audio-only when we receive metadata. From there, we can put CSS in the appropriate theme files to remove the box shadow from the <video> element.

I pushed this patch to try server yesterday. It's looking like everything is passing, with the exception of intermittent oranges: https://tbpl.mozilla.org/?tree=Try&rev=6b238d181094

Comment on attachment 631024 [details] [diff] [review] Bug 704326 - Standalone audio files should have different style so they don't look awkward Review of attachment 631024 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/widgets/videocontrols.xml @@ +372,5 @@ > show = true; > > // Explicitly hide the status fader if this > // is audio only until bug 619421 is fixed. > + if (this.isAudioOnly || (document.mozSyntheticDocument && window === window.top)) It looks like this will remove the status fader for top level videos.

Comment on attachment 631593 [details] [diff] [review] Status fader is no longer explicitly hidden for top-level synthetic a/v pages that contain what we know for sure is a video Fixing bug 619421 would hopefully make these things less messy to handle. :( Thanks for writing the patch, Jonathan. :) I'll defer to Jared here.

Comment on attachment 631593 [details] [diff] [review] Status fader is no longer explicitly hidden for top-level synthetic a/v pages that contain what we know for sure is a video Review of attachment 631593 [details] [diff] [review]: ----------------------------------------------------------------- Can we just set the audio-only class when we are sure that the file is audio only? There are a couple places within videocontrols.xml that we speculate that it's audio only, but I don't want the throbber to flicker while we are determining the final state. If all we do is remove the box-shadow, then I am OK with the size of the element being a little larger since there will be no visible outline of the element. ::: toolkit/themes/winstripe/global/TopLevelVideoDocument.css @@ +5,5 @@ > body { > background: #333 url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAFgAAABYBAMAAACDuy0HAAAAG1BMVEX+/v4BAQH///8KCgoDAwN/f3/19fWAgID8/PzhDwT2AAAACXRSTlMFBQUFBQUFBQWHDtP9AAALwklEQVR4Xg3KOWOyWhAA0Bn2ci57eXEvQY1JCZp8sQTjVoJLTAkaE0swbj/7ve4UB37FLW4q86Lwwlh86J/ASAkpWaj+Krbb31HzH0Kjc2tIl7SADaWbpZBPE5dds6jJNyNdjAyKWqdroIixWRQIY6E/kOY7hIciL/ZfrAO3XP/06AuUJ3mSd/z95OB9vIal0DPlaZWHP7RE6DIXjmKqKkuGr+xNZylOnj1GSlUKvnxZDBOIzTfMe0fJgJ7c/GIIOdUuKxYyBFUOzvY6AC5AXx8R+o5O4S0j0wqBND3ErIYm/XHFbQjtH1MXD5dUbp19OFdjkDlys+HSwrBgHRvL9wVN/pi8ViOIwcv/D1GRW6UuDvJLLQA5lCI17iUdsKYpOuYfMATGnpn/Zs3W6gov51G+/Vs9Ay//we5kh8uwvEPum6o5HkDMDb3ZWunwtq+UzENU8NphDdbvNtKM3knx5gi6UMSQl+eGs+27mraDtxeWdH+T62Us/GylEtr7Ct8jlbeXKvAf5onx8D2uVt1J/GblV+XQyKUInOUG44fqjcszK266yHWAAYG9ekhvy4l4Maa44jYVyV2RFEuS54e2HcswtmNdqR/+V4P0O9e4XnpWgxVSQkNXpYMCxJ4Vel0lmi56jnYIIJAQMndF+zTEiyuj92r3ijJT1O0alPQnLWJvJLR7Xx7Xg9fm9QOqFu8o29m3QQqFwZN4bki/RoprNtMKKtEET9iMsJyKpkiguAorn2yzkv0wG3M1EEVDJP5VN7muLjYCglzdGQ7boYGgRmorzhRDq83gglgylC+hBLEyy6ZQWNwCmmqt6PvExAqGEA9V2XIT4/fS+I2cx1n5td85kOCjHfPWTg72FJ/+vKOyggt+rytFbEDJWL+mPwpgw6HtFLIHmq4o2m1nZ9saKwiKEOTVZtWlnqHODPu949VfKD+zzpfynd/ZZU5IWZ0dgnqRHC4uOBpBsT8N7YbFJzADiW2eo/T979OKFxY8zk/+HR/NNEkzgSBsmA35Sayz1m/ubxgmYQOmffyRh9gdx42mUVX512oqWkfxAzyuSCxx1cywx3jIXuXJEEbssymo0xMy7SskJW9C5IPYroPwQunt7f5FEPPXJLWRbGHcL4Q3sx3TLAN6W672r/I5CKkL6zSwwk0AI8+iBCSv1Y7QQP5RSoLE227uy8vn22Y6dhLBgEsRh18cTGjIv3y+60Kmt3YAZQX8qf3bJDUc/5pdjti+KwAZ9GzzQzd23d1JBAnSvWkWB8YfsIGlspHitNiMPYPFfR+OecRuPyxgfoP9/HkR3cR27IohiaDXCk/3VNP6lIxP9TBnsMeAAUZloq6P8KURLBsNFuiA3LsN/d9qpCeKKIBgSzsN5k+rdh3uh0VbvMuOIomJD1fBOiCqIsvklS5bOQhMaahJC+Rc+6lz+Uvxmq05Py+LoGIQlLKvlcaHsFG9Ui66H/qdHz67sPRGho+ruC92QgN5JEMmLsZREEiJu78FJbyzT8FsdK90XoEcezn2R5iLUzZhczJmf1yNY3gJNJUQvbpTznTAbnV5J8iL4q2OWuhJEndWVTyEr8M5VGTWtvOmUo1DsnOsqXE5ZzKE8K4/8cl8+c1XArp1RUKz+iKP96j2FcUmA+v0HnEr0iUdSrRK5duAj1FQamvpiaXR2JddD6g8n4SyFx/fjT4LkC+ghJckj1e1wP+DrHrpIiMaPH5F1rcaRvwZWfEn6fx+/C7PdXABGLNKjr1USZ5XyHjsafXMEoXtguAfjykMioMMHISXVAc9yQY5o5Qg8MM0nhWCA2HoiEgBc1EH+warLjxH3Ln68M/ciFqI1bG0mBOxiNreOuShEf/9pIzhm1Bh2cbYVxn2IYQ7eljYpab/5EdPF2PSmcy+62j6e2HBPNbe+8JVMuRQBrWdL9uBh4bYbQaQJ07FyfcpCuvSuxUyYjP6avvw9gTcAj0uTVohSwOHDDaHTs8nyachMBcWoVDWp3/lWgqeCLMneAUhSuhD2RJpufLOSi7emxOVhYsOGomV2JCEKjWu7kuqwueyFEmDgVhR0l4oHn8W87UZuxb8id54SxHWiSnPKnMyAhzdhi2wN/AoH3OYwLajuybB8h/QeJJiX1gIt+dfij+gr0CJRXQ2Y04Q6q8xHzfWm9FIgchiW0+X86tIotIGzRG1gENaKokQkLn+FXZ2x3KUcp7d/NUsmOmFCG/i03YB8pi0eiNS4LUIfA06AKvfQmP/VAXS1AP2kzJ+9LAaTafvFyO7bz8U9OCpld2q1eHGts+ZFrt04AmIlubOPP7Xayfi/r0tiX2aaPT9Dz4+TVPBoXsjHDzWfrmawOsZfmBT/k2+c6sz/hvD5wjrjT7XgRlnEzPuZermi1jqfUrE3q7VdFfJu5oT9Ad+VUh1fIwIFhBy8TmMuhIeX2XpmogmvS1C3ZuwiyR87ZSrj0Jv1DpEAYkbcL3RpjZXmZpPV4mXH8z8Nh8CS+R+PpcTnkhyr5UJaSiz0wjK22Ewl+zS+pTug0PQ0CSnJQ5LfdR77vVZufgjkQ/ydf4V5zpEaNq+JZmrQK6WdZBacmMHL9RmLnPUs0/MYwYFzoyrXYQMTHGAUJOfumR5r79MZO28DIEXQVT5wGw99TY1T0GOCC/BzWv8READwICd0LjUNKnE6ORVa0lOnqhoO0v33lwWcwF0ynTgTpFxy+0OKdphNDWJlH8ubKoG6WJXtKxAwbsilpBJB+GBwimvTsCrv1R7LSX9ExkAw44ZEcxU3L50OHnKAyKZNe1fih+hVqItRGCDf7shuvme+lTWteX5oYuc58NrCaqjYIrIV0PFyQeh2ZzZEqNS60LuhnP5wweMkkaU93pDA/RWPNeGpPCBgiUeDvV0L1NfdRP/Hn5i7rUK7kftlIWeIUIYbtzzFl9nlIeaNfoX+x/qyWzIABLTZDbeq/hDZpxg2gkh+ICfSU8OUpJ8yWY17uQ5EGa+GGWFmnrBd9vX3KOteYkJaMpPwJ4TjzDjbhkOMKmWKClzVJ2g81YGFl/c0xPIKncgJGdUKvZoUUJu0gYaIAh6E0xNeQ15qpJXzNITgf4W+w/oUaKOM54EMUi1j5yvOCsEe8JYpwVGj53lNiPMY9Rltgd4icp82fvN69zkSBUI40nJSRTeHz7h1IX42Cr0klWjxjO05MSX1IaTeDmTRGEeKvAvtaaBaLQnjftGJz+4cjFyy6/iCjLGF2/gW+jQhEUxbEBPyQzXi+Bb4kc9wK4jIwNLWbwQAOtYKRLaipDH+X4TPPOG8DCNY4IC9yBk1qcibjhUgRnDcf35pl9d5otbvQjOIXlEu5dVtm5LRaK5KWcD/PX6LaGd25CuNHG/vgeIB1kcpCme+J8idlcjfBALAJSggznsGHGOAJgdGduMnZg+bAaeGASGV9bh/X2wPsVTmBLxmTTQsBGFkEOkZJTsGAm+HrtMDbWwvTXOutX1u7BxIq9Xib6DkFMbUitNdrYsULkahsAhBEh9FjdzL9BNARxTSr7T3u1rE+IWUmCIpwTZHZCu5l9THCuCcOhZqfekuQxjQ7EoyGUJAwCv/q1JOuJeCc/3lknb76zAquO/DAQhK/62cP8X2s3+IBLIhvL8RHopoHpIArJysYTTmMMeubPXh8W760AvMVH67jqgg06+/ne5MZ631z6yROhloh3dPQirZoEpr80wgt/cEbhbAQTmRLtGh8lxCwDBBb5OeJ4aEq25XBNMT2rzWedW2zIzj+CCDKlnlyJBzT81qBWp69h7vlb3TmEV+DNm2rqj1iT7BQuwVVsuPkwq1e5P8tgNjVbIlMzwXeM11kZqjx3KKFOJzc3CAyFVhi8fxVZ5FvhdAM5mM6kS6OgKu16MFglq3/b/QVIwdw7HUCyeW04JPjC5dO+GC9OfqfB4VX+wwuift+ths2Ss3i6nkOE+JFyD+wKFL+WMX6nwwDva0S1/O8Mlnida69Ph96fuFvCoRMvXnCfsLPPmC/hA5RnMNE4fDK0pVOQ4BHLaErzv/wD99ABmjNZk0AAAAABJRU5ErkJggg==); > } > > video { We should probably just do |video:not(.audio-only)| for the selector here instead of adding a rule that overrides the default. We also shouldn't worry about setting the width & height here. If we're a couple hundred pixels lower than centered I think that's fine. Having explicit width & height here will just add to maintenance costs in the future.

(In reply to Jared Wein [:jaws] from comment #11) > If all we do is remove the box-shadow, then I am OK with the size of the > element being a little larger since there will be no visible outline of the > element. A black rectangle still appears while it's loading.

Oh ok, I still think we should only add the audio-only class when we have metadata. We can hide the throbber at that time too.

The status fader hangs around for a little longer than the drop shadow now. I'm not sure how to handle this. Is there any way we could animate this?

Comment on attachment 631598 [details] [diff] [review] Massively simplified patch The area currently marked by the box-shadow is clickable. Please disable this when removing the shadow.

Comment on attachment 631598 [details] [diff] [review] Massively simplified patch Review of attachment 631598 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/widgets/videocontrols.xml @@ +531,5 @@ > (this.video.videoWidth == 0 || this.video.videoHeight == 0)) { > this.isAudioOnly = true; > this.clickToPlay.hidden = true; > this.startFadeIn(this.controlBar); > + if (document.mozSyntheticDocument && window === window.top) This can just be |window == top|, no need to preface top with |window.| since it is implied.

i am new, how should i proceed to fix this bug?

(In reply to Nikhil Johny from comment #17) > i am new, how should i proceed to fix this bug? You can use Johnathan's patch as a starting point. Comment #15 and #16 need to be addressed. You can address #15 by tweaking the code at http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#1041 to return early if the video has the "audio-only" class.

where do we notice the bug while using firefox, i mean when do we encounter it?

(In reply to Nikhil Johny from comment #19) > where do we notice the bug while using firefox, i mean when do we encounter > it? You can see the bug when you visit http://upload.wikimedia.org/wikipedia/commons/1/1d/Demo_chorus.ogg

Can you confirm that you're still working on this bug?

sorry for the delay, yes i am still working on this bug. there is a black box that appears during loading, i dont know what is causing it do so. the new patch solves both problems mentioned in comment 15 and 16. please let me know if there is something else that i need to do to solve this bug.

Comment on attachment 742711 [details] [diff] [review] new patch >diff --git a/toolkit/themes/winstripe/global/media/TopLevelVideoDocument.css b/toolkit/themes/winstripe/global/media/TopLevelVideoDocument.css >--- a/toolkit/themes/winstripe/global/media/TopLevelVideoDocument.css >+++ b/toolkit/themes/winstripe/global/media/TopLevelVideoDocument.css >-video { >+video:not(.audio-only) { > box-shadow: 0 0 15px #000; > } The /toolkit/themes/winstripe directory no longer exists. You'll need to update your local tree and rebase this patch. winstripe has changed to windows, pinstripe -> osx, and gnomestripe -> linux. You'll also need to include patches that update the relevant TopLevelVideoDocument.css files for the other platforms. >diff --git a/toolkit/content/widgets/videocontrols.xml b/toolkit/content/widgets/videocontrols.xml >--- a/toolkit/content/widgets/videocontrols.xml >+++ b/toolkit/content/widgets/videocontrols.xml > this.startFadeIn(this.controlBar); >+ if (document.mozSyntheticDocument && window === top) >+ this.video.classList.add("audio-only"); > } How did this work for you? I tried testing it today and referencing 'top' like this wasn't working for me. I had to prefix it with 'window.', but I'm not sure why. > clickToPlayClickHandler : function(e) { >+ if(this.getElementByClass("audio-only")) >+ return; > if (e.button != 0 || this.hasError()) Did you test this patch before uploading? The getElementByClass function doesn't exist. You should use this.video.classList.contains() here.

Nikhil, are you still working on this?

sorry, i am not currently working on this bug, my firefox build doesn't seem to be working at the moment. sorry again for the late reply.

Ok, let me know if you can pick this back up. If you get latest from mozilla-central and delete your object directory, your build should complete successfully. I'll reopen the bug now so someone else can work on it.

I would like to work on this bug. Do i Need to carry forward any patch or work from scratch ? Also I understood comment #16 but not comment #18. Tweaking code in what form ?

This bug turned out to be more complicated than I had thought. This patch implements the desired solution, but document.mozSyntheticDocument is ChromeOnly and so the XBL binding can't check the value of it. This means that we would set the "audio-only" class on elements that aren't in standalone documents, which isn't acceptable.

I worked around mozSyntheticDocument being chrome-only by setting a class on the body to state if the document is synthetic. I only applied the class to the body element if the document is also top-level, so that content doesn't start depending on the presence of this class name for subframes, plus we don't need it for subframes anyways.

Maybe we should make it work in xbl?

That seems quite reasonable to me.

Made mozSyntheticDocument available to XBL. I like this approach much better.

Comment on attachment 813942 [details] [diff] [review] Patch Review of attachment 813942 [details] [diff] [review]: ----------------------------------------------------------------- r+, though I'd like to know why the toggleFullscreen() change is needed. ::: toolkit/content/widgets/videocontrols.xml @@ +355,5 @@ > + if (val) > + this.video.classList.add("audio-only"); > + else > + this.video.classList.remove("audio-only"); > + }, This somehow feels a bit gross, but I don't have a simpler idea at hand. @@ +1051,5 @@ > }, > > toggleFullscreen : function () { > + if (this.isAudioOnly) > + return; I feel like I've asked about this before, but don't see any previous reviews here... Why this change? If it's audio-only, we should be making the button/menu hidden, not make it non-functional. @@ +1081,5 @@ > this.setFullscreenButtonState(); > }, > > clickToPlayClickHandler : function(e) { > + if (e.button != 0 || this.isAudioOnly) Ditto. ::: toolkit/themes/osx/global/media/TopLevelVideoDocument.css @@ +10,1 @@ > box-shadow: 0 0 15px #000; 'Twould be nice to keep the shadow. Perhaps the new audioOnly setter could simply set video.height = $CONTROL_BAR_HEIGHT? I was a bit wary of needing to watch for media changes and resetting it, but I suppose that can't really happen for mediadocuments, since there's no other content script running. Fine either way, I suppose.

(In reply to Justin Dolske [:Dolske] from comment #33) > Comment on attachment 813942 [details] [diff] [review] > Patch > > Review of attachment 813942 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+, though I'd like to know why the toggleFullscreen() change is needed. > > ::: toolkit/content/widgets/videocontrols.xml > @@ +355,5 @@ > > + if (val) > > + this.video.classList.add("audio-only"); > > + else > > + this.video.classList.remove("audio-only"); > > + }, > > This somehow feels a bit gross, but I don't have a simpler idea at hand. How about this.video.classList.toggle("audio-only", !!val)?

You don't even need the "!!" bit, do you? The last arg is coerced to a boolean anyway.

This patch implements the recommendation from comment #33 by setting the controls height and width to the minimum size required. The minimum width is probably too narrow for most use-cases. It's only 143 pixels wide and it doesn't leave much granularity for seeking in audio files, so it may be better to introduce a standalone-audio-minimum-width constant around 400px.

Comment on attachment 823565 [details] [diff] [review] Alternative patch Review of attachment 823565 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/widgets/videocontrols.xml @@ +346,5 @@ > + let win = doc.defaultView; > + if (win !== win.top || !doc.mozSyntheticDocument) > + return; > + this.video.style.height = this._controlBarHeight + "px"; > + this.video.style.width = this._minWidthAllControls + "px"; Why set it to the minimum width? this.video.style.width = "66%" would seem fine?

Switched to a width of 66%. Also fixed the volume mouse in/out so it doesn't show the volume slider on standalone audio documents. This is consistent with bug 502892 since we are now reducing the height of the element guaranteeing that the volume slider will be outside of the bounding box.

(In reply to Jared Wein [:jaws] from comment #38) > Created attachment 823718 [details] [diff] [review] > Alternative patch v1.1 > > Switched to a width of 66%. > > Also fixed the volume mouse in/out so it doesn't show the volume slider on > standalone audio documents. This is consistent with bug 502892 since we are > now reducing the height of the element guaranteeing that the volume slider > will be outside of the bounding box. So this patch makes it so that you can't adjust the volume for standalone audio files anymore? This cure sounds worse than the disease (i.e. the pointless box shadow).

This has caused a regression, see bug 974910.

Flagging for verification, the regression in comment 42 notwithstanding.

Verified as fixed with Firefox 28 beta 8 on: Win 8 x64, Ubuntu 12.04 x86, Mac OS X 10.8.5. More details in the attached screenshot.