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

VERIFIED FIXED in Firefox 28

Status

()

Toolkit
Video/Audio Controls
VERIFIED FIXED
7 years ago
3 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Depends on: 1 bug)

Trunk
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox28 verified)

Details

(URL)

Attachments

(3 attachments, 8 obsolete attachments)

Created attachment 575999 [details]
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.
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.
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
QA Contact: video.audio → video.audio
Whiteboard: [good first bug][mentor=jwein][lang=js]
Martijn: Would you like to work on this bug?

Comment 3

6 years ago
Jared: Yes, looks like something I can handle.
Assignee: nobody → mgerrits
Status: NEW → ASSIGNED
(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?
Whiteboard: [good first bug][mentor=jwein][lang=js] → [good first bug][mentor=jaws][lang=js]
Assignee: mgerrits → dan
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.
Assignee: dan → jonathan
Summary: Standalone audio files should have an intrinsic size so they don't look awkward → Standalone audio files should have different so they don't look awkward
Summary: Standalone audio files should have different so they don't look awkward → Standalone audio files should have different style so they don't look awkward
Created attachment 631024 [details] [diff] [review]
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
Attachment #631024 - Flags: review?(jaws)
Keywords: checkin-needed

Updated

6 years ago
Keywords: checkin-needed
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.
Attachment #631024 - Flags: review?(jaws)
Created 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
Attachment #631024 - Attachment is obsolete: true
Attachment #631593 - Flags: review?(jaws)
Attachment #631593 - Flags: feedback?(fryn)

Comment 10

6 years ago
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.
Attachment #631593 - Flags: feedback?(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

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.
Attachment #631593 - Flags: review?(jaws)

Comment 12

6 years ago
(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.
Created attachment 631598 [details] [diff] [review]
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?
Attachment #631593 - Attachment is obsolete: true
Attachment #631598 - Attachment is patch: true
Attachment #631598 - Flags: review?(jaws)
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.
Attachment #631598 - Flags: review-
Attachment #631598 - Flags: review?(jaws)
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.

Updated

6 years ago
Assignee: hello → nobody
Status: ASSIGNED → NEW

Comment 17

5 years ago
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.

Comment 19

5 years ago
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
Assignee: nobody → nikhiljohny
Status: NEW → ASSIGNED

Comment 21

5 years ago
Can you confirm that you're still working on this bug?
Flags: needinfo?(nikhiljohny)

Comment 22

5 years ago
Created attachment 742711 [details] [diff] [review]
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.
Attachment #742711 - Flags: review?(jaws)
Flags: needinfo?(nikhiljohny)
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.
Attachment #742711 - Flags: review?(jaws) → review-
Nikhil, are you still working on this?
Flags: needinfo?(nikhiljohny)

Comment 25

5 years ago
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.
Flags: needinfo?(nikhiljohny)
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.
Assignee: nikhiljohny → nobody
Status: ASSIGNED → NEW

Comment 27

5 years ago
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 ?
Whiteboard: [good first bug][mentor=jaws][lang=js]
Created attachment 813824 [details] [diff] [review]
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.
Assignee: nobody → jaws
Attachment #631598 - Attachment is obsolete: true
Attachment #742711 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Created attachment 813857 [details] [diff] [review]
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.
Attachment #813824 - Attachment is obsolete: true
Attachment #813857 - Flags: review?(dao)
Attachment #813857 - Flags: review?(Ms2ger)
Maybe we should make it work in xbl?
Flags: needinfo?(bzbarsky)
That seems quite reasonable to me.
Flags: needinfo?(bzbarsky)
Created attachment 813942 [details] [diff] [review]
Patch

Made mozSyntheticDocument available to XBL. I like this approach much better.
Attachment #813857 - Attachment is obsolete: true
Attachment #813857 - Flags: review?(dao)
Attachment #813857 - Flags: review?(Ms2ger)
Attachment #813942 - Flags: review?(dao)
Attachment #813942 - Flags: review?(bobbyholley+bmo)
Attachment #813942 - Flags: review?(bobbyholley+bmo) → review?(bugs)

Updated

5 years ago
Attachment #813942 - Flags: review?(bugs) → review+
Attachment #813942 - Flags: review?(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.
Attachment #813942 - Flags: review?(dolske)
Attachment #813942 - Flags: review?(dao)
Attachment #813942 - Flags: review+
(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.
Created attachment 823565 [details] [diff] [review]
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.
Attachment #823565 - Flags: review?(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?
Attachment #823565 - Flags: review?(dolske)
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.
Attachment #823565 - Attachment is obsolete: true
Attachment #823718 - Flags: review?(dolske)
Attachment #823718 - Flags: review?(dolske) → review+
Attachment #813942 - Attachment is obsolete: true
(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).
https://hg.mozilla.org/mozilla-central/rev/f9a5108cf2b2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

5 years ago
Depends on: 936695

Updated

5 years ago
Depends on: 937429

Updated

4 years ago
Whiteboard: [good first verify]
This has caused a regression, see bug 974910.
Whiteboard: [good first verify]
Flagging for verification, the regression in comment 42 notwithstanding.
status-firefox28: --- → fixed
Keywords: verifyme
Created attachment 8386090 [details]
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.

Updated

4 years ago
Status: RESOLVED → VERIFIED
status-firefox28: fixed → verified
Keywords: verifyme

Updated

3 years ago
Depends on: 1222374

Updated

3 years ago
Depends on: 1222669

Updated

3 years ago
No longer depends on: 1222374
You need to log in before you can comment on or make changes to this bug.