Bug 212055

Summary: [css-grid] marginLogicalSizeForChild checks auto margins in the opposite axis for orthogonal items
Product: WebKit Reporter: Oriol Brufau <obrufau>
Component: CSSAssignee: Oriol Brufau <obrufau>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, esprehn+autocc, ews-watchlist, glenn, jfernandez, kondapallykalyan, pdr, rego, simon.fraser, svillar, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=212113
Attachments:
Description Flags
testcase
none
Patch
none
Patch none

Description Oriol Brufau 2020-05-18 17:41:31 PDT
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.
Comment 1 Oriol Brufau 2020-05-19 14:06:05 PDT
Created attachment 399768 [details]
Patch
Comment 2 Oriol Brufau 2020-05-19 14:13:03 PDT
Created attachment 399769 [details]
Patch
Comment 3 Oriol Brufau 2020-05-19 14:16:14 PDT
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.
Comment 4 Manuel Rego Casasnovas 2020-05-19 15:11:09 PDT
Comment on attachment 399769 [details]
Patch

r=me, thanks for the fix.
Comment 5 EWS 2020-05-19 16:50:30 PDT
Committed r261894: <https://trac.webkit.org/changeset/261894>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399769 [details].
Comment 6 Radar WebKit Bug Importer 2020-05-19 16:51:12 PDT
<rdar://problem/63421556>
Comment 7 Ryan Haddad 2020-06-09 16:23:19 PDT
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.
Comment 8 Ryan Haddad 2020-06-09 17:36:02 PDT
(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
Comment 9 Simon Fraser (smfr) 2020-06-09 20:19:51 PDT
Let's just land failing results.
Comment 10 Oriol Brufau 2020-06-10 10:10:08 PDT
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.
Comment 11 Ryan Haddad 2020-06-10 13:22:10 PDT
(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.