Bug 223504 – [css-grid] Set hasIntrinsicWidth & hasIntrinsicHeight properties for SVG element's intrinsic size

• Component: CSS
• Status: REOPENED

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

[Javier Fernandez]

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"  ?

[Radar WebKit Bug Importer]

<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

[Javier Fernandez]

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

[EWS]

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].

[Simon Fraser (smfr)]

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?

[Simon Fraser (smfr)]

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.

[Tim Horton]

Reverting in https://bugs.webkit.org/show_bug.cgi?id=236743, reopening.

[Wenson Hsieh]

Created attachment 453361 [details]
Rebase the original fix

[Wenson Hsieh]

(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.

[Wenson Hsieh]

Comment on attachment 453361 [details]
Rebase the original fix

(iOS WK2 failures seem unrelated)

[Darin Adler]

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.

[Wenson Hsieh]

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.

[Ahmad Saleem]

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.