620317 - Audio controls sometimes enlarge and retain size unnecessarily

Status: RESOLVED WORKSFORME

(Toolkit :: Video/Audio Controls, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Screenshot - after second playthrough of testcase, showing incorrect size of audio controls/element

Sometimes the audio element's resizes and keeps its size, even when it doesn't use/require all the space it's taken up. This can happen when the throbber appears, or just by itself.

This is a spin off from bug 619421 and bug 538084.

STR:
1. Load http://www.elektronotdienst-nuernberg.de/bugs/tel-audio.html
2. Let the audio "telephone" clip play through once (it will autoplay).
3. Once the audio clip has played, press its controls' play button again to restart playback.
4. Observe the audio element will resize as if it were accomodating the throbber overlay, but no throbber appears, and the audio element has a big square of empty space above it.

This is a regression from Firefox 3.6.

OK: [Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a4pre) Gecko/20100330 Minefield/3.7a4pre]
BAD: [Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a4pre) Gecko/20100331 Minefield/3.7a4pre]

So this regressed in this range:

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2010-03-30+06&enddate=2010-03-31+06

I'm guessing that this is a regression from this changeset:

Tue Mar 30 19:17:54 2010 -0700	c41ee41f58ff	Justin Dolske — Bug 521890 - Use CSS Transitions for HTML5 videocontrols. r=enn

Confusingly that is dated 8 Jan 2010, but it was checked in on 30 March 2010.

This is a regression from FF3.6, so we should try to fix it for Firefox 4...

Comment 1

[Monty Montgomery (:xiphmont)]

Hitting this here as well in an audio demo page being put
together for CELT.  DOM load() will also trigger the problem.

In 3.6, the default audio element size appears to always be the same;
popping a throbber does not cause it to resize/reflow.

In 4.0b, the audio element resizes to a much larger size at onload or
DOM load(), sometimes pops a throbber (sometimes not; it appears to be
highly layout and timing specific if the element area actually dims,
or it it merely resizes to take up more space) and then when the load
finishes, it resizes again to a smaller size, though not as small as
it was originally.

In addition in 4.0b, if the element had an explicit height set via the
style, the height is obeyed in initial render with the controls
situated at the bottom of the requested vertical area.  Upon
internally 'resizing' to pop the throbber, however, the element
gravity changes to top so that the top of the element is always
visible and the bottom always clipped.  Thus, when an explicit size is
set, the controls will always be partially or completely chopped off
after the two internal resizings.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #1)
> the controls will always be partially or completely chopped off
> after the two internal resizings.

I think your describing bug 618203. This morning I backed out bug 604885 which was causing that. Try again tomorrow with tonight's nightly and if the problem still exists, please file a new bug. Thanks!

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

What's happening here is, when we play through the second time:

1. The "should we show the statusOverlay" logic sees that we're seeking, so decides to fade in the statusOverlay.
2. We set the statusOverlay hidden attribute to false, and initiate a CSS transition to fade in the statusOverlay. This CSS transition has a delay of 750ms.
2. The seek immediately completes and the "should we show the statusOverlay" logic decides to fade out the statusOverlay. We expect to get a transitioend event at the end of the fade out, and in that transitioend listener we set the hidden attribute to true to make the now opacity 0 statusOverlay 0 sized. However if 750ms has not past since we initiated the fade in transition (i.e. the delay period), then the opacity property of the statusOverlay will not have changed, so we won't need to transition the opacity to do the fade out. So no fade out transition will happen, and we'll never get a transitionend event, and so we won't set the hidden attribute on the statusOverlay back to true, so it will remain non-hidden (non 0 sized) but still be opacity 0. So it will take up space, but be non-visible.

I guess we need to use change to not use -moz-css-delay:750ms, and instead start the CSS transition from a real 750ms timer which changes the hidden attribute immediately before starting the CSS transition. That way we won't set the statusOverlay's hidden attribute to false and then back again in the case where we cancel the transition.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch

Use a timer instead of css-delay when fading statusOverlay, so that we don't unhide the statusOverlay and then forget to rehide it if a css-transition is interrupted.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

This is a pretty annoying regression, this should be a blocker, or at least we should approve and take the patch for FF4, once it's reviewed.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

This patch also fixes bug 613312, which is a blocker, so even more reason for it to block.

Comment 7

[Frank Yan (:fryn)]

Comment on attachment 498885 [details] [diff] [review]
Patch

