Bug 104818 - [CSS Grid Layout] Include paddings and borders into the grid element's logical height / width
Summary: [CSS Grid Layout] Include paddings and borders into the grid element's logica...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks: 60731
  Show dependency tree / graph
 
Reported: 2012-12-12 08:58 PST by Julien Chaffraix
Modified: 2012-12-12 18:51 PST (History)
6 users (show)

See Also:

Attachments
Proposed change. (5.23 KB, patch)
2012-12-12 09:05 PST, Julien Chaffraix 		no flags Details | Formatted Diff | Diff
Patch for landing (5.18 KB, patch)
2012-12-12 17:58 PST, Julien Chaffraix 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2012-12-12 08:58:24 PST 
Currently we don't account for paddings and borders on the grid element in computePreferredLogicalWidths and layoutGridItems. This means that we don't compute the right width / height during layout and needs to be fixed.

Patch forthcoming.
Comment 1 Julien Chaffraix 2012-12-12 09:05:07 PST 
Created attachment 179062 [details]
Proposed change.
Comment 2 Tony Chang 2012-12-12 11:14:34 PST 
Comment on attachment 179062 [details]
Proposed change.

View in context: https://bugs.webkit.org/attachment.cgi?id=179062&action=review

> Source/WebCore/rendering/RenderGrid.cpp:203
> +    LayoutUnit borderAndPaddingInBlockDirection = borderAndPaddingLogicalHeight();
> +    setLogicalHeight(logicalHeight() + borderAndPaddingInBlockDirection);

Nit: I would remove the temporary variable.

> LayoutTests/fast/css-grid-layout/grid-element-padding-margin.html:9
> +.grid {
> +    display: -webkit-grid;

We should consider making a .css file in resources that define classes for the various grid properties.  This makes it easier to handle property name changes and makes it possible to make the tests cross platform by adding -ms prefixed properties.  See css3/flexbox/resources/flexbox.css for an example of this.  Would be good as a separate patch.
Comment 3 Julien Chaffraix 2012-12-12 17:52:28 PST 
Comment on attachment 179062 [details]
Proposed change.

View in context: https://bugs.webkit.org/attachment.cgi?id=179062&action=review

>> Source/WebCore/rendering/RenderGrid.cpp:203
>> +    setLogicalHeight(logicalHeight() + borderAndPaddingInBlockDirection);
> 
> Nit: I would remove the temporary variable.

Will do.

>> LayoutTests/fast/css-grid-layout/grid-element-padding-margin.html:9
>> +    display: -webkit-grid;
> 
> We should consider making a .css file in resources that define classes for the various grid properties.  This makes it easier to handle property name changes and makes it possible to make the tests cross platform by adding -ms prefixed properties.  See css3/flexbox/resources/flexbox.css for an example of this.  Would be good as a separate patch.

Filed https://bugs.webkit.org/show_bug.cgi?id=104869 about that.
Comment 4 Julien Chaffraix 2012-12-12 17:58:21 PST 
Created attachment 179171 [details]
Patch for landing
Comment 5 WebKit Review Bot 2012-12-12 18:50:56 PST 
Comment on attachment 179171 [details]
Patch for landing

Clearing flags on attachment: 179171

Committed r137560: <http://trac.webkit.org/changeset/137560>
Comment 6 WebKit Review Bot 2012-12-12 18:51:00 PST 
All reviewed patches have been landed.  Closing bug.