Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-sizing-4] aspect-ratio via width and height attributes doesn't work for <video> #7524

Open
jakearchibald opened this issue Jul 22, 2022 · 10 comments

Comments

@jakearchibald
Copy link
Contributor

jakearchibald commented Jul 22, 2022

whatwg/html#6529 attempted to solve whatwg/html#4961 (auto aspect-ratio) for <img> and <video>. It works for <img>, but it doesn't work for <video>.

The problem is, <video width="16" height="9" …> creates an internal style rule like aspect-ratio: auto 16 / 9.

The auto part means the provided ratio (16 / 9) is ignored once the element has a natural aspect ratio.

Images start off without a natural aspect ratio, and they gain one once the resource is loaded enough to know its dimensions. The benefit is, if the provided width and height are wrong, that information is discarded in favour of the accurate values from the image resource.

However, <video> has a defined default object size of 300x150, meaning the aspect ratio derived from the width and height attributes is instantly ignored in favour of 300 / 150. So, although browsers are generating the aspect-ratio rule for <video>, it's useless.

We can't just remove the default object size from <video>, otherwise it'd be 0x0 by default, which would break cases where the element didn't have a width and height set, and the browser doesn't attempt to fetch the video until some kind of interaction. Because the element would be 0x0, that interaction would be impossible.

So, it feels like the definition of auto in aspect-ratio needs to altered in some way so the provided ratio has higher priority than the default object size.

@jakearchibald
Copy link
Contributor Author

cc @jensimmons

@tabatkins
Copy link
Member

Oh, huh, this seems like an obvious oversight. Definitely needs fixing, and should be fixable with a bit of tweaking.

@jakearchibald
Copy link
Contributor Author

jakearchibald commented Jul 28, 2022

I guess images/objects could have a "first resource size pending" boolean, which would be false by default. However, images and videos would immediately set it to true, and only set it to false once width/height is gained from the resource.

With aspect-ratio: auto 16 / 9, the 16 / 9 part would be used while the "first resource size pending" is true.

If an image/video's source is changed, "first resource size pending" is not reset to true.

@tabatkins
Copy link
Member

Okay, so this actually would require an edit to Images 3, since that's where the dimensions are defined. Agenda+ to add the concept of "pending resource-based size", so Sizing can hook that for aspect-ratio: auto.

@jakearchibald
Copy link
Contributor Author

Dunno if it's worth calling it "pending first resource-based size". When changing the src of the image/video, it should keep using the previous resource-based size until it has updated data. I believe that's what currently happens with <img>.

@astearns astearns moved this to Unsorted in 2022 New York Meeting Aug 3, 2022
@astearns astearns moved this from Unsorted to Wednesday in 2022 New York Meeting Aug 3, 2022
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed aspect-ratio via width/height doesn't work for <video>, and agreed to the following:

  • RESOLVED: Clarify that aspect-ratio:auto doesn't pull aspect ratio from the default object size, it can only pull from an actual loaded resource
