[css-flex] Implement 9.8.1 Definite and Indefinite Sizes
Created attachment 415408 [details] Patch
Created attachment 415414 [details] Patch
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.
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.
Created attachment 415740 [details] Patch
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.
Committed r270578: <https://trac.webkit.org/changeset/270578>
<rdar://problem/72136484>
This caused bug 221110
Reopening because r270578 was reverted in r272755.
Importing tests that this patch will fix in bug 221813
Created attachment 420287 [details] Patch
Ping reviewers
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.
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.
Created attachment 420810 [details] Patch
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.
Committed r273072 (234270@main): <https://commits.webkit.org/234270@main>
This causes image distortion in https://www.walmart.com/grocery/ which doesn't appear in other browsers.
(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