1368639 - Refine media controls size/layout adjustment metabug

Status: NEW

(Toolkit :: Video/Audio Controls, enhancement)

Description

[Ray Lin[:ralin]]

Since we've got several relevant issues about this, I thought it'd be better have a metabug to track these bugs.

Comment 1

[Ray Lin[:ralin]]

adding bugs regarding new media controls' min-required height

Comment 2

[Ray Lin[:ralin]]

adding bug about dynamically resizing media

Comment 3

[Ray Lin[:ralin]]

adding bugs about too much recursion was called when bogus is given to <audio>

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I am now fairly certain the foundational logic in adjustControlSize() is flawed.

For <video>, the interaction b/t controls and layout is easy: it figured out the size of <video> box, and the controls "fills" the box. Controls has no ability to influence the dimensions of <video>.

For <audio>, it is a lot tricker. The controls depends on the layout to tell it the right size, but the layout also depends the controls to figure out the intrinsic dimensions.

How do we break this circular dependency?

I don't think we could hard code the intrinsic dimensions in nsVideoFrame, because that size would depend on future changes in the JS and CSS, and it will be hard to track. Besides, it might be dynamic base on isTouchControls.

We could have a method/attribute on a DOM element and have nsVideoFrame to take the intrinsic dimensions. I don't know if it is feasible or not.

:dholbert, did I frame the problem correctly? Do you have an idea of how this should work? Let me know what you think. Thanks!

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #4)
> We could have a method/attribute on a DOM element and have nsVideoFrame to
> take the intrinsic dimensions. I don't know if it is feasible or not.

I wonder if there is a CSS property exists that could be purposed to do this.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #4)
> I am now fairly certain the foundational logic in adjustControlSize() is
> flawed.
> 
> For <video>, the interaction b/t controls and layout is easy: it figured out
> the size of <video> box, and the controls "fills" the box. Controls has no
> ability to influence the dimensions of <video>.

You mean, we get the size from the video itself?

What about when there is no video, e.g. data:text/html,<video controls src=""> ?  Where does the size come from there? (And do we end up falling back to the same problem we have for audio?)

> For <audio>, it is a lot tricker. The controls depends on the layout to tell
> it the right size, but the layout also depends the controls to figure out
> the intrinsic dimensions.
> 
> How do we break this circular dependency?

Could we have it start out with some default (perhaps invisible, but still space-occupying) controls, which nsVideoFrame can reflow when measuring its intrinsic size? And then the JS can adjust the actually-visible the element's actual size if needed, if it has e.g. "width: 50%" or something like that which produces a different size in the end...

(If that doesn't work, then I might not be fully understanding the problem here.)

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> You mean, we get the size from the video itself?
> 
> What about when there is no video, e.g. data:text/html,<video controls
> src=""> ?  Where does the size come from there? (And do we end up falling
> back to the same problem we have for audio?)
> 

For that example, we will fall into default intrinsic size of the web, i.e. 300x150.

This is what data:text/html,<video> behaves today, it's unrelated to having controls or not. Unless you are saying my assertion in comment 4 on <video> was incorrect...

(and no we don't falling back to the same problem we have for audio, though the adjustControlSize() impl does have some ill-adviced logic trying to probe isAudioOnly, which we do flip to true in <video> when there isn't a video stream in the file)

> > How do we break this circular dependency?
> 
> Could we have it start out with some default (perhaps invisible, but still
> space-occupying) controls, which nsVideoFrame can reflow when measuring its
> intrinsic size? And then the JS can adjust the actually-visible the
> element's actual size if needed, if it has e.g. "width: 50%" or something
> like that which produces a different size in the end...
> 
> (If that doesn't work, then I might not be fully understanding the problem
> here.)

If you look at bug 1495821, bug 1495817, and bug 1367875, what happened internally with them is (taking numbers from bug 1495821 as example):

1. nsVideoFrame reflow the <audio>, consider the initial size of the controls (270px), and decided the width of the <audio> should be 270px too.
2. When we do audioEl.style.width = '500px' and trigger reflow, nsVideoFrame sets the width of <audio> to 500px, and calls into adjustControlSize() by dispatch "resizevideocontrols" event to <div id="videocontrols">.
3. adjustControlSize(), measuring the width of <audio> with el.clientWidth, decided it's big enough and expand the controls to 500px accordingly.
4. Then, when audioEl.style.width = '' happens and reflow triggers, nsVideoFrame sets the width of <audio> to .... the width it sees on of the controls, which is 500px. nsVideoFrame would triggers adjustControlSize() again like step 3 but the result is no-op.

The desired width of step 4 should be 270px, but nsVideoFrame has no way to come up with that number. That's my problem.

Let me know if that explains the problem clearer. Love to discuss this in person too :)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (comparing default-size <audio> vs <audio> whose size dynamically becomes default)

