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".
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.
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.
Created attachment 446758 [details] Patch
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
Created attachment 446875 [details] Patch
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.
> 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.
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?
(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.
> 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 :)
Created attachment 447136 [details] Patch
Made all the tests to be ref tests.
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.
Created attachment 447228 [details] Patch
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"
Created attachment 447340 [details] Patch
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].
<rdar://problem/86568797>