534409 - Improper placement of html 5 audio control bar via css position

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Michael A. Peters]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.7pre) Gecko/20091204 Ubuntu/9.04 (jaunty) Shiretoko/3.5.7pre
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.7pre) Gecko/20091204 Ubuntu/9.04 (jaunty) Shiretoko/3.5.7pre

When positioning html 5 audio control bar via CSS, FireFox incorrectly places the control bar lower than it should.

While the audio is buffering, it places it correctly, but the control bar sinks once the audio has buffered.

I suspect (but do not know) it is related to the progress bubble on the control bar that sticks up above.

Reproducible: Always

Steps to Reproduce:
1. use css to place an audio control bar at bottom of a div
2. load page
3.
Actual Results:  
control bar sinks to below bottom of div

Expected Results:  
control bar at bottom of div

firefox correctly place FlowPlayer control bar.
Google Chrome correctly places both audio and flowplayer
Have not tested to see what Safari does.

I would like this fixed, it impacts a currently live web site of mine.

Comment 1

[Ria Klaassen (not reading all bugmail)]

This seems fixed with the latest trunk nightly build on Windows, but not with Firefox 3.5 and Firefox 3.6 nightly (Namoroko). Although the flash bar design looks better.
Trunk builds are here: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/

Comment 2

[Michael A. Peters]

The bug still exists for me in Minefield 3.7a1pre (x86_64 Linux)

Sometimes the bottom of the control bar gets cut off after loading the buffer making it appear to look like it is doing the right thing but when you play it shows up again.

http://www.shastaherps.org/bugs/minefield.png

Is screen shot showing bug in Minefield.

Comment 3

[Ria Klaassen (not reading all bugmail)]

Yeah, you're right. If I scroll the control in 3.5 and 3.6 builds out of sight the bar sinks and is no longer cut off. And then they look like trunk.

Comment 4

[Ria Klaassen (not reading all bugmail)]

With trunk the bar is lower but painting problems are solved. On Windows at least.

Comment 5

[Frank Yan (:fryn)]

Unassigning myself, since I likely won't be working on this in the near future. Patches are welcome! :)

I do plan to investigate this at some point though, as hacking CSS is often a fun challenge :P

Comment 6

[Patrick Hall]

I can confirm this bug on OSX / FF4.

Screenshots for Safari, FF4, Chrome.

The page contains an audio tag and a pre tag, and a style tag with the rules as shown.

Comment 7

[Patrick Hall]

Oops forgot to link the screenshots:

http://imgur.com/a/IpJH4

Comment 8

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

As Frank mentioned earlier, this is probably a bug in our CSS. The CSS file for this is at /toolkit/themes/{win,pin}stripe/global/media/videocontrols.css

There is also another CSS file at /toolkit/content/widgets/videocontrols.css

Comment 9

[Dan Princ]

The bug seems to still exist in the latest trunk, I'll try to fix it. Can you please assign me on it?

Comment 10

[Josh Matthews [:jdm]]

Are you actively working on this, Dan?

Comment 11

[Dan Princ]

Yes. I'm sorry, I was very busy recently. I will work on it after this weekend.

Comment 12

[Josh Matthews [:jdm]]

Dan, can you confirm that you're still working on this? If not, it would be good to free this up to allow someone else to solve it.

Comment 13

[Dan Princ]

Yes I am. I got some advices from jaws, I hope I will be able to solve it soon. If not, I'll free it for someone else.

Comment 14

[Dan Princ]

OK, here is what I found:

It seems to me, that the bug is caused by improper height of audio element. There is a file  toolkit/themes/windows/global/media/videocontrols.css, where the contents of audio tags are styled. Basically everything is inside .controlBar, that has height 28px, same as other elements inside audio. However audio tag itself seems to have the height only 25px, but I didn't manage to find any source, that could cause the problem.
There is also toolkit/content/widgets/videocontrols.css but there are mostly CSS transitions, nothing that could cause the bug. There are not any other css files that contain any definition for audio.

I have tried to set the height of audio in layout/style/html.css: 

audio {min-height:28px}

and it seems to fix the bug, but I guess that's not a good solution since it imho doesn't solve the source of the problem. Is it possible to fix it only as suggested? If not, do you have any idea where else the problem could be?

Comment 15

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

I placed a breakpoint in nsVideoFrame::ComputeSize and the size returned shows 28 CSS pixels (1680 application units). However both DOM Inspector and the built-in devtools show a computed height for the <audio> tag as 21 CSS pixels. So somewhere 7 pixels get lost. The scrubber thumb sits outside of the controlbar by the same 7 pixels, so the problem most likely lies with this.

I'll continue to look deeper here.

Comment 16

[mjh563]

I think the problem is happening because the height of the control gets sized in proportion to its width. The default size seems to be 300 x 28, but if you set a width smaller than 300px then the height is reduced and this is what causes it to be pushed down.

The opposite happens with the example shown in comment 7. Here, the width of the control is greater than 300px, which increases the height and so you get a gap.

I'll attach a testcase that shows how it appears at various widths.

Comment 17

[mjh563]

Attachment: testcase

In this testcase the <audio> elements have a red border.

The 300px wide element is the only one with the correct/expected height.

Comment 18

[mjh563]

Dan and Jaws, are either of you currently working on this?  If not, please re-assign the bug to me and I'll try to fix it.

Comment 19

[mjh563]

So... the height of the element is set in nsLayoutUtils::ComputeSizeWithIntrinsicDimensions, which is called from nsVideoFrame::ComputeSize. When the element has a specified width but an auto height, this function scales the height so that the intrinsic ratio of the element is maintained.

That may be appropriate for a video, but for an audio element containing only a fixed-height control bar it makes no sense to maintain the aspect ratio. The height should instead just be set to the intrinsic height in that case.

So I think that if it's an audio element, something different needs to be done in nsVideoFrame::ComputeSize.

(The problem also happens the other way round, when the element has a specified height and an auto width. Then, the width gets scaled instead of being set to the intrinsic width, which I think would be preferable.)

Comment 20

[mjh563]

(In reply to mjh563 from comment #19)
> The height should instead just be set to the intrinsic height in that
> case.

...which can be done by passing an intrinsic ratio of nsSize(0, 0) to ComputeSizeWithIntrinsicDimensions. So I think the fix for this is quite straightforward.

Comment 21

[mjh563]

Attachment: patch

This patch makes the following changes to layout/generic/nsVideoFrame.cpp:

(1) In ComputeSize, only set the intrinsic ratio for video elements. Audio elements are fixed height so they shouldn't have an intrinsic ratio.

(2) In GetIntrinsicRatio and GetVideoIntrinsicSize, simplify the logic so that GetVideoIntrinsicSize is no longer used to return an intrinsic ratio in the case of audio elements. (This change isn't required to fix the bug, but I think it makes the code clearer.)

Not sure who to request review from though...

Comment 22

[Paul Adenot (:padenot)]

Thanks for the patch!

When blaming the file, I think that Chris Double should review this, he wrote all the code around the line you touched. If not, maybe roc could review as well, since this deals with layout.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58774d8692cf

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/58774d8692cf