Last Comment Bug 462113 - Implement progress bar / scrubber for video controls
: Implement progress bar / scrubber for video controls
Status: VERIFIED FIXED
: verified1.9.1
Product: Toolkit
Classification: Components
Component: Video/Audio Controls (show other bugs)
: unspecified
: All All
: P1 normal with 2 votes (vote)
: mozilla1.9.2a1
Assigned To: Justin Dolske [:Dolske]
:
Mentors:
: 469726 469900 (view as bug list)
Depends on: 449307 464376 470852 472090 473107 473847 474336 474521
Blocks: TrackAVUI
  Show dependency treegraph
 
Reported: 2008-10-29 01:35 PDT by Justin Dolske [:Dolske]
Modified: 2013-12-27 14:36 PST (History)
20 users (show)
roc: blocking1.9.1+
chung808: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v.1 (WIP) (8.97 KB, patch)
2008-10-29 01:35 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch v.2 (WIP) (10.07 KB, patch)
2008-12-22 19:37 PST, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch v.3 (WIP) (28.83 KB, patch)
2009-01-11 19:38 PST, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch v.4 (28.74 KB, patch)
2009-01-13 20:21 PST, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch v.5 (22.28 KB, patch)
2009-01-16 02:26 PST, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch v.6 (28.74 KB, patch)
2009-01-21 17:42 PST, Justin Dolske [:Dolske]
enndeakin: review+
jboriss: ui‑review+
Details | Diff | Review

Description Justin Dolske [:Dolske] 2008-10-29 01:35:49 PDT
Created attachment 345251 [details] [diff] [review]
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] 2008-10-29 01:46:23 PDT
(Oops, bug 449307 is the one needed for the scrubber.)
Comment 2 Justin Dolske [:Dolske] 2008-12-15 13:19:33 PST
*** Bug 469726 has been marked as a duplicate of this bug. ***
Comment 3 Damon Sicore (:damons) 2008-12-22 11:55:41 PST
Progress on this?
Comment 4 Justin Dolske [:Dolske] 2008-12-22 15:12:38 PST
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] 2008-12-22 19:37:22 PST
Created attachment 354268 [details] [diff] [review]
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 6 Justin Dolske [:Dolske] 2008-12-23 14:38:09 PST
*** Bug 469900 has been marked as a duplicate of this bug. ***
Comment 7 Justin Dolske [:Dolske] 2009-01-11 19:38:48 PST
Created attachment 356455 [details] [diff] [review]
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] 2009-01-11 19:41:35 PST
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] 2009-01-13 20:21:59 PST
Created attachment 356893 [details] [diff] [review]
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:09-jdolske@mozilla.com-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] 2009-01-14 14:47:18 PST
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] 2009-01-16 02:26:19 PST
Created attachment 357331 [details] [diff] [review]
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 2009-01-20 09:35:37 PST
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 2009-01-20 09:38:02 PST
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] 2009-01-20 10:25:36 PST
(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 2009-01-20 10:39:51 PST
(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] 2009-01-21 17:42:38 PST
Created attachment 358100 [details] [diff] [review]
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] 2009-01-21 19:44:42 PST
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 2009-01-22 16:54:56 PST
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 2009-01-22 16:57:30 PST
(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] 2009-01-23 17:03:03 PST
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] 2009-01-23 18:33:09 PST
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] 2009-01-24 21:23:45 PST
Pushed http://hg.mozilla.org/mozilla-central/rev/07cf9bbba61a
Comment 23 neil@parkwaycc.co.uk 2009-01-25 16:14:38 PST
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 2009-01-25 19:19:04 PST
Seeking video by clicking on progress bar is not intuitive enough. 
Please see bug 475286
Comment 25 Alfred Kayser 2009-01-26 00:43:17 PST
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] 2009-01-26 15:24:21 PST
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] 2009-01-29 16:01:14 PST
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] 2009-02-03 16:51:41 PST
litmus test added: https://litmus.mozilla.org/show_test.cgi?id=7493
Comment 29 Justin Dolske [:Dolske] 2009-02-07 14:04:23 PST
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

Note You need to log in before you can comment on or make changes to this bug.