Add width and height attributes to source elements #5894

yoavweiss · 2020-09-07T15:33:01Z

This addresses #4968
I believe this has multi-implementer interest, based on #4968 (comment), but will let others comment to that effect.

This doesn't yet have tests, as I wanted to first verify the approach I took makes sense. I'll make sure tests are added before landing. Similarly, I'll make sure implementation bugs are opened.

/cc @zcorpan @annevk @domenic @chrishtr @emilio @jensimmons @cbiesinger

At least two implementers are interested (and none opposed):
- Chromium
- Firefox: in this thread and mapping a <source>'s width/height attributes to width/height/aspect-ratio CSS properties on the <img> in a <picture> mozilla/standards-positions#485
Tests are written and can be reviewed and commented upon at:
- Split the big test() up into multiple small ones web-platform-tests/wpt#27745
Implementation bugs are filed:
- Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1137794
- Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1694741
- Safari: https://bugs.webkit.org/show_bug.cgi?id=222368

(See WHATWG Working Mode: Changes for more details.)

/embedded-content-other.html ( diff )
/embedded-content.html ( diff )
/images.html ( diff )
/indices.html ( diff )
/rendering.html ( diff )

zcorpan · 2020-09-07T16:24:12Z

Yay! 😄

I think the resource selection shouldn't "set an attribute value" on the img. Apart from invoking mutation observers, it also breaks markup that uses width and height on the img when the environment changes and it gets selected:

<picture>
 <source width=400 height=200 srcset="A" media="(min-width: 800px)">
 <img width=200 height=200 src="B" alt="">
</picture>

If A is first selected, the img will get its width changed to 400, and not changed back when the environment changes and B is selected.

