Bug 106474

Created attachment 181958 [details] RFC patch.

After talking with Ojan, min-content and max-content in the row direction should resolve to the logical height (per http://dev.w3.org/csswg/css3-sizing/). Following this, I went ahead and extended the change for logical height too. Re-purposing the bug to cover both directions.

Created attachment 182667 [details] Proposed implementation: Handle both direction, the orthogonal writing mode (including testing) is postponed to a follow-up bug.

Comment on attachment 182667 [details] Proposed implementation: Handle both direction, the orthogonal writing mode (including testing) is postponed to a follow-up bug. Attachment 182667 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15884359

Comment on attachment 182667 [details] Proposed implementation: Handle both direction, the orthogonal writing mode (including testing) is postponed to a follow-up bug. Attachment 182667 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15838729

Comment on attachment 182667 [details] Proposed implementation: Handle both direction, the orthogonal writing mode (including testing) is postponed to a follow-up bug. Attachment 182667 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15869711

Comment on attachment 182667 [details] Proposed implementation: Handle both direction, the orthogonal writing mode (including testing) is postponed to a follow-up bug. Attachment 182667 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15845754

Comment on attachment 182667 [details] Proposed implementation: Handle both direction, the orthogonal writing mode (including testing) is postponed to a follow-up bug. Attachment 182667 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15870757

Created attachment 182684 [details] Proposed fix 2: Fixed the release build.

Comment on attachment 182684 [details] Proposed fix 2: Fixed the release build. Attachment 182684 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15884413

Created attachment 182808 [details] Proposed implementation 3: Fixed the build for real this time.

Comment on attachment 182808 [details] Proposed implementation 3: Fixed the build for real this time. View in context: https://bugs.webkit.org/attachment.cgi?id=182808&action=review I just have nits, seems like a fine first pass. I'll let Ojan set the r? flag after his review. > Source/WebCore/rendering/RenderGrid.cpp:51 > + if (m_maxBreadth == infinity) > + return m_usedBreadth; > + > + return m_maxBreadth; Nit: I would probably use a ternary here, but this is also OK. > Source/WebCore/rendering/RenderGrid.cpp:196 > + // Per the spec, we return 0 if the track length is min-content, max-content or a fraction. Nit: Rather than a comment, I would put an assert here that track length is min-content, max-content or a fraction (well, I guess we don't support fraction yet). > Source/WebCore/rendering/RenderGrid.cpp:204 > + if (trackLength.isFixed() || trackLength.isPercent() || trackLength.isViewportPercentage()) { > + LayoutUnit computedBreadth = computeUsedBreadthOfSpecifiedLength(direction, trackLength); > + ASSERT(computedBreadth != infinity); If a large fixed value or percent is used, couldn't the computedBreadth == infinity? > Source/WebCore/rendering/RenderGrid.cpp:214 > + // FIXME: We still need to support calc() here (bug 103761). Nit: Using a full link for the bug would be nice. > Source/WebCore/rendering/RenderGrid.cpp:253 > + // FIXME: Implement 3.a.ii and 3.b.ii. Nit: I would replace 3.a.ii and 3.b.ii with a textual description of what should happen. > LayoutTests/fast/css-grid-layout/minmax-min-content-column-resolution-columns.html:12 > + display: -webkit-grid; > + background-color: grey; > + -webkit-grid-columns: minmax(-webkit-min-content, 40px); > + -webkit-grid-rows: 50px; This would be a great time to add a resources/grid.css file and put the -webkit-grid values in it so we can handle new values easier. > LayoutTests/fast/css-grid-layout/minmax-min-content-column-resolution-columns.html:45 > +<p>Test that resolving minmax function with min-content on grid items works properly with different writing modes.</p> You mention different writing modes, but all the tests seem to be the same writing mode (verticalRL is unused). I think it's fine to either check in failing tests or add them in a follow up. > LayoutTests/fast/css-grid-layout/minmax-min-content-column-resolution-rows.html:45 > +<p>Test that resolving minmax function with min-content on grid items works properly with different writing modes.</p> Same here.

Comment on attachment 182808 [details] Proposed implementation 3: Fixed the build for real this time. View in context: https://bugs.webkit.org/attachment.cgi?id=182808&action=review Overall design looks good. Some questions on the details. > Source/WebCore/rendering/RenderGrid.cpp:209 > + if (trackLength.isFixed() || trackLength.isPercent() || trackLength.isViewportPercentage()) { > + LayoutUnit computedBreadth = computeUsedBreadthOfSpecifiedLength(direction, trackLength); > + ASSERT(computedBreadth != infinity); > + return computedBreadth; > + } > + > + // Per the spec we return Infinity if the track length is min-content, max-content or a fraction. > + return infinity; FWIW, the way RenderBox handles code like this is to do something like the following: LayoutUnit RenderGrid::computeUsedBreadthOfSpecifiedLength(...) { if (trackLength.isFixed() || ...) return valueForLength(...); return -1; } LayoutUnit RenderGrid::computeUsedBreadthOfMaxLength() { LayoutUnit computedBreadth = computeUsedBreadthOfSpecifiedLength(...); if (computedBreadth == -1) return infinity; return computedBreadth; } The nice thing about doing it this way is that you share more code and as you add other value types to computeUsedBreadthOfSpecifiedLength, you don't need to modify computeUsedBreadthOfMaxLength and computeUsedBreadthOfMinLength. > Source/WebCore/rendering/RenderGrid.cpp:229 > + return LayoutUnit(); Nit: I'd just return 0 here. > Source/WebCore/rendering/RenderGrid.cpp:232 > + return child->minPreferredLogicalWidth(); What should this return if the child has a fixed width/min-width/max-width set directly on it? If it should ignore them, then you may need to use minIntrsinsicLogicalWidth here. > Source/WebCore/rendering/RenderGrid.cpp:239 > + // FIXME: We shouldn't unconditionally relayout the child but detect a size change. > + child->setNeedsLayout(true, MarkOnlyThis); I don't understand why we need this here. We just did a layout. If we are going to setNeedsLayout, I think it probably belongs somewhere else in the code (i.e. the place where we modify the width/height such that the child now needs a layout). > Source/WebCore/rendering/RenderGrid.cpp:257 > + // FIXME: We should share this logical with the other branches. This sentence is missing some words. Not sure what's it's trying to say. > Source/WebCore/rendering/RenderGrid.cpp:259 > + size_t cellIndex = resolveGridPosition((direction == ForColumns) ? child->style()->gridItemColumn() : child->style()->gridItemRow()); Can we make a version of resolveGridPosition that takes a direction and a RenderObject* child? I think that would make this code here and below a bit more readable.

Comment on attachment 182808 [details] Proposed implementation 3: Fixed the build for real this time. View in context: https://bugs.webkit.org/attachment.cgi?id=182808&action=review >> Source/WebCore/rendering/RenderGrid.cpp:204 >> + ASSERT(computedBreadth != infinity); > > If a large fixed value or percent is used, couldn't the computedBreadth == infinity? It's entirely possible indeed and probably will occur more frequently with saturated arithmetic enabled on some platforms. It's difficult to use other values (e.g the magic constant -1) as the spec allows a <length> (an arbitrary number with a unit per http://www.w3.org/TR/2012/CR-css3-values-20120828/#lengths) which mean that any values can be potentially hit. We could potentially bring this back to the specification as I couldn't see of a good use case for negative values. Also AFAICT the code would still behave correctly as we still check the Length's type in the layout code. The ASSERT can be removed as it can be hit and doesn't provide much coverage. >> Source/WebCore/rendering/RenderGrid.cpp:209 >> + return infinity; > > FWIW, the way RenderBox handles code like this is to do something like the following: > LayoutUnit RenderGrid::computeUsedBreadthOfSpecifiedLength(...) > { > if (trackLength.isFixed() || ...) > return valueForLength(...); > return -1; > } > > LayoutUnit RenderGrid::computeUsedBreadthOfMaxLength() > { > LayoutUnit computedBreadth = computeUsedBreadthOfSpecifiedLength(...); > if (computedBreadth == -1) > return infinity; > return computedBreadth; > } > > The nice thing about doing it this way is that you share more code and as you add other value types to computeUsedBreadthOfSpecifiedLength, you don't need to modify computeUsedBreadthOfMaxLength and computeUsedBreadthOfMinLength. Sounds good, as long as we never expose -1, it should be fine (some of the code uses MaxBreadth as-is which requires us to have a really infinite value). >> Source/WebCore/rendering/RenderGrid.cpp:239 >> + child->setNeedsLayout(true, MarkOnlyThis); > > I don't understand why we need this here. We just did a layout. If we are going to setNeedsLayout, I think it probably belongs somewhere else in the code (i.e. the place where we modify the width/height such that the child now needs a layout). This was the purpose of the FIXME here. We should probably check the sizes when we constrain the child and check if there is some difference before doing the layout. Let me see if I can find some better check. >> Source/WebCore/rendering/RenderGrid.cpp:253 >> + // FIXME: Implement 3.a.ii and 3.b.ii. > > Nit: I would replace 3.a.ii and 3.b.ii with a textual description of what should happen. Those steps are just a call to ResolveContentBasedTrackSizingFunctionsForItems with different arguments so it's pretty difficult to present them. I could probably sum it up as: // FIXME: Implement MaxContent handling. >> Source/WebCore/rendering/RenderGrid.cpp:257 >> + // FIXME: We should share this logical with the other branches. > > This sentence is missing some words. Not sure what's it's trying to say. It's a typo: s/logical/logic/ Maybe in better English: // FIXME: We should share the growing / iterating logic as done with ResolveContentBasedTrackSizingFunctionsForItems in the specification. >> Source/WebCore/rendering/RenderGrid.cpp:259 >> + size_t cellIndex = resolveGridPosition((direction == ForColumns) ? child->style()->gridItemColumn() : child->style()->gridItemRow()); > > Can we make a version of resolveGridPosition that takes a direction and a RenderObject* child? I think that would make this code here and below a bit more readable. Sounds good. >> LayoutTests/fast/css-grid-layout/minmax-min-content-column-resolution-columns.html:12 >> + -webkit-grid-rows: 50px; > > This would be a great time to add a resources/grid.css file and put the -webkit-grid values in it so we can handle new values easier. It's the second time you told me about that. The first time went through the crack so I filed bug 107044 about it as it's a super good idea but orthogonal to this bug. >> LayoutTests/fast/css-grid-layout/minmax-min-content-column-resolution-columns.html:45 >> +<p>Test that resolving minmax function with min-content on grid items works properly with different writing modes.</p> > > You mention different writing modes, but all the tests seem to be the same writing mode (verticalRL is unused). I think it's fine to either check in failing tests or add them in a follow up. I would prefer to follow-up. The code currently disable orthogonal writing-mode so we will need to add the required writing mode testing.

Comment on attachment 182808 [details] Proposed implementation 3: Fixed the build for real this time. View in context: https://bugs.webkit.org/attachment.cgi?id=182808&action=review >>> Source/WebCore/rendering/RenderGrid.cpp:239 >>> + child->setNeedsLayout(true, MarkOnlyThis); >> >> I don't understand why we need this here. We just did a layout. If we are going to setNeedsLayout, I think it probably belongs somewhere else in the code (i.e. the place where we modify the width/height such that the child now needs a layout). > > This was the purpose of the FIXME here. We should probably check the sizes when we constrain the child and check if there is some difference before doing the layout. Let me see if I can find some better check. It looks to me like we're missing a setNeedsLayout in layoutGridItems around line 234.

Comment on attachment 182808 [details] Proposed implementation 3: Fixed the build for real this time. View in context: https://bugs.webkit.org/attachment.cgi?id=182808&action=review >>> Source/WebCore/rendering/RenderGrid.cpp:204 >>> + ASSERT(computedBreadth != infinity); >> >> If a large fixed value or percent is used, couldn't the computedBreadth == infinity? > > It's entirely possible indeed and probably will occur more frequently with saturated arithmetic enabled on some platforms. > > It's difficult to use other values (e.g the magic constant -1) as the spec allows a <length> (an arbitrary number with a unit per http://www.w3.org/TR/2012/CR-css3-values-20120828/#lengths) which mean that any values can be potentially hit. We could potentially bring this back to the specification as I couldn't see of a good use case for negative values. > > Also AFAICT the code would still behave correctly as we still check the Length's type in the layout code. The ASSERT can be removed as it can be hit and doesn't provide much coverage. I raised http://lists.w3.org/Archives/Public/www-style/2013Jan/0216.html about allowing negative <track-breadth>. As it's an external dependency, I filed bug 107053 to cover changing the value of infinity depending on what the resolution is. >>>> Source/WebCore/rendering/RenderGrid.cpp:239 >>>> + child->setNeedsLayout(true, MarkOnlyThis); >>> >>> I don't understand why we need this here. We just did a layout. If we are going to setNeedsLayout, I think it probably belongs somewhere else in the code (i.e. the place where we modify the width/height such that the child now needs a layout). >> >> This was the purpose of the FIXME here. We should probably check the sizes when we constrain the child and check if there is some difference before doing the layout. Let me see if I can find some better check. > > It looks to me like we're missing a setNeedsLayout in layoutGridItems around line 234. This change would be a lot broader but it would cover dynamic changes to -webkit-grid-rows / -webkit-grid-columns. Unfortunately we don't have dynamic tests that cover this and that's why it went unnoticed. Will add more testing for that.

Comment on attachment 182808 [details] Proposed implementation 3: Fixed the build for real this time. View in context: https://bugs.webkit.org/attachment.cgi?id=182808&action=review >>> Source/WebCore/rendering/RenderGrid.cpp:209 >>> + return infinity; >> >> FWIW, the way RenderBox handles code like this is to do something like the following: >> LayoutUnit RenderGrid::computeUsedBreadthOfSpecifiedLength(...) >> { >> if (trackLength.isFixed() || ...) >> return valueForLength(...); >> return -1; >> } >> >> LayoutUnit RenderGrid::computeUsedBreadthOfMaxLength() >> { >> LayoutUnit computedBreadth = computeUsedBreadthOfSpecifiedLength(...); >> if (computedBreadth == -1) >> return infinity; >> return computedBreadth; >> } >> >> The nice thing about doing it this way is that you share more code and as you add other value types to computeUsedBreadthOfSpecifiedLength, you don't need to modify computeUsedBreadthOfMaxLength and computeUsedBreadthOfMinLength. > > Sounds good, as long as we never expose -1, it should be fine (some of the code uses MaxBreadth as-is which requires us to have a really infinite value). I have to withdraw this comment. As the spec allows trackLength to be negative, nothing prevents valueForLength to return -1. Under your proposal, the code will misbehave in this case which it doesn't here. This is the same issue as infinity being set to -1 which I postponed to a bug 107053. >> Source/WebCore/rendering/RenderGrid.cpp:232 >> + return child->minPreferredLogicalWidth(); > > What should this return if the child has a fixed width/min-width/max-width set directly on it? If it should ignore them, then you may need to use minIntrsinsicLogicalWidth here. That's a very good question. The specification is silent on this point. I would have taken width/min-width/max-width into account but that seems to violate http://dev.w3.org/csswg/css3-sizing/#intrinsic-sizing. As we decided to interpret min-content / max-content as defined in css3 sizing, it would be nice to be consistent or have the spec be precise here so I raised http://lists.w3.org/Archives/Public/www-style/2013Jan/0245.html about that.

Created attachment 183340 [details] Proposed implementation 4: Updated after Ojan & Tony's comments.

Comment on attachment 183340 [details] Proposed implementation 4: Updated after Ojan & Tony's comments. View in context: https://bugs.webkit.org/attachment.cgi?id=183340&action=review LGTM, but I'll let Ojan add any additional comments. > Source/WebCore/rendering/RenderGrid.cpp:201 > + ASSERT(computedBreadth != infinity); Nit: Since it's possible to hit this ASSERT, I would remove it. Maybe replace it with a FIXME? > Source/WebCore/rendering/RenderGrid.cpp:256 > + for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) { > + size_t cellIndex = resolveGridPosition(direction, child); > + if (cellIndex != i) > + continue; This nested loop looks like it could be slow if there are a large number of tracks and children. We may need to make a map, but this is OK for now.

Comment on attachment 183340 [details] Proposed implementation 4: Updated after Ojan & Tony's comments. View in context: https://bugs.webkit.org/attachment.cgi?id=183340&action=review > Source/WebCore/rendering/RenderGrid.cpp:205 > + ASSERT(trackLength.isMinContent() || trackLength.isMaxContent() || trackLength.isAuto()); Can it not be fit-content, fill-available, etc? > Source/WebCore/rendering/RenderGrid.cpp:229 > + if (direction == ForColumns) > + return child->minPreferredLogicalWidth(); Nit: would be prefer to see a FIXME here to track whether this should return the preferred width or the intrinsic width.

Comment on attachment 183340 [details] Proposed implementation 4: Updated after Ojan & Tony's comments. View in context: https://bugs.webkit.org/attachment.cgi?id=183340&action=review >> Source/WebCore/rendering/RenderGrid.cpp:201 >> + ASSERT(computedBreadth != infinity); > > Nit: Since it's possible to hit this ASSERT, I would remove it. Maybe replace it with a FIXME? Sounds good. >> Source/WebCore/rendering/RenderGrid.cpp:205 >> + ASSERT(trackLength.isMinContent() || trackLength.isMaxContent() || trackLength.isAuto()); > > Can it not be fit-content, fill-available, etc? No, we only allow <length> | <percentage> | min-content | max-content for trackLength (see CSSParser::parseGridBreadth). We also allow 'auto' as we copy the 'auto' value to both of the minmax's argument in StyleResolver (createGridTrackMinMax). >> Source/WebCore/rendering/RenderGrid.cpp:229 >> + return child->minPreferredLogicalWidth(); > > Nit: would be prefer to see a FIXME here to track whether this should return the preferred width or the intrinsic width. OK, I will link to the www-style archives. >> Source/WebCore/rendering/RenderGrid.cpp:256 >> + continue; > > This nested loop looks like it could be slow if there are a large number of tracks and children. We may need to make a map, but this is OK for now. You are obviously right and we are not tackling scalability yet. Adding a map is on the radar as it will be required to implement the grid items' automatic placement algorithm.

Created attachment 183527 [details] Patch for landing

Comment on attachment 183527 [details] Patch for landing Clearing flags on attachment: 183527 Committed r140198: <http://trac.webkit.org/changeset/140198>

All reviewed patches have been landed. Closing bug.