Implement progress bar / scrubber for video controls

VERIFIED FIXED in mozilla1.9.2a1

Status

()

defect
P1
normal
VERIFIED FIXED
11 years ago
5 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

({verified1.9.1})

unspecified
mozilla1.9.2a1
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

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

Updated

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

Comment 1

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

Updated

11 years ago
Depends on: 464376
Flags: blocking1.9.1? → blocking1.9.1+
(Assignee)

Updated

11 years ago
Duplicate of this bug: 469726

Updated

11 years ago
Blocks: TrackAVUI
Progress on this?
(Assignee)

Comment 4

11 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

11 years ago
Depends on: 470852
(Assignee)

Comment 5

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

Updated

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

Updated

10 years ago
Depends on: 472090
(Assignee)

Comment 7

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

Comment 8

10 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

10 years ago
Depends on: 473107
(Assignee)

Comment 9

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

Comment 10

10 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

10 years ago
Depends on: 473847
(Assignee)

Comment 11

10 years ago
Posted 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 12

10 years ago
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

10 years ago
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

10 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...

Comment 15

10 years ago
(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

10 years ago
Depends on: 474521
(Assignee)

Comment 16

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

Comment 17

10 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.

Updated

10 years ago
Attachment #358100 - Flags: review?(enndeakin) → review+

Comment 18

10 years ago
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

10 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

10 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

10 years ago
Depends on: 474336
(Assignee)

Updated

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

Comment 21

10 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

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

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

Updated

10 years ago
Depends on: 475292

Updated

10 years ago
No longer depends on: 475292

Comment 25

10 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

10 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
litmus test added: https://litmus.mozilla.org/show_test.cgi?id=7493
Flags: in-litmus+
(Assignee)

Comment 29

10 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

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