462113 - Implement progress bar / scrubber for video controls

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect, P1)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (WIP)

This is part 2 of the work started in bug 448909. That reworked the basic XBL binding for video controls, along the lines of the mockups Boriss created:

http://jboriss.wordpress.com/2008/09/19/html-5-video-tag-pirate-edition/

We only implemented play/pause buttons due to time constraints for shipping in Beta 1. This bug continues on that work.

Attached is a rough snapshot my my current work-in-progress. The scrubber knob isn't hooked up yet (need bug 448909), but the styling is about right and the indicator bar for buffered data is working.

Comment 1

[Justin Dolske [:Dolske]]

(Oops, bug 449307 is the one needed for the scrubber.)

Comment 3

[Damon Sicore (:damons)]

Progress on this?

Comment 4

[Justin Dolske [:Dolske]]

Still need a fix to bug 464376 (already marked as blocking this), and need to see what the result of recent spec churn is for determining the amount of video buffered.

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch v.2 (WIP)

Unbitrot WIP patch. Shows playback position, looks correct, but changing position and buffering status not working due to other bugs.

Comment 7

[Justin Dolske [:Dolske]]

Attachment: Patch v.3 (WIP)

This is on top of the patches for bug 464376, bug 470852, and bug 472090 (all blockers for this bug). It also happens to be on top of bug 460155 and bug 469211, since they're almost ready to land.

Still a few rough spots to clean up in the code, but it's functional. The scrubber shows the current playback position, can be used to change the playback position with the mouse/keyboard, and has bar showing the amount of buffered data.

Comment 8

[Justin Dolske [:Dolske]]

Oh, bah, I forgot to update videocontrols.css for winstripe, so anyone wanting to try out the patch on Windows/Linux will need to fix up that file with the pinstripe changes.

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

Update of v.3, on top of the same bugs. Updated with tweaked icons from Boriss.

Tryserver builds are at https://build.mozilla.org/tryserver-builds/2009-01-13_18:[e-mail]-try-139ed0364eb/

(Seeking is slow and triggers redownloads of the video, bugs on file. Also just noticed that the play/mute buttons get focus rings on Windows.)

Comment 10

[Justin Dolske [:Dolske]]

Saw your comment on IRC asking about using a <progressmeter>:

It looks like I could (I did a quick experiment to see if I could roughly style it as desired), but I don't know if it's worthwhile. The controls won't want the native appearance, and don't really use the other things it provides, so it wouldn't seem to matter much either way. It looks like a progressmeter will dispatch ValueChanged events (which was a concern in bug 472090), so we might have to deal with that too...

Comment 11

[Justin Dolske [:Dolske]]

Attachment: Patch v.5

Per IRC discussion, decided to switch to <progressmeter>. I unfortunately ran into bug 473847, so the progressmeter isn't working right, but this patch should be good once that bug is fixed.

Comment 12

[Neil Deakin]

Comment on attachment 357331 [details] [diff] [review]
Patch v.5

>+            <stack class="scrubberStack" flex="1">
>+                <progressmeter class="backgroundBar" flex="1" value="100"/>
>+                <progressmeter class="bufferBar"     flex="1"/>
>+                <progressmeter class="progressBar"   flex="1" max="10000"/>
>+                <scale class="scrubber" flex="1"/>

Why are three progressmeters used here?

>+ this.bufferBar.setAttribute("max", total);

Properties should be used when available, not setting the attributes. This happens in a number of places.

Comment 13

[Neil Deakin]

Also, there are places where floats are set as the values of scales/progressmeters. It would be better to be explicit and convert them to integers.

Comment 14

[Justin Dolske [:Dolske]]

