AUDIO/VIDEO control UI should display numeric value of current position eg:- http://jboriss.wordpress.com/2008/09/19/html-5-video-tag-pirate-edition/

First cut. So, this visually looks like Boriss' updated mockup, but has a couple of glitchy things. 1) I was having a hard time getting the thumb to horizontally overhang the end of the scrubber bar. I then realized that the same effect could be had by shrinking the scrubber bar within the <scale>. [EG, with a 46 pixel wide thumb, add 23 pixels of margin to the left and the right of the bar.] Thinking about it that way made it easy to implement. However... Because the scrubber bar would then be 23 pixels from either end of the actual <scale> box, there would be big empty gaps around the scrubber and surrounding controls. I've "fixed" this by setting a negative margin on the immediate siblings of the <scale>, but this isn't quite correct. For example, only the leftmost few pixels of the play button can be clicked, because the rest is actually being overlapped by the <scale> (and so clicking there actually causes a seek). I'm not quite sure how to fix that. Maybe setting z-index would work (does that work in XUL?), but then we might not be able to click-to-seek to the beginning, because that region of the <scale> is overlapped by the play button. 2) I think I'm hitting some layout bug when dragging the thumb. The portion that's above the rest of the control bar tears and lags behind when dragging it back and forth. There are a number of other small polish issues, but these are the major problems for now.

