Bug 219538 – [css-flex] Implement section 9.8 Definite and Indefinite Sizes case 1

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Sergio Villar Senin]

[css-flex] Implement 9.8.1 Definite and Indefinite Sizes

[Sergio Villar Senin]

Created attachment 415408 [details]
Patch

[Sergio Villar Senin]

Created attachment 415414 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 415414 [details]
Patch

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

I've a question about an ASSERT.

> Source/WebCore/ChangeLog:9
> +        of any stretched flex items is the flex containerâs inner cross size (clamped to the flex itemâs

There's some weird thing here with "containerâs" and "itemâs".

> Source/WebCore/rendering/RenderFlexibleBox.cpp:768
> +        ASSERT(containerCrossSizeLength.isFixed());

Why style.height() is going to be fixed? I don't understand this assert, couldn't it be auto for example?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:823
> +        // stretched flex items is the flex containerâs inner cross size (clamped to the flex itemâs min and max cross size)

Again weird enconding.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:828
> +            if (auto& crossSize = isHorizontalFlow() ? style().height() : style().width(); crossSize.isFixed())

Never seen this kind of things in a single "check" before, I'd move the crossSize variable to the previous line, I don't see any benefit from not doing it.

BTW, there's another FIXME a few lines below. Is this related?
826842    // FIXME: Eventually we should support other types of sizes here.
827843    // Requires updating computeMainSizeFromAspectRatioUsing.

[Sergio Villar Senin]

Comment on attachment 415414 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/ChangeLog:9
>> +        of any stretched flex items is the flex containerâs inner cross size (clamped to the flex itemâs
> 
> There's some weird thing here with "containerâs" and "itemâs".

Ups, something wrong with encoding in the editor, will fix it.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:768
>> +        ASSERT(containerCrossSizeLength.isFixed());
> 
> Why style.height() is going to be fixed? I don't understand this assert, couldn't it be auto for example?

Right, it's far from obvious. I'll try to explain it .This method is only called if useChildAspectRatio() is true. That only happens when childCrossSizeIsDefinite() returns true as well. So far so good.

If the child cross size was auto (before this change) then the child cross size would be consider indefinite and thus this method wouldn't ever called. After this change this method could be called if the cross size is auto if and only if the cross size of the container is fixed (see the changes to childCrossSizeIsDefinite()). As I mention in a comment in the latter, we should include more cases where a length is definite but we're starting with fixed sizes for simplicity.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:823
>> +        // stretched flex items is the flex containerâs inner cross size (clamped to the flex itemâs min and max cross size)
> 
> Again weird enconding.

Fixing...

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:828
>> +            if (auto& crossSize = isHorizontalFlow() ? style().height() : style().width(); crossSize.isFixed())
> 
> Never seen this kind of things in a single "check" before, I'd move the crossSize variable to the previous line, I don't see any benefit from not doing it.
> 
> BTW, there's another FIXME a few lines below. Is this related?
> 826842    // FIXME: Eventually we should support other types of sizes here.
> 827843    // Requires updating computeMainSizeFromAspectRatioUsing.

The if statement with initializer is a C++17 feature that is becoming popular inside WebKit, there are several other instances of it.

WRT the FIXME bellow is not related, it's about supporting intrinsic sizes.

[Sergio Villar Senin]

Created attachment 415740 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 415414 [details]
Patch

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

r=me

>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:768
>>> +        ASSERT(containerCrossSizeLength.isFixed());
>> 
>> Why style.height() is going to be fixed? I don't understand this assert, couldn't it be auto for example?
> 
> Right, it's far from obvious. I'll try to explain it .This method is only called if useChildAspectRatio() is true. That only happens when childCrossSizeIsDefinite() returns true as well. So far so good.
> 
> If the child cross size was auto (before this change) then the child cross size would be consider indefinite and thus this method wouldn't ever called. After this change this method could be called if the cross size is auto if and only if the cross size of the container is fixed (see the changes to childCrossSizeIsDefinite()). As I mention in a comment in the latter, we should include more cases where a length is definite but we're starting with fixed sizes for simplicity.

Ok, thanks for the explanation, I got it now.

>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:828
>>> +            if (auto& crossSize = isHorizontalFlow() ? style().height() : style().width(); crossSize.isFixed())
>> 
>> Never seen this kind of things in a single "check" before, I'd move the crossSize variable to the previous line, I don't see any benefit from not doing it.
>> 
>> BTW, there's another FIXME a few lines below. Is this related?
>> 826842    // FIXME: Eventually we should support other types of sizes here.
>> 827843    // Requires updating computeMainSizeFromAspectRatioUsing.
> 
> The if statement with initializer is a C++17 feature that is becoming popular inside WebKit, there are several other instances of it.
> 
> WRT the FIXME bellow is not related, it's about supporting intrinsic sizes.

I think it's harder to read than a variable first and then the if in a different line. I see a few other examples in other WebKit code, I guess I should get used to it at some point.

[Sergio Villar Senin]

Committed r270578: <https://trac.webkit.org/changeset/270578>

[Radar WebKit Bug Importer]

<rdar://problem/72136484>

[Jon Lee]

This caused bug 221110

[Said Abou-Hallawa]

Reopening because r270578 was reverted in r272755.

[Sergio Villar Senin]

Importing tests that this patch will fix in bug 221813

[Sergio Villar Senin]

Created attachment 420287 [details]
Patch

[Sergio Villar Senin]

Ping reviewers

[Manuel Rego Casasnovas]

Comment on attachment 420287 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [css-flex] Implement 9.8.1 Definite and Indefinite Sizes

Nit: There's no section 9.8.1 in css-flexbox spec, I guess this is 9.8:
https://drafts.csswg.org/css-flexbox/#definite-sizes

It'd be nice to add that link on the ChangeLog too.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:838
> +        if (auto& crossSize = isHorizontalFlow() ? style().height() : style().width(); crossSize.isFixed())
> +            return true;

I'd write this like:
        if (auto& crossSize = isHorizontalFlow() ? style().height() : style().width())
            return crossSize.isFixed();

> Source/WebCore/rendering/RenderFlexibleBox.cpp:847
> +

Nit: I looks like this change is not needed.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1178
> +        if (child.hasAspectRatio() && child.intrinsicSize().height())

Why you removed "> 0" in this condition?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1198
> +

Nit: Unneeded change.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1247
> +

Ditto.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1252
> +

Ditto.

[Sergio Villar Senin]

Comment on attachment 420287 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        [css-flex] Implement 9.8.1 Definite and Indefinite Sizes
> 
> Nit: There's no section 9.8.1 in css-flexbox spec, I guess this is 9.8:
> https://drafts.csswg.org/css-flexbox/#definite-sizes
> 
> It'd be nice to add that link on the ChangeLog too.

It's section 9.8 and then bullet #1. I thought it was a good way of simplifying it but it can be misleading indeed.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:838
>> +            return true;
> 
> I'd write this like:
>         if (auto& crossSize = isHorizontalFlow() ? style().height() : style().width())
>             return crossSize.isFixed();

OK

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1178
>> +        if (child.hasAspectRatio() && child.intrinsicSize().height())
> 
> Why you removed "> 0" in this condition?

The only important thing here is being != 0 because otherwise you'd end up with a division by zero when computing the aspect ratio. A negative intrinsic height does not make any sense anyway. I can bring it back but I thought it was just adding noise.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1198
>> +
> 
> Nit: Unneeded change.

Sure, but it's a style fix, same for the others.

[Sergio Villar Senin]

Created attachment 420810 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 420810 [details]
Patch

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

r+, let's hope it doesn't cause regressions this time.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:833
> +    // stretched flex items is the flex container's inner cross size (clamped to the flex itemâs min and max cross size)

Nit: there's some weird character on this line.

[Sergio Villar Senin]

Committed r273072 (234270@main): <https://commits.webkit.org/234270@main>

[Jon Lee]

This causes image distortion in https://www.walmart.com/grocery/ which doesn't appear in other browsers.

[zalan]

(In reply to Jon Lee from comment #19)
> This causes image distortion in https://www.walmart.com/grocery/ which
> doesn't appear in other browsers.
see bug 228656