| Summary: | [CSS Grid Layout] computePreferredLogicalWidths doesn't handle minmax tracks | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> |
| Component: | Layout and Rendering | Assignee: | Julien Chaffraix <jchaffraix> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | eric, ojan.autocc, ojan, tony, webkit.review.bot, xan.lopez |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Bug Depends on: | |||
| Bug Blocks: | 103311 | ||
| Attachments: | |||
|
Description
Julien Chaffraix
2013-01-30 16:35:59 PST
Created attachment 185859 [details]
Proposed fix I: Updated computePreferredLogicalWidths to handle all minmax combination.
Comment on attachment 185859 [details]
Proposed fix I: Updated computePreferredLogicalWidths to handle all minmax combination.
I defer computePreferredLogicalWidths to Ojan.
Comment on attachment 185859 [details] Proposed fix I: Updated computePreferredLogicalWidths to handle all minmax combination. View in context: https://bugs.webkit.org/attachment.cgi?id=185859&action=review Just a few minor issues. > Source/WebCore/rendering/RenderGrid.cpp:164 > +LayoutUnit RenderGrid::resolvePreferredLogicalWidth(PreferredWidthType type, size_t trackIndex) const Instead of passing in the type, can we just pass in the Length itself? Then we don't need the enum, which seems better to me. > Source/WebCore/rendering/RenderGrid.cpp:169 > + if (length.isFixed()) > + return length.intValue(); It's not possible to put border/padding/margin on a grid cell, right? If so, it might be worth adding a comment here about that being why it's OK to not worry about them here. > Source/WebCore/rendering/RenderGrid.cpp:178 > + // FIXME: We should include the child's fixed margins like RenderBlock. RenderBlock does crazy with margins. You might want to point to RenderFlexibleBox as a closer example. In fact, I think we probably want to do exactly what RenderFlexibleBox does, which might involve moving marginLogicalWidthForChild up into RenderBlock so it can be shared. > Source/WebCore/rendering/RenderGrid.cpp:198 > + // FIXME: What should we do for the other values (e.g. <percentage> and calc()) > + return 0; Right now, every other class treats anything other than Fixed the same a Auto. The sizing spec says to resolve "definite" lengths, but we don't do that anywhere currently. We should try to and see if we can get away with it without breaking the web, but I've been putting that off until I'm done reducing code duplication across computePreferredLogicalWidth overrides. > LayoutTests/fast/css-grid-layout/grid-preferred-logical-widths.html:23 > +.gridMinMinContent { > + -webkit-grid-columns: minmax(-webkit-min-content, 40px) minmax(-webkit-min-content, 40px); > + -webkit-grid-rows: 10px; > + font: 10px/1 Ahem; > +} > + > +.gridMinMaxContent { > + -webkit-grid-columns: minmax(30px, -webkit-min-content) minmax(30px, -webkit-min-content); > + -webkit-grid-rows: 10px; > + font: 10px/1 Ahem; > +} > + > +.gridMaxContent { Nit: these class names don't seem to use a consistent naming scheme. Comment on attachment 185859 [details] Proposed fix I: Updated computePreferredLogicalWidths to handle all minmax combination. View in context: https://bugs.webkit.org/attachment.cgi?id=185859&action=review > Source/WebCore/rendering/RenderGrid.cpp:173 > + for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) { Also, maybe add a FIXME to come up with a more efficient way of iterating over the grid items in a given track? Comment on attachment 185859 [details] Proposed fix I: Updated computePreferredLogicalWidths to handle all minmax combination. View in context: https://bugs.webkit.org/attachment.cgi?id=185859&action=review >> Source/WebCore/rendering/RenderGrid.cpp:164 >> +LayoutUnit RenderGrid::resolvePreferredLogicalWidth(PreferredWidthType type, size_t trackIndex) const > > Instead of passing in the type, can we just pass in the Length itself? Then we don't need the enum, which seems better to me. I started with that and came to the opposite conclusion :) Will do the change as it's fine either way. >> Source/WebCore/rendering/RenderGrid.cpp:169 >> + return length.intValue(); > > It's not possible to put border/padding/margin on a grid cell, right? If so, it might be worth adding a comment here about that being why it's OK to not worry about them here. It's not possible (and the grid cell is now called grid area) >> Source/WebCore/rendering/RenderGrid.cpp:173 >> + for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) { > > Also, maybe add a FIXME to come up with a more efficient way of iterating over the grid items in a given track? This is at the back of my mind as we are starting to have *a lot* of grid items' iterations in the code. grid-auto-flow is really the point where having a grid representation for the grid items is required to determine efficiently if a grid area is empty or not. >> Source/WebCore/rendering/RenderGrid.cpp:178 >> + // FIXME: We should include the child's fixed margins like RenderBlock. > > RenderBlock does crazy with margins. You might want to point to RenderFlexibleBox as a closer example. In fact, I think we probably want to do exactly what RenderFlexibleBox does, which might involve moving marginLogicalWidthForChild up into RenderBlock so it can be shared. Sounds good. >> Source/WebCore/rendering/RenderGrid.cpp:198 >> + return 0; > > Right now, every other class treats anything other than Fixed the same a Auto. The sizing spec says to resolve "definite" lengths, but we don't do that anywhere currently. We should try to and see if we can get away with it without breaking the web, but I've been putting that off until I'm done reducing code duplication across computePreferredLogicalWidth overrides. Sounds good, will keep the FIXME and expand it to add some of this explanation. Created attachment 185910 [details]
Proposed fix 2: Updated after Ojan's comments.
Comment on attachment 185910 [details] Proposed fix 2: Updated after Ojan's comments. View in context: https://bugs.webkit.org/attachment.cgi?id=185910&action=review > Source/WebCore/rendering/RenderGrid.cpp:164 > +LayoutUnit RenderGrid::resolvePreferredLogicalWidth(const Length& length, size_t trackIndex) const Didn't notice this in the last patch. This might be a bit more consistent with RenderBox if you called this computePreferredTrackWidthUsing or something like that (e.g. see RenderBox::computeLogicalHeightUsing). resolve is a fairly overloaded verb. :) > Source/WebCore/rendering/RenderGrid.cpp:200 > + // (including <percentage> and calc()) but we don't it elsewhere. Missing a word or two here. :) Comment on attachment 185910 [details] Proposed fix 2: Updated after Ojan's comments. View in context: https://bugs.webkit.org/attachment.cgi?id=185910&action=review >> Source/WebCore/rendering/RenderGrid.cpp:164 >> +LayoutUnit RenderGrid::resolvePreferredLogicalWidth(const Length& length, size_t trackIndex) const > > Didn't notice this in the last patch. This might be a bit more consistent with RenderBox if you called this computePreferredTrackWidthUsing or something like that (e.g. see RenderBox::computeLogicalHeightUsing). resolve is a fairly overloaded verb. :) Fair enough. My take would be computePreferredTrackWidth, dropping the Using as I don't see much value in it. Created attachment 186040 [details]
Patch for landing.
Created attachment 186072 [details]
Patch for landing
Comment on attachment 186072 [details] Patch for landing Clearing flags on attachment: 186072 Committed r141616: <http://trac.webkit.org/changeset/141616> All reviewed patches have been landed. Closing bug. |