Bug 209871 – [css-flexbox] align-content should apply even when there's just a single line

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Carlos Alberto Lopez Perez]

In Jan 2015 the spec changed in https://drafts.csswg.org/css-flexbox/#change-201409-align-content-wrapping

This is tested by WPT tests:

https://wpt.live/css/css-flexbox/align-content-wrap-001.html
https://wpt.live/css/css-flexbox/align-content-wrap-002.html
https://wpt.live/css/css-flexbox/align-content-wrap-003.html

Which fail only on WebKit: https://wpt.fyi/results/css/css-flexbox?label=master&label=experimental&product=chrome&product=edge&product=firefox&product=safari&product=webkitgtk&aligned&q=css%2Fcss-flexbox%2Falign-content-wrap

This was fixed for Chrome on https://crbug.com/599828

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

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

[Sergio Villar Senin]

Created attachment 400732 [details]
Patch

[Manuel Rego Casasnovas]

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?

[Sergio Villar Senin]

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

[Sergio Villar Senin]

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.

[Sergio Villar Senin]

Adding a required dependency.

[Sergio Villar Senin]

Created attachment 400916 [details]
Patch

[Sergio Villar Senin]

PTAL as I've significantly changed the original patch.

[Manuel Rego Casasnovas]

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?

[Sergio Villar Senin]

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?

[Sergio Villar Senin]

Created attachment 401330 [details]
Patch

iOS rebaseline

[Manuel Rego Casasnovas]

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.

[Sergio Villar Senin]

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

[Radar WebKit Bug Importer]

<rdar://problem/64120424>