Bug 155199

Summary: [css-grid] Allow to place positioned grid items on the padding
Product: WebKit Reporter: Manuel Rego Casasnovas <rego>
Component: Layout and RenderingAssignee: Manuel Rego Casasnovas <rego>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, jfernandez, kondapallykalyan, rego, simon.fraser, svillar
Priority: P2 Keywords: BlinkMergeCandidate
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 155014    
Bug Blocks: 153488    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews116 for mac-yosemite
none
Patch for landing. none

Manuel Rego Casasnovas
Reported 2016-03-08 15:15:42 PST
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/
Attachments
Patch (21.05 KB, patch)
2016-03-08 15:23 PST, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews116 for mac-yosemite (950.15 KB, application/zip)
2016-03-09 03:05 PST, Build Bot 		no flags
Details
Patch for landing. (21.03 KB, patch)
2016-03-09 04:15 PST, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Manuel Rego Casasnovas
Comment 1 2016-03-08 15:23:40 PST
Created attachment 273347 [details] Patch
Sergio Villar Senin
Comment 2 2016-03-09 02:55:01 PST
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 3 2016-03-09 03:05:40 PST
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
Comment 4 2016-03-09 03:05:45 PST
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
Comment 5 2016-03-09 04:15:52 PST
Created attachment 273421 [details] Patch for landing.
Manuel Rego Casasnovas
Comment 6 2016-03-09 04:17:49 PST
Thanks for the quick review! Applied suggested changes.
WebKit Commit Bot
Comment 7 2016-03-09 06:26:29 PST
Comment on attachment 273421 [details] Patch for landing. Clearing flags on attachment: 273421 Committed r197857: <http://trac.webkit.org/changeset/197857>
WebKit Commit Bot
Comment 8 2016-03-09 06:26:33 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.