475317 - Need UI for volume control on <video>

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, enhancement)

Description

[Biju]

Need UI for volume control for AUDIO/VIDEO

Also please avoid issue similar to bug #475286
ie, clicking on the "Track" should seek the volume to point of clicking.

Comment 1

[Fritz]

For all three of these bugs you should have put 470629 as a "blocks" and not "depends on".

Comment 2

[Biju]

(In reply to comment #1)
> For all three of these bugs you should have put 470629 as a "blocks" and not
> "depends on".

Sorry it is an error, please feel free correct if you find any in the future.

Comment 3

[Justin Dolske [:Dolske]]

This is planned, per Boriss' mockup, but I don't think it's a priority for 1.9.1 at this point.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (WIP)

I took a quick stab at prototyping this, but can't seem to get it to work. :(

The basic idea works in normal XUL, ala http://dolske.net/mozilla/vol.xul. But when I add it to the video controls XBL, nothing happens after calling openPopup(), and onpopupshowing isn't even called. A slider in a panel should appear when hovering over the volume button (or the blue box in in XUL). [Eventually this would be an animated to slide out from the button, and would have to deal with onmouseout firing when moving the pointer from the button to the slider.]

Enn: would XBL or native anonymous content be expected to break <panel> like this? Maybe there's a better way to do this without a <panel>? [Though I thought a <panel> would also be used for implementing the time display over the video scrubber -- bug 475318]

Comment 5

[Biju]

Why cant we make it simple. Just make a horizontal volume contol like
http://video.yahoo.com/
http://www.cnn.com/video/
http://www.hulu.com/

Also please see this VIDEO controls bookmarklet
http://www.tapper-ware.net/devel/js/JS.TinyVidPlayer/v002/bookmarklet/index.html
it even have video resizer grippy at bottom left.
read comment thread http://tinyvid.tv/feedback#IDComment16782064

Comment 6

[Justin Dolske [:Dolske]]

I don't think including the volume control in the main control bar works well. It takes up space and adds visual noise/complexity, and doesn't work well for small videos. One little site you didn't mention, YouTube, implements a volume control like our design.

Comment 7

[Biju]

(In reply to comment #6)
> One little site you didn't mention, YouTube, implements a volume
> control like our design.

Yes I know, I was listing those had horizontal volume controls

> I don't think including the volume control in the main control bar works well.
Why not? I am suggesting this as I am eager to see it and as a quick solution just for firefox 3.5 release, because in Comment #4 it was mentioned there are some issues with current design. If not or if it is resolved please ignore my comments.

> It takes up space and adds visual noise/complexity,
So why commercial content providers like hulu, cnn, nbc, msnbc as well as vimeo all selected that. We cant say they dont know how to design.

see
http://www.vimeo.com/
http://www.nbc.com/Video/
http://www.msnbc.msn.com/id/3096434/

> and doesn't work well for small videos. 
that is edge case, if video is small most probably they wont be using any controls.

Comment 8

[Neil Deakin]

(In reply to comment #4)

> Enn: would XBL or native anonymous content be expected to break <panel> like
> this? Maybe there's a better way to do this without a <panel>? [Though I
> thought a <panel> would also be used for implementing the time display over the
> video scrubber -- bug 475318]

HTML documents don't have the special popup container (an nsPopupSetFrame.cpp) needed for toplevel popups for their layout. But they do work for menubutton-type popups. Conveniently, you actually want to use one here anyway. Just put the panel inside the mute-button and use <button type="panel" ...>

Although for this case you'll probably need the button.xml#menu-button binding or some variation of it as you want to have a separate function for clicking the button versus opening the menu.

Comment 9

[Justin Dolske [:Dolske]]

(In reply to comment #8)

> Just put the panel inside the mute-button and use <button type="panel" ...>

Ok, I've got that working, with a mouseover <handler> to trigger opening it.

> Although for this case you'll probably need the button.xml#menu-button binding
> or some variation of it as you want to have a separate function for clicking
> the button versus opening the menu.

I don't get this part.

This leaves me with 2 click-related problems (different flavors of the same thing?):

1) A mouse click now just causes the panel to show/hide, and the button's existing on command handler isn't ever called.

2) What code is opening/closing the panel on clicks? I want to suppress that... I think clicking should only trigger a mute/unmute (as it does today), and panel visibility will be driven from my mouseover/mouseout handlers.

Comment 10

[Neil Deakin]

(In reply to comment #9)

> > Although for this case you'll probably need the button.xml#menu-button binding
> > or some variation of it as you want to have a separate function for clicking
> > the button versus opening the menu.
> 
> I don't get this part.
> 

What I'm saying is that there isn't a binding for a button which allows a press to do one thing but a hover to open the menu. Normal menu-type buttons are designed to open on press. So you'd need to create a variation of the menu-button binding in button.xml

It's tricky because much of the menu handling in done in layout frames.

Note that I'm assuming you want this for 1.9.1. For trunk, now that 311053 is fixed, you may be able to handle this just by preventDefault-ing the various mouse events.

Comment 11

[Justin Dolske [:Dolske]]

Attachment: Patch v.2 (WIP)

Different approach... Avoids the problems with <panel>, and just uses an element with negative margins and the "position: relative" trick to accomplish the same effect. By changing margin-top from 0 to -70 (or whatever), the volume widget can be made to slide up and down from the mute button.

[It's static in this WIP patch; need to hook up to mouseover and animate.]

Comment 12

[Justin Dolske [:Dolske]]

Attachment: Patch v.3 (WIP)

Still static, but functionality hooked up.

Enn: I'm hitting a weird bug... Start playing a video, then click the mute button. The volume slider should move to the bottom (0), but it stays at the top (100). Now move the mouse away (so the controls fade out), then back over and... ta-da, the volume slider is now being drawn at the correct position.

Further clicks on the mute button make the volume <scale> work normally.

The immediate cause of this is that the CurrentPositonChanged() code in nsSliderFrame is hitting the "do nothing if the position did not change" case, and bails out (curpospx is 0, instead of the expected 100).

The real problem occurs earlier on... Calls to _setIntegerAttribute() in scale.xul and calls to nsSliderFrame::AttributeChanged() should occur in pairs. But I'm not seeing the call to AttributeChanged() when the scale's value is set from the video control's setupInitialState(), nor does it get called when I hardcode <scale value="50"> in the XUL. The code in _setIntegerAttribute() is calling setAttribute() in all these cases, so I'm not sure where things are going wrong early on.

Comment 13

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

Added animation effects. Patch is on top of Patch v.4 in bug 475318.

I've noticed that the volume control will sometimes not slide out on mouseover, or will hide slide out once and then disappear. I think this is hitting some layout glitch again, as it seems to be timing dependent. Local files always work fine, videos from air.mozilla.com usually don't, tinvid.tv videos usually do. Will do some more debugging, but I don't think it's in the controls.

Comment 14

[Neil Deakin]

Comment on attachment 371591 [details] [diff] [review]
Patch v.4

>+          this.type = this.getAttribute("class");

You could just use className directly instead of setting a separate field.

Comment 15

[Jennifer Morrow [:Boriss] (UX)]

Comment on attachment 371591 [details] [diff] [review]
Patch v.4

+ui-r+ with the caveat that while the functionality is here, there's a few kinks that need to be worked out in the next version - especially making the marker look similar to the marker on the timeslider and smoothing the animation

Comment 16

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/dddef115ddd2

Comment 17

[Justin Dolske [:Dolske]]

Backed out (as part of the bug 475318 backout)

Comment 18

[Peter Lairo]

Jumping in a bit late, sorry.

In case this is not already so: I would suggest/request that the volume slider be always visible (i.e. horizontal). 

In my experience, I use the volume control much more often than the video slider. I think the volume control could be x% (where x < 50%)  of the horizontal space (e.g. 30%). That would allow a quicker and more intuitive access to the volume.

Mockup:

+----------------------------------------------------------+
| (>) [======(1:37)====__________]9:59  [<<<<<<(80%)__] (X)|
+----------------------------------------------------------+

See https://bugzilla.mozilla.org/show_bug.cgi?id=448909#c77

Comment 19

[Alfred Kayser]

One could also add an splitter between the progress/position and the volume slider, so that one can change the ratio between these. Even better if this ratio is then preserved.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 371591 [details] [diff] [review]
Patch v.4

Renom after it's relanded on m-c?

Comment 21

[Mathieu Pellerin]

if video.width < 300px {
 hide volume slider in fading hover panel
} else {
 add volume slider to main panel next to time line
}

It seems to me the rational above would satisfy everybody:
- as Justin mentioned before, volume slider in main bar on small videos gets too crowded.
- on the other hand, when you have the space, it'd be ridiculous not to insure volume control is visible and modifiable in one dragging click :)

Comment 22

[Biju]

can we consider about horizontal volume control on another bug.
I feel if we change plan now, it will delay implementing it.
And I wish there is at least "A Volume Control" in Fx 3.5

Comment 23

[Justin Dolske [:Dolske]]

This fix is basically complete, it's just waiting to land with 475318 (which is blocked by something else). We can revisit the design later, as with any part of the product, but this is settled as the intended design for now.

Comment 24

[Peter Lairo]

OK. I filed bug 489017 for the always visible horizontal volume slider.

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 371591 [details] [diff] [review]
Patch v.4

a191=beltzner

Comment 26

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/33ae4bca85cd

Comment 27

[Justin Dolske [:Dolske]]

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

Comment 28

[Boris Zbarsky [:bzbarsky]]

So... this checkin didn't break the a11y test for audio controls.  Is that because it didn't change <audio>, or because the new controls aren't exposed to a11y?

Comment 29

[Justin Dolske [:Dolske]]

(In reply to comment #28)
> So... this checkin didn't break the a11y test for audio controls.

Ahha. The volume UI added by this patch is hidden by default (the container it's in has .hidden set, which is equivalent to display:none). Seems this results in the extra role not being exposed, and so from the A11Y test's point of view there is no volume slider.

If the A11Y tests want to test that the volume slider exposes the proper role, they'll probably need to first fire a mouseover event at the lower-right corner of the media element (ie, over where the mute button is).

Comment 30

[Justin Dolske [:Dolske]]

...though it also now occurs to me that A11Y features requiring a mouseover is probably a bit of an oxymoron. I guess we should spin off a bug for figuring out how to always expose the volume slider to A11Y, even when it's not being visually displayed.

Comment 31

[alexander :surkov (:asurkov)]

(In reply to comment #30)
> ...though it also now occurs to me that A11Y features requiring a mouseover is
> probably a bit of an oxymoron. I guess we should spin off a bug for figuring
> out how to always expose the volume slider to A11Y, even when it's not being
> visually displayed.

I think everything should be ok, iirc xul scale is used for volume control and therefore it will be accessible when it's shown. Sure we need to add tests for this.

Comment 32

[David Bolter [:davidb] (NeedInfo me for attention)]

Do we have a way to invoke the volume control via keyboard?

Comment 33

[Biju]

Justin, 
thanks a lot, lot, lot, lot .... for fixing this.

Will this make into next beta release? 

(In reply to comment #32)
> Do we have a way to invoke the volume control via keyboard?

see Bug 486899 - Keyboard Accessibility on video element (also audio)

Comment 34

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090423 Minefield/3.6a1pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4) Gecko/20090423 Firefox/3.5b4

Comment 35

[Tony Chung [:tchung]]

Added litmus testcase to Fx3.5 FFTs: https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=7691