(In reply to comment #2) > Created an attachment (id=370149) > Updated mockup from Boriss Mockup look like showing only 1. Mouse over position 2. Total duration. Originally this bug was for current "playing" position. Are we doing that?

> 1. Mouse over position Bug 475320

No, the current position is now an integral part of the scrubber thumb, and is always shown. No mouseover effects. (Other than the existing stuff, which fades in/out the entire set of controls.) Not dealing with 475320 here.

I don't think this needs to block.

(In reply to comment #3) > However... Because the scrubber bar would then be 23 pixels from either end of > the actual <scale> box, there would be big empty gaps around the scrubber and > surrounding controls. I've "fixed" this by setting a negative margin on the > immediate siblings of the <scale>, but this isn't quite correct. For example, > only the leftmost few pixels of the play button can be clicked, because the > rest is actually being overlapped by the <scale> (and so clicking there > actually causes a seek). > Yeah, until we have the pointer-events property, you can't really have overlapping elements where clicks go to where the image is rendered and pass through non-transparent areas. You could however, make the play button appear above the scale by setting 'position: relative' on it. At least the thumb area above the play button would still be draggable.

I think this is ready for a review pass, though it's not quite perfect. * Still iterating on the thumb design with Boriss, but I think we're mostly down to small shape/color tweaks. * On OS X, the text is subpixel aligned but the thumb is pixel snapped. This results in the text wobbling around in the thumb (making fuzzy shifts before the thumb makes a whole-pixel shift). I think this needs to be spun out to a separate bug (probably some rounding/snapping to do in nsSliderFrame?). * As in comment 3, the top part of the thumb can't be dragged and exhibits tearing. Presumably a layout bug, will spin out as it's not critical. * Clicking on the scrubber bar to jump around doesn't work if you click too close to the pointy bit of the thumb, because that's actually a (transparent) part of the thumb's box. I suspect there isn't a good workaround for this. :( * I worry that getting the times' font-size and position exactly correct on all systems might be impossible. I've got it as I want it now on my stock OS X, Ubuntu, and XP boxes. But considering all the edge cases of changing installed/default fonts, locale settings, and the unix font mess means... Ugh. Hopefully we can get it mostly-right for the first landing, and adjust as needed based on nightly/beta feedback.

Aaaand just noticed I left |debug| set to true, and that formatTime() can be moved into Utils as a common helper, since valueChanged() has to reach into Utils anyway.

Comment on attachment 370599 [details] [diff] [review] Patch v.2 >diff --git a/toolkit/content/widgets/videocontrols.css b/toolkit/content/widgets/videocontrols.css >--- a/toolkit/content/widgets/videocontrols.css >+++ b/toolkit/content/widgets/videocontrols.css >@@ -1,5 +1,9 @@ > @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"); > >+.scale-thumb { >+ -moz-binding: url("chrome://global/content/bindings/videocontrols.xml#timeThumb"); >+} >+ Presumably this doesn't affect other scales right? >+ <binding id="timeThumb" >+ extends="chrome://global/content/bindings/scale.xml#scalethumb"> >+ <xbl:content xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> >+ <xbl:children/> >+ <hbox class="timeThumb"> >+ <label class="timeLabel" value="0:00"/> >+ </hbox> >+ </xbl:content> >+ </binding> Is it possible to move the time handling code into this binding, such that one could just have a 'time' property or somesuch? Or would that also cause a security exception?

(In reply to comment #11) > Presumably this doesn't affect other scales right? Only where videocontrols.css is included. I should probably make it ".scrubber .scale-thumb", so that it doesn't interfere when a volume <scale> is added... > >+ <binding id="timeThumb" > > Is it possible to move the time handling code into this binding, such that one > could just have a 'time' property or somesuch? Or would that also cause a > security exception? Hmm, maybe, I can try shifting some stuff around.

Cleaned up previous comments, tweaked time display, new artwork from Boriss.

I've also pushed this to the tryserver, for anyone interested in trying this out.

Filed bug 486879 on the text shifting around within the scrubber thumb, and bug 486882 on some portions of the thumb being undraggable.

Comment on attachment 371085 [details] [diff] [review] Patch v.3 >+ <property name="showHours"> >+ <getter> >+ <![CDATA[ >+ return this.getAttribute("showHours") == "true"; xul attributes should be all lowercase. >+.durationBox { >+ -moz-box-pack: center; >+} Why is a pack of center used in various places? I'd probably be a bit pickier about some other things, but I guess the timeThumb is only used internally for this widget so it doesn't matter too much. So this looks ok.

Fixed showHours nit. Talked with Boriss, we'll good with this as-is but will do a post-landing followup to tweak a couple of design bits still in flux (thumb shape overlaps with pause/duration at extremes, time text might look better bolded and/or with a slight text-shadow). (In reply to comment #16) > Why is a pack of center used in various places? For centering the time labels (the duration is centered vertically in the control bar, the current time is centered horizontally in the thumb).

Comment on attachment 371590 [details] [diff] [review] Patch v.4 Maybe one day I'll be clearer ;) XUL attributes are lowercase, properties in Javascript should be mixed case.

(In reply to comment #18) > (From update of attachment 371590 [details] [diff] [review]) > Maybe one day I'll be clearer ;) > > XUL attributes are lowercase, properties in Javascript should be mixed case. Ok, now the attribute is "showhours" and the property is ".Showhours". [;-)]

Comment on attachment 371720 [details] [diff] [review] Patch v.5 uir+ good for now, tweaking planned for things like text moving at the same rate as the marker

Oops. :( This fixes the the videocontrols test to account for the presence of a duration and the changed thumb size. I'm shortly going to rewrite this test to be more extensive and detailed, so this will do for now. I also noticed that the test still fails, because it notices a couple of errors like "JavaScript error: , line 0: Permission denied for file://; to call method XULElement.QueryInterface on file://;". It turns out these are actually coming from nsTextBoxFrame::UpdateAccesskey(). Harmless, but I'm surprised that Mochitests even sees it. I thought it was just console spam noise. :( Conveniently, mrbkap tells me his upcoming patch should fix this problem, and generally resolving the annoying security errors the videocontrols have been tapdancing around. So this patch can land as-is after his patch, which should hopefully be done as soon as tomorrow.

Pushed http://hg.mozilla.org/mozilla-central/rev/1e38c7369a3d bkap's fixes won't be ready for a bit longer. While trying to find some other way to suppress that QI error, I found that an empty <script> in the test before the <video> was, oddly enough, enough to make it go away. I don't understand why, but it wasn't an important problem so better to get this baking...

Backed out. *sigh* I'm stupid. The other video related mochitests hit the QI error. I don't want to add temporary workarounds to them all, may need to wait for mrbkap's fix to land first after all. :-(

Comment on attachment 371993 [details] [diff] [review] Patch v.6 Clearing nomination request based on previous comment.

Comment on attachment 371993 [details] [diff] [review] Patch v.6 a191=beltzner

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

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

This broke the test_elm_media.html a11y test. I've pushed http://hg.mozilla.org/mozilla-central/rev/a0b5b3125f60 to fix that. Do we not run that test on branch?

(In reply to comment #28) > Do we not run that test on branch? Not yet, because bug 483573 is awaiting approval and hasn't landed on branch yet. This is where the file you fixed got introduced.

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