The current implementation does not comply with what specs say about the size available for the track sizing algorithm when the grid container is sized under min/max size restrictions and/or when it has intrinsic sizes. Apart from that, we are not properly detecting whether the grid container has definite or indefinite sizes in both inline and block axis.
Created attachment 264323 [details] Patch
Comment on attachment 264323 [details] Patch Attachment 264323 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/354092 New failing tests: fast/events/key-events-in-editable-gridbox.html
Created attachment 264327 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 264323 [details] Patch Attachment 264323 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/354111 New failing tests: fast/events/key-events-in-editable-gridbox.html
Created attachment 264329 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 264323 [details] Patch Attachment 264323 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/354155 New failing tests: fast/events/key-events-in-editable-gridbox.html
Created attachment 264330 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 264323 [details] Patch The changes in the patch look fine, but it looks like this breaks fast/events/key-events-in-editable-gridbox.html -- I think the breakage is real and just not just an anomaly of some sort.
Created attachment 264573 [details] Patch Fixed the contenteditable behavior. I took the chance to improve a bit that test and check more cases (different kinds of heights)
Comment on attachment 264573 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264573&action=review > Source/WebCore/rendering/RenderGrid.cpp:210 > class RenderGrid::GridSizingData { Something that has this much public data should probably be a struct, rather than a class. > Source/WebCore/rendering/RenderGrid.cpp:235 > + void setFreeSpaceForDirection(GridTrackSizingDirection direction, Optional<LayoutUnit> freeSpace) > + { > + if (direction == ForColumns) > + freeSpaceForColumns = freeSpace; > + else > + freeSpaceForRows = freeSpace; > + } Not a big fan of functions like this in the class definition. I think it would be more elegant to put the function definition outside the GridSizingData class. > Source/WebCore/rendering/RenderGrid.cpp:418 > + const_cast<RenderGrid*>(this)->computeUsedBreadthOfGridTracks(ForColumns, sizingData, minLogicalWidth, maxLogicalWidth); Not a huge fan of the const_cast idiom used in this class.
(In reply to comment #10) > Comment on attachment 264573 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=264573&action=review > > > Source/WebCore/rendering/RenderGrid.cpp:210 > > class RenderGrid::GridSizingData { > > Something that has this much public data should probably be a struct, rather > than a class. Ack > > Source/WebCore/rendering/RenderGrid.cpp:235 > > + void setFreeSpaceForDirection(GridTrackSizingDirection direction, Optional<LayoutUnit> freeSpace) > > + { > > + if (direction == ForColumns) > > + freeSpaceForColumns = freeSpace; > > + else > > + freeSpaceForRows = freeSpace; > > + } > > Not a big fan of functions like this in the class definition. I think it > would be more elegant to put the function definition outside the > GridSizingData class. I'll do it > > Source/WebCore/rendering/RenderGrid.cpp:418 > > + const_cast<RenderGrid*>(this)->computeUsedBreadthOfGridTracks(ForColumns, sizingData, minLogicalWidth, maxLogicalWidth); > > Not a huge fan of the const_cast idiom used in this class. Note that RenderBox also needs them for the preferred logical widths computations. All these renderers need to store the computed values but we don't consider them a modification on state of the object.
(In reply to comment #11) > Note that RenderBox also needs them for the preferred logical widths > computations. All these renderers need to store the computed values but we > don't consider them a modification on state of the object. I believe the preferred style is to mark the data members that store the computed values mutable rather than using const_cast.
(In reply to comment #12) > (In reply to comment #11) > > Note that RenderBox also needs them for the preferred logical widths > > computations. All these renderers need to store the computed values but we > > don't consider them a modification on state of the object. > > I believe the preferred style is to mark the data members that store the > computed values mutable rather than using const_cast. Sure, but if you don't mind I'll do it in a separate patch since this one does not add/modify any existing const_cast.
Committed r192154: <http://trac.webkit.org/changeset/192154>