Another detail to consider is when you want the dimension swap to happen. Should it happen when MQ changes (in the render step of the event loop), or when the new image is fully loaded? (For sizes, I believe it's MQ-time.)

yoavweiss · 2020-09-07T20:46:31Z

I think the resource selection shouldn't "set an attribute value" on the img. Apart from invoking mutation observers, it also breaks markup that uses width and height on the img when the environment changes and it gets selected:
<picture>
 <source width=400 height=200 srcset="A" media="(min-width: 800px)">
 <img width=200 height=200 src="B" alt="">
</picture>
If A is first selected, the img will get its width changed to 400, and not changed back when the environment changes and B is selected.

What would be a better way to set those values on the img element? Change "if img's width and height attribute values" to take other values hanging off the element and modify those? Something else?

Another detail to consider is when you want the dimension swap to happen. Should it happen when MQ changes (in the render step of the event loop), or when the new image is fully loaded? (For sizes, I believe it's MQ-time.)

We definitely want this at MQ time, as the main goal here is to avoid layout shifts, by declaring the aspect ratio ahead of time.

zcorpan · 2020-09-07T23:52:45Z

@yoavweiss well so how dynamic should it be? If you update the width attribute on a source element, should it affect rendering (without causing "update the image data" to rerun)? I'm tempted to say yes. Which makes me think the img needs an internal property that points to the element that has the width and height attributes to use (either the img element itself or an earlier source sibling). And then the places that currently look at an img element's width and height attributes should use this indirection to get the right dimensions.

Then the interesting bit are the width and height IDL attributes. Getting them give the rendered dimensions. Setting them update the width and height attributes on the img -- which might not affect the rendering, and thus what the getters return. Should they also use the indirection on setting? And add new IDL that properly reflect width/height on img? It seems a bit risky to change what these setters do though.

yoavweiss · 2020-09-08T05:28:45Z

The main use-case for which I think we care about this is the intrinsic aspect ratio calculations that I linked to above.
So maybe we can keep an internal property on img that will point at the right element containing the values required for that, but avoid messing with width and height getters and setters?

zcorpan · 2020-09-08T11:51:03Z

Sounds good to me

source

yoavweiss · 2020-09-08T16:09:44Z

@zcorpan - PTAL?

zcorpan

Apart from the aspect ratio, I think this should also update the Rendering section's mapping of the width and height attributes to the width and height properties (to use the "aspect ratio source"). Otherwise it will still use the img's width and height attributes to set the dimensions (without author CSS).

https://html.spec.whatwg.org/multipage/#attributes-for-embedded-content-and-images

source

mtrootyy · 2020-09-08T21:36:47Z

Is it okay if the source element is not added to the following?

https://html.spec.whatwg.org/multipage/embedded-content-other.html#dimension-attributes

Author requirements: The width and height attributes on img, iframe, embed, object, video, and, when their type attribute is in the Image Button state, input elements may be specified to give the dimensions of the visual content of the element (the width and height respectively, relative to the nominal direction of the output medium), in CSS pixels. The attributes, if specified, must have values that are valid non-negative integers.

zcorpan · 2020-09-08T21:59:15Z

@mtrootyy it should be in there, but with the condition: when the parent is a picture. Good catch!

source

yoavweiss · 2020-09-09T11:56:27Z

Comments addressed. PTAL?

zcorpan

Getting there

source

zcorpan · 2020-09-10T12:19:00Z

So I thought about a problem here.

What happens if you remove the aspect ratio source source element from the DOM?

It will be a "relevant mutation", which will invoke "update the image data". But that algorithm waits a microtask before invoking "selecting an image source". So before that happens, aspect ratio source points to a removed element. Maybe it's ok? If aspect ratio source is a strong reference, the source can't be garbage collected...

emilio · 2020-09-10T17:03:44Z

source

+  data-x="attr-input-type-image">Image Button</span> state and that either represents an image or
+  that the user expects will eventually represent an image, <span data-x="maps to the dimension
+  property">map to the dimension properties</span> <span>'width'</span> and <span>'height'</span>
+  on the element respectively.</p>


I'm not quite sure this makes sense (or at least I don't think we have a precedent for this). This is saying that for:

<picture> <source srcset="bar.png" width="100" height="100"> <img width="200" height="200"> </picture>

Something like getComputedStyle(img).width should be 100px, right? While I think this is the behavior you want, I'm not clear in how to accomplish it with the usual attribute mapping primitives.

Right. I think it's indeed new. But the mapping for 'aspect-ratio' would need this primitive as well, right?

For <source> yeah... with this patch, you also want to map to aspect-ratio somehow.

The "intrinsic aspect ratio" bit does that, no?

That is not technically defined in terms of attribute mapping (and Chrome doesn't implement it that way, though Gecko does), that's why I didn't bring it up.

The reason for that is that when we initially added that section to the spec, we didn't have the CSS value we needed to map into. That value was added later, and we should update the spec to define it maps to aspect-ratio: <width> / <height> auto instead of changing the definition of "intrinsic aspect ratio".

Aha. Right. That does seem like a separate issue though. Can you file an issue?

yoavweiss · 2020-09-29T09:26:57Z

So I thought about a problem here.

What happens if you remove the aspect ratio source source element from the DOM?

It will be a "relevant mutation", which will invoke "update the image data". But that algorithm waits a microtask before invoking "selecting an image source". So before that happens, aspect ratio source points to a removed element. Maybe it's ok? If aspect ratio source is a strong reference, the source can't be garbage collected...

That seems fine to me, but if that's not desired, maybe we could add a check to see that ~~aspect ratio~~dimensions attribute source is connected?

zcorpan · 2020-09-29T10:11:28Z

Yeah I think it's fine. I just figured it's something that could crash if the UA doesn't hold a strong reference, and you force a reflow or something while aspect ratio source points to the removed element.

cbiesinger · 2021-01-13T15:28:53Z

So... how will this interact with the mapping of width/height to CSS that we now do?

yoavweiss · 2021-01-15T14:48:21Z

@cbiesinger - as far as I can tell, https://github.com/whatwg/html/pull/6032/files and this PR can peacefully co-exist :) Doesn't seem to me like they are conflicting in any way.

cbiesinger · 2021-01-21T14:22:22Z

source

+    <p>The <code data-x="attr-dim-width">width</code> and <code
+    data-x="attr-dim-height">height</code> attributes may also be present.  If present, their
+    values must contain valid lengths. The user agent will use these values to calculate the
+    <span>intrinsic aspect ratio</span> as well as the <span>intrinsic dimensions</span> of a


Just a sidenote, setting this as the intrinsic ratio means this won't be used when contain:size is used (by definition, size containment ignores intrinsic size of images)

cbiesinger · 2021-01-21T14:46:38Z

source

+    <p>The <code data-x="attr-dim-width">width</code> and <code
+    data-x="attr-dim-height">height</code> attributes may also be present.  If present, their
+    values must contain valid lengths. The user agent will use these values to calculate the
+    <span>intrinsic aspect ratio</span> as well as the <span>intrinsic dimensions</span> of a


Also, I believe this text as written means that these attributes will override the actual dimensions of the image (once loaded); this is different from the behavior for img's attributes. Is that intentional?

Since this sentence uses the word "will" and not "must", it's a non-normative statement of fact and has no effect. The part about intrinsic dimensions (now named natural dimensions) I think is incorrect; the width and height attributes map to the 'width' and 'height' properties, which are the specified size, not natural dimensions.

Thanks! Corrected

So this refers to DOM properties? But an img's width/height properties also map to the attribute as well as the CSS width/height property... I don't even know how to implement this in a reasonable way...

@mfreed7 in case he has input on that

No, not the DOM properties. I think this sentence might be confusing matters, as it's not trying to require anything, but instead helpfully describing what purpose or effect the width and height attributes have. As an implementer, you can ignore non-normative statements completely. The normative part is in the Rendering section: https://whatpr.org/html/5894/16667c3...d10ef5c/rendering.html#attributes-for-embedded-content-and-images:attr-dim-width

FWIW, I also clarified the sentence, based on @zcorpan's feedback.

Oh, thanks for clarifying that.

cbiesinger · 2021-02-23T19:28:08Z

I've put tests for this PR into web-platform-tests/wpt#27745, please review!

This is for whatwg/html#5894

domenic

This LGTM. I'll give folks another day or two before we merge?

source

This is for whatwg/html#5894

cbiesinger · 2021-02-24T19:08:06Z

To complete the checklist here:

Implementation bugs:
https://bugs.chromium.org/p/chromium/issues/detail?id=1137794
https://bugzilla.mozilla.org/show_bug.cgi?id=1694741
https://bugs.webkit.org/show_bug.cgi?id=222368

Tests:
At web-platform-tests/wpt#27745 but will land as part of https://chromium-review.googlesource.com/c/chromium/src/+/2642939 (ie. web-platform-tests/wpt#27460)

yoavweiss · 2021-02-24T19:13:00Z

@cbiesinger - Thanks! Updated the checklist up top

[It's best practice to specify width and height on images][1], even if they are fluid, to help avoid layout shift. It tells the browser the expected size before it loads any image data, and an aspect ratio is derived from it so the browser can still know the image size even if CSS or something else is altering the image's width from its intrinsic one. Before this patch, there was not a way to allow width and height attributes to get through to the `img` element sent to the browser when using the react-imgix component while leaving the image fluid width (that is, with a srcset containing width descriptors). As soon as one specified width and height, react-imgix assumed the author wanted a fixed-width image, and it gave resolution descriptors in the srcset instead of width descriptors (and then the sizes attribute was ignored by the browser, [as per spec][2]). Imgix currently uses the width and height attributes to decide whether to use width or resolution descriptors, so to avoid altering that logic (which would be a breaking change), this patch allows width and height to be passed through the `htmlAttributes` prop. Prior to the patch they were swallowed if given in `htmlAttributes`. As of this patch they are allowed and passed through. This closes imgix#891 In future it may make sense to decide srcset type based on the presence of the `sizes` attribute, as discussed in the above ticket. This would be a breaking change, however. As part of this patch, width and height are also allowed through when using this component in `Source` mode, since [width and height attributes are now allowed and encouraged in that context][3]. [1]: https://web.dev/optimize-cls/ [2]: https://html.spec.whatwg.org/multipage/images.html#srcset-attributes [3]: whatwg/html#5894

[It's best practice to specify width and height on images][1], even if they are fluid, to help avoid layout shift. It tells the browser the expected size before it loads any image data, and an aspect ratio is derived from it so the browser can still know the image size even if CSS or something else is altering the image's width from its intrinsic one. Before this patch, there was not a way to allow width and height attributes to get through to the `img` element sent to the browser when using the react-imgix component while leaving the image fluid width (that is, with a srcset containing width descriptors). As soon as one specified width and height, react-imgix assumed the author wanted a fixed-width image, and it gave resolution descriptors in the srcset instead of width descriptors (and then the sizes attribute was ignored by the browser, [as per spec][2]). Imgix currently uses the width and height attributes to decide whether to use width or resolution descriptors, so to avoid altering that logic (which would be a breaking change), this patch allows width and height to be passed through the `htmlAttributes` prop. Prior to the patch they were swallowed if given in `htmlAttributes`. As of this patch they are allowed and passed through. This closes #891 In future it may make sense to decide srcset type based on the presence of the `sizes` attribute, as discussed in the above ticket. This would be a breaking change, however. As part of this patch, width and height are also allowed through when using this component in `Source` mode, since [width and height attributes are now allowed and encouraged in that context][3]. [1]: https://web.dev/optimize-cls/ [2]: https://html.spec.whatwg.org/multipage/images.html#srcset-attributes [3]: whatwg/html#5894

yoavweiss commented Sep 8, 2020

View reviewed changes

source Outdated Show resolved Hide resolved

yoavweiss commented Sep 8, 2020

View reviewed changes

source Outdated Show resolved Hide resolved

zcorpan reviewed Sep 8, 2020

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

yoavweiss commented Sep 9, 2020

View reviewed changes

source Outdated Show resolved Hide resolved

zcorpan reviewed Sep 9, 2020

View reviewed changes

source Outdated Show resolved Hide resolved

zcorpan reviewed Sep 10, 2020

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

emilio reviewed Sep 10, 2020

View reviewed changes

emilio mentioned this pull request Sep 11, 2020

Define image intrinsic aspect ratio by mapping to the aspect-ratio property. #5907

Closed

polarathene mentioned this pull request Sep 17, 2020

feat(gatsby-image): Art Direction SSR fix (compliments hydration fix PR) gatsbyjs/gatsby#26117

Closed

zcorpan added the needs tests Moving the issue forward requires someone to write tests label Sep 29, 2020

yoavweiss force-pushed the picture_width_height_based_aspect_ratio branch from 0a84780 to cd685f5 Compare September 29, 2020 09:23

zcorpan mentioned this pull request Oct 13, 2020

Remove specification of intrinsic aspect-ratio, and instead map width… #6032

Merged

3 tasks

dvoytenko mentioned this pull request Oct 27, 2020

Intent-to-Deprecate: amp-img ampproject/amphtml#30442

Closed

Base automatically changed from master to main January 15, 2021 07:58

cbiesinger reviewed Jan 21, 2021

View reviewed changes

cbiesinger mentioned this pull request Feb 23, 2021

Split the big test() up into multiple small ones web-platform-tests/wpt#27745

Merged

cbiesinger added a commit to cbiesinger/web-platform-tests that referenced this pull request Feb 23, 2021

Add test for <source> width/height

a944d88

This is for whatwg/html#5894

cbiesinger added a commit to cbiesinger/web-platform-tests that referenced this pull request Feb 23, 2021

Add test for <source> width/height

33ebbc4

This is for whatwg/html#5894

yoavweiss added 2 commits February 24, 2021 08:11

Revert dom-dim-* explicit reflection prose

7fbd75f

Style the revert

e9112e4

domenic approved these changes Feb 24, 2021

View reviewed changes

domenic reviewed Feb 24, 2021

View reviewed changes

source Show resolved Hide resolved

Clarify what is "the element"

79535c6

cbiesinger added a commit to cbiesinger/web-platform-tests that referenced this pull request Feb 24, 2021

Add test for <source> width/height

6e43734

This is for whatwg/html#5894

domenic merged commit c2d1a36 into whatwg:main Feb 25, 2021

domenic added addition/proposal New features or enhancements topic: img labels Feb 25, 2021

This was referenced Mar 4, 2021

Utilizing Aspect-Ratio with Art Direction (Multiple Sources with various Aspect Ratios known) #4968

Closed

Track support for newly added spec: Dimensions (width/height) in <source> tags Fyrd/caniuse#5791

Closed

johannesodland mentioned this pull request Apr 28, 2021

Add alt attributes to source elements #6627

Open

tomohiro-iwana mentioned this pull request Jul 30, 2021

Add width and height attributes to source elements @types/react @types/babel-plugin-react-html-attrs DefinitelyTyped/DefinitelyTyped#54837

Merged

8 tasks

BorisChiou mentioned this pull request Aug 10, 2022

The missing property values of width/height attributes on source elements #8178

Open

luca-rath mentioned this pull request Nov 21, 2022

Add access to calculated size of image formats sulu/sulu#6911

Open

tremby mentioned this pull request Dec 24, 2022

Allow width and height to be passed through htmlAttributes imgix/react-imgix#892

Merged

9 tasks

Add width and height attributes to source elements #5894

Add width and height attributes to source elements #5894

Conversation

yoavweiss commented Sep 7, 2020 • edited by pr-preview bot Loading

zcorpan commented Sep 7, 2020

yoavweiss commented Sep 7, 2020

zcorpan commented Sep 7, 2020

yoavweiss commented Sep 8, 2020

zcorpan commented Sep 8, 2020

yoavweiss commented Sep 8, 2020

zcorpan left a comment

Choose a reason for hiding this comment

mtrootyy commented Sep 8, 2020

zcorpan commented Sep 8, 2020

yoavweiss commented Sep 9, 2020

zcorpan left a comment

Choose a reason for hiding this comment

zcorpan commented Sep 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoavweiss commented Sep 29, 2020

zcorpan commented Sep 29, 2020

cbiesinger commented Jan 13, 2021

yoavweiss commented Jan 15, 2021

Choose a reason for hiding this comment

cbiesinger Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

zcorpan Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbiesinger commented Feb 23, 2021

domenic left a comment

Choose a reason for hiding this comment

cbiesinger commented Feb 24, 2021

yoavweiss commented Feb 24, 2021

yoavweiss commented Sep 7, 2020 •

edited by pr-preview bot

Loading

cbiesinger Jan 21, 2021 •

edited

Loading

zcorpan Jan 25, 2021 •

edited

Loading