Bug 113546

Created attachment 195685 [details] Proposed fix 1: Update our handling, our tests. Doesn't handle the start/before case as it could grow the grid.

Comment on attachment 195685 [details] Proposed fix 1: Update our handling, our tests. Doesn't handle the start/before case as it could grow the grid. View in context: https://bugs.webkit.org/attachment.cgi?id=195685&action=review > Source/WebCore/rendering/RenderGrid.cpp:338 > + // Negative explicit values are considered positive for the purpose of estimating our size. > + // See http://lists.w3.org/Archives/Public/www-style/2013Mar/0593.html I don't think that's what the email is saying. I think it's saying if there are 3 grid columns and the position is -1, translate the position to 3, not 1.

Comment on attachment 195685 [details] Proposed fix 1: Update our handling, our tests. Doesn't handle the start/before case as it could grow the grid. View in context: https://bugs.webkit.org/attachment.cgi?id=195685&action=review >> Source/WebCore/rendering/RenderGrid.cpp:338 >> + // See http://lists.w3.org/Archives/Public/www-style/2013Mar/0593.html > > I don't think that's what the email is saying. I think it's saying if there are 3 grid columns and the position is -1, translate the position to 3, not 1. Confirmed with Tab that you are totally right, I misread the answer. The specification was updated yesterday so let me loop back on it for this patch.

Created attachment 196033 [details] Proposed change 2: Updated to better match Tab's answers.

Comment on attachment 196033 [details] Proposed change 2: Updated to better match Tab's answers. View in context: https://bugs.webkit.org/attachment.cgi?id=196033&action=review > Source/WebCore/ChangeLog:10 > + to always resolve against the start/before edge (the previous code would resolve end/after > + grid lines resolve against the end/after edge). I think you have one too many resolves here. > Source/WebCore/rendering/RenderGrid.cpp:776 > + // FIXME: This returns the wrong result for side == StartSide or BeforeSide (off-by-one) but > + // that avoids the issue of growing the grid due to e.g. -1 / auto. I don't really understand this comment. Can you explain it a bit more? Here or in the ChangeLog is fine. > Source/WebCore/rendering/RenderGrid.cpp:778 > + // Per http://lists.w3.org/Archives/Public/www-style/2013Mar/0589.html, we should Typically, we put links to email threads or specs in the ChangeLog. It'll be there in the blame history should someone try to understand why these lines were added. I don't think this line really requires an explanatory comment in addition to the explanation in the ChangeLog. In the limit, nearly every line of code in this file could point to a bit of the specification, which would obviously be unwieldy.

Comment on attachment 196033 [details] Proposed change 2: Updated to better match Tab's answers. View in context: https://bugs.webkit.org/attachment.cgi?id=196033&action=review >> Source/WebCore/rendering/RenderGrid.cpp:776 >> + // that avoids the issue of growing the grid due to e.g. -1 / auto. > > I don't really understand this comment. Can you explain it a bit more? Here or in the ChangeLog is fine. Sure, this function converts a grid line number into a grid column or row (ie a grid track which is represented as an index into grid representation). Let me take the example of -1 to explain the issue: -1 refers to the last grid line, the line that is *after* the last grid column / row. For grid-{end|after}, it is fine as it means we stop at this line, but for grid-{start|before}, it means we start after the last grid line. What this means is that -1 for grid-{start|before} can end up growing the grid, which we currently don't support. Now, you can apply the previous reasoning to other negative value and will find that our result is actually one less than the expected result (-2 should be the last grid column / row but is the penultimate, etc...). > Source/WebCore/rendering/RenderGrid.cpp:777 > + const size_t endOfTrack = (side == StartSide || side == EndSide) ? gridColumnCount() : gridRowCount(); I realized that this is not right: we need to count from the explicit grid's size which doesn't match this line. >> Source/WebCore/rendering/RenderGrid.cpp:778 >> + // Per http://lists.w3.org/Archives/Public/www-style/2013Mar/0589.html, we should > > Typically, we put links to email threads or specs in the ChangeLog. It'll be there in the blame history should someone try to understand why these lines were added. I don't think this line really requires an explanatory comment in addition to the explanation in the ChangeLog. > > In the limit, nearly every line of code in this file could point to a bit of the specification, which would obviously be unwieldy. Sounds good, this hasn't made it into the specification yet but it will probably at some point.

Created attachment 196193 [details] Updated change: Same as previously but properly resolve against the explicit grid.

