Bug 212055 – [css-grid] marginLogicalSizeForChild checks auto margins in the opposite axis for orthogonal items

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

[Oriol Brufau]

Created attachment 399689 [details]
testcase

This function is wrong:

    LayoutUnit marginLogicalSizeForChild(const RenderGrid& grid, GridTrackSizingDirection direction, const RenderBox& child)
    {
        if (child.needsLayout())
            return computeMarginLogicalSizeForChild(grid, direction, child);
        bool isRowAxis = flowAwareDirectionForChild(grid, child, direction) == ForColumns;
        LayoutUnit marginStart = marginStartIsAuto(child, direction) ? 0_lu : isRowAxis ? child.marginStart() : child.marginBefore();
        LayoutUnit marginEnd = marginEndIsAuto(child, direction) ? 0_lu : isRowAxis ? child.marginEnd() : child.marginAfter();
        return marginStart + marginEnd;
    }

It uses `direction` in order to know which side to check in `marginStartIsAuto` and `marginEndIsAuto`.
But when retrieving the margin amount it uses `flowAwareDirectionForChild` instead.

In the attached testcase, there should be a green square, not a green rectangle.

[Oriol Brufau]

Created attachment 399768 [details]
Patch

[Oriol Brufau]

Created attachment 399769 [details]
Patch

[Oriol Brufau]

There is still some problem here since the testcase in this bug and the new cases in grid-items-minimum-width-orthogonal-001.html still fail.

But this patch fixes grid-items-minimum-height-orthogonal-001.html, so it's an improvement.

[Manuel Rego Casasnovas]

Comment on attachment 399769 [details]
Patch

r=me, thanks for the fix.

[EWS]

Committed r261894: <https://trac.webkit.org/changeset/261894>

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

[Radar WebKit Bug Importer]

<rdar://problem/63421556>

[Ryan Haddad]

The test added with this change is failing on iOS and macOS: 
https://build.webkit.org/results/Apple-Catalina-Release-WK2-Tests/r262814%20(6219)/results.html

https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fcss%2Fcss-grid%2Fgrid-items%2Fgrid-items-minimum-height-orthogonal-001.html

This started after r261841 was reverted in r262809, so I assume that r261894 had a dependency upon r261841. Perhaps we should revert this until https://bugs.webkit.org/show_bug.cgi?id=209461 is resolved again.

[Ryan Haddad]

(In reply to Ryan Haddad from comment #7)
> Perhaps we should revert this until
> https://bugs.webkit.org/show_bug.cgi?id=209461 is resolved again.

Looks like reverting this change is blocked by conflicts in Source/WebCore/rendering/GridLayoutFunctions.cpp because of the changes for https://bugs.webkit.org/show_bug.cgi?id=212113

[Simon Fraser (smfr)]

Let's just land failing results.

[Oriol Brufau]

Yes, this was a follow-up of bug 209461, so it's not surprising that the test fails after the revert.

I'm reimporting the WPT tests in bug 213028, I will mark grid-items-minimum-height-orthogonal-001.html as failing on iOS and macOS while I'm at it.

[Ryan Haddad]

(In reply to Oriol Brufau from comment #10)
> Yes, this was a follow-up of bug 209461, so it's not surprising that the
> test fails after the revert.
> 
> I'm reimporting the WPT tests in bug 213028, I will mark
> grid-items-minimum-height-orthogonal-001.html as failing on iOS and macOS
> while I'm at it.
I ended up landing a new baseline in https://trac.webkit.org/r262852 before I noticed this comment.