704326 - Standalone audio files should have different style so they don't look awkward

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Screenshot of bug

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.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Martijn: Would you like to work on this bug?

Comment 3

[Martijn Gerrits]

Jared: Yes, looks like something I can handle.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 6

[Jonathan Wilde [:jwilde]]

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.

Comment 7

[Jonathan Wilde [:jwilde]]

Attachment: Bug 704326 - Standalone audio files should have different style so they don't look awkward

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 8

[Jared Wein [:jaws] (please needinfo? me)]

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 9

[Jonathan Wilde [:jwilde]]

Attachment: Status fader is no longer explicitly hidden for top-level synthetic a/v pages that contain what we know for sure is a video

Comment 10

[Frank Yan (:fryn)]

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 11

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 12

[Frank Yan (:fryn)]

(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.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 14

[Jonathan Wilde [:jwilde]]

Attachment: Massively simplified patch

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 15

[Dão Gottwald [:dao]]

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 16

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 17

[Nikhil Johny]

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

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 19

[Nikhil Johny]

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

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

(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

Comment 21

[Mike Hoye [:mhoye]]

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

Comment 22

[Nikhil Johny]

Attachment: new patch

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 23

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Nikhil, are you still working on this?

Comment 25

[Nikhil Johny]

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.

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 27

[Amod [:AMoz]]

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 ?

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch (not working perfectly though)

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.

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

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.

Comment 30

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Maybe we should make it work in xbl?

Comment 31

[Boris Zbarsky [:bzbarsky]]

That seems quite reasonable to me.

Comment 32

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

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

Comment 33

[Justin Dolske [:Dolske]]

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.

Comment 34

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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)?

Comment 35

[Boris Zbarsky [:bzbarsky]]

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

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Alternative patch

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 37

[Justin Dolske [:Dolske]]

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?

Comment 38

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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.

Comment 39

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/f9a5108cf2b2

Comment 40

[Dão Gottwald [:dao]]

(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).

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f9a5108cf2b2

Comment 42

[u279076]

This has caused a regression, see bug 974910.

Comment 43

[u279076]

Flagging for verification, the regression in comment 42 notwithstanding.

Comment 44

[Manuela Muntean [Away]]

Attachment: Firefox 28 beta 8.png

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.