The full IRC log of that discussion <fantasai> Topic: aspect-ratio via width/height doesn't work for <video>
<fantasai> github: https://github.com//issues/7524
<fantasai> scribe+ bramus
<bramus> TabAtkins: so, the aaspect ratio property has the auto keyword and it will take the ar from the resource otherwise use the other one as the fallback
<bramus> TabAtkins: this works great, expect for video
<bramus> TabAtkins: bc video does not communicate the ar
<bramus> TabAtkins: instead the size is 300 by 150
<bramus> TabAtkins: and the algo will say cool i have an ar
<bramus> TabAtkins: needs a small fix, suggested by jaffathecake
<emilio> q+
<bramus> TabAtkins:
<bramus> TabAtkins: new concept of pending your first resoruce size instead of "do you have an ar" ask "have you ever received an ar from you rcontent?"
<bramus> TabAtkins: if not use the fallback from the ar property
<bramus> TabAtkins: this would fix the problem so taht img and videos ar enot considered to have an r until they load their resource
<bramus> TabAtkins: it is flipped when they obtain the ar
<bramus> TabAtkins: other reason for a f2f bc the relevant algos are in images-3
<bramus> s/an r/an ar/
<bramus> fantasai:
<bramus> fantasai: why do we ignore wdth and height in favor of the 300 by 150?
<bramus> fantasai: if we have better info shoulndt we us that instead?
<bramus> TabAtkins: correct
<bramus> emilio: to clarify, thi sonly changes behavior before the video loads
<bramus> TabAtkins: correct
<iank_> there is also some special behaviour for video image placeholders i think.
<bramus> emilio: i htink images are a bit special ?? when there is mapped ar
<emilio> https://searchfox.org/mozilla-central/rev/417eb46de41c31c3223d7b84c275ba56fc7d4343/layout/generic/nsImageFrame.cpp#659-661
<bramus> emilio: so why dont we do the same for video?
<bramus> emilio: here is the gecko code for image intrinsic sizing
<bramus> emilio: fallback has a lot of stuff
<bramus> emilio: but basically intrinsic size is 0 if broken
<bramus> emilio: ???
<bramus> emilio: why cant we do same for video
<iank_> q+
<Rossen_> ack emilio
<bramus> emilio: that function does a lot of stuff … once you get to th ebottom the intrinsic size of a broken img is 0x0
<bramus> emilio: instead of that, have it check if you have an ar then return no intrinsic size and use the ar instead
<bramus> emilio: i dont see how that is gonna work for video
<fantasai> s/is/isn't/
<bramus> emilio: i think we need just the same check also for video
<bramus> iank_: i wnat to point out that videos are more complicated
<bramus> iank_: they define a ??? and we get the ar from there
<bramus> emilio: we also do that
<Rossen_> s/???/poster image/
<bramus> emilio: check the actual video, check the poster, and then the fallback
<Rossen_> ack iank_
<bramus> TabAtkins: in the html spec, images do not have a fallback size
<bramus> emilio: yes, 0x0
<bramus> TabAtkins: so we do not want that for video
<bramus> fantasai: ??? if you have an ar use that and not the default object size
<bramus> TabAtkins: where would this change be? html?
<bramus> emilio: possibly
<bramus> emilio: if the fallback is defined in html
<bramus> fantasai: it is fine to say the fallback size is what it is
<bramus> fantasai: we should make it clear if the ar is set, you do not need that fallback size
<bramus> TabAtkins: yes, you are saying what i suggested
<florian> q+
<bramus> Rossen: everyone is agreein, which is great
<bramus> Rossen_: other opinions?
<bramus> florian: given that videos have controls, how realistic is that w ecan nkow the ar wihtout the size?
<bramus> TabAtkins: controls are overlaid
<bramus> Rossen_: tab, can you verify?
<bramus> TabAtkins: if a video or any replaced element has not yet loaded, then we do not consider it to have an ar to come from its fallback size
<iank_> q+
<emilio> q+
<florian> q-
<bramus> fantasai: the default object size should not ???
<TabAtkins> s/???/provide an aspect ratio/
<bramus> Rossen_: no, this is not the case.
<fantasai> s/not ???/never provide an aspect ratio, it's just a width+height/
<bramus> Rossen_: if we have a defined ar, we do not have a prob?
<bramus> Rossen_: we need to figure out in case of auto to fallback to 350
<bramus> TabAtkins: you should not be doing that, that is the point
<Rossen_> q?
<bramus> TabAtkins: if your default sizes are used, do not consider that to provide an aspect-ratio but us the auto
<bramus> Rossen_: auto by default?
<bramus> fantasai: 16/9 set … use that inti loaded
<bramus> fantasai: what tab is trying to say that auto is not giving the aspect ratio, until the resource is loaded
<bramus> TabAtkins: it is what we intended for images, but a clear error we need to fix (in images-3)
<bramus> fantasai: images-3 does not define that
<bramus> TabAtkins: it does ?? with size
<bramus> TabAtkins: the default object size is defined in 2.1
<fantasai> fantasai: sizing rules for replaced elements is not in css-images-3
<fantasai> fantasai: css-images-3 defines sizing algorithms, but not for replaced elements
<Rossen_> q?
<fantasai> fantasai: it covers things like list-style-image and background-image
<Rossen_> ack iank_
<bramus> iank_: we may only want to do this for video and ?? elements because ther eis some funky stuff with object elements
<bramus> Rossen_: object and embed have similar proble which you do not want to change
<bramus> fantasai: we are not gonna run into probs here
<bramus> iank_: any other elems?
<bramus> TabAtkins: not sure
<bramus> iank_: canvas?
<bramus> TabAtkins: maybe
<bramus> emilio: input type image, video, image, canvas
<bramus> florian: svg?
<bramus> emilio: no
<bramus> iank_: it might take us a while to do bc the way our intrinsic logic handles it historically
<bramus> Rossen_: ok, ty
<bramus> Rossen_: but shouldt prevent us from resolving
<Rossen_> ack emilio
<bramus> emilio: i dont know if saying getting the ar from the fallback size or not, or if you hav ean ar make sure you do not have a falback intrinsic size to being with
<bramus> iank_: you mean the natural size?
<bramus> emilio: yeah
<bramus> emilio: i need to check how it works
<bramus> emilio: but we are in agreement
<bramus> emilio: i think you may not want an intrinsic size to begin with
<bramus> iank_: (thinks)
<TabAtkins> q+ to finish this up without requiring us to nail down all details right there
<bramus> iank_: thi swill lose the natural fallback sizing as well?
<bramus> emilio: i htink it should
<bramus> emilio: i need to check where tha tmakes a diff
<bramus> iank_: that will make the diff when you do sth like width fit-content
<bramus> iank_: in certain circusstances
<bramus> emilio: maybe yes
<bramus> TabAtkins: can i interrupt
<bramus> TabAtkins: suggestion is to resolve to fix it, without detailing it
<bramus> fantasai: mayb e more precise
<bramus> fantasai: that the auto part does not provide an aspect ratio
<bramus> fantasai: that you do no get it from the fallback size
<bramus> TabAtkins: sure
<bramus> fantasai: the spec was never intended to pull the ar from the fallback size
<bramus> fantasai: we should clarify that it doesnt
<bramus> fantasai: it always fall back to the provide aspect ratio
<bramus> emilio: for iframes?
<bramus> fantasai: no
<bramus> fantasai: they dont get one automagically
<bramus> fantasai: same with svg, until you load it
<bramus> Rossen_: back to resolution
<bramus> fantasai: 2 resolutions
<fantasai> Proposed: aspect-ratio:auto doesn't pull aspect ratio from the default object size, it can only pull from an actual loaded resource
<fantasai> Proposed: Clarify that aspect-ratio:auto doesn't pull aspect ratio from the default object size, it can only pull from an actual loaded resource
<bramus> fantasai: when providing the ar through the property then yo u no longer have a naturla size tha tis a default size
<bramus> Rossen_: so ar overrides default
<bramus> fantasai: yeah
<bramus> TabAtkins: would like to address in separate issue if needed
<bramus> Rossen_: first one is easy to resolve one
<bramus> s/one/on/
<bramus> emilio: poster image?
<bramus> fantasai: that is a loaded resource
<bramus> emilio: but not a video
<bramus> florian: proposed resol says a resource
<bramus> fantasai: if video is 16 by 9 and poster 1x1, then loaded poster will give 1 by 1 aspect ratio
<bramus> emilio: ok
<bramus> we agree on behavior
<bramus> emilio: we agree on behavior
<bramus> Rossen_: anyone objecting?
<fantasai> RESOLVED: Clarify that aspect-ratio:auto doesn't pull aspect ratio from the default object size, it can only pull from an actual loaded resource
<bramus> Rossen_: no, ok
<bramus> fantasai: second one when providing ar via aspect-ratio property then dont fallback to default object size
<bramus> iank_: unsure about that
<bramus> fantasai: if you want to hang on to the ar, you have to at least hang on to one of the sizes
<fantasai> s/hang on to one/let go of one/
<bramus> iank_: (missed)
<bramus> iank_: i think the behavior today you will still size at width 300 and take into account the new ar, so you wont end up with 150 height
<bramus> iank_: and i think that might be fine
<bramus> emilio: not sure
<bramus> emilio: images may want to overrride that
<bramus> emilio: bc their intrinsic size is 0x0
<bramus> emilio: but video maybe we do not need to change intrinsic size
<bramus> emilio: consistency would b egood though
<bramus> Rossen_: so?
<bramus> Rossen_: we have resolution for original post
<bramus> Rossen_: suggest to open new issue for the 2nd thing
<bramus> Rossen_: we can record info there and give iank_ and emilio time to discuss
<bramus> Rossen_: we can come back to this later
<bramus> Rossen_: everyone ok with that?
<bramus> fantasai: ok

