Closed Bug 726240 Opened 12 years ago Closed 6 years ago

[meta] Video controls should use HTML instead of XUL

Categories

(Toolkit :: Video/Audio Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Dolske, Unassigned)

References

(Blocks 2 open bugs)

Details

(Keywords: meta)

Attachments

(1 file, 5 obsolete files)

Looks like this should mostly be a drop-in replacement as the two elements unsurprisingly have basically the same API. Patch in a bit!
I should note there is no argument as to why this should be done.  I can understand page designers wanting to style the players to better fit the site.  And a rule like "audio > progress" would help there.  However, I have two concerns:

1. As I understand it, XUL elements are faster than their HTML counterparts.  So unless the HTML <progress> tag is really just the XUL <progressmeter> tag in disguise, this could slow down browsing.

2. Leaving the player unstyled (naked) means that the user knows the player is the authentic Gecko HTML5 player control.  If it is styled, they might be seeing malware (might is the key word here).  I don't know how big of a concern this is, but feel it should be mentioned.
Uhh, this bug has nothing to do with giving the page control over style. It's entirely an implementation detail, there is no difference visible to the user or to the page. And, if anything, XUL tends to be less performant because more effort is spent making HTML paths fast.
Attached patch Patch v.0 (WIP) (obsolete) — Splinter Review
Working now, stupid CSS namespace had me confused for a bit.

Todo: winstripe, ensure the small-control stuff still works correctly (istr one experimentally-derived size).
Assignee: nobody → dolske
Assignee: dolske → mdeboer
Stealing this per email from dolske.
Assignee: mdeboer → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attached patch Unbitrotted patch (WIP) (obsolete) — Splinter Review
pinstripe is dead (long live pinstripe) and this had bitrotted a bit. Sadly, even unbitrotted it looks like it needs some styling work - on Mountain Lion, at least, I'm seeing a beautiful native-colored (bluey) progressbar rather than halftransparent white. Looking into why that is, but DOMI and Inspector have so far proved unhelpful...
Attachment #596268 - Attachment is obsolete: true
OK, so the issues I was seeing earlier were caused by mach build toolkit not doing what I thought it would. When I built correctly, it worked fine on OS X, except when you session-restored a page with <video>s. For some reason some bits are not fully loaded and the code tries to set the progress bar's value to NaN, which will cause it to throw a hissy fit and not work anymore until you reload the page. I've worked around that now, but it's filed already as bug 842500.

(In reply to Justin Dolske [:Dolske] from comment #3)
> Todo: winstripe, ensure the small-control stuff still works correctly (istr
> one experimentally-derived size).

What exactly do you mean by the bits after winstripe? :-)
Attachment #732720 - Attachment is obsolete: true
I sent the earlier patch to try (after unsuccessful attempts to hack omni.ja on windows without doing a full build, which my old windows machine wouldn't have done well at; I hadn't thought of the fact that the optimizations on omni.ja make this impossible now). That came back with test failures in mochitest because of other assignments to .value that now complain if they don't get a proper number (see my previous comment). I've fixed that and fired off another round of windows+linux + mochi/reftests. I've also checked what the previous try build's appearance looks like on linux (VM), and that seems to be fine. I'll check windows tomorrow.

I've realized that the size=small version of things doesn't quite work yet. I've adapted the CSS for it, but I'm still seeing the controls protruding from the video on the right hand side. I'll look at that some more tomorrow.
Attachment #732770 - Attachment is obsolete: true
Attached patch Working patch (obsolete) — Splinter Review
Tested this locally (OSX) and it seems to work, also for the small UI (which took a lot of fiddling to get right).

I fired off a try build (the last one still had some mochitest failures in more of the same floating point setting errors for .value/.max): https://tbpl.mozilla.org/?tree=Try&rev=ad3d72797777

Once that's in I'll doublecheck if it looks good on linux and win32 and request review if so.

