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.
<rdar://problem/70161811>
Created attachment 429128 [details] Patch
Comment on attachment 429128 [details] Patch Hi, I think this patch is ready for review now. Thanks:)
(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 :)
(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.
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?
(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.
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?
(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!
Created attachment 429180 [details] Patch
Comment on attachment 429180 [details] Patch Added the spec link to the ChangeLog.
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?
(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.
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.
...and HTMLBodyElement::isPresentationAttribute(
In fact width/height are already presentation attributes for img/video, just not for aspect-ratio.
(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!
Created attachment 429304 [details] Patch
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.
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.
Created attachment 429497 [details] Patch
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].