(In reply to comment #12)
> (From update of attachment 357331 [details] [diff] [review])
> >+            <stack class="scrubberStack" flex="1">
> >+                <progressmeter class="backgroundBar" flex="1" value="100"/>
> >+                <progressmeter class="bufferBar"     flex="1"/>
> >+                <progressmeter class="progressBar"   flex="1" max="10000"/>
> >+                <scale class="scrubber" flex="1"/>
> 
> Why are three progressmeters used here?

The bottom-most is just for the static background bar/track, and never changes value. The earlier patch stuck it on the <box> wrapping the pseudo-progressmeter, but that doesn't work here. Seemed clearer to just use another progressmeter, instead of adding a dummy <box> or something only used for style.

> >+ this.bufferBar.setAttribute("max", total);
> 
> Properties should be used when available, not setting the attributes. This
> happens in a number of places.

I had problems early on with security errors being thrown when setting properties, but setting attributes seemed to avoid the problem. I can look to see if that's still happening...

Comment 15

[Neil Deakin]

(In reply to comment #14)

> The bottom-most is just for the static background bar/track, and never changes
> value. The earlier patch stuck it on the <box> wrapping the
> pseudo-progressmeter, but that doesn't work here. Seemed clearer to just use
> another progressmeter, instead of adding a dummy <box> or something only used
> for style.

We need to use right element for accessibility though, otherwise it will see three progressmeters.

> 
> > >+ this.bufferBar.setAttribute("max", total);
> > 
> > Properties should be used when available, not setting the attributes. This
> > happens in a number of places.
> 
> I had problems early on with security errors being thrown when setting
> properties, but setting attributes seemed to avoid the problem. I can look to
> see if that's still happening...

OK, that's fine with attributes then.

Comment 16

[Justin Dolske [:Dolske]]

Attachment: Patch v.6

* Switched background progressmeter to just a styled <box>
* Used Math.round() to ensure integers where needed
* Switched setAttribute("foo") to .foo where appropriate (didn't get errors)

Comment 17

[Justin Dolske [:Dolske]]

Oh, and also moved some code around since the fix to bug 462114 wants to be able to use some of it, and it makes handleEvent() a little less jampacked.

Comment 18

[Neil Deakin]

Comment on attachment 358100 [details] [diff] [review]
Patch v.6

OK, this looks ok. I still don't understand what the second progressmeter is for though.

Comment 19

[Fritz]

(In reply to comment #18)
> (From update of attachment 358100 [details] [diff] [review])
> OK, this looks ok. I still don't understand what the second progressmeter is
> for though.

Isn't it 1) for progress on video downloaded and 2) for progress on video played?

Comment 20

[Justin Dolske [:Dolske]]

Right. One progressmeter shows amount of the video that has been downloaded, and the other shows the current playback position. I thought you had asked to use a <progressmeter> here instead of the former <box>+<spacer> (as in Patch v.4), so I didn't explain this one back in comment 14.

I'm not sure which approach is better here, from an accessibility point of view. I'd guess that the <scale> is not normally expected to be an output indicator, so having the <progressmeter> wouldn't be a problem. Codewise, I'd prefer to use a <progressmeter> for both the position and buffering, for consistency.

Comment 21

[Justin Dolske [:Dolske]]

Comment on attachment 358100 [details] [diff] [review]
Patch v.6

Doh, I thought Boriss had already ui-r'd this. Probably a formality at this point (it's based on her mockup, we've discussed it and adjusted things to be pixel-perfect), so I may checkin this weekend anyway if she doesn't get to it first.

Comment 22

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/07cf9bbba61a

Comment 23

[[e-mail]]

Comment on attachment 358100 [details] [diff] [review]
Patch v.6

>+            <stack class="scrubberStack" flex="1">
>+                <box class="backgroundBar" flex="1"/>
>+                <progressmeter class="bufferBar"     flex="1"/>
>+                <progressmeter class="progressBar"   flex="1" max="10000"/>
>+                <scale class="scrubber" flex="1"/>
>+            </stack>
flex makes no sense for children of stacks.

Comment 24

[Biju]

Seeking video by clicking on progress bar is not intuitive enough. 
Please see bug 475286

Comment 25

[Alfred Kayser]

The box element is not needed.
The styling set on the box to get the background painted of the two progressmeters can also be applied on the .bufferBar progressmeter itself.

And regarding the progressmeter versus scale (or slider).
The bufferBar cannot be interacted with, and only shows the *progress* of the buffering, so it makes sense to use a progressmeter for that.
But for the progressBar, which can be interacted with (slide, set) should be therefor a <scale> element.

So:
<stack>
  <progressmeter class="bufferProgress"/>
  <scale class="position"/>
</stack>

That would also make it easier for accessibility.

Comment 26

[Justin Dolske [:Dolske]]

Pushed to 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f9ab1660a905

This differed from Patch v.6 slightly, due to bug 474521. To get the same visual appearance, I had to change the -moz-border-radius in videocontrols.css from 4px to 3px.

Comment 27

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090129 Minefield/3.2a1pre 
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090129 Shiretoko/3.1b3pre Ubiquity/0.1.5

Comment 28

[Tony Chung [:tchung]]

litmus test added: https://litmus.mozilla.org/show_test.cgi?id=7493

Comment 29

[Justin Dolske [:Dolske]]

I updated the mochitest to exercise the scrubber, and synced the 1.9.1 test back up with trunk (it had a few minor differences).

http://hg.mozilla.org/mozilla-central/rev/dc047fe66164
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b3260c2d81cf