Summary: | Position grid items by row/column index | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tony Chang <tony> | ||||||||
Component: | New Bugs | Assignee: | Tony Chang <tony> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | eric, gustavo, jchaffraix, ojan, webkit.review.bot, xan.lopez | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 60731 | ||||||||||
Attachments: |
|
Description
Tony Chang
2012-07-13 15:44:44 PDT
Created attachment 152363 [details]
Patch
First patch for grid layout! It's very basic and only handles fixed size tracks and doesn't resize grid items. Comment on attachment 152363 [details] Patch Attachment 152363 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13235287 Comment on attachment 152363 [details] Patch Attachment 152363 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13237291 Created attachment 152377 [details]
Patch
Comment on attachment 152377 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152377&action=review Looks great! Just a bunch of request for FIXMEs. :) > Source/WebCore/rendering/RenderGrid.cpp:43 > + LayoutUnit m_usedBreadth; I'm not a huge fan of usedBreadth for this. How about just m_size? Then you'd have computeSizeOfGridTracks, which seems very natural to me. Ugh, I see now that this is matching the terminology in the spec. I suppose that makes sense and will make development/maintenance easier. Can you add a comment to that effect in the ChangeLog? > Source/WebCore/rendering/RenderGrid.cpp:57 > +void RenderGrid::layoutBlock(bool relayoutChildren, LayoutUnit) Lol. This looks familiar. At some point we should step back and see if RenderFlexibleBox and RenderGrid could share more of this code with RenderBlock. There's just too much boilerplate. Add a FIXME? > Source/WebCore/rendering/RenderGrid.cpp:81 > + // For overflow:scroll blocks, ensure we have both scrollbars in place always. Meh to this useless comment. > Source/WebCore/rendering/RenderGrid.cpp:148 > + Extra line break. > Source/WebCore/rendering/RenderGrid.cpp:151 > + // FIXME: Handle border & padding on the grid element. Duplicated comment with line 146. > Source/WebCore/rendering/RenderGrid.cpp:159 > + Length column = child->style()->gridItemColumn(); > + Length row = child->style()->gridItemRow(); Length doesn't seem like the right type for these. From the spec: [[<integer>|<string>|start][<integer>|<string>|end]?]|auto. Not sure what the type should be, but it's certainly not Length. We might need to define a new type. Add a FIXME? > Source/WebCore/rendering/RenderGrid.cpp:161 > + if (!column.isPositive() || !row.isPositive()) The spec doesn't seem to cover what to do with non-positive values, but specifies the type as integer...so non-positive is allowed. Not sure if there's a use-case. Either way, we should bring it up with the CSSWG. If non-positive isn't allowed, we should be clamping these to 1 in the CSSParser, right? For now, can you add a FIXME? > Source/WebCore/rendering/RenderGrid.cpp:169 > + size_t columnTrack = static_cast<size_t>(column.getFloatValue()) - 1; > + size_t rowTrack = static_cast<size_t>(row.getFloatValue()) - 1; These should just use intValue() and ints instead of size_t's, no? Created attachment 152574 [details]
Patch for landing
Comment on attachment 152377 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152377&action=review >> Source/WebCore/rendering/RenderGrid.cpp:151 >> + // FIXME: Handle border & padding on the grid element. > > Duplicated comment with line 146. We have to handle it in both places and I didn't want to forget. >> Source/WebCore/rendering/RenderGrid.cpp:159 >> + Length row = child->style()->gridItemRow(); > > Length doesn't seem like the right type for these. From the spec: [[<integer>|<string>|start][<integer>|<string>|end]?]|auto. > > Not sure what the type should be, but it's certainly not Length. We might need to define a new type. Add a FIXME? Right, it shouldn't be a Length, but that's what we're currently using to hold auto or integer. We can fix this in a follow up (once we can parse more than just auto and ints. >> Source/WebCore/rendering/RenderGrid.cpp:169 >> + size_t rowTrack = static_cast<size_t>(row.getFloatValue()) - 1; > > These should just use intValue() and ints instead of size_t's, no? Switched to intValue, but still using size_t because {column,row}Tracks.size() returns a size_t. These should both be unsigned since we checked isPositive above. Comment on attachment 152574 [details] Patch for landing Clearing flags on attachment: 152574 Committed r122747: <http://trac.webkit.org/changeset/122747> All reviewed patches have been landed. Closing bug. |