Bug 234143 – Flexbox ignores margins of absolute positioned children when `align-items: flex-end` or `justify-content: flex-end`

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

[Vitaly Dyachkov]

Created attachment 446717 [details]
Test page showing the bug

`position: absolute` children aligned to `flex-end` (either by `align-items` or `self-align`) are always positioned to the bottom (when `flex-direction: row`) or to the right (when `flex-direction: column`).
In a similar way, `position: absolute` children are always positioned to the right when `justify-content: flex-end`.

https://www.w3.org/TR/css-flexbox-1/#abspos-items says that the item should be positioned "as if it was the sole child" of the flex container and also https://www.w3.org/TR/css-flexbox-1/#algo-available says that to determine the available cross (and main) space for flex items one must "subtract the flex container’s margin".

[Vitaly Dyachkov]

There is a WPT test css/css-flexbox/abspos/flex-abspos-staticpos-margin-002.html where you can see this bug but it's not failing. Maybe because it doesn't use testharness.

[Vitaly Dyachkov]

Ok, I misinterpreted this part of the spec - https://www.w3.org/TR/css-flexbox-1/#algo-available. But the general idea is the same - we must take margins into account when aligning absolutely-positioned items.

Also, flex-abspos-staticpos-margin-002.html was "expectedly" failing and not reported.

[Vitaly Dyachkov]

Created attachment 446758 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Vitaly Dyachkov]

Created attachment 446875 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 446875 [details]
Patch

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

First question I have is why are you changing the current ref tests with checkLayout() versions? The former are generally preferred, so unless there is something wrong in the current ones, I wouldn't change them. Or is it because you want to test negative offsets?

> Source/WebCore/ChangeLog:10
> +        To do that the code was correctly substructing the size of the item from the size of the container but was not substructing the margins.

Nit: substructing -> substracting.

[Vitaly Dyachkov]

> First question I have is why are you changing the current ref tests with
> checkLayout() versions? The former are generally preferred, so unless there
> is something wrong in the current ones, I wouldn't change them. Or is it
> because you want to test negative offsets?

I didn't know that reftests are preferred. Instinctively I thought that API tests are more reliable than "comparing pixels". I'll roll back the test changes.

[Sergio Villar Senin]

I knew I had seen something like that before (facepalming). 

I have uploaded https://bugs.webkit.org/show_bug.cgi?id=234244 which was in my local git for some time, but I had completely forgotten about uploading it.

Don't want to sound pretentious but I prefer the approach of my patch as it uses already existing code instead of reimplementing it. However my patch is only for alignment not for justification, so your patch still applies for the inline direction. Maybe you could wait for bug 234244 to land and then implement yours on top of it, what do you think?

[Sergio Villar Senin]

(In reply to Vitaly Dyachkov from comment #7)
> > First question I have is why are you changing the current ref tests with
> > checkLayout() versions? The former are generally preferred, so unless there
> > is something wrong in the current ones, I wouldn't change them. Or is it
> > because you want to test negative offsets?
> 
> I didn't know that reftests are preferred. Instinctively I thought that API
> tests are more reliable than "comparing pixels". I'll roll back the test
> changes.

The problem with checkLayout() tests (which do not necessarily apply to your tests) is that they're pretty sensible to device/platform specifics, like for example how scrollbars or buttons are rendered (different sizes in different platforms/devices). Ref tests do not have that problem. Feel free to propose the test changes in the WPT repository if you still think they're more suitable for this case, I wouldn't oppose if some other WPT maintainer think the other way around.

Anyway in general it's better not to add changes to things that do not require those changes. It makes reviews easier and also ease the eventual process of reverting a patch causing regressions.

[Vitaly Dyachkov]

> Maybe you could wait for bug 234244 to land and then
> implement yours on top of it, what do you think?

No problem! I'll wait until it's landed and then rebase my patch. And welcome for the reminder :)

[Vitaly Dyachkov]

Created attachment 447136 [details]
Patch

[Vitaly Dyachkov]

Made all the tests to be ref tests.

[Sergio Villar Senin]

Comment on attachment 447136 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1692
> +    const LayoutUnit availableSpace = mainAxisContentExtent(contentLogicalHeight()) - childMainExtent;

Just use auto instead of LayoutUnit. And remove the const as we don't normally use it for variables in WebKit.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1703
> +    LayoutUnit availableSpace = crossAxisContentExtent() - childCrossExtent;

Ditto LayoutUnit->auto.

> LayoutTests/ChangeLog:12
> +        * imported/w3c/web-platform-tests/css/css-flexbox/abspos/flex-abspos-staticpos-margin-003.html: Added.

You have to rebuild the ChangeLog as it's wrong.

> LayoutTests/imported/w3c/ChangeLog:13
> +        * web-platform-tests/css/css-flexbox/abspos/position-absolute-015-expected.txt:

Ditto.

> LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/abspos/flex-abspos-staticpos-margin-003-expected.html:5
> +-->

Please remove this as the WPT project has its own license policies.

> LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/abspos/flex-abspos-staticpos-margin-003-expected.html:7
> +<head>

No need for <html> or <head> tags.

> LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/abspos/flex-abspos-staticpos-margin-003-expected.html:10
> +  <meta charset="utf-8">

You don't have to include all this in the expected (or -ref in WPT terminology) file.

> LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/abspos/flex-abspos-staticpos-margin-003-expected.html:39
> +<body>

Also no need for <body>

> LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/abspos/flex-abspos-staticpos-margin-003.html:7
> +<head>

Ditto license & unneeded tags.

> LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/abspos/position-absolute-014-expected.txt:3
> +PASS The bottom of each pair of boxes should be the same

This should not be needed now.

[Vitaly Dyachkov]

Created attachment 447228 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 447228 [details]
Patch

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

Looking great!

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1693
> +    auto offset = initialJustifyContentOffset(style(), availableSpace, 1, isReverse);

This was not strictly needed as we are not modifying this code but it's ok (it's in general a good idea not to change unneeded lines of code to help future developers when doing code archeology).

> LayoutTests/ChangeLog:8
> +        * TestExpectations:

Add something here like "unskipeed a test that is working fine now" or similar.

> LayoutTests/imported/w3c/ChangeLog:10
> +        * web-platform-tests/css/css-flexbox/abspos/position-absolute-015-expected.txt:

And here something like "replaced FAIL by PASS expectations"

[Vitaly Dyachkov]

Created attachment 447340 [details]
Patch

[EWS]

Committed r287128 (245312@main): <https://commits.webkit.org/245312@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447340 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86568797>