Closed Bug 462113 Opened 16 years ago Closed 15 years ago

Implement progress bar / scrubber for video controls

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

(Keywords: verified1.9.1)

Attachments

(1 file, 5 obsolete files)

Attached patch Patch v.1 (WIP) (obsolete) — Splinter Review
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.
Flags: blocking1.9.1?
No longer depends on: 448909
(Oops, bug 449307 is the one needed for the scrubber.)
Depends on: 449307
Depends on: 464376
Flags: blocking1.9.1? → blocking1.9.1+
Blocks: TrackAVUI
Progress on this?
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.
Depends on: 470852
Attached patch Patch v.2 (WIP) (obsolete) — Splinter Review
Unbitrot WIP patch. Shows playback position, looks correct, but changing position and buffering status not working due to other bugs.
Attachment #345251 - Attachment is obsolete: true
Depends on: 472090
Attached patch Patch v.3 (WIP) (obsolete) — Splinter Review
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.
Attachment #354268 - Attachment is obsolete: true
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.
Depends on: 473107
Attached patch Patch v.4 (obsolete) — Splinter Review
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.)
Attachment #356455 - Attachment is obsolete: true
Attachment #356893 - Flags: review?(enndeakin)
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...
Depends on: 473847
Attached patch Patch v.5 (obsolete) — Splinter Review
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.
Attachment #356893 - Attachment is obsolete: true
Attachment #357331 - Flags: review?(enndeakin)
Attachment #356893 - Flags: review?(enndeakin)
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.
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.
(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...
(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.
Depends on: 474521
Attached patch Patch v.6Splinter Review
* 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)
Attachment #357331 - Attachment is obsolete: true
Attachment #358100 - Flags: review?(enndeakin)
Attachment #357331 - Flags: review?(enndeakin)
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.
Attachment #358100 - Flags: review?(enndeakin) → review+
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.
(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?
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.
Depends on: 474336
Attachment #358100 - Flags: ui-review?(jboriss)
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.
Attachment #358100 - Flags: ui-review?(jboriss) → ui-review+
Pushed http://hg.mozilla.org/mozilla-central/rev/07cf9bbba61a
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [needs 1.9.1 landing][waiting on blockers to land on 1.9.1]
Target Milestone: mozilla1.9.1 → mozilla1.9.2a1
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.
Seeking video by clicking on progress bar is not intuitive enough. 
Please see bug 475286
Depends on: 475292
No longer depends on: 475292
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.
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.
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing][waiting on blockers to land on 1.9.1]
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
Status: RESOLVED → VERIFIED
litmus test added: https://litmus.mozilla.org/show_test.cgi?id=7493
Flags: in-litmus+
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
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
QA Contact: video.audio → video.audio
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.