@fantasai
Copy link
Collaborator

fantasai commented Aug 3, 2022

The default object size is not a natural size, and doesn't have an aspect ratio. It shouldn't be providing an aspect ratio to the aspect-ratio property in the first place.

@fantasai
Copy link
Collaborator

fantasai commented Aug 3, 2022

I think it's also worth pointing out that the default object sizing algorithm defined in https://www.w3.org/TR/css-images-3/#default-sizing does not apply to replaced elements.

@emilio
Copy link
Collaborator

emilio commented Aug 3, 2022

Firefox patch: https://bugzilla.mozilla.org/show_bug.cgi?id=1783069

moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 3, 2022
See w3c/csswg-drafts#7524 for discussion. This
matches what we do for images.

Differential Revision: https://phabricator.services.mozilla.com/D153664

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1783069
gecko-commit: 4a697ec6ca7ca5e5c69883a102797757e4752831
gecko-reviewers: boris, layout-reviewers
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 4, 2022
…out-reviewers

See w3c/csswg-drafts#7524 for discussion. This
matches what we do for images.

Differential Revision: https://phabricator.services.mozilla.com/D153664
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 16, 2022
See w3c/csswg-drafts#7524 for discussion. This
matches what we do for images.

Differential Revision: https://phabricator.services.mozilla.com/D153664

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1783069
gecko-commit: e1262ce83376f5e1109db8e91a115904ad41317f
gecko-reviewers: boris, Oriol, layout-reviewers
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 16, 2022
…ol,layout-reviewers

