Summary: | [css-grid] marginLogicalSizeForChild checks auto margins in the opposite axis for orthogonal items | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Oriol Brufau <obrufau> | ||||||||
Component: | CSS | Assignee: | 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: |
|
Created attachment 399768 [details]
Patch
Created attachment 399769 [details]
Patch
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 on attachment 399769 [details]
Patch
r=me, thanks for the fix.
Committed r261894: <https://trac.webkit.org/changeset/261894> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399769 [details]. 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. (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 Let's just land failing results. 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. (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. |
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.