Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2039223002:
[css-grid] Fix percentage height on grid item's children (Closed)
|
Created:
4 years, 6 months ago by Manuel Rego Modified:
4 years, 6 months ago CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, svillar, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, leviw+renderwatch, jchaffraix+rendering, blink-reviews, eae+blinkwatch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[css-grid] Fix percentage height on grid item's children
Grid items can be vertically stretched in that case
they store its height on LayoutBox::overrideLogicalContentHeight().
In order to resolve the percentage height on the grid item's children
we need to use that size.
BUG=612755
TEST=fast/css-grid-layout/percent-resolution-grid-item-children.html
Committed: https://crrev.com/8df0592ed1d737d3d9db4b704ffe8d033f14ac28
Cr-Commit-Position: refs/heads/master@{#398971}
Patch Set 1 #
Total comments: 5
Patch Set 2 : New version using cbstyle #
Total comments: 2
Patch Set 3 : New version using overrideLogicalContentHeight #
Messages
Total messages: 23 (7 generated)
rego@igalia.com changed reviewers: + cbiesinger@chromium.org, eae@chromium.org, jfernandez@igalia.com, svillar@igalia.com
non-owner LGTM https://codereview.chromium.org/2039223002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2039223002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:2736: } else if (cb->styleRef().logicalHeight().isAuto() && cb->hasOverrideContainingBlockLogicalHeight()) { Nit: better using cbstyle ? If we are computing the "availableHeight" why we are adding logicalHeight().isAuto() to the condition ? Shouldn't we just use that condition to discriminate from the cases below (eg, isFixed ? )
https://codereview.chromium.org/2039223002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2039223002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:2736: } else if (cb->styleRef().logicalHeight().isAuto() && cb->hasOverrideContainingBlockLogicalHeight()) { On 2016/06/06 15:15:54, jfernandez wrote: > Nit: better using cbstyle ? Sure, done. > If we are computing the "availableHeight" why we are adding > logicalHeight().isAuto() to the condition ? Shouldn't we just use that condition > to discriminate from the cases below (eg, isFixed ? ) The problem is that we'd be using the wrong size in that case. E.g. <div style="display: grid; grid: 100px / 200px;"> <div style="height: 400px;"> <div style="height: 50%;">child</div> </div> </div> For the grid item, overrideContainingBlockLogicalHeight() will be 100px (the height of the row). However the item has 400px height, so the 50% of the child should be resolved against those 400px. If I remove the logicalHeight().isAuto() we'd be doing this wrong.
On 2016/06/06 15:55:46, Manuel Rego wrote: > https://codereview.chromium.org/2039223002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > https://codereview.chromium.org/2039223002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/LayoutBox.cpp:2736: } else if > (cb->styleRef().logicalHeight().isAuto() && > cb->hasOverrideContainingBlockLogicalHeight()) { > On 2016/06/06 15:15:54, jfernandez wrote: > > Nit: better using cbstyle ? > > Sure, done. > > > If we are computing the "availableHeight" why we are adding > > logicalHeight().isAuto() to the condition ? Shouldn't we just use that > condition > > to discriminate from the cases below (eg, isFixed ? ) > > The problem is that we'd be using the wrong size in that case. > E.g. > <div style="display: grid; grid: 100px / 200px;"> > <div style="height: 400px;"> > <div style="height: 50%;">child</div> > </div> > </div> > > For the grid item, overrideContainingBlockLogicalHeight() will be 100px > (the height of the row). > However the item has 400px height, so the 50% of the child should be resolved > against those 400px. > If I remove the logicalHeight().isAuto() we'd be doing this wrong. why not moving the new branch after the one checking out whether "isfixed" ?
https://codereview.chromium.org/2039223002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2039223002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:2736: } else if (cb->styleRef().logicalHeight().isAuto() && cb->hasOverrideContainingBlockLogicalHeight()) { On 2016/06/06 15:55:46, Manuel Rego wrote: > On 2016/06/06 15:15:54, jfernandez wrote: > > Nit: better using cbstyle ? > > Sure, done. > > > If we are computing the "availableHeight" why we are adding > > logicalHeight().isAuto() to the condition ? Shouldn't we just use that > condition > > to discriminate from the cases below (eg, isFixed ? ) > > The problem is that we'd be using the wrong size in that case. > E.g. > <div style="display: grid; grid: 100px / 200px;"> > <div style="height: 400px;"> > <div style="height: 50%;">child</div> > </div> > </div> > > For the grid item, overrideContainingBlockLogicalHeight() will be 100px > (the height of the row). > However the item has 400px height, so the 50% of the child should be resolved > against those 400px. > If I remove the logicalHeight().isAuto() we'd be doing this wrong. why not moving the new branch after the one checking out whether "isfixed" ?
https://codereview.chromium.org/2039223002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2039223002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:2736: } else if (cb->styleRef().logicalHeight().isAuto() && cb->hasOverrideContainingBlockLogicalHeight()) { On 2016/06/06 16:18:04, jfernandez wrote: > why not moving the new branch after the one checking out whether "isfixed" ? I can move it but I still need the isAuto() check. Note that isFixed() doesn't mean that !isAuto(), there're more cases like: percentage, calc, intrinsic size, etc. I put it there to keep it close to the one with hasOverrideContainingBlockLogicalHeight(), the one for the grid item itself.
cbiesinger@google.com changed reviewers: + cbiesinger@google.com
https://codereview.chromium.org/2039223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2039223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:2736: } else if (cbstyle.logicalHeight().isAuto() && cb->hasOverrideContainingBlockLogicalHeight()) { This does not seem correct in general. You are now always using the cb's cb size, not the box's cb size. This works for grid items but not in general, right? Maybe add a check if cb is a grid item?
https://codereview.chromium.org/2039223002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2039223002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:2736: } else if (cb->styleRef().logicalHeight().isAuto() && cb->hasOverrideContainingBlockLogicalHeight()) { On 2016/06/06 16:39:59, Manuel Rego wrote: > On 2016/06/06 16:18:04, jfernandez wrote: > > why not moving the new branch after the one checking out whether "isfixed" ? > > I can move it but I still need the isAuto() check. > Note that isFixed() doesn't mean that !isAuto(), there're more cases like: > percentage, calc, intrinsic size, etc. > Yeah, there is a case for the percentage cases below. I don't know, I don't have a strong opinion, so I think what you thing fits better int he code style of that method is fine to me. It was that new code looked like a special case and those branches are dealing with specific length types of the containing block. I thought that the 'auto' case was just another one. > I put it there to keep it close to the one with > hasOverrideContainingBlockLogicalHeight(), > the one for the grid item itself. >
https://codereview.chromium.org/2039223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2039223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:2736: } else if (cbstyle.logicalHeight().isAuto() && cb->hasOverrideContainingBlockLogicalHeight()) { On 2016/06/06 16:56:57, cbiesinger1 wrote: > This does not seem correct in general. You are now always using the cb's cb > size, not the box's cb size. This works for grid items but not in general, > right? Maybe add a check if cb is a grid item? overrideContainingBlockLogicalHeight is only used for grid items. I can add the check cb->isGridItem() but it's not really needed. If someone else uses overrideContainingBlockLogicalHeight in the future, probably they'll want the same behavior than for Grid or things won't work. If we want to add the isGridItem() check here, I'd do the same on the previous if.
On 2016/06/06 19:24:09, Manuel Rego wrote: > https://codereview.chromium.org/2039223002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > https://codereview.chromium.org/2039223002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutBox.cpp:2736: } else if > (cbstyle.logicalHeight().isAuto() && > cb->hasOverrideContainingBlockLogicalHeight()) { > On 2016/06/06 16:56:57, cbiesinger1 wrote: > > This does not seem correct in general. You are now always using the cb's cb > > size, not the box's cb size. This works for grid items but not in general, > > right? Maybe add a check if cb is a grid item? > > overrideContainingBlockLogicalHeight is only used for grid items. > I can add the check cb->isGridItem() but it's not really needed. > If someone else uses overrideContainingBlockLogicalHeight in the future, > probably they'll want the same behavior than for Grid or things won't work. > > If we want to add the isGridItem() check here, > I'd do the same on the previous if. Hmm, I see. The previous if doesn't really need the check because that works in a very straightforward way. But please do add a comment explaining how this code works, because it is not obvious at all. Also, maybe we should rename override logical height to something that makes it clearer what it's for / that it's grid-specific.
Description was changed from ========== [css-grid] Fix percentage height on grid item's children Grid item's size is determined by the grid area, which is stored in LayoutBox::overrideContainingBlockContentLogicalWidth|Height(). In order to resolve the percentage height on the grid item's children we need to use the grid area size. BUG=612755 TEST=fast/css-grid-layout/percent-resolution-grid-item-children.html ========== to ========== [css-grid] Fix percentage height on grid item's children Grid items can be vertically stretched in that case they store its height on LayoutBox::overrideLogicalContentHeight(). In order to resolve the percentage height on the grid item's children we need to use that size. BUG=612755 TEST=fast/css-grid-layout/percent-resolution-grid-item-children.html ==========
On 2016/06/06 19:51:44, cbiesinger1 wrote: > On 2016/06/06 19:24:09, Manuel Rego wrote: > > > https://codereview.chromium.org/2039223002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > > > > https://codereview.chromium.org/2039223002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/LayoutBox.cpp:2736: } else if > > (cbstyle.logicalHeight().isAuto() && > > cb->hasOverrideContainingBlockLogicalHeight()) { > > On 2016/06/06 16:56:57, cbiesinger1 wrote: > > > This does not seem correct in general. You are now always using the cb's cb > > > size, not the box's cb size. This works for grid items but not in general, > > > right? Maybe add a check if cb is a grid item? > > > > overrideContainingBlockLogicalHeight is only used for grid items. > > I can add the check cb->isGridItem() but it's not really needed. > > If someone else uses overrideContainingBlockLogicalHeight in the future, > > probably they'll want the same behavior than for Grid or things won't work. > > > > If we want to add the isGridItem() check here, > > I'd do the same on the previous if. > > Hmm, I see. > > The previous if doesn't really need the check because that works in a very > straightforward way. But please do add a comment explaining how this code works, > because it is not obvious at all. Also, maybe we should rename override logical > height to something that makes it clearer what it's for / that it's > grid-specific. Finally I've a different version adding an specific check for grid item, and using overrideLogicalContentHeight() as the issue was only happening for stretched grid items. I've modified the test to include more cases too. PTAL, thanks!
lgtm, thanks
The CQ bit was checked by rego@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from jfernandez@igalia.com Link to the patchset: https://codereview.chromium.org/2039223002/#ps40001 (title: "New version using overrideLogicalContentHeight")
Thank you for the reviews!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2039223002/40001
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Fix percentage height on grid item's children Grid items can be vertically stretched in that case they store its height on LayoutBox::overrideLogicalContentHeight(). In order to resolve the percentage height on the grid item's children we need to use that size. BUG=612755 TEST=fast/css-grid-layout/percent-resolution-grid-item-children.html ========== to ========== [css-grid] Fix percentage height on grid item's children Grid items can be vertically stretched in that case they store its height on LayoutBox::overrideLogicalContentHeight(). In order to resolve the percentage height on the grid item's children we need to use that size. BUG=612755 TEST=fast/css-grid-layout/percent-resolution-grid-item-children.html ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Fix percentage height on grid item's children Grid items can be vertically stretched in that case they store its height on LayoutBox::overrideLogicalContentHeight(). In order to resolve the percentage height on the grid item's children we need to use that size. BUG=612755 TEST=fast/css-grid-layout/percent-resolution-grid-item-children.html ========== to ========== [css-grid] Fix percentage height on grid item's children Grid items can be vertically stretched in that case they store its height on LayoutBox::overrideLogicalContentHeight(). In order to resolve the percentage height on the grid item's children we need to use that size. BUG=612755 TEST=fast/css-grid-layout/percent-resolution-grid-item-children.html Committed: https://crrev.com/8df0592ed1d737d3d9db4b704ffe8d033f14ac28 Cr-Commit-Position: refs/heads/master@{#398971} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8df0592ed1d737d3d9db4b704ffe8d033f14ac28 Cr-Commit-Position: refs/heads/master@{#398971} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