>-                startFade : function (element, fadeIn, immediate) {
>-                    // Bug 493523, the scrubber doesn't call valueChanged while hidden,
>-                    // so our dependent state (eg, timestamp in the thumb) will be stale.
>-                    // As a workaround, update it manually when it first becomes unhidden.
>-                    if (element.className == "controlBar" && fadeIn && element.hidden)
>-                        this.scrubber.valueChanged("curpos", this.video.currentTime * 1000, false);
>+                 doFade : function (element, fadeIn) {
>+                     // We must force style resolution (by accessing element.clientTop)
>+                     // in order to ensure our size changes correctly when the status
>+                     // overlay hides and shows.
>+                     if (fadeIn) {
>+                         element.setAttribute("hidden", false);
>+                         element.clientTop;
>+                         element.removeAttribute("fadeout");
>+                     } else {
>+                         element.setAttribute("fadeout", true);
>+                     }
>+                 },
>+                
>+                 startFade : function (element, fadeIn, immediate, delay) {

nit: indentation is off; and blank line of whitespace added.

Comment 8

[(no longer active)]

Is there a good reason that this blocks betaN?  If not, it should be moved over to final+.

Comment 9

[(no longer active)]

Dolske: any chance you can take a look at this?  If this doesn't block beta (which I suspect it shouldn't), then taking this fix would also fix the blocker bug 613312.

Comment 10

[sbohdjal]

Personally, I think it should be fixed for the release of FF4.0. As far as I can tell, it affects ALL subsequent playback of HTML5 audio with native controls. This bug renders native HTML5 audio controls unusable when elements are immediately below.

For example, at...
http://www.memidex.com/test#audio
...click "Load/Play" next to "Wiktionary" and then click "Play" again. Oops.

The problem is embarrassing for FF since it's so easily reproduced and Chrome, Safari, and Opera don't have this problem. As a web developer, I'm used to needing to work around IE bugs, not FF bugs.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

I would love to fix this. dolske, review please?

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

** PRODUCT DRIVERS PLEASE NOTE **

This bug is one of 7 automatically changed from blocking2.0:betaN+ to blocking2.0:.x during the endgame of Firefox 4 for the following reasons:

 - it was marked as a soft blocking issue for a beta
 - the whiteboard indicated that it might be appropriate for a .x release

Comment 13

[Frank Yan (:fryn)]

Attachment: Unbitrotted & white-space adjusted version of above patch

I'd like to second Pearce's request.
I've been hitting this every time I simply try to move the time scrubber.
The patch fixes this.

Here's a unbitrotted and white-space adjusted version of the above patch.
(Credit still goes to Chris, of course!)

Chris, this doesn't seem to fix bug 619421 though, when applied to tip of tree.
Has something changed in the code since the patch was made?

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thanks Frank! It looks to me like the only thing that's changed in the video controls code since this patch was written was your patch for bug 485696, and I don't think that should cause any problems.

The testcase from bug 619421 (http://www.elektronotdienst-nuernberg.de/bugs/tel-audio.html) works perfectly with the patch applied for me...

Comment 15

[Frank Yan (:fryn)]

(In reply to comment #14)
> The testcase from bug 619421
> (http://www.elektronotdienst-nuernberg.de/bugs/tel-audio.html) works perfectly
> with the patch applied for me...

Ditto, but this testcase from bug 502892 isn't fixed:
https://bug502892.bugzilla.mozilla.org/attachment.cgi?id=522552

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 523695 [details] [diff] [review]
Unbitrotted & white-space adjusted version of above patch

(In reply to comment #15)
> Ditto, but this testcase from bug 502892 isn't fixed:
> https://bug502892.bugzilla.mozilla.org/attachment.cgi?id=522552

Frank pointed out to me that when you seek in this testcase, the controls size increases, which is probably related to the problems which my patch fixed. So cancelling review so I can fix that problem as well...

Comment 17

[sbohdjal]

Not to be a nag, but can anyone provide a timeline for when this will be fixed? (If there are no immediate plans to fix this, I need to investigate a workaround since FF audio controls on my site are unusable after a second playback.)

Comment 18

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

This bug is now more visible when viewing a standalone audio file. How much of a regression would it be to simply hide the throbber when the file is audio only?

Comment 19

[Frank Yan (:fryn)]

(In reply to Jared Wein [:jwein] from comment #18)
> This bug is now more visible when viewing a standalone audio file. How much
> of a regression would it be to simply hide the throbber when the file is
> audio only?

Hardly a regression in my humble opinion. I support hiding the throbber for audio files. It has rarely provided useful information to me, and hiding it is significantly preferable to having this bug.

Comment 20

[Patrick Hall]

I agree that the throbber is useless -- the progress bar in the controls show if the file is still loading anyway.

Comment 21

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

The throbber was removed from audio elements by bug 698940. I can't reproduce this bug anymore. Please reopen this bug and add new STR if you can reproduce it.