Bug 155199 – [css-grid] Allow to place positioned grid items on the padding

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Manuel Rego Casasnovas]

Per discussion on www-style we should be able to place positioned items between the padding edge and the first/last line of the grid:
https://lists.w3.org/Archives/Public/www-style/2015Nov/0070.html

We've to port the following patch from Blink:
https://codereview.chromium.org/1607463004/

And we'll port the next one too, as it's very related:
https://codereview.chromium.org/1608943003/

[Manuel Rego Casasnovas]

Created attachment 273347 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 273347 [details]
Patch

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

Keep going!

> Source/WebCore/rendering/RenderGrid.cpp:1486
> +    bool isForColumns = direction == ForColumns;

We've been lately using isRowAxis.

> Source/WebCore/rendering/RenderGrid.cpp:1495
> +    // For positioned items we cannot use GridSpan::translate(). Because we could end up with negative values, as the positioned items do not create implicit tracks per spec.

Grammar is not completely right here I think. You should remove the dot after translate() and lowercase "because".

> Source/WebCore/rendering/RenderGrid.cpp:1518
> +    LayoutUnit end = endIsAuto ? isForColumns ? logicalWidth() : logicalHeight() : isForColumns ?  m_columnPositions[finalPosition] : m_rowPositions[finalPosition];

This is rather difficult to read, mind adding some parentheses to clarify the scopes here?

Although I like the ternary operation very much, I think this perhaps deserves a more verbose syntax.

[Build Bot]

Comment on attachment 273347 [details]
Patch

Attachment 273347 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/946581

New failing tests:
js/function-apply.html

[Build Bot]

Created attachment 273419 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Manuel Rego Casasnovas]

Created attachment 273421 [details]
Patch for landing.

[Manuel Rego Casasnovas]

Thanks for the quick review!

Applied suggested changes.

[WebKit Commit Bot]

Comment on attachment 273421 [details]
Patch for landing.

Clearing flags on attachment: 273421

Committed r197857: <http://trac.webkit.org/changeset/197857>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.