The following WPT tests on stretched SVGs without an aspect-ratio are failing in WebKit: css/css-grid/alignment/grid-item-no-aspect-ratio-stretch-6.html css/css-grid/alignment/grid-item-no-aspect-ratio-stretch-7.html
Created attachment 423712 [details] Patch
Comment on attachment 423712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423712&action=review > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:83 > + intrinsicSize.hasWidth = svgSVGElement().hasIntrinsicWidth(); I'm not sure I understand well enough the meaning of the hasWidth/hasHeight attributes. Looking at how you are using it, wouldn't be a clearer name something like "isIntrinsicWidth/isIntrinsiHeight" ?
<rdar://problem/75879440>
(In reply to Javier Fernandez from comment #2) > Comment on attachment 423712 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=423712&action=review > > > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:83 > > + intrinsicSize.hasWidth = svgSVGElement().hasIntrinsicWidth(); > > I'm not sure I understand well enough the meaning of the hasWidth/hasHeight > attributes. Looking at how you are using it, wouldn't be a clearer name > something like "isIntrinsicWidth/isIntrinsiHeight" ? hasWidth/hasHeight here means that whether width/height has been set. Since we are introducing these properties in FloatSize.h, I thought it might be better to make it more generic. When we call intrinsicSize.*, we know that we are referring width/height of the intrinsicSize. Also it's to be inline with has_width/has_height in Chromium for this situation.
Created attachment 425263 [details] Patch
Created attachment 425275 [details] Patch
Comment on attachment 425275 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425275&action=review r=me > Source/WebCore/rendering/RenderReplaced.cpp:630 > + bool hasIntrinsicHeight = constrainedSize.hasIntrinsicHeight || constrainedSize.height() > 0; I'd suggest a comment here to note that we are only updating the hasIntrinsicHeight flag for SVG elements, but that ideally, we would like to use it in all the cases and get rid of the constrainedSize.height() > 0 clause.
Created attachment 425614 [details] Patch
Committed r275772 (236347@main): <https://commits.webkit.org/236347@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425614 [details].
Comment on attachment 425614 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425614&action=review > Source/WebCore/platform/graphics/FloatSize.h:76 > + bool hasIntrinsicWidth = false; > + bool hasIntrinsicHeight = false; Why are these here?
Comment on attachment 425614 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425614&action=review >> Source/WebCore/platform/graphics/FloatSize.h:76 >> + bool hasIntrinsicHeight = false; > > Why are these here? This increased the size of FloatSize, FloatRect and all classes that use them, and means that FloatSize no longer fits into a 64-bit register. It's also a layering violation; it makes no sense for this basic geometry class to hold information related to layout.
Reverting in https://bugs.webkit.org/show_bug.cgi?id=236743, reopening.
Created attachment 453361 [details] Rebase the original fix
(In reply to Tim Horton from comment #12) > Reverting in https://bugs.webkit.org/show_bug.cgi?id=236743, reopening. (Let's also ensure these WPT fixes don't get lost moving forward due to the revert) I uploaded a version of Ziran's original change that doesn't add the new flags to FloatSize (but instead plumbs them as arguments through the affected rendering codepaths).
Thanks very much Wenson for helping with this. Sorry that I didn't manage to find time last week to look into it. I think Wenson's patch pretty much wraps up what I initially intended to achieve but without increasing FloatSize. Thanks for the lovely solution.
Comment on attachment 453361 [details] Rebase the original fix (iOS WK2 failures seem unrelated)
Comment on attachment 453361 [details] Rebase the original fix View in context: https://bugs.webkit.org/attachment.cgi?id=453361&action=review > Source/WebCore/rendering/RenderBox.h:615 > + virtual void computeIntrinsicRatioInformation(FloatSize& /* intrinsicSize */, double& /* intrinsicRatio */, bool& /* hasIntrinsicWidth */, bool& /* hasIntrinsicHeight */) const { } In future I think this needs to start returning a structure, not take 4 out arguments.
Comment on attachment 453361 [details] Rebase the original fix (In reply to Darin Adler from comment #17) > Comment on attachment 453361 [details] > Rebase the original fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453361&action=review > > > Source/WebCore/rendering/RenderBox.h:615 > > + virtual void computeIntrinsicRatioInformation(FloatSize& /* intrinsicSize */, double& /* intrinsicRatio */, bool& /* hasIntrinsicWidth */, bool& /* hasIntrinsicHeight */) const { } > > In future I think this needs to start returning a structure, not take 4 out > arguments. Sounds good! Since this patch touches all of the method signatures anyways, I think we should just do it as a part of this change.
Based on Comment 0, we only have now 'grid-item-no-aspect-ratio-stretch-6.html' and we are still failing this and other two as well: https://wpt.fyi/results/css/css-grid/alignment?label=master&label=experimental&aligned&q=grid-item-no-aspect-ratio-stretch- Adding 'WPTImpact' tag as well for tracking purposes.