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