Daniel Holbert [:dholbert] Reporter
	Description • 9 years ago

Per discussion w/ Dolske, we'd like to convert the HTML <video>/<audio> controls to be HTML instead of XUL.

Filing this bug for that. We did some earlier work on this in bug 726240 (which was initially focused on just a piece of the controls), but this bug here is for a fresh start.

dolske mentions that some piece (the scrubber, I think) might want to remain XUL for the time being, too.

Current videocontrols impl is here, I think:
https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml?rev=4e188de86d86&force=1
https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.css?rev=55758d00de02&force=1

	Daniel Holbert [:dholbert] Reporter
	Comment 1 • 9 years ago

Here's a "part 1" patch, which removes some nsVideoFrame special-case code that assumes our video controls are XUL.  Somewhat to my surprise, things seem to Just Work (without any other de-XUL-ification) if we just rip out this special-case code and share the caption code instead.

Anyway -- any de-XUL-ification work will need to be based on top of this patch or something like it.  (or else we'll run into crashes like the one Gijs tripped over in bug 1218974 comment 6.)

	Justin Dolske [:Dolske]
	Comment 2 • 9 years ago

This patch makes just the topmost container (the mediaControlsFrame) be inline-block, with some possibly-hacky CSS-fixup to make the controls normally sized. Your part1 patch makes this not crash.

Is your patch, alone, expected to work fine? I didn't see any obvious problems, so my patch doesn't seem to really be required (other than as a first step to removing a trivial smidgen of XUL for this bug).

	Ray Lin[:ralin]
	Comment 3 • 8 years ago

per discussion in Bug 855149, it seems not ready to convert whole XUL to HTML at present. I think this might be a good step to move forward for now.

A couple reasons for doing it:
- modern layout standard support like flexbox
- get rid of mostly XUL for followup visual refresh bug
- better basis for further refactor, e.g. web component

Gijs, do you think it make sense? 
If possible, I'd like to pick up prior patch and start this bug. Thanks.

	:Gijs (he/him)
	Comment 4 • 8 years ago

(In reply to Ray Lin[:ralin] from comment #3)
> per discussion in Bug 855149, it seems not ready to convert whole XUL to
> HTML at present. I think this might be a good step to move forward for now.
> 
> A couple reasons for doing it:
> - modern layout standard support like flexbox
> - get rid of mostly XUL for followup visual refresh bug
> - better basis for further refactor, e.g. web component
> 
> Gijs, do you think it make sense? 
> If possible, I'd like to pick up prior patch and start this bug. Thanks.

I don't understand what you're asking. You say "it seems not ready" but also "this might be a good step to move forward for now". What is "it" and "this" and what is the difference?

	Ray Lin[:ralin]
	Comment 5 • 8 years ago

Sorry I didn't make it clear.

In short, Bug 855149 is not doable yet, and this Bug 1222273 should move forward.

	:Gijs (he/him)
	Comment 6 • 8 years ago

(In reply to Ray Lin[:ralin] from comment #5)
> Sorry I didn't make it clear.
> 
> In short, Bug 855149 is not doable yet,

I'm not sure that's true - it's just going to involve work to fix the event leak.

> and this Bug 1222273 should move forward.

Sure, but I don't know how much we can do here without running into the issues discussed in bug 855149. You'd have to try and see.

	Ray Lin[:ralin]
	Comment 7 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/67052/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67052/

	Ray Lin[:ralin]
	Comment 8 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/67054/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67054/

	Ray Lin[:ralin]
	Comment 9 • 8 years ago

Some updates:

- changing reflow breaks Android unless we convert touchcontrol too. If possible, we should not take the risk of updating both at one time.
- as :gijs mentioned, bug 855149 is still a problem. So, I'd like to do Bug 1271765 first rather than spend effort converting <scale> to pure HTML(and refresh visual again...).

	Ray Lin[:ralin]
	Comment 10 • 8 years ago

De-XUL work(template and style) is mostly done in Bug 1271765, but breaks <audio> now. Need frame-class to be fixed here and land before Bug 1271765.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 11 • 8 years ago

Daniel, are you going to actively work on this? If you are not available or if you think it's more appropriate, I will work with Astley to find a layout engineer in Taipei to help this out.

Thanks!

	Daniel Holbert [:dholbert] Reporter
	Comment 13 • 8 years ago

Sorry for the delay here -- it's probably simplest for someone who's written a reflow method before to implement this (to have some context for what we currently do / what we should do instead), so I should probably take a first crack at it.

I intend to look into this by the end of the week -- leaving needinfo open.

	Daniel Holbert [:dholbert] Reporter
	Comment 14 • 8 years ago

(In reply to Daniel Holbert [:dholbert] from comment #13)
> I intend to look into this by the end of the week -- leaving needinfo open.

(Following up here -- I had initially missed that this was closed when I posted comment 13, and ralin has been good enough to fix up this reflow code as part of bug 1271765, so I think we're good here. --> Canceling needinfo.)