Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
613273002:
[CSS Grid Layout] Stretch value for align and justify properties. (Closed)
|
Created:
6 years, 2 months ago by jfernandez Modified:
6 years, 1 month ago CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, jfernandez, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, Manuel Rego, rune+blink, svillar, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
Description[CSS Grid Layout] Stretch value for align and justify properties.
If the ‘width’ or ‘height’ (as appropriate) of the alignment subject
is ‘auto’, its used value is the length necessary to make the alignment
subject’s outer size as close to the size of the alignment container as
possible, while still respecting the constraints imposed by
‘min/max-width/height’. Otherwise, this is equivalent to ‘start’.
BUG=249451, 376823
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184782
Patch Set 1 : #Patch Set 2 : Added specific layout test. #
Total comments: 9
Patch Set 3 : min/max constrain logic. #
Total comments: 12
Patch Set 4 : Applied suggested changes. #Patch Set 5 : resolveAlignment and resolveJustification now defined in RenderStyle. #
Total comments: 12
Patch Set 6 : Applied suggested changes. #Patch Set 7 : Patch rebased. #Patch Set 8 : Fixed some layout tests. #Messages
Total messages: 36 (15 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
jfernandez@igalia.com changed reviewers: + cbiesinger@chromium.org, jchaffraix@chromium.org, timloh@chromium.org
Added specific layout test.
Patchset #2 (id:60001) has been deleted
timloh@chromium.org changed reviewers: - timloh@chromium.org
https://codereview.chromium.org/613273002/diff/80001/LayoutTests/fast/css-gri... File LayoutTests/fast/css-grid-layout/grid-align-justify-stretch.html (right): https://codereview.chromium.org/613273002/diff/80001/LayoutTests/fast/css-gri... LayoutTests/fast/css-grid-layout/grid-align-justify-stretch.html:18: .cell { Let's try better names: cell -> widthAndHeightSet cell1 -> onlyWidthSet cell2 -> onlyHeightSet https://codereview.chromium.org/613273002/diff/80001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (left): https://codereview.chromium.org/613273002/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1305: Instead of moving this code, can we just move applyStretchAlignmentToChildIfNeeded below? (less history churn) https://codereview.chromium.org/613273002/diff/80001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/613273002/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:619: child.clearOverrideLogicalContentHeight(); I don't understand the need for this as we do the following below (and there is no early return): child.setOverrideContainingBlockContentLogicalHeight(-1); https://codereview.chromium.org/613273002/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1123: child.setOverrideLogicalContentHeight(overrideLogicalContentHeight); This is good for a first pass but it's not totally correct. You need to constrain the overrideLogicalHeight by min-max. We should match flexbox's implementation as much as possible in this case as there is a lot of similarities. See constrainedChildIntrinsicContentLogicalHeight.
Applied suggested changes. https://codereview.chromium.org/613273002/diff/80001/LayoutTests/fast/css-gri... File LayoutTests/fast/css-grid-layout/grid-align-justify-stretch.html (right): https://codereview.chromium.org/613273002/diff/80001/LayoutTests/fast/css-gri... LayoutTests/fast/css-grid-layout/grid-align-justify-stretch.html:18: .cell { On 2014/10/08 15:21:47, Julien Chaffraix - PST wrote: > Let's try better names: > cell -> widthAndHeightSet > cell1 -> onlyWidthSet > cell2 -> onlyHeightSet Done. https://codereview.chromium.org/613273002/diff/80001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/613273002/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:619: child.clearOverrideLogicalContentHeight(); On 2014/10/08 15:21:47, Julien Chaffraix - PST wrote: > I don't understand the need for this as we do the following below (and there is > no early return): > > child.setOverrideContainingBlockContentLogicalHeight(-1); Well, if I understood it correctly, OverrideContainingBlockContentLogicalHeight is different from OverrideLogicalContentHeight. Actually, the former seems Grid specific which is something confused me. The OverrideLogicalContentHeight api handles some fields in the RenderBoxRareData struct and it seems to be used to override the intrinsic height/width of the box. The RenderFlexibleBox implementation uses such api to implement the 'stretch' behavior, so that's why I used it. Also, the RenderBox::computeLogicalHeight uses the OverrideLogicalContentHeight api to compute the actual/computed height of the box. On the other hand, the OverrideContainingBlockLogicalHeigh used in the RenderGrid implementation is independent of the OverrideLogicalContentHeight and is not used at all in the RenderBox::computeLogicalHeight. I don't really understand why grid needs its own stuff for this; there is actually a FIXME to move the logic to the RenderBoxRareData structure. So, the reason why the clearOverrideLogicalContentHeight call is needed is that etOverrideContainingBlockContentLogicalHeight(-1) does not do the job, so the OverrideLogicalContentHeight flag is still there and it causes some repaint tests to fail. https://codereview.chromium.org/613273002/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1123: child.setOverrideLogicalContentHeight(overrideLogicalContentHeight); On 2014/10/08 15:21:47, Julien Chaffraix - PST wrote: > This is good for a first pass but it's not totally correct. You need to > constrain the overrideLogicalHeight by min-max. > > We should match flexbox's implementation as much as possible in this case as > there is a lot of similarities. See > constrainedChildIntrinsicContentLogicalHeight. Acknowledged.
https://codereview.chromium.org/613273002/diff/80001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/613273002/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:619: child.clearOverrideLogicalContentHeight(); On 2014/10/12 22:40:27, jfernandez wrote: > On 2014/10/08 15:21:47, Julien Chaffraix - PST wrote: > > I don't understand the need for this as we do the following below (and there > is > > no early return): > > > > child.setOverrideContainingBlockContentLogicalHeight(-1); > > Well, if I understood it correctly, OverrideContainingBlockContentLogicalHeight > is different from OverrideLogicalContentHeight. Actually, the former seems Grid > specific which is something confused me. You're right, I misread. RenderGrid needs to pretend that its children's sizes are resolved against a different containing block's size (the grid cell's, not the grid container's). We introduced OverrideContainingBlockContentLogicalHeight to do this without having to introduce a synthetic grid cell's renderer. Btw OverrideLogicalContentHeight is also used by table. > The OverrideLogicalContentHeight api handles some fields in the > RenderBoxRareData struct and it seems to be used to override the intrinsic > height/width of the box. The RenderFlexibleBox implementation uses such api to > implement the 'stretch' behavior, so that's why I used it. > > Also, the RenderBox::computeLogicalHeight uses the OverrideLogicalContentHeight > api to compute the actual/computed height of the box. Yes, that's the right API to stretch / constrain a logical height. > On the other hand, the OverrideContainingBlockLogicalHeigh used in the > RenderGrid implementation is independent of the OverrideLogicalContentHeight and > is not used at all in the RenderBox::computeLogicalHeight. I don't really > understand why grid needs its own stuff for this; there is actually a FIXME to > move the logic to the RenderBoxRareData structure. > > So, the reason why the clearOverrideLogicalContentHeight call is needed is that > etOverrideContainingBlockContentLogicalHeight(-1) does not do the job, so the > OverrideLogicalContentHeight flag is still there and it causes some repaint > tests to fail. https://codereview.chromium.org/613273002/diff/100001/Source/core/rendering/R... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/613273002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1075: // FIXME: Grid items should stretch to fill their cells. Once we Let's narrow this FIXME as part of this change to reflect the state of the code. https://codereview.chromium.org/613273002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1317: bool RenderGrid::needToStretchChildLogicalHeight(RenderBox& child) const const RenderBox& child for most of the functions. https://codereview.chromium.org/613273002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1319: if (resolveAlignment(style(), child.style()) != ItemPositionStretch) resolveAlignment is now on RenderStyle so it seems like this function can be static. https://codereview.chromium.org/613273002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1339: LayoutUnit RenderGrid::intrinsicExtentForChild(RenderBox& child) const We usually don't use Extent in the code. Instead let's stick to LogicalHeight for consistency. https://codereview.chromium.org/613273002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1346: return isHorizontalWritingMode() ? child.marginHeight() : child.marginWidth(); It really seems like that this should be on RenderBox. https://codereview.chromium.org/613273002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1366: LayoutUnit heightBeforeStretching = needToStretchChildLogicalHeight(child) ? constrainedChildIntrinsicContentLogicalHeight(child) : child.logicalHeight(); It's bad smell that needToStretchChildLogicalHeight checks 2 attributes we have already checked: the grid container's writing mode and the child's logical height.
Applied suggested changes. https://codereview.chromium.org/613273002/diff/80001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/613273002/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:619: child.clearOverrideLogicalContentHeight(); On 2014/10/21 00:43:12, Julien Chaffraix - PST wrote: > On 2014/10/12 22:40:27, jfernandez wrote: > > On 2014/10/08 15:21:47, Julien Chaffraix - PST wrote: > > > I don't understand the need for this as we do the following below (and there > > is > > > no early return): > > > > > > child.setOverrideContainingBlockContentLogicalHeight(-1); > > > > Well, if I understood it correctly, > OverrideContainingBlockContentLogicalHeight > > is different from OverrideLogicalContentHeight. Actually, the former seems > Grid > > specific which is something confused me. > > You're right, I misread. RenderGrid needs to pretend that its children's sizes > are resolved against a different containing block's size (the grid cell's, not > the grid container's). We introduced OverrideContainingBlockContentLogicalHeight > to do this without having to introduce a synthetic grid cell's renderer. > > Btw OverrideLogicalContentHeight is also used by table. > > > The OverrideLogicalContentHeight api handles some fields in the > > RenderBoxRareData struct and it seems to be used to override the intrinsic > > height/width of the box. The RenderFlexibleBox implementation uses such api to > > implement the 'stretch' behavior, so that's why I used it. > > > > Also, the RenderBox::computeLogicalHeight uses the > OverrideLogicalContentHeight > > api to compute the actual/computed height of the box. > > Yes, that's the right API to stretch / constrain a logical height. > > > On the other hand, the OverrideContainingBlockLogicalHeigh used in the > > RenderGrid implementation is independent of the OverrideLogicalContentHeight > and > > is not used at all in the RenderBox::computeLogicalHeight. I don't really > > understand why grid needs its own stuff for this; there is actually a FIXME to > > move the logic to the RenderBoxRareData structure. > > > > So, the reason why the clearOverrideLogicalContentHeight call is needed is > that > > etOverrideContainingBlockContentLogicalHeight(-1) does not do the job, so the > > OverrideLogicalContentHeight flag is still there and it causes some repaint > > tests to fail. > Done. https://codereview.chromium.org/613273002/diff/100001/Source/core/rendering/R... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/613273002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1075: // FIXME: Grid items should stretch to fill their cells. Once we On 2014/10/21 00:43:12, Julien Chaffraix - PST wrote: > Let's narrow this FIXME as part of this change to reflect the state of the code. Done. https://codereview.chromium.org/613273002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1317: bool RenderGrid::needToStretchChildLogicalHeight(RenderBox& child) const On 2014/10/21 00:43:12, Julien Chaffraix - PST wrote: > const RenderBox& child for most of the functions. Done. https://codereview.chromium.org/613273002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1319: if (resolveAlignment(style(), child.style()) != ItemPositionStretch) On 2014/10/21 00:43:12, Julien Chaffraix - PST wrote: > resolveAlignment is now on RenderStyle so it seems like this function can be > static. I'll send a new patch, rebased, to include this change. I reviewed the current implementation at RenderStyle and it's not totally correct. Being now a shared logic of any renderer, it should resolve "auto" to stretch only for flex/grid elements and "start" otherwise. I'll include such change in the new patch. https://codereview.chromium.org/613273002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1339: LayoutUnit RenderGrid::intrinsicExtentForChild(RenderBox& child) const On 2014/10/21 00:43:12, Julien Chaffraix - PST wrote: > We usually don't use Extent in the code. Instead let's stick to LogicalHeight > for consistency. Done. https://codereview.chromium.org/613273002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1346: return isHorizontalWritingMode() ? child.marginHeight() : child.marginWidth(); On 2014/10/21 00:43:12, Julien Chaffraix - PST wrote: > It really seems like that this should be on RenderBox. Agree, but perhaps in a different patch ? Otherwise it would touch RenderFlexibleBox as well, so it would complicate the review, I think. https://codereview.chromium.org/613273002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1366: LayoutUnit heightBeforeStretching = needToStretchChildLogicalHeight(child) ? constrainedChildIntrinsicContentLogicalHeight(child) : child.logicalHeight(); On 2014/10/21 00:43:12, Julien Chaffraix - PST wrote: > It's bad smell that needToStretchChildLogicalHeight checks 2 attributes we have > already checked: the grid container's writing mode and the child's logical > height. I've got the code from the flexbox implementation, but I agree is a bit weird and I spend some time trying to understand the idea. I'm not totally sure whether the orthogonal writing modes FIXME would apply as well to grid layout, though. I think the idea behind this logic is that if vertical mode is being used, the child must use it as well, so heightBeforeStretching is just the child's logicalHeight, instead of constrainedChildIntrinsicContentLogicalHeight, since needToStretchChildLogicalHeight would be false in that case.
Applied suggested changes.
Patch Set5 is a rebase in order to use the resolveAlignment defined now in the RenderSyle class. I also moved the resolveJustification method.
I think I've implemented all the suggested changes and it'd be good to land this patch before continuing implementing more alignment related stuff. The 'stretch' value is the default for flex and grid, and it will affect many tests in both layout models.
lgtm https://codereview.chromium.org/613273002/diff/150001/LayoutTests/fast/css-gr... File LayoutTests/fast/css-grid-layout/grid-align.html (right): https://codereview.chromium.org/613273002/diff/150001/LayoutTests/fast/css-gr... LayoutTests/fast/css-grid-layout/grid-align.html:115: <div class="alignSelfAuto firstRowFirstColumn" data-offset-x="0" data-offset-y="0" data-expected-width="100" data-expected-height="200"></div> Why do we remove the cell class here? https://codereview.chromium.org/613273002/diff/150001/Source/core/rendering/R... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/613273002/diff/150001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:621: child.clearOverrideLogicalContentHeight(); If we don't need this call before everything else, we should put it next to the call to setOverrideContainingBlockContentLogicalHeight below. https://codereview.chromium.org/613273002/diff/150001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1339: return child.constrainLogicalHeightByMinMax(childIntrinsicContentLogicalHeight + child.borderAndPaddingLogicalHeight(), childIntrinsicContentLogicalHeight); The min / max constraining logic is not tested :( We should fix that before landing. https://codereview.chromium.org/613273002/diff/150001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1369: LayoutUnit RenderGrid::marginLogicalHeightForChild(const RenderBox& child) const This doesn't look grid related so it should be moved to RenderBlock like the similar helper functions. https://codereview.chromium.org/613273002/diff/150001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1403: } A lot of this logic is similar to RenderFlexibleBox. It would be nice to share code with them. At least a FIXME about it is important (to avoid someone copying it again) https://codereview.chromium.org/613273002/diff/150001/Source/core/rendering/s... File Source/core/rendering/style/RenderStyle.h (right): https://codereview.chromium.org/613273002/diff/150001/Source/core/rendering/s... Source/core/rendering/style/RenderStyle.h:350: static ItemPosition resolveJustification(const RenderStyle* parentStyle, const RenderStyle* childStyle, ItemPosition defaultPosition = ItemPositionStretch); The default parameter seems very artificial (why ItemPositionStretch when the initial is ItemPositionStart? Also the spec has no real 'default' for this value), it would be better without a default. Also let's rename |defaultPosition| to something else. Suggestions: |resolvedAutoPositionForRenderer|, |positionForAutoBasedOnRenderer|
https://codereview.chromium.org/613273002/diff/150001/LayoutTests/fast/css-gr... File LayoutTests/fast/css-grid-layout/grid-align.html (right): https://codereview.chromium.org/613273002/diff/150001/LayoutTests/fast/css-gr... LayoutTests/fast/css-grid-layout/grid-align.html:115: <div class="alignSelfAuto firstRowFirstColumn" data-offset-x="0" data-offset-y="0" data-expected-width="100" data-expected-height="200"></div> On 2014/10/31 17:17:12, Julien Chaffraix - PST wrote: > Why do we remove the cell class here? Well, 'auto' is resolved to 'stretch' for grid items, so in order to actually test it, height property has to be auto. https://codereview.chromium.org/613273002/diff/150001/Source/core/rendering/R... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/613273002/diff/150001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:621: child.clearOverrideLogicalContentHeight(); On 2014/10/31 17:17:12, Julien Chaffraix - PST wrote: > If we don't need this call before everything else, we should put it next to the > call to setOverrideContainingBlockContentLogicalHeight below. Acknowledged. https://codereview.chromium.org/613273002/diff/150001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1339: return child.constrainLogicalHeightByMinMax(childIntrinsicContentLogicalHeight + child.borderAndPaddingLogicalHeight(), childIntrinsicContentLogicalHeight); On 2014/10/31 17:17:12, Julien Chaffraix - PST wrote: > The min / max constraining logic is not tested :( > The grid-align-justify-stretch.html test contains some cases to check out the min / max constraining logic: <div class="maxHeight alignSelfStretch justifySelfStart firstRowFirstColumn" data-offset-x="0" data-offset-y="0" data-expected-width="100" data-expected-height="160"></div> There is another one testing maxWidth in vertical mode; I think regarding the 'stretch' behavior the min / max constrain is tested. I think there are other Layout Tests for verifying the min / max constrain is applied in the regular grid track sizing algorithm, so in my opinion the cases defined for stretch are enough. https://codereview.chromium.org/613273002/diff/150001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1369: LayoutUnit RenderGrid::marginLogicalHeightForChild(const RenderBox& child) const On 2014/10/31 17:17:13, Julien Chaffraix - PST wrote: > This doesn't look grid related so it should be moved to RenderBlock like the > similar helper functions. I agree. As I commented before, I'll do a new patch to move this logic and the one implemented in Flexbox to the RenderBlock class. https://codereview.chromium.org/613273002/diff/150001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1403: } On 2014/10/31 17:17:12, Julien Chaffraix - PST wrote: > A lot of this logic is similar to RenderFlexibleBox. It would be nice to share > code with them. At least a FIXME about it is important (to avoid someone copying > it again) Acknowledged. https://codereview.chromium.org/613273002/diff/150001/Source/core/rendering/s... File Source/core/rendering/style/RenderStyle.h (right): https://codereview.chromium.org/613273002/diff/150001/Source/core/rendering/s... Source/core/rendering/style/RenderStyle.h:350: static ItemPosition resolveJustification(const RenderStyle* parentStyle, const RenderStyle* childStyle, ItemPosition defaultPosition = ItemPositionStretch); On 2014/10/31 17:17:13, Julien Chaffraix - PST wrote: > The default parameter seems very artificial (why ItemPositionStretch when the > initial is ItemPositionStart? Also the spec has no real 'default' for this > value), it would be better without a default. > I agree is not the best approach, but I couldn't come out with anything better. It ight be that it'd be better after all not to move this logic to RenderStyle to be shared between Flexbox and Grid, for now, and other kinds of block. The main problem is that the spec states that "auto' is resolved to 'stretch" for Flex and Grid containers, and 'start' otherwise. I've decided to initialize the parameter to 'stretch' to avoid touching the only Renders which use the alignment properties nowadays. Other renders should pass 'start' as value for such argument. The thing is that we have had solved the 'auto' resolution during the cascade (StyleAdjuster), at least for align-items/justify-items, and for the computed value. However, there are cases where the RenderTree is modified after the cascade, new elements introduced or updated, so the alignment properties might be 'auto' anyway. Having the 'auto' resolution at Rendering side was something useful to ensure the correct value. > Also let's rename |defaultPosition| to something else. Suggestions: > |resolvedAutoPositionForRenderer|, |positionForAutoBasedOnRenderer| I agree, though, that default may be not the best name. Any of your suggestions are really better. I'll think about this for the final version of the patch to be landed.
The CQ bit was checked by jfernandez@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/613273002/170001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/core/rendering/RenderGrid.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/rendering/RenderGrid.cpp
Hunk #3 FAILED at 1221.
Hunk #4 succeeded at 1310 (offset 16 lines).
Hunk #5 FAILED at 1342.
Hunk #6 succeeded at 1416 (offset 16 lines).
2 out of 6 hunks FAILED -- saving rejects to file
Source/core/rendering/RenderGrid.cpp.rej
Patch: Source/core/rendering/RenderGrid.cpp
Index: Source/core/rendering/RenderGrid.cpp
diff --git a/Source/core/rendering/RenderGrid.cpp
b/Source/core/rendering/RenderGrid.cpp
index
0c343bf05560ca4605969c74b5ea390c9957caed..a7877f240582d6997511f653b72e0e38fda41553
100644
--- a/Source/core/rendering/RenderGrid.cpp
+++ b/Source/core/rendering/RenderGrid.cpp
@@ -624,6 +624,8 @@ LayoutUnit RenderGrid::logicalHeightForChild(RenderBox&
child, Vector<GridTrack>
if (child.style()->logicalHeight().isPercent() ||
oldOverrideContainingBlockContentLogicalWidth !=
overrideContainingBlockContentLogicalWidth)
layoutScope.setNeedsLayout(&child);
+ child.clearOverrideLogicalContentHeight();
+
child.setOverrideContainingBlockContentLogicalWidth(overrideContainingBlockContentLogicalWidth);
// If |child| has a percentage logical height, we shouldn't let it override
its intrinsic height, which is
// what we are interested in here. Thus we need to set the override logical
height to -1 (no possible resolution).
@@ -1117,9 +1119,8 @@ void RenderGrid::layoutGridItems()
child->setOverrideContainingBlockContentLogicalWidth(overrideContainingBlockContentLogicalWidth);
child->setOverrideContainingBlockContentLogicalHeight(overrideContainingBlockContentLogicalHeight);
- // FIXME: Grid items should stretch to fill their cells. Once we
- // implement grid-{column,row}-align, we can also shrink to fit. For
- // now, just size as if we were a regular child.
+ applyStretchAlignmentToChildIfNeeded(*child,
overrideContainingBlockContentLogicalHeight);
+
child->layoutIfNeeded();
#if ENABLE(ASSERT)
@@ -1220,20 +1221,11 @@ LayoutUnit
RenderGrid::centeredColumnPositionForChild(const RenderBox& child) co
return columnPosition + std::max<LayoutUnit>(0, endOfColumn - startOfColumn
- child.logicalWidth()) / 2;
}
-static ItemPosition resolveJustification(const RenderStyle* parentStyle, const
RenderStyle* childStyle)
-{
- ItemPosition justify = childStyle->justifySelf();
- if (justify == ItemPositionAuto)
- justify = (parentStyle->justifyItems() == ItemPositionAuto) ?
ItemPositionStretch : parentStyle->justifyItems();
-
- return justify;
-}
-
LayoutUnit RenderGrid::columnPositionForChild(const RenderBox& child) const
{
bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() !=
isHorizontalWritingMode();
- switch (resolveJustification(style(), child.style())) {
+ switch (RenderStyle::resolveJustification(style(), child.style(),
ItemPositionStretch)) {
case ItemPositionSelfStart:
// For orthogonal writing-modes, this computes to 'start'
// FIXME: grid track sizing and positioning do not support orthogonal
modes yet.
@@ -1293,6 +1285,7 @@ LayoutUnit RenderGrid::columnPositionForChild(const
RenderBox& child) const
case ItemPositionAuto:
break;
case ItemPositionStretch:
+ return startOfColumnForChild(child);
case ItemPositionBaseline:
case ItemPositionLastBaseline:
// FIXME: Implement the previous values. For now, we always start align
the child.
@@ -1340,12 +1333,87 @@ LayoutUnit RenderGrid::centeredRowPositionForChild(const
RenderBox& child) const
return startOfRow + std::max<LayoutUnit>(0, endOfRow - startOfRow -
child.logicalHeight()) / 2;
}
-LayoutUnit RenderGrid::rowPositionForChild(const RenderBox& child) const
+static inline LayoutUnit constrainedChildIntrinsicContentLogicalHeight(const
RenderBox& child)
+{
+ LayoutUnit childIntrinsicContentLogicalHeight =
child.intrinsicContentLogicalHeight();
+ return
child.constrainLogicalHeightByMinMax(childIntrinsicContentLogicalHeight +
child.borderAndPaddingLogicalHeight(), childIntrinsicContentLogicalHeight);
+}
+
+// FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to
RenderBox.
+bool RenderGrid::needToStretchChildLogicalHeight(const RenderBox& child) const
{
+ if (RenderStyle::resolveAlignment(style(), child.style(),
ItemPositionStretch) != ItemPositionStretch)
+ return false;
+
+ return isHorizontalWritingMode() && child.style()->height().isAuto();
+}
+
+// FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to
RenderBox.
+LayoutUnit RenderGrid::childIntrinsicHeight(const RenderBox& child) const
+{
+ if (child.isHorizontalWritingMode() &&
needToStretchChildLogicalHeight(child))
+ return constrainedChildIntrinsicContentLogicalHeight(child);
+ return child.height();
+}
+
+// FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to
RenderBox.
+LayoutUnit RenderGrid::childIntrinsicWidth(const RenderBox& child) const
+{
+ if (!child.isHorizontalWritingMode() &&
needToStretchChildLogicalHeight(child))
+ return constrainedChildIntrinsicContentLogicalHeight(child);
+ return child.width();
+}
+
+// FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to
RenderBox.
+LayoutUnit RenderGrid::intrinsicLogicalHeightForChild(const RenderBox& child)
const
+{
+ return isHorizontalWritingMode() ? childIntrinsicHeight(child) :
childIntrinsicWidth(child);
+}
+
+// FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to
RenderBox.
+LayoutUnit RenderGrid::marginLogicalHeightForChild(const RenderBox& child)
const
+{
+ return isHorizontalWritingMode() ? child.marginHeight() :
child.marginWidth();
+}
+
+// FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to
RenderBox.
+LayoutUnit
RenderGrid::availableAlignmentSpaceForChildBeforeStretching(LayoutUnit
gridAreaBreadthForChild, const RenderBox& child) const
+{
+ ASSERT(!child.isOutOfFlowPositioned());
+ LayoutUnit childLogicalHeight = marginLogicalHeightForChild(child) +
intrinsicLogicalHeightForChild(child);
+ return gridAreaBreadthForChild - childLogicalHeight;
+}
+
+// FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to
RenderBox.
+void RenderGrid::applyStretchAlignmentToChildIfNeeded(RenderBox& child,
LayoutUnit gridAreaBreadthForChild)
+{
+ if (RenderStyle::resolveAlignment(style(), child.style(),
ItemPositionStretch) != ItemPositionStretch)
+ return;
+
bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() !=
isHorizontalWritingMode();
- ItemPosition alignSelf = RenderStyle::resolveAlignment(style(),
child.style());
+ if (child.style()->logicalHeight().isAuto()) {
+ // FIXME: If the child has orthogonal flow, then it already has an
override height set, so use it.
+ // FIXME: grid track sizing and positioning do not support orthogonal
modes yet.
+ if (!hasOrthogonalWritingMode) {
+ LayoutUnit heightBeforeStretching =
needToStretchChildLogicalHeight(child) ?
constrainedChildIntrinsicContentLogicalHeight(child) : child.logicalHeight();
+ LayoutUnit stretchedLogicalHeight = heightBeforeStretching +
availableAlignmentSpaceForChildBeforeStretching(gridAreaBreadthForChild, child);
+ LayoutUnit desiredLogicalHeight =
child.constrainLogicalHeightByMinMax(stretchedLogicalHeight,
heightBeforeStretching - child.borderAndPaddingLogicalHeight());
+ LayoutUnit desiredLogicalContentHeight = desiredLogicalHeight -
child.borderAndPaddingLogicalHeight();
+
+ // FIXME: Can avoid laying out here in some cases. See
https://webkit.org/b/87905.
+ if (desiredLogicalHeight != child.logicalHeight() ||
!child.hasOverrideHeight() || desiredLogicalContentHeight !=
child.overrideLogicalContentHeight()) {
+
child.setOverrideLogicalContentHeight(desiredLogicalContentHeight);
+ child.setLogicalHeight(0);
+ child.forceChildLayout();
+ }
+ }
+ }
+}
- switch (alignSelf) {
+LayoutUnit RenderGrid::rowPositionForChild(const RenderBox& child) const
+{
+ bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() !=
isHorizontalWritingMode();
+ switch (RenderStyle::resolveAlignment(style(), child.style(),
ItemPositionStretch)) {
case ItemPositionSelfStart:
// If orthogonal writing-modes, this computes to 'Start'.
// FIXME: grid track sizing and positioning does not support orthogonal
modes yet.
@@ -1398,7 +1466,6 @@ LayoutUnit RenderGrid::rowPositionForChild(const
RenderBox& child) const
case ItemPositionEnd:
return endOfRowForChild(child);
case ItemPositionStretch:
- // FIXME: Implement the Stretch value. For now, we always start align
the child.
return startOfRowForChild(child);
case ItemPositionBaseline:
case ItemPositionLastBaseline:
The CQ bit was checked by jfernandez@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/613273002/190001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/core/rendering/RenderGrid.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/rendering/RenderGrid.cpp
Hunk #1 succeeded at 624 (offset 1 line).
Hunk #2 succeeded at 1119 (offset 7 lines).
Hunk #3 FAILED at 1214.
Hunk #4 succeeded at 1310 (offset 23 lines).
Hunk #5 FAILED at 1335.
Hunk #6 succeeded at 1416 (offset 23 lines).
2 out of 6 hunks FAILED -- saving rejects to file
Source/core/rendering/RenderGrid.cpp.rej
Patch: Source/core/rendering/RenderGrid.cpp
Index: Source/core/rendering/RenderGrid.cpp
diff --git a/Source/core/rendering/RenderGrid.cpp
b/Source/core/rendering/RenderGrid.cpp
index
8ad2d787df8d70497a998b80100fb615d471e3d6..d90e65b58573e9270545e90c29a2df5839f02fe6
100644
--- a/Source/core/rendering/RenderGrid.cpp
+++ b/Source/core/rendering/RenderGrid.cpp
@@ -623,6 +623,8 @@ LayoutUnit RenderGrid::logicalHeightForChild(RenderBox&
child, Vector<GridTrack>
if (child.style()->logicalHeight().isPercent() ||
oldOverrideContainingBlockContentLogicalWidth !=
overrideContainingBlockContentLogicalWidth)
layoutScope.setNeedsLayout(&child);
+ child.clearOverrideLogicalContentHeight();
+
child.setOverrideContainingBlockContentLogicalWidth(overrideContainingBlockContentLogicalWidth);
// If |child| has a percentage logical height, we shouldn't let it override
its intrinsic height, which is
// what we are interested in here. Thus we need to set the override logical
height to -1 (no possible resolution).
@@ -1110,9 +1112,8 @@ void RenderGrid::layoutGridItems()
child->setOverrideContainingBlockContentLogicalWidth(overrideContainingBlockContentLogicalWidth);
child->setOverrideContainingBlockContentLogicalHeight(overrideContainingBlockContentLogicalHeight);
- // FIXME: Grid items should stretch to fill their cells. Once we
- // implement grid-{column,row}-align, we can also shrink to fit. For
- // now, just size as if we were a regular child.
+ applyStretchAlignmentToChildIfNeeded(*child,
overrideContainingBlockContentLogicalHeight);
+
child->layoutIfNeeded();
#if ENABLE(ASSERT)
@@ -1213,20 +1214,11 @@ LayoutUnit
RenderGrid::centeredColumnPositionForChild(const RenderBox& child) co
return columnPosition + std::max<LayoutUnit>(0, endOfColumn - startOfColumn
- child.logicalWidth()) / 2;
}
-static ItemPosition resolveJustification(const RenderStyle* parentStyle, const
RenderStyle* childStyle)
-{
- ItemPosition justify = childStyle->justifySelf();
- if (justify == ItemPositionAuto)
- justify = (parentStyle->justifyItems() == ItemPositionAuto) ?
ItemPositionStretch : parentStyle->justifyItems();
-
- return justify;
-}
-
LayoutUnit RenderGrid::columnPositionForChild(const RenderBox& child) const
{
bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() !=
isHorizontalWritingMode();
- switch (resolveJustification(style(), child.style())) {
+ switch (RenderStyle::resolveJustification(style(), child.style(),
ItemPositionStretch)) {
case ItemPositionSelfStart:
// For orthogonal writing-modes, this computes to 'start'
// FIXME: grid track sizing and positioning do not support orthogonal
modes yet.
@@ -1286,6 +1278,7 @@ LayoutUnit RenderGrid::columnPositionForChild(const
RenderBox& child) const
case ItemPositionAuto:
break;
case ItemPositionStretch:
+ return startOfColumnForChild(child);
case ItemPositionBaseline:
case ItemPositionLastBaseline:
// FIXME: Implement the previous values. For now, we always start align
the child.
@@ -1333,12 +1326,87 @@ LayoutUnit RenderGrid::centeredRowPositionForChild(const
RenderBox& child) const
return startOfRow + std::max<LayoutUnit>(0, endOfRow - startOfRow -
child.logicalHeight()) / 2;
}
-LayoutUnit RenderGrid::rowPositionForChild(const RenderBox& child) const
+static inline LayoutUnit constrainedChildIntrinsicContentLogicalHeight(const
RenderBox& child)
+{
+ LayoutUnit childIntrinsicContentLogicalHeight =
child.intrinsicContentLogicalHeight();
+ return
child.constrainLogicalHeightByMinMax(childIntrinsicContentLogicalHeight +
child.borderAndPaddingLogicalHeight(), childIntrinsicContentLogicalHeight);
+}
+
+// FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to
RenderBox.
+bool RenderGrid::needToStretchChildLogicalHeight(const RenderBox& child) const
{
+ if (RenderStyle::resolveAlignment(style(), child.style(),
ItemPositionStretch) != ItemPositionStretch)
+ return false;
+
+ return isHorizontalWritingMode() && child.style()->height().isAuto();
+}
+
+// FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to
RenderBox.
+LayoutUnit RenderGrid::childIntrinsicHeight(const RenderBox& child) const
+{
+ if (child.isHorizontalWritingMode() &&
needToStretchChildLogicalHeight(child))
+ return constrainedChildIntrinsicContentLogicalHeight(child);
+ return child.height();
+}
+
+// FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to
RenderBox.
+LayoutUnit RenderGrid::childIntrinsicWidth(const RenderBox& child) const
+{
+ if (!child.isHorizontalWritingMode() &&
needToStretchChildLogicalHeight(child))
+ return constrainedChildIntrinsicContentLogicalHeight(child);
+ return child.width();
+}
+
+// FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to
RenderBox.
+LayoutUnit RenderGrid::intrinsicLogicalHeightForChild(const RenderBox& child)
const
+{
+ return isHorizontalWritingMode() ? childIntrinsicHeight(child) :
childIntrinsicWidth(child);
+}
+
+// FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to
RenderBox.
+LayoutUnit RenderGrid::marginLogicalHeightForChild(const RenderBox& child)
const
+{
+ return isHorizontalWritingMode() ? child.marginHeight() :
child.marginWidth();
+}
+
+// FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to
RenderBox.
+LayoutUnit
RenderGrid::availableAlignmentSpaceForChildBeforeStretching(LayoutUnit
gridAreaBreadthForChild, const RenderBox& child) const
+{
+ ASSERT(!child.isOutOfFlowPositioned());
+ LayoutUnit childLogicalHeight = marginLogicalHeightForChild(child) +
intrinsicLogicalHeightForChild(child);
+ return gridAreaBreadthForChild - childLogicalHeight;
+}
+
+// FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to
RenderBox.
+void RenderGrid::applyStretchAlignmentToChildIfNeeded(RenderBox& child,
LayoutUnit gridAreaBreadthForChild)
+{
+ if (RenderStyle::resolveAlignment(style(), child.style(),
ItemPositionStretch) != ItemPositionStretch)
+ return;
+
bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() !=
isHorizontalWritingMode();
- ItemPosition alignSelf = RenderStyle::resolveAlignment(style(),
child.style());
+ if (child.style()->logicalHeight().isAuto()) {
+ // FIXME: If the child has orthogonal flow, then it already has an
override height set, so use it.
+ // FIXME: grid track sizing and positioning do not support orthogonal
modes yet.
+ if (!hasOrthogonalWritingMode) {
+ LayoutUnit heightBeforeStretching =
needToStretchChildLogicalHeight(child) ?
constrainedChildIntrinsicContentLogicalHeight(child) : child.logicalHeight();
+ LayoutUnit stretchedLogicalHeight = heightBeforeStretching +
availableAlignmentSpaceForChildBeforeStretching(gridAreaBreadthForChild, child);
+ LayoutUnit desiredLogicalHeight =
child.constrainLogicalHeightByMinMax(stretchedLogicalHeight,
heightBeforeStretching - child.borderAndPaddingLogicalHeight());
+ LayoutUnit desiredLogicalContentHeight = desiredLogicalHeight -
child.borderAndPaddingLogicalHeight();
+
+ // FIXME: Can avoid laying out here in some cases. See
https://webkit.org/b/87905.
+ if (desiredLogicalHeight != child.logicalHeight() ||
!child.hasOverrideHeight() || desiredLogicalContentHeight !=
child.overrideLogicalContentHeight()) {
+
child.setOverrideLogicalContentHeight(desiredLogicalContentHeight);
+ child.setLogicalHeight(0);
+ child.forceChildLayout();
+ }
+ }
+ }
+}
- switch (alignSelf) {
+LayoutUnit RenderGrid::rowPositionForChild(const RenderBox& child) const
+{
+ bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() !=
isHorizontalWritingMode();
+ switch (RenderStyle::resolveAlignment(style(), child.style(),
ItemPositionStretch)) {
case ItemPositionSelfStart:
// If orthogonal writing-modes, this computes to 'Start'.
// FIXME: grid track sizing and positioning does not support orthogonal
modes yet.
@@ -1391,7 +1459,6 @@ LayoutUnit RenderGrid::rowPositionForChild(const
RenderBox& child) const
case ItemPositionEnd:
return endOfRowForChild(child);
case ItemPositionStretch:
- // FIXME: Implement the Stretch value. For now, we always start align
the child.
return startOfRowForChild(child);
case ItemPositionBaseline:
case ItemPositionLastBaseline:
Patchset #7 (id:190001) has been deleted
The CQ bit was checked by jfernandez@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/613273002/210001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)
The CQ bit was checked by jfernandez@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/613273002/210001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)
The CQ bit was checked by jfernandez@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/613273002/230001
Message was sent while issue was closed.
Committed patchset #8 (id:230001) as 184782 |
