Bug 107062 - [CSS Grid Layout] Updating -webkit-grid-rows or -webkit-grid-columns doesn't work as expected
Summary: [CSS Grid Layout] Updating -webkit-grid-rows or -webkit-grid-columns doesn't ...
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 treegraph
 
Reported: 2013-01-16 16:42 PST by Julien Chaffraix
Modified: 2013-01-17 14:36 PST (History)
7 users (show)

See Also:


Attachments
Proposed fix 1: Force a relayout if the grid area size changed and fixed a boolean logic in the style diff method. (12.30 KB, patch)
2013-01-16 17:34 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (11.92 KB, patch)
2013-01-17 11:10 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2013-01-16 16:42:32 PST
Hinted by Ojan in https://bugs.webkit.org/show_bug.cgi?id=106474#c15.

To reproduce, just try to update either properties and watch as we:
* don't properly trigger a layout.
* don't relayout the grid items, leaving them with a wrong size.

Taking this bug as it will likely help me solve bug 106474.
Comment 1 Julien Chaffraix 2013-01-16 17:34:35 PST
Created attachment 183071 [details]
Proposed fix 1: Force a relayout if the grid area size changed and fixed a boolean logic in the style diff method.
Comment 2 Tony Chang 2013-01-17 10:54:27 PST
Comment on attachment 183071 [details]
Proposed fix 1: Force a relayout if the grid area size changed and fixed a boolean logic in the style diff method.

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

> Source/WebCore/rendering/RenderGrid.cpp:236
> +            LayoutUnit oldOverrideContainingBlockContentLogicalWidth = child->hasOverrideContainingBlockLogicalWidth() ? child->overrideContainingBlockContentLogicalWidth() : LayoutUnit();
> +            LayoutUnit oldOverrideContainingBlockContentLogicalHeight = child->hasOverrideContainingBlockLogicalHeight() ? child->overrideContainingBlockContentLogicalHeight() : LayoutUnit();
> +            LayoutUnit newOverrideContainingBlockContentLogicalWidth = columnTracks[columnTrack].m_usedBreadth;
> +            LayoutUnit newOverrideContainingBlockContentLogicalHeight = rowTracks[rowTrack].m_usedBreadth;

This is pretty hard to read.  How about:
LayoutUnit oldOverrideContainingBlockContentLogicalWidth = child->hasOverrideContainingBlockLogicalWidth() ? child->overrideContainingBlockContentLogicalWidth() : LayoutUnit();
LayoutUnit oldOverrideContainingBlockContentLogicalHeight = child->hasOverrideContainingBlockLogicalHeight() ? child->overrideContainingBlockContentLogicalHeight() : LayoutUnit();

bool widthOrHeightChanged = oldOverrideContainingBlockContentLogicalWidth != columnTracks[columnTrack].m_usedBreadth || oldOverrideContainingBlockContentLogicalHeight != rowTracks[rowTrack].m_usedBreadth;
if (widthOrHeightChanged)
    child->setNeedsLayout(true, MarkOnlyThis);

child->setOverride...
child->setOverride...
Comment 3 Julien Chaffraix 2013-01-17 11:01:45 PST
Comment on attachment 183071 [details]
Proposed fix 1: Force a relayout if the grid area size changed and fixed a boolean logic in the style diff method.

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

>> Source/WebCore/rendering/RenderGrid.cpp:236
>> +            LayoutUnit newOverrideContainingBlockContentLogicalHeight = rowTracks[rowTrack].m_usedBreadth;
> 
> This is pretty hard to read.  How about:
> LayoutUnit oldOverrideContainingBlockContentLogicalWidth = child->hasOverrideContainingBlockLogicalWidth() ? child->overrideContainingBlockContentLogicalWidth() : LayoutUnit();
> LayoutUnit oldOverrideContainingBlockContentLogicalHeight = child->hasOverrideContainingBlockLogicalHeight() ? child->overrideContainingBlockContentLogicalHeight() : LayoutUnit();
> 
> bool widthOrHeightChanged = oldOverrideContainingBlockContentLogicalWidth != columnTracks[columnTrack].m_usedBreadth || oldOverrideContainingBlockContentLogicalHeight != rowTracks[rowTrack].m_usedBreadth;
> if (widthOrHeightChanged)
>     child->setNeedsLayout(true, MarkOnlyThis);
> 
> child->setOverride...
> child->setOverride...

No issue with transforming the code, will change it before landing.
Comment 4 Julien Chaffraix 2013-01-17 11:10:26 PST
Created attachment 183220 [details]
Patch for landing
Comment 5 WebKit Review Bot 2013-01-17 14:36:50 PST
Comment on attachment 183220 [details]
Patch for landing

Clearing flags on attachment: 183220

Committed r140045: <http://trac.webkit.org/changeset/140045>
Comment 6 WebKit Review Bot 2013-01-17 14:36:53 PST
All reviewed patches have been landed.  Closing bug.