See w3c/csswg-drafts#7524 for discussion. This
matches what we do for images.

Differential Revision: https://phabricator.services.mozilla.com/D153664
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 17, 2022
See w3c/csswg-drafts#7524 for discussion. This
matches what we do for images.

Differential Revision: https://phabricator.services.mozilla.com/D153664

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1783069
gecko-commit: e1262ce83376f5e1109db8e91a115904ad41317f
gecko-reviewers: boris, Oriol, layout-reviewers
@Mushr0000m
Copy link

Mushr0000m commented Oct 26, 2022

I don't know if it's the right place to put this but I ended up on this thread because I had this exact issue. After some testing I found a workaround until it's fixed on the browser level. The idea is to put a transparent svg image with the same width and height as poster and then the initial ratio is good... here is an example for a 1000x600 video:

<video width="1000" height="600" poster="data:image/svg+xml;utf8,<svg width='1000' height='600' xmlns='http://www.w3.org/2000/svg'></svg>">
    <source src="video.mp4" type="video/mp4">
</video>

Just in case it can help others.

Loirooriol added a commit to eerii/servo that referenced this issue Oct 29, 2024
…ect size

As resolved in w3c/csswg-drafts#7524 (comment)

Signed-off-by: Oriol Brufau <obrufau@igalia.com>
Loirooriol added a commit to eerii/servo that referenced this issue Oct 29, 2024
…ect size

As resolved in w3c/csswg-drafts#7524 (comment)

Signed-off-by: Oriol Brufau <obrufau@igalia.com>
github-merge-queue bot pushed a commit to servo/servo that referenced this issue Oct 29, 2024
* feat: patch for video layout sizes

added rebase from main 2024/10/05

Co-authored-by: Josh Matthews <josh@joshmatthews.net>
Signed-off-by: eri <epazos@igalia.com>

* feat: take width and height parameters if provided

Signed-off-by: eri <epazos@igalia.com>

* chore: tidy the code and update test expectations

Signed-off-by: eri <epazos@igalia.com>

* feat: handle removing poster

Signed-off-by: eri <epazos@igalia.com>

* chore: update test expectations and remove debug code

Signed-off-by: eri <epazos@igalia.com>

* fix: issues after rebasing to main

Signed-off-by: eri <epazos@igalia.com>

* feat: pass src remove test and tidy

Signed-off-by: eri <epazos@igalia.com>

* chore: clippy fixes

Signed-off-by: eri <epazos@igalia.com>

* chore: update passing test expectations

Signed-off-by: eri <epazos@igalia.com>

* fix object-position-svg test

Signed-off-by: eri <epazos@igalia.com>

* fix unintentional override of video size and resize events

Signed-off-by: eri <epazos@igalia.com>

* change how resize events are sent to better match the spec

Signed-off-by: eri <epazos@igalia.com>

* simplify poster mutation handling

Co-authored-by: Oriol Brufau <obrufau@igalia.com>
Signed-off-by: eri <eri@inventati.org>

* improved handling of intrinsic sizes

- differentiate between natural size and css size
- presentational attributes
- fallback ratio for video element
- handle more cases where the src/poster are added/removed
- aspect ratio hints

Signed-off-by: eri <epazos@igalia.com>

* update test expectations

Signed-off-by: eri <epazos@igalia.com>

* fix cleaning current frame

Signed-off-by: eri <epazos@igalia.com>

* update test expectations

Signed-off-by: eri <epazos@igalia.com>

* Apply suggestions from code review

Co-authored-by: Oriol Brufau <obrufau@igalia.com>
Signed-off-by: eri <eri@inventati.org>

* More code review suggestions

Signed-off-by: eri <epazos@igalia.com>

* Prevent aspect-ratio:auto from pulling the ratio from the default object size

As resolved in w3c/csswg-drafts#7524 (comment)

Signed-off-by: Oriol Brufau <obrufau@igalia.com>

---------

Signed-off-by: eri <epazos@igalia.com>
Signed-off-by: eri <eri@inventati.org>
Signed-off-by: Oriol Brufau <obrufau@igalia.com>
Co-authored-by: Josh Matthews <josh@joshmatthews.net>
Co-authored-by: Oriol Brufau <obrufau@igalia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Wednesday
Development

No branches or pull requests

7 participants