Closed
Bug 649490
Opened 14 years ago
Closed 11 years ago
Horizontal html5 audio/video volume control
Categories
(Toolkit :: Video/Audio Controls, defect)
Toolkit
Video/Audio Controls
Tracking
()
VERIFIED
FIXED
mozilla28
Tracking | Status | |
---|---|---|
relnote-firefox | --- | 28+ |
People
(Reporter: fryn, Assigned: jaws)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(2 files, 12 obsolete files)
(deleted),
application/zip
|
Details | |
(deleted),
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
While some have pointed out that the OS volume control is sufficient and additional element-specific volume controls are superfluous, I think it is still worthwhile to investigate merging the volume slider into controls (within the element rect).
IE9 does this, while taking up a lot of horizontal space.
I like Vimeo's, because it has no visible scrubber while clearly showing state, and it takes up little horizontal space.
Reporter | ||
Comment 1•14 years ago
|
||
I'm assigning it to myself, since I have a proof-of-concept already (attaching shortly), and I'll write a complete patch, once we come up with a complete design.
If you have your own ideas, patches are welcome.
Assignee: nobody → fryn
Status: NEW → ASSIGNED
Comment 3•13 years ago
|
||
Frank, any news regarding this bug ?
Reporter | ||
Comment 4•13 years ago
|
||
I don't expect to be working on this soon, so unassigning from self.
Chris, if you'd like to take it, be my guest!
Assignee: fryn → nobody
Status: ASSIGNED → NEW
Reporter | ||
Updated•13 years ago
|
Assignee: nobody → fryn
Status: NEW → ASSIGNED
Reporter | ||
Comment 5•13 years ago
|
||
I came up with a prototype a few months ago, and I've revised it, and here it is.
I pushed it to the ux branch, but I don't think it made the nightly cutoff, so see it in Friday's ux nightly.
https://hg.mozilla.org/projects/ux/rev/c77d621f4d91
Assignee | ||
Comment 6•13 years ago
|
||
I updated the patch to include support for winstripe, as well as remove most (hopefully all) of the vertical slider implementation for the volume control.
Standardizing on the in-bar horizontal slider will also fix bug 658096, as well as remove a lot of code. As it stands now, this prototype contributes approximately a net -75 LOC :)
Attachment #559149 -
Attachment is obsolete: true
Reporter | ||
Comment 7•13 years ago
|
||
(In reply to Jared Wein [:jwein] from comment #6)
> I updated the patch to include support for winstripe, as well as remove most
> (hopefully all) of the vertical slider implementation for the volume
> control.
Thanks, Jared!
The horizontal slider also removes the mute control, of which I've been pondering the merits. The mute control saves the volume state while muting, which clicking on the left side of the volume control doesn't do. Then again, you'd only ever click mute, instead of press the mute multimedia key or OS control when you have more than one media track playing, in which you probably want to pause and not mute. After this thinking in writing, I think removing the visual mute control isn't actually a loss at all.
> Standardizing on the in-bar horizontal slider will also fix bug 658096, as
> well as remove a lot of code. As it stands now, this prototype contributes
> approximately a net -75 LOC :)
I concur that this is a good idea! :)
Assignee | ||
Comment 8•13 years ago
|
||
On the other hand, we probably need some kind of graphic to let users know that this is a volume meter (which would double fine as a mute control). YouTube has a graphic for this, while Vimeo doesn't.
Reporter | ||
Comment 9•13 years ago
|
||
(In reply to Jared Wein [:jwein] from comment #8)
> On the other hand, we probably need some kind of graphic to let users know
> that this is a volume meter (which would double fine as a mute control).
> YouTube has a graphic for this, while Vimeo doesn't.
This isn't completely necessary if the volume slider is self-descriptive enough. The problem with Vimeo's and Youtube's sliders is that they are hardly descriptive at all.
For example, the dynamic Wi-Fi signal indicator on MacBooks don't have a separate icon to indicate that it's for Wi-Fi.
Let's have Faaborg and/or Shorlander give this a pass (when they see it in the UX branch) before deciding.
(To be clear, I'm not using uiwanted as a stalling tactic at all because I want this fixed either way.)
I'll push an updated patch to ux soon.
Keywords: uiwanted
Assignee | ||
Comment 10•13 years ago
|
||
Reporter | ||
Comment 11•13 years ago
|
||
This is still a work in progress.
Landed on UX branch.
The final patch when it's done should look something like https://bugzilla.mozilla.org/attachment.cgi?id=561355
Attachment #559402 -
Attachment is obsolete: true
Reporter | ||
Updated•13 years ago
|
Keywords: uiwanted
Summary: Investigate merging html5 video/audio volume slider into controls → Horizontal html5 audio/video volume control
Reporter | ||
Comment 12•13 years ago
|
||
I don't have images to make the slider look like https://bugzilla.mozilla.org/attachment.cgi?id=561355 but I think this looks quite good already.
Putting it up for review for that reason.
Attachment #562272 -
Attachment is obsolete: true
Attachment #562391 -
Flags: ui-review?(shorlander)
Attachment #562391 -
Flags: review?(dolske)
Reporter | ||
Comment 13•13 years ago
|
||
Reporter | ||
Comment 14•13 years ago
|
||
Also, try clicking the mute button.
Repeat for more animated fun each time!
Comment 15•13 years ago
|
||
I would rather have this in secondary UI to keep the core video controls keep. Also dragging vertically for some reason seems to have a natural mapping to "louder."
Reporter | ||
Comment 16•13 years ago
|
||
(In reply to Alex Faaborg [:faaborg] (Firefox UX) from comment #15)
> I would rather have this in secondary UI to keep the core video controls
> keep. Also dragging vertically for some reason seems to have a natural
> mapping to "louder."
While they may be preferable, we simply don't have the platform capability to do this as explained in bug 502892. (TL;DR We can't do UI on hover if that UI is outside of the element's initially rendered rectangle.) I don't think it makes sense to prioritize the immensely complex platform fix that this would require. In this situation, would you prefer no element-local volume control or an element-local volume control in primary UI?
I know that Limi would prefer no volume control because an OS-level control already exists. I would prefer having a volume control only because I wonder whether it's obvious how to change volume quickly on Windows computers with their mess of a system tray and the unreliable availability of multimedia keys.
Comment 17•13 years ago
|
||
(In reply to Frank Yan [:fryn] from comment #16)
> I know that Limi would prefer no volume control because an OS-level control
> already exists.
There is an OS-level control to mute items on single tabs? Note that users very much will want to mute stuff on some tabs or windows to be able to listen to (or view) stuff on others.
Reporter | ||
Comment 18•13 years ago
|
||
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #17)
> (In reply to Frank Yan [:fryn] from comment #16)
> > I know that Limi would prefer no volume control because an OS-level control
> > already exists.
>
> There is an OS-level control to mute items on single tabs? Note that users
> very much will want to mute stuff on some tabs or windows to be able to
> listen to (or view) stuff on others.
That's not what I said. Volume control != mute button.
Comment 19•13 years ago
|
||
>I know that Limi would prefer no volume control because an OS-level control already exists.
volume control on windows XP is pretty broken (is it even in the system tray?), but things are fine on OS X, Vista and 7. Perhaps we might consider showing these controls just on XP? (not sure what the state of volume is on linux).
Assignee | ||
Comment 20•13 years ago
|
||
Relying on OS-level volume control does not help when there are multiple audio sources in the browser and the user wants to hear them at varying levels.
Reporter | ||
Comment 21•13 years ago
|
||
(In reply to Jared Wein [:jwein] from comment #20)
> Relying on OS-level volume control does not help when there are multiple
> audio sources in the browser and the user wants to hear them at varying
> levels.
Have you ever used this?
I don't remember ever rapidly switching between audio sources.
I would consider this an edge case.
Also, we don't persist volume level, and I'm glad we don't.
Even music player software don't provide song-specific volume control.
iTunes, one of the few software that have this, places this deep in quaternary UI.
Comment 22•13 years ago
|
||
(In reply to Frank Yan [:fryn] from comment #21)
> (In reply to Jared Wein [:jwein] from comment #20)
> > Relying on OS-level volume control does not help when there are multiple
> > audio sources in the browser and the user wants to hear them at varying
> > levels.
>
> Have you ever used this?
I have. I routinely want to control audio volume level of the browser independent of the other playing audio in the system (typically when I have background music playing in another app - often but not always in this case I don't want to mute the browser's audio, just lower it).
Assignee | ||
Comment 23•13 years ago
|
||
(In reply to Frank Yan [:fryn] from comment #21)
> (In reply to Jared Wein [:jwein] from comment #20)
> > Relying on OS-level volume control does not help when there are multiple
> > audio sources in the browser and the user wants to hear them at varying
> > levels.
>
> Have you ever used this?
Yes I have, in fact I do it often. In particular, I often play music in one tab while watching a video in another tab.
Comment 24•13 years ago
|
||
Audio by it's very nature of synchronous ear-based input doesn't multitask all that well, so cases where people want to mix multiple audio sources inside of the browser are going to only be used by our most powerful of power users.
Can we get a slider functioning in a context menu? A similar topic came up in bug 592147. For platforms that have a clear global volume control, having secondary control over the audio of a particular element seems to match up with the relative discoverability of a context menu.
Comment 25•13 years ago
|
||
(In reply to Alex Faaborg [:faaborg] (Firefox UX) from comment #24)
> Audio by it's very nature of synchronous ear-based input doesn't multitask
> all that well, so cases where people want to mix multiple audio sources
> inside of the browser are going to only be used by our most powerful of
> power users.
I generally agree with this. Though I'd suspect there's 1 exception / semi-common case -- "mute the other stuff so I can hear this one". But that's out of scope here. :)
That said, mute feels like a pretty important function (for OMGLOUD media, or clicking HarryMetSally.ogg). It also feels a little strange not to offer some form of volume control (especially with a mute button). Both seem to be included on every set of media controls I can remember seeing.
We do have leeway in that not every volume level needs to be selectable. EG, Vimeo has 7 visible levels, Youtube has a fairly tiny slider ~50 pixels wide. (Apple's WiFi indicator has 4 levels, many call phones have ~5 bars for signal strength -- though I'll grant this is an apples-to-oranges comparison for many reasons). Our keyboard volume controls give you 11 levels (0-100% in 10% steps).
Perhaps more relevant, both OS X and Window's system volume controls are a speaker with 0-3 "sound waves" coming out of it, depending on level (which, again to be fair, is set via a larger fly-out slider like we currently have). I'd sorta favor something like that -- minimal, compact UI that allows coarse volume adjustment.
Reporter | ||
Comment 26•13 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #25)
> That said, mute feels like a pretty important function (for OMGLOUD media,
> or clicking HarryMetSally.ogg).
Just to be clear, we are not debating getting rid of the mute button. We absolutely want it. :)
Comment 27•13 years ago
|
||
Top 3 media controls show differing volume level. (A bit rough, they're a bit more bold than ideal.)
Random idea in the bottom one: have click-and-drag bring up a tiny slider affordance; the "soundwave bars" would still have large discrete levels, but the slider would show that moving left/right does indeed do fine-tuned adjustments.
Comment 28•13 years ago
|
||
Comment on attachment 562391 [details] [diff] [review]
patch v1
General changes are basically fine, but I think it's too early to really review yet... We should (1) settle on a target design and (2) at least use placeholder images until we get polished artwork from shorlander. [I hear Jared is good with MS Paint ;)]
Attachment #562391 -
Flags: review?(dolske)
Comment 29•13 years ago
|
||
yeah, mute button seems important enough to warrant primary UI. Also I'm fine with having the mute button display volume level with waves. My only stipulation is that the actual volume selection should be obfuscated into secondary UI (vertical panel from the speaker icon, etc.), and it should start at 100% since most users are going to use the platform volume controls.
Comment 30•13 years ago
|
||
I'm happy for mute to have a dedicated button.
I'm happy for the volume control to be in secondary UI (e.g. in a popup panel or somesuch, as we currently implement it for <video>).
The problem with having the volume control in a popup panel is that they can only render inside the element's initial rendered area (due to a non-trivial bug in Gecko as Frank explained in comment 16), and for <audio> elements the initial area is limited to the controls rectangle. So the volume control would have to appear entirely inside the controls.
I'm not sure how such a volume control would conveniently co-exist with a mute button whilst still being discoverable.
Reporter | ||
Comment 31•13 years ago
|
||
(In reply to Alex Faaborg [:faaborg] (Firefox UX) from comment #29)
> My only
> stipulation is that the actual volume selection should be obfuscated into
> secondary UI (vertical panel from the speaker icon, etc.)
Like I already stated in comment 16, we simply don't have the platform capability to draw a vertical panel like that. Do you have any other ideas, or are you not willing to create an imperfect control and would rather ship nothing?
> and it should
> start at 100% since most users are going to use the platform volume controls.
I agree, and we already do this.
Comment 32•13 years ago
|
||
(In reply to Frank Yan (:fryn) from comment #31)
> (In reply to Alex Faaborg [:faaborg] (Firefox UX) from comment #29)
> > and it should
> > start at 100% since most users are going to use the platform volume controls.
>
> I agree, and we already do this.
It's actually defined in the HTML standard what the video/audio element's volume should default to, which is 100%.
Comment 33•13 years ago
|
||
right, I meant something like the vertical panel, doesn't have to be that specifically. It's the secondary UI part that is more key. Youtube uses a horizontal control on hover right now. It seems ok, but it is kind of strange to have two bars (volume and scrubbing).
Updated•13 years ago
|
Attachment #562391 -
Flags: ui-review?(shorlander)
Reporter | ||
Comment 34•13 years ago
|
||
Faaborg wants volume controls not to be in primary UI.
Limi wants them not to be in visible UI and for users to use the OS volume controls.
There are no secondary UI proposals that I think are sensible.
Therefore, this is going to be WONTFIX.
Thanks everyone for your input and design contributions.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
Comment 36•13 years ago
|
||
I think we should try harder to find some solution to this before taking the route of removing the volume control entirely.
Let's meet up next week and talk about some options.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Assignee | ||
Comment 37•13 years ago
|
||
Frank: Do you think you will be able to get this fixed for Firefox 10? The cut-off date is November 8th.
Comment 38•13 years ago
|
||
What about full screen videos? Users can't reach the OS's volume control. And most keyboards don't have (good) volume controls). Removing the volume control seems a baaad idea.
Assignee | ||
Comment 39•13 years ago
|
||
(In reply to Peter Lairo from comment #38)
> What about full screen videos? Users can't reach the OS's volume control.
> And most keyboards don't have (good) volume controls). Removing the volume
> control seems a baaad idea.
Our current plan is to keep the volume controls but make them horizontal.
Reporter | ||
Updated•12 years ago
|
Assignee: fryn → nobody
Status: REOPENED → NEW
Reporter | ||
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago → 11 years ago
Resolution: --- → WONTFIX
Comment 40•11 years ago
|
||
lol rly?!
I mean sorry for the spam but the audio und video tag will become more and more imoprtant in the feature and this should be an easy thing to to for a core dev. (about an hour? If drinking a lot of coffe maybee a day...)
Well your choice we the aluheads will have to stick with you nonetheless...
Assignee | ||
Comment 41•11 years ago
|
||
I'm not sure why this was closed. Frank, if you are going to close a bug as wontfix it would be nice to include some background. I would still like to have something like this, considering we now also hide the volume slider on standalone audio files.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Assignee | ||
Comment 42•11 years ago
|
||
I talked with shorlander today and he agreed with me that we could do an always-visible horizontal volume slider here.
Assignee | ||
Comment 44•11 years ago
|
||
Assignee | ||
Comment 46•11 years ago
|
||
WIP patch, still need to fix some things with the interaction.
Assignee: nobody → jaws
Attachment #561355 -
Attachment is obsolete: true
Attachment #562391 -
Attachment is obsolete: true
Attachment #562392 -
Attachment is obsolete: true
Attachment #565849 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Assignee | ||
Comment 47•11 years ago
|
||
I still need to test this on OS X (building at the moment) and test it on Android.
Attachment #8335434 -
Attachment is obsolete: true
Attachment #8337922 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 48•11 years ago
|
||
Tested on OSX, and also fixed two issues:
1) No longer showing the volume slider on videos that don't have audio
2) Hiding the volume sliders for small dimension media.
Attachment #8337922 -
Attachment is obsolete: true
Attachment #8337922 -
Flags: review?(gijskruitbosch+bugs)
Attachment #8338004 -
Flags: review?(gijskruitbosch+bugs)
Comment 49•11 years ago
|
||
Comment on attachment 8338004 [details] [diff] [review]
Patch v1.1
Review of attachment 8338004 [details] [diff] [review]:
-----------------------------------------------------------------
Just some comments/questions for now. I need to go get sleep, I'll test out the patch in my morning. Clearing r? for now pending questions either with answers or a new patch. :-)
::: toolkit/content/widgets/videocontrols.css
@@ +91,5 @@
>
> .controlBar[size="hidden"],
> .controlBar[size="small"] .durationBox,
> .controlBar[size="small"] .durationLabel,
> +.controlBar[size="small"] .positionLabel,
Nit: I'd prefer the additional selector on a line that doesn't necessitate touching the other lines. Don't feel strongly if you want to keep the order for DOM order / alphabetical reasons.
::: toolkit/content/widgets/videocontrols.xml
@@ +344,5 @@
> this._isAudioOnly = val;
> if (!this.isTopLevelSyntheticDocument)
> return;
> if (this._isAudioOnly) {
> + this.controlBar.setAttribute("audio-only", true);
But there's already an "audio" attribute on the controlBar set in the constructor. Why are we using a different one here?
@@ +562,2 @@
> this.setMuteButtonState(this.video.muted);
> + this.volumeControl.value = volumePercentage;
I'm a little bit disconcerted that we're disconnecting the value of the scale and the thing we're displaying. Are we just having the scale because it gets us some of the interaction for free? Is there any way that this value could change that doesn't trigger a volumechange event?
@@ +562,3 @@
> this.setMuteButtonState(this.video.muted);
> + this.volumeControl.value = volumePercentage;
> + let foregroundTemp = document.getAnonymousElementByAttribute(this.videocontrols,
You said you were making this a member, that sounds good.
@@ +1423,5 @@
> this.stats.framesDecoded = document.getAnonymousElementByAttribute(binding, "class", "statFramesDecoded");
> this.stats.framesPresented = document.getAnonymousElementByAttribute(binding, "class", "statFramesPresented");
> this.stats.framesPainted = document.getAnonymousElementByAttribute(binding, "class", "statFramesPainted");
>
> + this.isAudioOnly = (this.video instanceof HTMLAudioElement);
AFAICT there's no need to move this, right?
@@ +1526,5 @@
> <spacer flex="1"/>
> <button class="muteButton"
> mutelabel="&muteButton.muteLabel;"
> unmutelabel="&muteButton.unmuteLabel;"/>
> + <scale class="volumeControl" movetoclick="true"/>
How come this no longer needs the stack, but the other one does?
Attachment #8338004 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 50•11 years ago
|
||
Comment on attachment 8338004 [details] [diff] [review]
Patch v1.1
Review of attachment 8338004 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/content/widgets/videocontrols.xml
@@ +1423,5 @@
> this.stats.framesDecoded = document.getAnonymousElementByAttribute(binding, "class", "statFramesDecoded");
> this.stats.framesPresented = document.getAnonymousElementByAttribute(binding, "class", "statFramesPresented");
> this.stats.framesPainted = document.getAnonymousElementByAttribute(binding, "class", "statFramesPainted");
>
> + this.isAudioOnly = (this.video instanceof HTMLAudioElement);
This needs to move because the setter in this patch now touches this.controlBar which isn't defined at the earlier call site.
Assignee | ||
Comment 51•11 years ago
|
||
Fixed issues. I left the ordering of the classes in the videocontrols.css because the selectors are simple enough still that it would be easy to notice a typo in this patch and it would be better to keep them in some type of ordering.
Attachment #8338004 -
Attachment is obsolete: true
Attachment #8338739 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 52•11 years ago
|
||
Tested it on Android and it works fine. Try push,
https://tbpl.mozilla.org/?tree=Try&rev=c17bd2289702
Comment 53•11 years ago
|
||
Comment on attachment 8338739 [details] [diff] [review]
Patch v1.2
Review of attachment 8338739 [details] [diff] [review]:
-----------------------------------------------------------------
Looks great, thanks! :-)
::: toolkit/content/tests/widgets/test_videocontrols.html
@@ +30,5 @@
> const muteButtonHeight = 28;
> const durationWidth = 34;
> const fullscreenButtonWidth = document.mozFullScreenEnabled ? 28 : 0;
> +const volumeSliderWidth = 32;
> +const scrubberWidth = videoWidth - playButtonWidth - muteButtonWidth - durationWidth - fullscreenButtonWidth - volumeSliderWidth;
Nit: probably makes sense to have this in the order in which the buttons appear - I'm hoping the mute and volume bits are next to each other?
::: toolkit/themes/osx/global/media/videocontrols.css
@@ +81,5 @@
> +.volumeForeground {
> + background-repeat: no-repeat;
> + background-position: center;
> + width: 32px;
> + height: 16px;
At least on OS X, when testing this patch I found that the actual height was 28px, to fill up the control bar (hello, -moz-box-stretch, presumably?). Is this necessary for android, or can you just drop it as it has no effect anyway? This then also helps explain why you need background-position: center - to center vertically, because all these boxes and the stack are the same width so horizontally it's not necessary.
::: toolkit/themes/windows/global/media/videocontrols.css
@@ +71,5 @@
> background-image: -moz-image-rect(url("chrome://global/skin/media/fullscreenButton.png"), 0, 32, 16, 16);
> }
>
> +.volumeControl {
> + height: 16px;
Ditto.
@@ +81,5 @@
> +.volumeForeground {
> + background-repeat: no-repeat;
> + background-position: center;
> + width: 32px;
> + height: 16px;
Ditto.
Attachment #8338739 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 54•11 years ago
|
||
Assignee | ||
Comment 55•11 years ago
|
||
Pushed a follow-up fix for the two a11y tests:
https://hg.mozilla.org/integration/fx-team/rev/2d644b3b7063
Assignee | ||
Comment 57•11 years ago
|
||
Backed out the previous two changesets for introducing a test failure on test_videocontrols_video_direction.html:
> /tests/toolkit/content/tests/widgets/test_videocontrols_video_direction.html | Rendering of reftest videocontrols_direction-1d.html should not be different to the reference
The attached patch is a rollup of the previous two patches and includes the fix for this test failure. The issue with the previous patch is that we were no longer setting the "audio-only" attribute on media elements if they weren't in a top-level synthetic document. We also weren't correctly accounting for the absence of the fullscreen button on audio-only elements. Both of those fixes are now in this patch.
The reftest exposed this as it didn't open the media files directly. I'm not sure why this didn't appear on non-PGO builds but the failure and fix makes sense to me.
Attachment #8338739 -
Attachment is obsolete: true
Attachment #8339515 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 58•11 years ago
|
||
And for the backout changeset URLs:
https://hg.mozilla.org/integration/fx-team/rev/4c68e0532792
https://hg.mozilla.org/integration/fx-team/rev/1ee94eaeca88
Comment 59•11 years ago
|
||
Comment on attachment 8339515 [details] [diff] [review]
Patch v1.3
Review of attachment 8339515 [details] [diff] [review]:
-----------------------------------------------------------------
Hmm. Now I have a bunch more questions, so f+ for now (sorry...). As for PGO, PGO builds are clobber. That shouldn't have made a difference for these changes though...
::: accessible/tests/mochitest/actions/test_media.html
@@ +64,5 @@
>
> var audioElm = getAccessible("audio");
> var playBtn = audioElm.firstChild;
> var scrubber = playBtn.nextSibling.nextSibling.nextSibling;
> + var muteBtn = audioElm.lastChild.previousSibling;
This seems pretty fragile. It was already, but hey. Can we improve this by just making it fetch the button by anonid?
::: toolkit/content/widgets/videocontrols.xml
@@ +1374,4 @@
> this._fullscreenButtonWidth;
> +
> + let isAudioOnly = this.isAudioOnly;
> + if (isAudioOnly) {
Can this ever turn false if it was previously true? If so, probably need to be able to undo this somehow.
@@ +1376,5 @@
> + let isAudioOnly = this.isAudioOnly;
> + if (isAudioOnly) {
> + // When the fullscreen button is hidden we add margin-end to the volume stack.
> + let volumeStack = document.getAnonymousElementByAttribute(this.videocontrols, "class", "volumeStack");
> + let volumeStackCS = doc.defaultView.getComputedStyle(volumeStack);
Hohum. This will trigger a reflow. Can we avoid this, or hardcode the value instead? A lot of other stuff is hardcoded here, so I'm not really sure why we need to fetch this dynamically.
::: toolkit/themes/osx/global/media/videocontrols.css
@@ +71,5 @@
> background-image: -moz-image-rect(url("chrome://global/skin/media/fullscreenButton.png"), 0, 32, 16, 16);
> }
>
> +.volumeControl {
> + height: 32px;
Now I'm even more confused. Why did you change the height to be 32px? That's *more* than the 28px I found earlier. As I tried to ask then, do we need a height at all? (same question for the other 32px height below and in the Windows variant of this file)
Attachment #8339515 -
Flags: review?(gijskruitbosch+bugs) → feedback+
Assignee | ||
Comment 60•11 years ago
|
||
I couldn't change the code in the accessible test because neither querySelector or getElementsByAttribute are available on an accessible node.
Attachment #8339515 -
Attachment is obsolete: true
Attachment #8342596 -
Flags: review?(gijskruitbosch+bugs)
Comment 61•11 years ago
|
||
Comment on attachment 8342596 [details] [diff] [review]
Patch v1.4
Review of attachment 8342596 [details] [diff] [review]:
-----------------------------------------------------------------
This generally looks good. The 8px thing is kind of sad but it was also sad the other way, and we do the same thing with e.g. the overlaid play button. If you feel like it, let's file a followup to get that info dynamically rather than hardcoding it.
(Yes, I know I said to hardcode it; that's just because a reflow in that place isn't great, but there's other code in the video controls that fetches a bunch of clientWidths, we could probably bunch it up there with some refactoring work, but I don't want to hold up this bug)
::: toolkit/content/tests/widgets/test_videocontrols.html
@@ +30,5 @@
> const muteButtonHeight = 28;
> const durationWidth = 34;
> const fullscreenButtonWidth = document.mozFullScreenEnabled ? 28 : 0;
> +const volumeSliderWidth = 32;
> +const scrubberWidth = videoWidth - playButtonWidth - durationWidth - muteButtonWidth - volumeSliderWidth - fullscreenButtonWidth;
Nit: this one has the slider and the fullscreen button the other way around compared to the one below in this file. Please make the order match.
::: toolkit/content/widgets/videocontrols.xml
@@ +342,5 @@
> get isAudioOnly() { return this._isAudioOnly; },
> set isAudioOnly(val) {
> this._isAudioOnly = val;
> + if (this._isAudioOnly) {
> + this.controlBar.setAttribute("audio-only", true);
Nit: 4-space indentation
@@ +352,3 @@
> if (!this.isTopLevelSyntheticDocument)
> return;
> +
Nit: no need to add this newline
@@ +1376,5 @@
> +
> + let isAudioOnly = this.isAudioOnly;
> + if (isAudioOnly) {
> + // When the fullscreen button is hidden we add margin-end to the volume stack.
> + minWidthAllControls -= this._fullscreenButtonWidth - 8;
Nit: 4-space indentation.
Nit: please make the 8 thing another property like the overlayPlayButtonHeight/Width.
Attachment #8342596 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 62•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/9915d93be7b6
> If you feel like it, let's file a followup to get that info dynamically rather than hardcoding it.
Filed bug 946819 to track this.
Depends on: 946819
Flags: in-testsuite+
Comment 63•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Comment 64•11 years ago
|
||
I'm nominating this for the relnote meeting later today. Jared: is this visible for the end user?
relnote-firefox:
--- → ?
Flags: needinfo?(jaws)
Comment 65•11 years ago
|
||
(In reply to Jean-Yves Perrier [:teoli] from comment #64)
> I'm nominating this for the relnote meeting later today. Jared: is this
> visible for the end user?
Yes.
Flags: needinfo?(jaws)
Updated•11 years ago
|
Comment 67•11 years ago
|
||
I was able to confirm the fix for this bug on Windows 7 64-bit [1], Windows 8.1 64-bit [2], Mac OS X 10.7.5 [3] and Ubuntu 13.10 64-bit [4], using the latest Beta (Build ID: 20140224220227) with the HTML5 video samples available at [5] and [6].
Additional note:
- Bug 962560 seems to be related to this change, adding it as a dependency.
[1] Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
[2] Mozilla/5.0 (Windows NT 6.3; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
[3] Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:28.0) Gecko/20100101 Firefox/28.0
[4] Mozilla/5.0 (X11; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0
[5] http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_video_all
[6] http://www.html5rocks.com/en/tutorials/video/basics/
Comment 68•11 years ago
|
||
RE: "Volume control for HTML5 audio/video"
Lukas, the release notes make it seem like we just added audio controls to HTML5 <video> and <audio> which is not what this bug did. We have had audio controls on <video> and <audio> probably from the beginning of their support in Firefox. "Horizontal" is the key word in the summary that was missed. The <audio> controls got removed in bug 502892 since the vertical orientation was causing problems. This bug made the controls horizontal so they will also work on <audio> and <video>. In summary, the functional change in this bug is that the volume control on <audio> is back.
Since there have been multiple times that the release notes have been inaccurate and lead to misinformation spreading in the press, I would like to suggest (again) that release note text is shared in the bug before being published so developers can ensure it is accurate.
Thanks
Flags: needinfo?(lsblakk)
Comment 69•11 years ago
|
||
(In reply to Matthew N. [:MattN] from comment #68)
> RE: "Volume control for HTML5 audio/video"
> Lukas, the release notes make it seem like we just added audio controls to
> HTML5 <video> and <audio> which is not what this bug did. We have had audio
> controls on <video> and <audio> probably from the beginning of their support
> in Firefox. "Horizontal" is the key word in the summary that was missed. The
> <audio> controls got removed in bug 502892 since the vertical orientation
> was causing problems. This bug made the controls horizontal so they will
> also work on <audio> and <video>. In summary, the functional change in this
> bug is that the volume control on <audio> is back.
>
> Since there have been multiple times that the release notes have been
> inaccurate and lead to misinformation spreading in the press, I would like
> to suggest (again) that release note text is shared in the bug before being
> published so developers can ensure it is accurate.
>
> Thanks
Thank you for your help in catching this, I've pushed an update to reflect the *horizontal* aspect of this. Interestingly, notes are on Aurora, then Beta, then Release and we have several opportunities for people to look them over. There is a longer-term plan to work with a new publishing system that would allow us to automate the posting of notes *from* bugs so the text of the note would be in the bug, as you suggest - this is something we want - however at the moment it's a lot of effort and something that changes regularly so I think we must try to do better at sharing out the notes for review in the meantime to continue to catch these discrepancies. While I currently share the notes with release-drivers and press mailing lists, they could also be sent to mailing lists where more engineering eyes will see them so let's try that and see if that helps in the pre-release period.
Flags: needinfo?(lsblakk)
You need to log in
before you can comment on or make changes to this bug.
Description
•