Bug 209871

This bug also affects (indirectly) the test https://wpt.live/css/css-flexbox/flex-wrap-005.html since this test sets align-content on a flexbox with wrappability

Note:

We have the test css3/flexbox/alignContent-applies-with-flexWrap-wrap-with-single-line.html which is just an old version (testing the old spec) of the WPT test css/css-flexbox/align-content-wrap-001.html (which now tests the new spec)

once this bug its fixed, the test css3/flexbox/alignContent-applies-with-flexWrap-wrap-with-single-line.html can be removed

And layout test css3/flexbox/flexbox-wordwrap.html can be removed in favor of imported/w3c/web-platform-tests/css/css-flexbox/align-content-wrap-002.html

And layout test css3/flexbox/multiline-align-content.html can be removed in favor of  imported/w3c/web-platform-tests/css/css-flexbox/align-content-wrap-003.html

Created attachment 400732 [details]
Patch

Comment on attachment 400732 [details]
Patch

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

Nice catch, thanks for the fix.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1732
>      // flex line, the line height is all the available space. For

The comment was already accurate...

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1735
> +    if (!isMultiline()) {

Nit: Could we add an ASSERT(lineContexts.size() == 1) here?

Comment on attachment 400732 [details]
Patch

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

Thanks for the review

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1732
>>      // flex line, the line height is all the available space. For
> 
> The comment was already accurate...

Right the comment was accurate but the code was not.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1735
>> +    if (!isMultiline()) {
> 
> Nit: Could we add an ASSERT(lineContexts.size() == 1) here?

I am not sure. The invariant you mention should be true in any method not only here, and we know that the array access bellow is correct because !lineContexts.isEmpty()

I'm checking the test failures. 3 of them come from tests that can be simply removed as described in comments #2, #3 and #4.

However it looks like there are 3 valid regressions. I'll double check.

Adding a required dependency.

Created attachment 400916 [details]
Patch

PTAL as I've significantly changed the original patch.

Comment on attachment 400916 [details]
Patch

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

What's going on with iOS EWS?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:365
> +    if (!isMultiline() && !lineContexts.isEmpty())

We might want to move this condition to a separated method with a descriptive name to be called here and later.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:486
> +    auto resetOverride = makeScopeExit([&] {
> +        if (childHasOverrideWidth)
> +            const_cast<RenderBox*>(&child)->setOverrideContentLogicalWidth(overrideWidth);
> +    });

What does these lines do? Wouldn't be the same if we just this this two lines before the return?

Comment on attachment 400916 [details]
Patch

Regarding iOS it looks like 1 failure is unrelated and the other test needs a rebaseline

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

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:365
>> +    if (!isMultiline() && !lineContexts.isEmpty())
> 
> We might want to move this condition to a separated method with a descriptive name to be called here and later.

Hmm the condition here and the other one are not the same.

 (lineContexts.isEmpty() || !isMultiline())

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:486
>> +    });
> 
> What does these lines do? Wouldn't be the same if we just this this two lines before the return?

Yes, it's another way to do it. Do you prefer the other way?

Created attachment 401330 [details]
Patch

iOS rebaseline

Comment on attachment 400916 [details]
Patch

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

r=me

>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:365
>>> +    if (!isMultiline() && !lineContexts.isEmpty())
>> 
>> We might want to move this condition to a separated method with a descriptive name to be called here and later.
> 
> Hmm the condition here and the other one are not the same.
> 
>  (lineContexts.isEmpty() || !isMultiline())

Ok, true, I didn't realize sorry.

>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:486
>>> +    });
>> 
>> What does these lines do? Wouldn't be the same if we just this this two lines before the return?
> 
> Yes, it's another way to do it. Do you prefer the other way?

makeScopeExit() is not used at all in WebCore/rendering/, and it's not like this is a lot of code and you lose the context, so I'd prefer the other way around.

Committed r262716: <https://trac.webkit.org/changeset/262716>

<rdar://problem/64120424>