I also ran into an existing bug. Essentially, in calculating the size at which we should switch to small UI, we don't take the fullscreen button into account, which means that when we calculate there is enough room for the play and volume buttons, the controls bar is still slightly too big. I'll file that separately with its own patch.
Attachment #733069 - Attachment is obsolete: true
This is going in the right direction, but there's still a bunch of things to do. Off the top of my head:
- make the play button and the scrubber stack position absolute so as to allow the play button to be "on top" of the scrubber's thumb (otherwise you can't click play/pause...)
- align the mute/fullscreen buttons correctly
- perfect the scrubber sizing (possibly resize when we resize all the controls rather than using css calc?)
- figure out why after a page reload, videos show a full "played" progress bar.
- the volume control's animation is visible outside the video's box (?!)
- the volume control's slider background is wrong (and naive attempts to change it have failed, need to really figure out what's causing it)
- tweak text alignment in the duration/position timing labels
- check small UI
- fix all the tests
Attachment #733342 - Attachment is obsolete: true
(In reply to :Gijs Kruitbosch from comment #9)
> Created attachment 737757 [details] [diff] [review]
> WIP patch to HTML-ify everything
> 
> - the volume control's animation is visible outside the video's box (?!)

Is this issue the same that https://bugzilla.mozilla.org/show_bug.cgi?id=502892 worked around? In other words, does this only happen for very small videos?
(In reply to Jared Wein [:jaws] from comment #10)
> (In reply to :Gijs Kruitbosch from comment #9)
> > Created attachment 737757 [details] [diff] [review]
> > WIP patch to HTML-ify everything
> > 
> > - the volume control's animation is visible outside the video's box (?!)
> 
> Is this issue the same that
> https://bugzilla.mozilla.org/show_bug.cgi?id=502892 worked around? In other
> words, does this only happen for very small videos?

AFAICT, no, it's the opposite: when the volume control animates in, you can briefly see it sliding up below the video box, which is very weird. :-)
I have heard rumours that other people are interested in taking this. I'm not currently working on it, but I'm happy to provide advice/encouragement if others would like to. It's probably easier now than it was a year ago because of the horizontal volume control.
Assignee: gijskruitbosch+bugs → nobody
Status: ASSIGNED → NEW
Whiteboard: [mentor=Gijs][lang=js][lang=xbl]
Danny, would you like to work on this? Note that Gijs says it should now be a bit easier than it was when he started it.
Flags: needinfo?(dannychen210)
Yeah, I'm interested in trying this. What would be a good starting point?
Flags: needinfo?(dannychen210)
I think a good first step will be un-conflicting/bitrotting the WIP patch I put on the bug, getting a build with it (a local build on your machine at first, but a try build might also be useful - if you don't have access to creating those, let me know and I can do one for you if you upload a new patch) and seeing how well it works. You should be able to inspect the current state of affairs with DOM Inspector on recent builds, and compare the results before and after patch.

Then you'll need to make the volume slider horizontal (if the unbitrotting above didn't already do that) in the HTML version.

Finally, you'd need to have a look at correcting the issues in comment #9, insofar as they still apply. :-)

So because it's XUL, the current controls use flex in order to fit to the size of the video box. The HTML version uses absolute positioning because back when I started work on this, we didn't have CSS3 flexbox. It would probably be useful to try to use that instead of the absolute positioning it's using now, at least for the things inside the control container ( .controlsContainer, IIRC)

This is a pretty general outline, and it'd be totally normal if you have questions as a result. Please, feel free to ask about whatever isn't clear. :-)

(needinfo'ing me is usually effective, although I will be on holiday some of the coming days, so replies might not always arrive within 24 hours - I will do my best!)
Could we replace the entire controls with HTML? One of the ways that the memory used by a document bloats is the inclusion of elements from different namespaces. That causes multiple UA style sheets to be pulled in (xul.css and html.css, for example) which can add hundreds of kilobytes to a document. The video controls is one of those cases, it pulling in xul.css specifically.
(In reply to Jonathan Watt [:jwatt] from comment #16)
> Could we replace the entire controls with HTML? One of the ways that the
> memory used by a document bloats is the inclusion of elements from different
> namespaces. That causes multiple UA style sheets to be pulled in (xul.css
> and html.css, for example) which can add hundreds of kilobytes to a
> document. The video controls is one of those cases, it pulling in xul.css
> specifically.

I suspect it won't be easy to replace the controls to use native anon content with HTML, and forego XBL altogether. I don't know if using only XBL + HTML would have the advantage of no longer including xul.css. Needinfo'ing you to get some feedback on this. :-)

I'm also surprised because I would have assumed that seeing as they're the same stylesheets that are loaded in chrome, this shouldn't cause that much overhead?

When I wrote the WIP, there wasn't a good HTML replacement for a <scale>. I don't know if that's changed now. It could be re-implemented in HTML + JS, of course, if there was enough reason to do so... Hundreds of kb per document seems like enough to warrant some attention...
Flags: needinfo?(jwatt)
(In reply to :Gijs Kruitbosch from comment #17)
> I don't know if using only XBL
> + HTML would have the advantage of no longer including xul.css.

It should do.

> I'm also surprised because I would have assumed that seeing as they're the
> same stylesheets that are loaded in chrome, this shouldn't cause that much
> overhead?

The sheet is shared between documents, but the cascade data that results is not. It's the cascade data that causes the memory use issues, and presumably slows down style computation, although I don't know if to a significant extent.

> When I wrote the WIP, there wasn't a good HTML replacement for a <scale>. I
> don't know if that's changed now.

<input type=number>

> It could be re-implemented in HTML + JS,
> of course, if there was enough reason to do so... Hundreds of kb per
> document seems like enough to warrant some attention...

I believe for xul.css on OS X it was about 70 KB last I checked.

It would also be nice to take steps to get us to a future where XUL is gone from our tree. We're a long way from that ATM, but every step will count. :)
Flags: needinfo?(jwatt)
(In reply to Jonathan Watt [:jwatt] from comment #18)
> (In reply to :Gijs Kruitbosch from comment #17)
> > I don't know if using only XBL
> > + HTML would have the advantage of no longer including xul.css.
> 
> It should do.
> 
> > I'm also surprised because I would have assumed that seeing as they're the
> > same stylesheets that are loaded in chrome, this shouldn't cause that much
> > overhead?
> 
> The sheet is shared between documents, but the cascade data that results is
> not. It's the cascade data that causes the memory use issues, and presumably
> slows down style computation, although I don't know if to a significant
> extent.
> 
> > When I wrote the WIP, there wasn't a good HTML replacement for a <scale>. I
> > don't know if that's changed now.
> 
> <input type=number>


Errr? <scale> is: "A scale (sometimes referred to as a "slider") allows the user to select a value from a range. A bar displayed either horizontally or vertically allows the user to select a value by dragging a thumb on the bar."

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/scale

This can't be replaced by input type="number".
Sorry, I meant <input type=range>.
An example of styling:

https://mxr.mozilla.org/mozilla-central/source/layout/reftests/forms/input/range/moz-range-progress-1.html

If it doesn't meet our needs for video controls please file bugs and CC me. I specifically added pseudo-elements so that it could.
Depends on: 855149
Summary: Video controls should use HTML <progress> instead of XUL <progressmeter> → Video controls should use HTML instead of XUL (<input type=range> for <scale>, <progress> for <progressmeter>, …)
It's not yet possible to use <html:scale> here, see the dep bug.
Summary: Video controls should use HTML instead of XUL (<input type=range> for <scale>, <progress> for <progressmeter>, …) → Video controls should use HTML <progress> instead of XUL <progressmeter>
So the bug should be removed from the dep tree then? It was my impression that both will be happening once bug 855149 is resolved.

Is there another way to avoid the XUL stuff in the controls e.g. by amending the spec or creating a new element (possibly using WebComponents) that covers the use cases of the video controls?
Mentor: gijskruitbosch+bugs
Whiteboard: [mentor=Gijs][lang=js][lang=xbl] → [lang=js][lang=xbl]
(In reply to Florian Bender from comment #23)
> So the bug should be removed from the dep tree then? It was my impression
> that both will be happening once bug 855149 is resolved.
> 
> Is there another way to avoid the XUL stuff in the controls e.g. by amending
> the spec or creating a new element (possibly using WebComponents) that
> covers the use cases of the video controls?

It would be possible to use more complex HTML, but ideally it should be possible to just use <html:scale> and some CSS. Web components are still out because they're not turned on for release by default, AIUI.
See Also: → 1222273
After discussion with dolske, I filed fresh bug 1222273 on converting <video> controls to be HTML instead of XUL (to the extent that makes sense at least).

Not sure what the current scope of this bug is, but we might want to dupe it forward, or make this bug depend on that bug. I'll let folks who are in-the-know (Gijs?) decide what makes the most sense here.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Daniel Holbert [:dholbert] from comment #25)
> After discussion with dolske, I filed fresh bug 1222273 on converting
> <video> controls to be HTML instead of XUL (to the extent that makes sense
> at least).
> 
> Not sure what the current scope of this bug is, but we might want to dupe it
> forward, or make this bug depend on that bug. I'll let folks who are
> in-the-know (Gijs?) decide what makes the most sense here.

Because we won't be replacing the scrubber/progressmeter there, going to just make this into a meta bug and remove the mentoring info.
Mentor: gijskruitbosch+bugs
Depends on: 1222273
Flags: needinfo?(gijskruitbosch+bugs)
Keywords: meta
See Also: 1222273
Summary: Video controls should use HTML <progress> instead of XUL <progressmeter> → [meta] Video controls should use HTML instead of XUL
Whiteboard: [lang=js][lang=xbl]
Blocks: 1271768
Depends on: 1310907
Depends on: 1444489
All fixed!
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Blocks: 1446829
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: