Bug 217529 – Define image intrinsic aspect ratio by mapping to the `aspect-ratio` property.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Tab Atkins Jr.]

Per <https://github.com/whatwg/html/pull/6032> (and linked related issues), <img width height> should define an aspect ratio for the element by creating a presentational hint for the 'aspect-ratio' property, rather than by changing the image's intrinsic aspect ratio.

[Radar WebKit Bug Importer]

<rdar://problem/70161811>

[cathiechen]

Created attachment 429128 [details]
Patch

[cathiechen]

Comment on attachment 429128 [details]
Patch

Hi,
I think this patch is ready for review now. Thanks:)

[Rob Buis]

(In reply to cathiechen from comment #3)
> Comment on attachment 429128 [details]
> Patch
> 
> Hi,
> I think this patch is ready for review now. Thanks:)

Thanks, Cathie, the patch LGTM. IIUC this is needed for some WPT tests and it is nice to move some HTML parsing code out of rendering/ IMHO. I prefer somebody from Apple to do the review though :)

[cathiechen]

(In reply to Rob Buis from comment #4)
> (In reply to cathiechen from comment #3)
> > Comment on attachment 429128 [details]
> > Patch
> > 
> > Hi,
> > I think this patch is ready for review now. Thanks:)
> 
> Thanks, Cathie, the patch LGTM. IIUC this is needed for some WPT tests and
> it is nice to move some HTML parsing code out of rendering/ IMHO. I prefer
> somebody from Apple to do the review though :)

Thanks, Rob!

Yeah, this is needed by the WPT tests for mapping attributes w h to aspect ratio, like, img-aspect-ratio.html, video-aspect-ratio.html, picture-aspect-ratio.html and so on.

[Antti Koivisto]

Comment on attachment 429128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429128&action=review

> Source/WebCore/style/StyleAdjuster.cpp:509
> +    if (m_element && m_document.settings().aspectRatioOfImgFromWidthAndHeightEnabled() && style.aspectRatioType() == AspectRatioType::Auto) {
> +        double attributeWidth = -1;
> +        double attributeHeight = -1;
> +        if (m_element->useWidthHeightAsAspectRatioHint(attributeWidth, attributeHeight)) {
> +            style.setAspectRatioType(AspectRatioType::AutoAndRatio);
> +            style.setAspectRatio(attributeWidth, attributeHeight);
> +        }
> +    }
> +

Won't this change the getComputedStyle value of the aspect-ratio property too? Is that what the spec wants?

[cathiechen]

(In reply to Antti Koivisto from comment #6)
> Comment on attachment 429128 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429128&action=review
> 
> > Source/WebCore/style/StyleAdjuster.cpp:509
> > +    if (m_element && m_document.settings().aspectRatioOfImgFromWidthAndHeightEnabled() && style.aspectRatioType() == AspectRatioType::Auto) {
> > +        double attributeWidth = -1;
> > +        double attributeHeight = -1;
> > +        if (m_element->useWidthHeightAsAspectRatioHint(attributeWidth, attributeHeight)) {
> > +            style.setAspectRatioType(AspectRatioType::AutoAndRatio);
> > +            style.setAspectRatio(attributeWidth, attributeHeight);
> > +        }
> > +    }
> > +
> 
> Won't this change the getComputedStyle value of the aspect-ratio property
> too? Is that what the spec wants?


Yes, that's want the spec wants. https://html.spec.whatwg.org/#map-to-the-aspect-ratio-property-(using-dimension-rules)
And `test_computed_style()` in the WPT test img-aspect-ratio.html is testing this behavior.

[Simon Fraser (smfr)]

Comment on attachment 429128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429128&action=review

> Source/WebCore/ChangeLog:10
> +        This patch uses CSS property aspect-ratio to implement mapping width / height to intrinsic aspect ratio.
> +        Style adjust changes the value CSS property aspect-ratio by getting the values of attributes width and height
> +        if allowed from Element::useWidthHeightAsAspectRatioHint.

Could you link to the spec here please?

[cathiechen]

(In reply to Simon Fraser (smfr) from comment #8)
> Comment on attachment 429128 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429128&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        This patch uses CSS property aspect-ratio to implement mapping width / height to intrinsic aspect ratio.
> > +        Style adjust changes the value CSS property aspect-ratio by getting the values of attributes width and height
> > +        if allowed from Element::useWidthHeightAsAspectRatioHint.
> 
> Could you link to the spec here please?

Sure, will do!

[cathiechen]

Created attachment 429180 [details]
Patch

[cathiechen]

Comment on attachment 429180 [details]
Patch

Added the spec link to the ChangeLog.

[Simon Fraser (smfr)]

Comment on attachment 429180 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429180&action=review

> Source/WebCore/ChangeLog:12
> +        [1] https://html.spec.whatwg.org/#map-to-the-aspect-ratio-property-(using-dimension-rules)

The spec says "The width and height attributes map to the aspect-ratio property (using dimension rules) on img and video elements, and input elements with a type attribute in the Image Button state." so do we need code for <video> and image buttons too?

[cathiechen]

(In reply to Simon Fraser (smfr) from comment #12)
> Comment on attachment 429180 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429180&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        [1] https://html.spec.whatwg.org/#map-to-the-aspect-ratio-property-(using-dimension-rules)
> 
> The spec says "The width and height attributes map to the aspect-ratio
> property (using dimension rules) on img and video elements, and input
> elements with a type attribute in the Image Button state." so do we need
> code for <video> and image buttons too?

Yeah, we need to override `useWidthHeightAsAspectRatioHint` for HTMLVideoElement and HTMLInputElement.

The plan is to work on a new bug for video and input, based on this patch.

[Antti Koivisto]

Comment on attachment 429180 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429180&action=review

> Source/WebCore/style/StyleAdjuster.cpp:508
> +    if (m_element && m_document.settings().aspectRatioOfImgFromWidthAndHeightEnabled() && style.aspectRatioType() == AspectRatioType::Auto) {
> +        double attributeWidth = -1;
> +        double attributeHeight = -1;
> +        if (m_element->useWidthHeightAsAspectRatioHint(attributeWidth, attributeHeight)) {
> +            style.setAspectRatioType(AspectRatioType::AutoAndRatio);
> +            style.setAspectRatio(attributeWidth, attributeHeight);
> +        }
> +    }

"When the text below says that a pair of attributes w and h on an element element map to the aspect-ratio property (using dimension rules), it means that if element has both attributes w and h, and parsing those attributes' values using the rules for parsing dimension values doesn't generate an error or return a percentage for either, then the user agent is expected to use the parsed dimensions as a presentational hint for the 'aspect-ratio' property of the form auto w / h."

So the concept used in the spec here is "presentational hint", https://html.spec.whatwg.org/#the-css-user-agent-style-sheet-and-presentational-hints

It is the same concept as used for stuff like <body bgcolor="blue">

In WebKit this is implemented as Element::presentationAttributeStyle(). To generate it correctly see for example HTMLBodyElement::collectStyleForPresentationAttribute.

You shouldn't need anything in style adjuster for this feature.

[Antti Koivisto]

...and HTMLBodyElement::isPresentationAttribute(

[Antti Koivisto]

In fact width/height are already presentation attributes for img/video, just not for aspect-ratio.

[cathiechen]

(In reply to Antti Koivisto from comment #14)
> Comment on attachment 429180 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429180&action=review
> 
> > Source/WebCore/style/StyleAdjuster.cpp:508
> > +    if (m_element && m_document.settings().aspectRatioOfImgFromWidthAndHeightEnabled() && style.aspectRatioType() == AspectRatioType::Auto) {
> > +        double attributeWidth = -1;
> > +        double attributeHeight = -1;
> > +        if (m_element->useWidthHeightAsAspectRatioHint(attributeWidth, attributeHeight)) {
> > +            style.setAspectRatioType(AspectRatioType::AutoAndRatio);
> > +            style.setAspectRatio(attributeWidth, attributeHeight);
> > +        }
> > +    }
> 
> "When the text below says that a pair of attributes w and h on an element
> element map to the aspect-ratio property (using dimension rules), it means
> that if element has both attributes w and h, and parsing those attributes'
> values using the rules for parsing dimension values doesn't generate an
> error or return a percentage for either, then the user agent is expected to
> use the parsed dimensions as a presentational hint for the 'aspect-ratio'
> property of the form auto w / h."
> 
> So the concept used in the spec here is "presentational hint",
> https://html.spec.whatwg.org/#the-css-user-agent-style-sheet-and-
> presentational-hints
> 
> It is the same concept as used for stuff like <body bgcolor="blue">
> 
> In WebKit this is implemented as Element::presentationAttributeStyle(). To
> generate it correctly see for example
> HTMLBodyElement::collectStyleForPresentationAttribute.
> 
> You shouldn't need anything in style adjuster for this feature.

Hi Antti, thanks for the review!

Yeah, indeed, it makes more sense to implement in collectStyleForPresentationAttribute.
And supporting for <video> and <input> elements becomes easier.

I will upload a new patch for this. Thanks!

[cathiechen]

Created attachment 429304 [details]
Patch

[Antti Koivisto]

Comment on attachment 429304 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429304&action=review

Much better!

> Source/WebCore/html/HTMLElement.cpp:641
> +void HTMLElement::applyAspectRatioToStyle(const AtomString& widthAttribute, const AtomString& heightAttribute, MutableStyleProperties& style)
> +{
> +    if (!document().settings().aspectRatioOfImgFromWidthAndHeightEnabled())
> +        return;
> +
> +    double width = parseValidHTMLFloatingPointNumber(widthAttribute).valueOr(-1);
> +    double height = parseValidHTMLFloatingPointNumber(heightAttribute).valueOr(-1);
> +    if (width < 0 || height < 0)
> +        return;
> +    auto ratioList = CSSValueList::createSlashSeparated();
> +    ratioList->append(CSSValuePool::singleton().createValue(width, CSSUnitType::CSS_NUMBER));
> +    ratioList->append(CSSValuePool::singleton().createValue(height, CSSUnitType::CSS_NUMBER));
> +    auto list = CSSValueList::createSpaceSeparated();
> +    list->append(CSSValuePool::singleton().createIdentifierValue(CSSValueAuto));
> +    list->append(ratioList);
> +
> +    style.setProperty(CSSPropertyAspectRatio, RefPtr<CSSValue>(WTFMove(list)));
> +}

This could test in the beginning if CSSPropertyAspectRatio is already set and bail out (since it gets called twice, separately for both and width and height).

It would look nicer (but be slightly less efficient) in the call sites if it didn't take width/height as parameters but grabbed both here ('applyAspectRatioFromWidthAndHeightAttributesToStyle')

> Source/WebCore/html/HTMLImageElement.cpp:121
> +        if (hasAttribute(heightAttr))
> +            applyAspectRatioToStyle(value, getAttribute(heightAttr), style);

The hasAttribute test here is not needed, applyAspectRatioToStyle will just bail out if one is missing.
This should use attributeWithoutSynchronization() instead of getAttribute.

> Source/WebCore/html/HTMLImageElement.cpp:125
> +        if (hasAttribute(widthAttr))
> +            applyAspectRatioToStyle(getAttribute(widthAttr), value, style);

Same here.

> Source/WebCore/html/HTMLInputElement.cpp:711
> +        if (hasAttribute(heightAttr) && isImageButton())
> +            applyAspectRatioToStyle(value, getAttribute(heightAttr), style);

Same here.

> Source/WebCore/html/HTMLInputElement.cpp:716
> +        if (hasAttribute(widthAttr) && isImageButton())
> +            applyAspectRatioToStyle(getAttribute(widthAttr), value, style);

Same here.

> Source/WebCore/html/HTMLVideoElement.cpp:119
> +        if (hasAttribute(heightAttr))
> +            applyAspectRatioToStyle(value, getAttribute(heightAttr), style);

Same here.

> Source/WebCore/html/HTMLVideoElement.cpp:123
> +        if (hasAttribute(widthAttr))
> +            applyAspectRatioToStyle(getAttribute(widthAttr), value, style);

Same here.

[cathiechen]

Comment on attachment 429304 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429304&action=review

Hi Antti, thanks for the review!

>> Source/WebCore/html/HTMLElement.cpp:641
>> +}
> 
> This could test in the beginning if CSSPropertyAspectRatio is already set and bail out (since it gets called twice, separately for both and width and height).
> 
> It would look nicer (but be slightly less efficient) in the call sites if it didn't take width/height as parameters but grabbed both here ('applyAspectRatioFromWidthAndHeightAttributesToStyle')

Done, thanks!

>> Source/WebCore/html/HTMLImageElement.cpp:121
>> +            applyAspectRatioToStyle(value, getAttribute(heightAttr), style);
> 
> The hasAttribute test here is not needed, applyAspectRatioToStyle will just bail out if one is missing.
> This should use attributeWithoutSynchronization() instead of getAttribute.

Done, thanks!

>> Source/WebCore/html/HTMLImageElement.cpp:125
>> +            applyAspectRatioToStyle(getAttribute(widthAttr), value, style);
> 
> Same here.

Done.

>> Source/WebCore/html/HTMLInputElement.cpp:711
>> +            applyAspectRatioToStyle(value, getAttribute(heightAttr), style);
> 
> Same here.

Done.

>> Source/WebCore/html/HTMLInputElement.cpp:716
>> +            applyAspectRatioToStyle(getAttribute(widthAttr), value, style);
> 
> Same here.

Done.

>> Source/WebCore/html/HTMLVideoElement.cpp:119
>> +            applyAspectRatioToStyle(value, getAttribute(heightAttr), style);
> 
> Same here.

Done.

>> Source/WebCore/html/HTMLVideoElement.cpp:123
>> +            applyAspectRatioToStyle(getAttribute(widthAttr), value, style);
> 
> Same here.

Done.

[cathiechen]

Created attachment 429497 [details]
Patch

[EWS]

Committed r277997 (238110@main): <https://commits.webkit.org/238110@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429497 [details].