Implement progress bar / scrubber for video controls

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Video/Audio Controls
P1
normal
VERIFIED FIXED
9 years ago
4 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

({verified1.9.1})

unspecified
mozilla1.9.2a1
verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

9 years ago
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.
Flags: blocking1.9.1?
(Assignee)

Updated

9 years ago
No longer depends on: 448909
(Assignee)

Comment 1

9 years ago
(Oops, bug 449307 is the one needed for the scrubber.)
Depends on: 449307
(Assignee)

Updated

9 years ago
Depends on: 464376
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P1
(Assignee)

Updated

9 years ago
Duplicate of this bug: 469726

Updated

9 years ago
Blocks: 470629
Progress on this?
(Assignee)

Comment 4

9 years ago
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.
(Assignee)

Updated

9 years ago
Depends on: 470852
(Assignee)

Comment 5

9 years ago
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.
Attachment #345251 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Duplicate of this bug: 469900
(Assignee)

Updated

9 years ago
Depends on: 472090
(Assignee)

Comment 7

9 years ago
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.
Attachment #354268 - Attachment is obsolete: true
(Assignee)

Comment 8

9 years ago
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.
(Assignee)

Updated

9 years ago
Depends on: 473107
(Assignee)

Comment 9

9 years ago
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.)
Attachment #356455 - Attachment is obsolete: true
Attachment #356893 - Flags: review?(enndeakin)
(Assignee)

Comment 10

9 years ago
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...
(Assignee)

Updated

9 years ago
Depends on: 473847
(Assignee)

Comment 11

9 years ago
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.
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.
(Assignee)

Comment 14

9 years ago
(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.
(Assignee)

Updated

9 years ago
Depends on: 474521
(Assignee)

Comment 16

9 years ago
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)
Attachment #357331 - Attachment is obsolete: true
Attachment #358100 - Flags: review?(enndeakin)
Attachment #357331 - Flags: review?(enndeakin)
(Assignee)

Comment 17

9 years ago
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.

Comment 19

9 years ago
(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?
(Assignee)

Comment 20

9 years ago
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.
(Assignee)

Updated

9 years ago
Depends on: 474336
(Assignee)

Updated

9 years ago
Attachment #358100 - Flags: ui-review?(jboriss)
(Assignee)

Comment 21

9 years ago
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+
(Assignee)

Comment 22

9 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/07cf9bbba61a
Status: NEW → RESOLVED
Last Resolved: 9 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.

Comment 24

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

Updated

9 years ago
Depends on: 475292

Updated

9 years ago
No longer depends on: 475292

Comment 25

9 years ago
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.
(Assignee)

Comment 26

9 years ago
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
Keywords: fixed1.9.1 → verified1.9.1
litmus test added: https://litmus.mozilla.org/show_test.cgi?id=7493
Flags: in-litmus+
(Assignee)

Comment 29

9 years ago
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
(Assignee)

Updated

8 years ago
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.