1065766 - <video> needs "object-fit:contain" by default

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

The initial value of "object-fit" is "fill", which is defined as:
 "The replaced content is sized to fill the element's content box"

...which basically means "scale without regard for intrinsic aspect ratio".

This is the traditional behavior for <img>, but not for <video>. Right now (without 'object-fit' support), <video> renders with the equivalent of "object-fit: contain" behavior. (So e.g. if we have a tall-and-skinny <video> element with a normal-sized video playing in it, the video content doesn't get stretched vertically -- it's scaled such that it fits without anything being clipped.)

To preserve that behavior, we'll need to set "object-fit:contain" for <video> in our UA stylesheet.

Comment 1

[Daniel Holbert [:dholbert]]

This behavior for <video> is specified explicitly in the WHATWG HTML spec:
> video { object-fit: contain; }
https://html.spec.whatwg.org/multipage/rendering.html#video-object-fit

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v2

(correctness nit: I'd forgotten that <video> technically takes a closing tag, </video> [unlike <img>] -- added that here.)

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8489690 [details] [diff] [review]
fix v2

Review of attachment 8489690 [details] [diff] [review]:
-----------------------------------------------------------------

Given my comment just now in bug 1067634, how are we going to handle dynamic changes to the layout.css.object-fit-and-position.enabled pref?  It's probably more important here, as it could result in the video displaying incorrectly after the pref change (depending on how you end up implementing the actual layout behaviour changes for <video>).

::: layout/style/html.css
@@ +717,5 @@
>  }
>  
> +/* XXXdholbert This '@supports' wrapper should be removed once we remove the
> + * pref "layout.css.object-fit-and-position.enabled". */
> +@supports(object-fit: contain) {

Although it is not required, I think it'd look more natural to have a space after the "@supports".

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #4)
> Given my comment just now in bug 1067634, how are we going to handle dynamic
> changes to the layout.css.object-fit-and-position.enabled pref?

We don't re-parse UA stylesheets when there's a dynamic change to a CSS-property-controlling pref -- that's a known issue.  That's the one thing about CSS property-controlling prefs that requires a restart (depending on how much the rule matters).  (I filed bug 1068477 on investigating ways to address this, as you suggested on 1067634.)

This brings up a related point, though -- if we rip out the current hardcoded <video>-centering code, and instead rely on this rule being honored to get the default <video> behavior, then <video> will be broken in profiles that have this pref disabled! (because they won't see this rule)

That's bad, but given the relatively small size of this feature (and the probably-short-lifespan of the pref), I think that's not too concerning. It just means that:
 (1) As soon as we fix bug 624647 & start depending on this UA stylesheet rule for correct <video> rendering (i.e. as soon as we rip out the current hardcoded <video> centering code), we need to have the pref enabled-by-default.  (We can't have a trial period where it's opt-in.)
 (2) If for some reason we need to back out "object-fit"/"object-position" support, we'll have to add back the hardcoded video centering code.  (A pref tweak will not be sufficient.)
 (3) In the meantime, anyone who manually disables the pref after we've landed full object-fit will end up with not-centered <video> rendering. (I think that's OK, given that this isn't exposed in the UI anywhere and it'll be removed entirely before long.)

I don't find any of this too concerning, but it's good to be aware of.

(We also could hack around these concerns by adding custom code to nsVideoFrame to check whether the pref is enabled or not, and run the current hardcoded centering code if the pref is off; but I don't think that code complexity is worth it, given that this pref won't exist for long.)

> ::: layout/style/html.css
> Although it is not required, I think it'd look more natural to have a space
> after the "@supports".

Cool -- I'll fix that. Thanks!

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v3 (added space)

Added space, per review comment.

Comment 7

[Cameron McCormack (:heycam)]

Is the plan to enable the pref by default as soon as we have the rest of the object-fit/position implementation?  Or will there be some period where the pref will be off by default?  I'm just wondering how useful the pref will be, as a quick switch to disable the feature if we need to, if setting the pref to false after we've landed object-fit/position won't result in correct <video> rendering unless we also restore the custom sizing code.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #7)
> Is the plan to enable the pref by default as soon as we have the rest of the
> object-fit/position implementation?

Yes.

> Or will there be some period where the
> pref will be off by default?

(Per concern (1) in comment 5, that's not possible, at least not after we've made <video> rely on this UA stylesheet rule).


> I'm just wondering how useful the pref will
> be, as a quick switch to disable the feature if we need to

Per concern (2) in comment 5, it won't quite be sufficient to disable the feature -- but it's nearly so. (We'll just to restore some code in nsVideoFrame.)

This is way, way better than the non-pref-controlled alternative of "rip out all the code related to these properties (including the style-system code that we've added in bug 1055285)" as a means for disabling the feature.

Comment 9

[Cameron McCormack (:heycam)]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> This is way, way better than the non-pref-controlled alternative of "rip out
> all the code related to these properties (including the style-system code
> that we've added in bug 1055285)" as a means for disabling the feature.

OK, fair enough!

Comment 10

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d5258ba61a22

Comment 11

[Daniel Holbert [:dholbert]]

The cset in comment 10 was actually the pre-comment 6 version of the patch. (I forgot to 'hg pull -u' in my m-i clone's .hg/patches directory before landing -- oops.)

I landed a DONTBUILD followup to add the space character:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/a30de23be927

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d5258ba61a22
https://hg.mozilla.org/mozilla-central/rev/a30de23be927