Comment on attachment 196193 [details] Updated change: Same as previously but properly resolve against the explicit grid. View in context: https://bugs.webkit.org/attachment.cgi?id=196193&action=review Would like to understand why we're adding the member variables before r+. > Source/WebCore/rendering/RenderGrid.cpp:348 > +void RenderGrid::computeExplicitGridSizes() > { > - const Vector<GridTrackSize>& trackStyles = (direction == ForColumns) ? style()->gridColumns() : style()->gridRows(); > + m_explicitGridRowCount = style()->gridRows().size(); > + m_explicitGridColumnCount = style()->gridColumns().size(); > +} Why do we need this? Why not just use style()->gridRows().size() and style()->gridColumns().size() directly? > Source/WebCore/rendering/RenderGrid.cpp:783 > + // FIXME: This returns one less than the expected result for side == StartSide or BeforeSide as we don't properly convert > + // the grid line to its grid track. However this avoids the issue of growing the grid when inserting the item (e.g. -1 / auto). I now understand this...I think the spec is dumb. I've nagged Tab and fantasai about this on IRC. In either case, this is fine for now.

Comment on attachment 196193 [details] Updated change: Same as previously but properly resolve against the explicit grid. View in context: https://bugs.webkit.org/attachment.cgi?id=196193&action=review >> Source/WebCore/rendering/RenderGrid.cpp:348 >> +} > > Why do we need this? Why not just use style()->gridRows().size() and style()->gridColumns().size() directly? Currently it is actually unneeded. In the longer term, we will need to aggregate the result of 2 style properties: grid-definition-{rows|columns} and grid-template. Thinking again, it's probably better to just add wrappers and wait until we need to cache this information (if at all).

Created attachment 204757 [details] Patch

Seems this patch has been landed in blink so I would like to merge here.

Comment on attachment 204757 [details] Patch Attachment 204757 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/916056 New failing tests: webaudio/convolution-mono-mono.html

Created attachment 204759 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3

(In reply to comment #12) > (From update of attachment 204757 [details]) > Attachment 204757 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/916056 > > New failing tests: > webaudio/convolution-mono-mono.html I don't think this patch affects to webaudio stuff.

Any chance to get this reviewed? Looks like the test failure in EWS is totally unrelated.

(In reply to comment #15) > Any chance to get this reviewed? Looks like the test failure in EWS is totally unrelated. If I am not wrong the patch just need to replace StartSide and EndSide by ColumnStartSide and ColumnEndSide which were recently renamed to fit latest spec changes.

Comment on attachment 204757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=204757&action=review Added a couple of comments to help rebasing the patch against the latest renames in the names of some properties. > Source/WebCore/rendering/RenderGrid.cpp:784 > + const size_t endOfTrack = (side == StartSide || side == EndSide) ? explicitGridColumnCount() : explicitGridRowCount(); These are now ColumnStartSide and ColumnEndSide > LayoutTests/fast/css-grid-layout/grid-auto-flow-resolution.html:29 > +/* These 2 classes forces the grid to be sized after the grid-{end|after}, thus end up in the Replace grid-{end|after} by the new names, i.e., grid-{column|row}-end > LayoutTests/fast/css-grid-layout/grid-item-negative-integer-explicit-grid-resolution.html:11 > + /* -webkit-grid-rows is left unset so that the grid items' row is implicit. */ These are now -webkit-grid-definition-columns and -webkit-grid-definition-rows. > LayoutTests/fast/css-grid-layout/grid-item-negative-integer-explicit-grid-resolution.html:17 > + /* -webkit-grid-columns is left unset so that the grid items' column is implicit. */ Ditto.

@svillar: FOA, thanks for having interest on this bug. Unfortunately, I don't have plan to continue this work in nearest future. Furthermore, seems you are working on grid layout these days. Therefore, considering my personal reason and keep consistency on implementation, would you take this bug and continue work on patch? :)

(In reply to comment #18) > @svillar: > FOA, thanks for having interest on this bug. > Unfortunately, I don't have plan to continue this work in nearest future. > Furthermore, seems you are working on grid layout these days. > Therefore, considering my personal reason and keep consistency on implementation, would you take this bug and continue work on patch? :) Sure, I can take over it

Created attachment 208611 [details] Patch

Created attachment 208612 [details] Patch Now rebased against master

Committed r154044: <http://trac.webkit.org/changeset/154044>