Thanks. Here's a testcase that roughly demonstrates what you're talking about -- we fail to shrink the dynamically-adjusted audio element in this testcase.

Comment 9

[Daniel Holbert [:dholbert]]

I have two thoughts, but neither is a great/complete solution at this point.

(1) Hacky but functional: specify a non-"auto" default width for audio elements. This seems to be what Chrome does -- if I inspect an <audio controls> element there, I see that it has
    audio {        // user agent stylesheet
      width: 300px;
      height: 54px;
    }

The downside of this approach is that an explicit "width:auto" might break, as it does in Chrome -- e.g. this renders a 0-width box in Chrome:   data:text/html,<audio controls style="width:auto">


(2) Perhaps more ideal -- express the internal layout using flexbox or similar (rather than hardcoded sizes for the internal components) so that it Just Works to accommodate resizes.  This handles the case of going from default --> 500px --> back to default.  But it probably doesn't handle cases where we need to hide/un-hide components, e.g. from default --> 50px --> back to default...  Container queries might help in that scenario (once we've got them), but we don't have them implemented yet, and that would also mean that the container wouldn't be able to depend on the size of its contents which would break intrinsic sizing.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

What Chrome does is interesting. This perhaps is actually an interopt issue? Do we know if anything in the spec says what the UA should do?

If the behavior of Chrome is what to match, perhaps that's our easy way out.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I guess these are the relevant texts in the spec:

https://html.spec.whatwg.org/multipage/rendering.html#replaced-elements

The audio element, when it is exposing a user interface, is expected to be treated as a replaced element about one line high, as wide as is necessary to expose the user agent's user interface features. When an audio element is not exposing a user interface, the user agent is expected to force its 'display' property to compute to 'none', irrespective of CSS rules.

and it links to CSS2

https://drafts.csswg.org/css2/conform.html#replaced-element

An element whose content is outside the scope of the CSS formatting model, such as an image, embedded document, or applet. For example, the content of the HTML IMG element is often replaced by the image that its "src" attribute designates. Replaced elements often have intrinsic dimensions: an intrinsic width, an intrinsic height, and an intrinsic ratio. For example, a bitmap image has an intrinsic width and an intrinsic height specified in absolute units (from which the intrinsic ratio can obviously be determined). On the other hand, other documents may not have any intrinsic dimensions (for example, a blank HTML document).

User agents may consider a replaced element to not have any intrinsic dimensions if it is believed that those dimensions could leak sensitive information to a third party. For example, if an HTML document changed intrinsic size depending on the user's bank balance, then the UA might want to act as if that resource had no intrinsic dimensions.

The content of replaced elements is not considered in the CSS rendering model.

==

So the spec basically explicitly says the intrinsic dimensions of <audio> controls is outside of its scope. Our bugs mentioned in comment 7 are not spec violations, they are just awkward behaviors if you consider HTML/CSS are declarative programming languages.

I am now more convinced that we should do what Chrome does.

Comment 12

[Daniel Holbert [:dholbert]]

Note: our issues with my attached "testcase 1" here seem to have been fixed in bug 1683979.

Two dependent bugs remain open:

• Bug 1495817 (another layout statefulness bug, with a testcase similar to the one here but with height instead of width having unexpected persistent effects)
• Bug 497164 (which doesn't seem to be about size/layout at all and hence maybe isn't really related to this bug? It seems to be a feature-request to have the video/audio volume setting persist across reload)

Maybe we can close this metabug? I'm not sure it's useful anymore. shrug