Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
815833005:
[css-grid] Handle min-content/max-content with orthogonal flows (Closed)
|
Created:
5 years, 11 months ago by jfernandez Modified:
4 years, 6 months ago Reviewers:
cbiesinger, leviw_travelin_and_unemployed, svillar, esprehn, Manuel Rego, Julien - ping for review CC:
blink-reviews, blink-reviews-rendering, Dominik Röttsches, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, Manuel Rego, svillar, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[css-grid] Handle min-content/max-content with orthogonal flows
Currently there is no support for orthogonal flows in many aspects of the
Grid Layout logic.
The Grid sizing algorithm should be adapted to this scenario, hence this
patch focus on the min-content and max-content functions, used to resolve
content based track sizes.
BUG=234194
Committed: https://crrev.com/fe619f7270786d15f9b2de8962835d47b1e7823b
Cr-Commit-Position: refs/heads/master@{#401246}
Patch Set 1 #Patch Set 2 : Solving positioning issues. #
Total comments: 4
Patch Set 3 : New approach: extra sizing algorithm iteration. #
Total comments: 6
Patch Set 4 : Utility functions for orthogonality. #
Total comments: 8
Patch Set 5 : Using physical position for determining overflow. #Patch Set 6 : Patch rebased. #
Total comments: 11
Patch Set 7 : New approach (Step1) - Make sizing alg being orthogonality aware. #Patch Set 8 : New approach (Step2) - Add orthogonal flow support in the sizing algorithm. #Patch Set 9 : New approach (Step3) - Assumed row track's breadth for intrinsic size computation. #
Total comments: 19
Patch Set 10 : Patch rebased. Added an extra cycle for rows sizing. #Patch Set 11 : Patch rebased. #Patch Set 12 : Patch rebased and applied the suggested changes. #
Total comments: 19
Patch Set 13 : Patch rebased and applied the suggested changes. #
Total comments: 34
Patch Set 14 : Applied suggested changes. #
Total comments: 15
Patch Set 15 : Added TODO for a correct definition of indefinite size with orthogonal flows. #Patch Set 16 : Rebaseline some tests with percentage cases. #Patch Set 17 : Removed debug guards. #Patch Set 18 : Avoid layouts during intrinsic size computation. #Patch Set 19 : Avoid layout only when not in the middle of grid layout. #Patch Set 20 : More layouts to avoid while computing intrinsic size. #Patch Set 21 : Avoid layouts, at all, during intrinsic width computation. #Patch Set 22 : Don't update container's width after track sizing. #
Total comments: 2
Patch Set 23 : Comments and assertions to make the code clearer. #Messages
Total messages: 68 (19 generated)
jfernandez@igalia.com changed reviewers: + esprehn@chromium.org, jchaffraix@chromium.org, leviw@chromium.org
https://codereview.chromium.org/815833005/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/815833005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:654: direction = direction == ForColumns ? ForRows : ForColumns; As a side note, we have a lot of direction==ForColumns checks inside RenderGrid. I remember having an inline isForColumns() at some point. I think it makes sense to re-add it.
https://codereview.chromium.org/815833005/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/815833005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1131: bool hasOrthogonalWritingMode = child->isHorizontalWritingMode() != isHorizontalWritingMode(); Why do we need to _always_ force a layout in the orthogonal writing mode case? https://codereview.chromium.org/815833005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1667: LayoutPoint childLocation = hasOrthogonalWritingMode ? LayoutPoint(rowPosition, columnPosition) : LayoutPoint(columnPosition, rowPosition); I really don't understand this line. All the offset computation should be done in the grid container's coordinate system as per CSS writing-mode: "In the positioning phase [...] calculations are performed according to the writing mode of the containing block of the box establishing the orthogonal flow." https://codereview.chromium.org/815833005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1670: childLocation.move(contentAlignmentOffset); This is also incoherent with flipping coordinates above, which means we probably don't support alignment with writing mode correctly.
Patchset #3 (id:40001) has been deleted
New patch follows a new approach based on what the specs state for handling orthogonal flows in the grid sizing algorithm. Once column and row track sizes are computed, if min-content contribution of any item has changed base on row sizes, which is the case of orthogonal flows, we will compute column tracks based on this new min-content contribution.
https://codereview.chromium.org/815833005/diff/60001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/815833005/diff/60001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:624: } I don't think we should fork this function as it's too easy to get extra bugs with the new code. Instead we should use or introduce new functions that would handle setting / getting the right value when dealing with orthogonal flows. You can find examples of this on RenderBlock with e.g. setMarginStartForChild (which checks for orthogonality) or logicalHeightForChild (which doesn't). This can be applied to other snippets of this change, when we need to get the right dimension based on the writing mode. https://codereview.chromium.org/815833005/diff/60001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1113: // contribution (once only). I wonder if we shouldn't just do this step if we know we have a child with an orthogonal writing mode as this is what it's meant to cover. https://codereview.chromium.org/815833005/diff/60001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1739: // resulting LayoutPoint will be used in 'setLogicalPosition' in order to set the child's setLogicalPosition?
New patch added with the suggested changes. https://codereview.chromium.org/815833005/diff/60001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/815833005/diff/60001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:624: } On 2015/01/29 13:43:48, Julien Chaffraix - AEST wrote: > I don't think we should fork this function as it's too easy to get extra bugs > with the new code. > > Instead we should use or introduce new functions that would handle setting / > getting the right value when dealing with orthogonal flows. You can find > examples of this on RenderBlock with e.g. setMarginStartForChild (which checks > for orthogonality) or logicalHeightForChild (which doesn't). > > This can be applied to other snippets of this change, when we need to get the > right dimension based on the writing mode. Done. https://codereview.chromium.org/815833005/diff/60001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1113: // contribution (once only). On 2015/01/29 13:43:48, Julien Chaffraix - AEST wrote: > I wonder if we shouldn't just do this step if we know we have a child with an > orthogonal writing mode as this is what it's meant to cover. I think it's a better approach, indeed. https://codereview.chromium.org/815833005/diff/60001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1739: // resulting LayoutPoint will be used in 'setLogicalPosition' in order to set the child's On 2015/01/29 13:43:48, Julien Chaffraix - AEST wrote: > setLogicalPosition? Sorry, I meant setLogicalLocation. This function transposes the LayoutPoint received as argument depending on the container's writing mode. My theory is that setLogicalLocation misses orthogonal flows support, which in that case should just use the point as it's passed, independently of the container's writing mode. We could perhaps adding such support to the RenderBox::setLogicalLocation but I thought it would be safer handling in just in RenderGrid logic.
More comments but it's looking much better. https://codereview.chromium.org/815833005/diff/80001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/815833005/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:634: child.setOverrideContainingBlockContentLogicalHeight(-1); We do 2 hash lookups in the percentage logical height case. We can avoid it by doing: LayoutUnit logicalBreadthInlineDirection = child.style()->logicalHeight().isPercent() ? -1 : breadth; https://codereview.chromium.org/815833005/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:638: LayoutUnit RenderGrid::gridAreaBreadthForChildInlineDirection(const RenderBox& child, Vector<GridTrack>& tracksInlineDirection) const This name is confusing and I still am unsure I understand the meaning of 'inline direction' as meant by this function. https://codereview.chromium.org/815833005/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:663: LayoutUnit RenderGrid::minContentForChild(RenderBox& child, GridTrackSizingDirection direction, Vector<GridTrack>& tracksInlineDirection) In correct English, this would be tracksInInlineDirection. I don't understand what's wrong with using column / row direction insofar as it is clear we are talking about the grid container's direction. https://codereview.chromium.org/815833005/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1120: } This seems super complicated to me. We don't *need* to do this computation in logical coordinates and it should be easier in physical ones. // You need to implement the trivial physical function for the override containing block sizes. LayoutUnit containingBlockContentHeight = child.hasOverrideContainingBlockHeight() ? child.overrideContainingBlockContentHeight() : LayoutUnit(); return child.height() > containingBlockContentHeight;
Added a new patch with some of the suggested change. However, I've got questions regarding some of the suggestions. I'm not completely satisfied with the Inline (or even InInline) term in the function names, but I think it describes well the nature of the function and that it takes orthogonality into account. https://codereview.chromium.org/815833005/diff/80001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/815833005/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:634: child.setOverrideContainingBlockContentLogicalHeight(-1); On 2015/02/18 23:00:07, Julien Chaffraix - PST wrote: > We do 2 hash lookups in the percentage logical height case. We can avoid it by > doing: > Actually we don't; we need to set both, xxxLogicalHeight and xxxlogicalWidth. However, My patch assumes that setting -1 (no possible resolution) was only useful in the percentage logical height case. In the original code we were setting -1 in all the cases. I think it has an effect anyway, as it will be used in the layoutGridItems later, so I'll modify the patch to keep the previous behavior. I have to admit that I have several doubts about the percentage height case; rego and me have been discussing about it and he shares my doubts. Actually, removing such condition does not make any test to fail, which is weird. https://codereview.chromium.org/815833005/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:638: LayoutUnit RenderGrid::gridAreaBreadthForChildInlineDirection(const RenderBox& child, Vector<GridTrack>& tracksInlineDirection) const On 2015/02/18 23:00:07, Julien Chaffraix - PST wrote: > This name is confusing and I still am unsure I understand the meaning of 'inline > direction' as meant by this function. I took the name inspired by other methods like computeLogicalWidth, where the concept InlineDirection is used in some variables. LayoutUnit containerWidthInInlineDirection = containerLogicalWidth; if (hasPerpendicularContainingBlock) containerWidthInInlineDirection = perpendicularContainingBlockLogicalHeight(); The idea is that the breadth InlineDirection is the one causing the blockDirection to be adapted to the new dimensions. This breadth can be either logicalWidth or logicalHeight depending on being under an orthogonal flows or not. The fact of being vertical or horizontal modes does not affect, that's why we are using logical dimensions; it just matter whether it's an orthogonal flow or not. I have to admit that all of this is a bit confusing to me, so I might be misunderstanding some concepts, but that's what I could get so far. Perhaps now that you understand my idea, if that's the case :), we could define a better name for these functions. https://codereview.chromium.org/815833005/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:663: LayoutUnit RenderGrid::minContentForChild(RenderBox& child, GridTrackSizingDirection direction, Vector<GridTrack>& tracksInlineDirection) On 2015/02/18 23:00:07, Julien Chaffraix - PST wrote: > In correct English, this would be tracksInInlineDirection. That s right. However, I saw source code using just Inline, where other functions and variables are using the preposition as well. I thought it was clearer the shorter name, but I had doubts too. > > I don't understand what's wrong with using column / row direction insofar as it > is clear we are talking about the grid container's direction. What makes me think that column / row concepts are not what we are looking for is that we use either column or row tracks for computing the min content based on both, container's and item's direction; whether they are orthogonal or not. In case of parallel flows we always use the column tracks to determine the row tracks breadth, independently of the container's direction. https://codereview.chromium.org/815833005/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1120: } On 2015/02/18 23:00:07, Julien Chaffraix - PST wrote: > This seems super complicated to me. We don't *need* to do this computation in > logical coordinates and it should be easier in physical ones. > Oh, you are so right. I'll implement it that way in the next patch.
leviw@chromium.org changed reviewers: + cbiesinger@chromium.org
I think this looks right, this patch is so huge it's a little hard for me to tell though. In the future it'd be nice to try to break this stuff up. We can also review small patches a lot faster. :) https://codereview.chromium.org/815833005/diff/120001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/815833005/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1079: LayoutUnit LayoutBox::overrideContainingBlockContentWidth() const logical? https://codereview.chromium.org/815833005/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1084: LayoutUnit LayoutBox::overrideContainingBlockContentHeight() const does this need the word Logical in it? that matches all the other names. https://codereview.chromium.org/815833005/diff/120001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/815833005/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:664: LayoutUnit overrideContainingBlockContentLogicalBreadthInlineDirection = gridAreaBreadthForChildInlineDirection(child, tracksInlineDirection); longest variable names ever :P https://codereview.chromium.org/815833005/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:677: return (hasOverrideHeight ? childIntrinsicHeight(child) : child.logicalHeight()) + child.marginLogicalHeight(); I find this ternary + expr thing hard to read, can we write it has two statements instead? https://codereview.chromium.org/815833005/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1343: bool LayoutGrid::childOverflowingContainingBlockHeight(const LayoutBox& child) const static? or static and file level? https://codereview.chromium.org/815833005/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1349: bool LayoutGrid::childOverflowingContainingBlockWidth(const LayoutBox& child) const these methods don't appear to use any internal state, why are they methods and not just static functions in the file? https://codereview.chromium.org/815833005/diff/120001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/815833005/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.h:145: bool childOverflowingContainingBlockHeight(const LayoutBox&) const; isFoo() ? I think you want this to be more clear that it's a test. https://codereview.chromium.org/815833005/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.h:146: bool childOverflowingContainingBlockWidth(const LayoutBox&) const; ditto
looks good to me modulo Elliott's comments https://codereview.chromium.org/815833005/diff/120001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css-grid-layout/resources/grid.css (right): https://codereview.chromium.org/815833005/diff/120001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css-grid-layout/resources/grid.css:283: -webkit-writing-mode: horizontal-tb; fyi you don't need the -webkit-prefix anymore https://codereview.chromium.org/815833005/diff/120001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/815833005/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1079: LayoutUnit LayoutBox::overrideContainingBlockContentWidth() const On 2015/10/30 23:21:31, esprehn wrote: > logical? Elliott - no, these are the non-logical versions of the existing function https://codereview.chromium.org/815833005/diff/120001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/815833005/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:607: LayoutUnit LayoutGrid::overrideContainingBlockLogicalBreadthInlineDirection(const LayoutBox& child) const Hmm, this is the grid's inline direction, not the child's inline direction, but I can't think of a way to make that clearer...
jfernandez@igalia.com changed reviewers: + rego@igalia.com, svillar@igalia.com
Added Patch Set 7 - 9 with a completely new approach. Functionality it's basically the same than in previous patches, but code is much simpler and easy to follow, I hope. The only addition in terms of logic is the changes added in Patch Set 9, which adds a new function with assumptions on how to estimate the size of the row tracks when we are still computing columns; this is required when dealing with orthogonal flows during the first step of the track sizing algorithm. This estimates size is also required during the grid container's intrinsic size computation.
Looks good except for the questions below. https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:498: tracks.grow(gridRowCount()); why is this now needed, when it wasn't before? https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1643: for (unsigned i = 0; i < nextToLastLine; ++i) Why this change now?
https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:498: tracks.grow(gridRowCount()); On 2015/11/26 02:26:16, cbiesinger wrote: > why is this now needed, when it wasn't before? Since I needed a way to detect we are using rowTracks vector before populating it with data, I decided to not initializing it so I can check out whether it was null or not. So, we have initialize the vector the first time we run the track sizing algorithm in the ForRow direction. https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1643: for (unsigned i = 0; i < nextToLastLine; ++i) On 2015/11/26 02:26:16, cbiesinger wrote: > Why this change now? I admit that this change is unrelated to the issue, but it's a silly mistake we made long time ago and,. being one line, I like to keep it. The mistake didn't cause a bug because the same value is overwritten in the next line, but I wanted to avoid doing an unnecessary lopp iteration.
lgtm
Reviewed the code part. Let's leave the tests for a follow up revision. https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:396: // TODO (lajava): Why we dont need to repeat row tracks sizing as spec suggests (I was unable to find any test case needing so) ?" We need to do that for example if we have two items in the same cell, one with horizontal writing mode and the other with vertical writing mode. In such a configuration, changing the column/row size will force one of the items to re-layout forcing us to recompute the row/column. https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:402: This should be placed before the if (hasLineIfEmpty()) block. Otherwise step 3 could compute a grid height too small to be editable. https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:686: { I'd ASSERT() on hasOverrideContainingBlockXXX() and force the caller to check it before calling this function. Returning LayoutUnit() is not correct because 0 is a valid override value. https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:724: bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode(); This is repeated so many times that we should have a private method to handle it. https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:754: } With the private method I mentioned above we could transform this in another private method which does not need the bool (would call the other one internally). Also this can be a one-liner using another ternary operator. https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1578: } I panicked when I saw this one :). We should not use hasDefiniteLogicalXXX in new code as it is completely wrong whenever we specify a min|max content restriction (like width: 10px; min-width: min-content;) as it only checks width/height properties. Couldn't you just check that the growthLimit() of the track is not -1? https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1585: for (GridSpan::iterator trackPosition = span.begin(); trackPosition != span.end(); ++trackPosition) { Better do: for (auto trackPosition : span) since GridSpan is iteratable https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1602: if (child.isHorizontalWritingMode() != isHorizontalWritingMode() && tracks.isEmpty()) Don't get why you need the isEmpty().
Thanks for the review. See my replies and comments inline. https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:402: On 2015/12/01 12:43:14, svillar wrote: > This should be placed before the if (hasLineIfEmpty()) block. Otherwise step 3 > could compute a grid height too small to be editable. Acknowledged. https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:686: { On 2015/12/01 12:43:14, svillar wrote: > I'd ASSERT() on hasOverrideContainingBlockXXX() and force the caller to check it > before calling this function. Returning LayoutUnit() is not correct because 0 is > a valid override value. I really wanted to return 0 (actually, it was the original implementation), since we use this method to compare such value with the one computed via gridAreaBreadthForChild. We wouldn't need a new assert in any case, since overrideContainingBlockContentLogicalX has got it already. What we could do, instead, is to call hasXXX before, so if it returns false, we will directly use the computed value, but I think such logic is equivalent and it provides no advantage. https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:724: bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode(); On 2015/12/01 12:43:14, svillar wrote: > This is repeated so many times that we should have a private method to handle > it. Acknowledged. https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:754: } On 2015/12/01 12:43:14, svillar wrote: > With the private method I mentioned above we could transform this in another > private method which does not need the bool (would call the other one > internally). > We would change an static inline method for a class method; are you really sure it'd be better ? > Also this can be a one-liner using another ternary operator. I'm not sure whether a nested ternary operator results n a clearer code. actually, I was asked several times to avoid it. https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1578: } On 2015/12/01 12:43:14, svillar wrote: > I panicked when I saw this one :). We should not use hasDefiniteLogicalXXX in > new code as it is completely wrong whenever we specify a min|max content > restriction (like width: 10px; min-width: min-content;) as it only checks > width/height properties. > > Couldn't you just check that the growthLimit() of the track is not -1? Umm, can you confirm that checking that out is enough to fulfill the specification statement of "a track having a definite max track sizing function" ? https://drafts.csswg.org/css-grid/#algo-overview https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1585: for (GridSpan::iterator trackPosition = span.begin(); trackPosition != span.end(); ++trackPosition) { On 2015/12/01 12:43:14, svillar wrote: > Better do: > > for (auto trackPosition : span) > > since GridSpan is iteratable Acknowledged. https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1602: if (child.isHorizontalWritingMode() != isHorizontalWritingMode() && tracks.isEmpty()) On 2015/12/01 12:43:14, svillar wrote: > Don't get why you need the isEmpty(). I needed a way to detect whether row tracks were already set when computing column tracks. The only/easier way was to avoid initializing the row tracks vector. That's why I use "isEmpty".
On 2015/12/01 15:13:39, jfernandez wrote: > Thanks for the review. See my replies and comments inline. > > https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): > > https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutGrid.cpp:402: > On 2015/12/01 12:43:14, svillar wrote: > > This should be placed before the if (hasLineIfEmpty()) block. Otherwise step 3 > > could compute a grid height too small to be editable. > > Acknowledged. > > https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutGrid.cpp:686: { > On 2015/12/01 12:43:14, svillar wrote: > > I'd ASSERT() on hasOverrideContainingBlockXXX() and force the caller to check > it > > before calling this function. Returning LayoutUnit() is not correct because 0 > is > > a valid override value. > > I really wanted to return 0 (actually, it was the original implementation), > since we use this method to compare such value with the one computed via > gridAreaBreadthForChild. > > We wouldn't need a new assert in any case, since > overrideContainingBlockContentLogicalX has got it already. What we could do, > instead, is to call hasXXX before, so if it returns false, we will directly use > the computed value, but I think such logic is equivalent and it provides no > advantage. > > https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutGrid.cpp:724: bool > hasOrthogonalWritingMode = child.isHorizontalWritingMode() != > isHorizontalWritingMode(); > On 2015/12/01 12:43:14, svillar wrote: > > This is repeated so many times that we should have a private method to handle > > it. > > Acknowledged. > > https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutGrid.cpp:754: } > On 2015/12/01 12:43:14, svillar wrote: > > With the private method I mentioned above we could transform this in another > > private method which does not need the bool (would call the other one > > internally). > > > > We would change an static inline method for a class method; are you really sure > it'd be better ? > > > Also this can be a one-liner using another ternary operator. > > I'm not sure whether a nested ternary operator results n a clearer code. > actually, I was asked several times to avoid it. > > https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1578: } > On 2015/12/01 12:43:14, svillar wrote: > > I panicked when I saw this one :). We should not use hasDefiniteLogicalXXX in > > new code as it is completely wrong whenever we specify a min|max content > > restriction (like width: 10px; min-width: min-content;) as it only checks > > width/height properties. > > > > Couldn't you just check that the growthLimit() of the track is not -1? > > Umm, can you confirm that checking that out is enough to fulfill the > specification statement of "a track having a definite max track sizing function" > ? > > https://drafts.csswg.org/css-grid/#algo-overview > > https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1585: for > (GridSpan::iterator trackPosition = span.begin(); trackPosition != span.end(); > ++trackPosition) { > On 2015/12/01 12:43:14, svillar wrote: > > Better do: > > > > for (auto trackPosition : span) > > > > since GridSpan is iteratable > > Acknowledged. > > https://codereview.chromium.org/815833005/diff/180001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1602: if > (child.isHorizontalWritingMode() != isHorizontalWritingMode() && > tracks.isEmpty()) > On 2015/12/01 12:43:14, svillar wrote: > > Don't get why you need the isEmpty(). > > I needed a way to detect whether row tracks were already set when computing > column tracks. The only/easier way was to avoid initializing the row tracks > vector. That's why I use "isEmpty". Actually, that changes was motivated by the need of recursively looking at the parent's style when justify-items is 'auto' because any value with the 'legacy' keyword will make the affected descendants to use the inherited value. https://drafts.csswg.org/css-align/#justify-items-property Current implementation performs an initial resolution in the StyleAdjuster class, so any value with the legacy keyword will be used to resolve 'auto' values in descendants. However, if such value is changed, via JS for instance, we would need to resolve again the auto values. That code is not present in current patch, but I think I'd need to get it back. As a matter of fact, there are some people expressing concerns on this 'legacy' keyword usage, so perhaps I'd need to contribute to that thread because it indeed add an excessive complexity to the computed style logic (is the only property needing such behavior) and it's debatable it's usefulness for the CSS designers.
Looking pretty good although we need to change some stuff. BTW the patch is IMO too big. I'd suggest to split it. One easy first task would be to create a CL with just the refactoring needed to pass sizingData instead a vector of columns and also perhaps the new functions you added to set the override sizes. That way we could later reduce the size of the patch for orthogonal flows. https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1166: LayoutUnit LayoutBlock::marginIntrinsicLogicalWidthForChild(const LayoutBox& child) const Why? https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:235: , rowTracks(0) This is weird, you're totally ignoring the passed row count https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:391: // TODO (lajava): Could we avoid iterating over all the grid items ?" I think the answer is yes. You have total control of the items in your grid because you insert them one by one. It'd be easy just to keep a boolean attribute in LayoutGrid. https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:396: computeTrackSizesForDirection(ForRows, sizingData, logicalHeight()); I guess this should be contentLogicalHeight() ? https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:501: tracks.grow(gridRowCount()); I don't think this is the right way to do this. https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:684: } See my comment above about not needing this method. https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1616: return length.isContentSized() || length.isFlex() || (length.hasPercentage() && !hasDefiniteLogicalSize(direction)); Really dangerous. We should not use hasDefiniteXXX methods, they're basically wrong.
Ill proceed with the patch simplification, but see my replies to the particular issues below. https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1166: LayoutUnit LayoutBlock::marginIntrinsicLogicalWidthForChild(const LayoutBox& child) const On 2016/03/23 11:02:00, svillar wrote: > Why? Because we use this function from assumedRowsBreadthForOrthogonalChild, which requires it to be const. https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:235: , rowTracks(0) On 2016/03/23 11:02:00, svillar wrote: > This is weird, you're totally ignoring the passed row count Yeah, not the best way to do it but since we are going to perform several iterations of the tracks sizing algorithm we need a way to detect that we haven't executed the first cycle on the row tracks. I wanted to explicitly spot this change so we can figure out better ways to do it. If we eventually decide to go with this solution, we obviously have to remove the gridRowCount argument. https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:391: // TODO (lajava): Could we avoid iterating over all the grid items ?" On 2016/03/23 11:02:00, svillar wrote: > I think the answer is yes. You have total control of the items in your grid > because you insert them one by one. It'd be easy just to keep a boolean > attribute in LayoutGrid. Ok, we can do that. https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:396: computeTrackSizesForDirection(ForRows, sizingData, logicalHeight()); On 2016/03/23 11:02:00, svillar wrote: > I guess this should be contentLogicalHeight() ? Why ? I'm not totally sure. We have just called to udpateLogicalHeight after step 2, so logicalHeight should return the right value. I'm open to discuss it, though. https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:501: tracks.grow(gridRowCount()); On 2016/03/23 11:02:00, svillar wrote: > I don't think this is the right way to do this. Remember that we only do this to be able to detect a particular situation in the track sizing alg lifecycle. https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:684: } On 2016/03/23 11:02:00, svillar wrote: > See my comment above about not needing this method. Acknowledged. https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1616: return length.isContentSized() || length.isFlex() || (length.hasPercentage() && !hasDefiniteLogicalSize(direction)); On 2016/03/23 11:02:00, svillar wrote: > Really dangerous. We should not use hasDefiniteXXX methods, they're basically > wrong. I've just used what it's being done for the case of minTrackBreadth.hasPercentage() || maxTrackBreadth.hasPercentage() in the gridTrackSize function. This is just to consider the same case. If this is wrong we might have to fix it there as well and we need a consistent way to determine whether a Length is Indefinite or not.
https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:235: , rowTracks(0) On 2016/04/01 16:39:56, jfernandez wrote: > On 2016/03/23 11:02:00, svillar wrote: > > This is weird, you're totally ignoring the passed row count > > Yeah, not the best way to do it but since we are going to perform several > iterations of the tracks sizing algorithm we need a way to detect that we > haven't executed the first cycle on the row tracks. > > I wanted to explicitly spot this change so we can figure out better ways to do > it. If we eventually decide to go with this solution, we obviously have to > remove the gridRowCount argument. I'd prefer to have a state machine where to track the different iterations (perhaps just a single enum attribute?). https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:396: computeTrackSizesForDirection(ForRows, sizingData, logicalHeight()); On 2016/04/01 16:39:56, jfernandez wrote: > On 2016/03/23 11:02:00, svillar wrote: > > I guess this should be contentLogicalHeight() ? > > Why ? I'm not totally sure. We have just called to udpateLogicalHeight after > step 2, so logicalHeight should return the right value. I'm open to discuss it, > though. logical height includes border padding.... We cannot use those for track sizing just the content size. https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:501: tracks.grow(gridRowCount()); On 2016/04/01 16:39:56, jfernandez wrote: > On 2016/03/23 11:02:00, svillar wrote: > > I don't think this is the right way to do this. > > Remember that we only do this to be able to detect a particular situation in the > track sizing alg lifecycle. Again, an state machine is much better IMO. https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1022: return maxContentForChild(gridItem, direction, sizingData); Perhaps you could refactor this patch as the sizingData refactoring was already done. That will simplify and reduce the size of this patch. https://codereview.chromium.org/815833005/diff/240001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1616: return length.isContentSized() || length.isFlex() || (length.hasPercentage() && !hasDefiniteLogicalSize(direction)); On 2016/04/01 16:39:56, jfernandez wrote: > On 2016/03/23 11:02:00, svillar wrote: > > Really dangerous. We should not use hasDefiniteXXX methods, they're basically > > wrong. > > I've just used what it's being done for the case of > minTrackBreadth.hasPercentage() || maxTrackBreadth.hasPercentage() in the > gridTrackSize function. This is just to consider the same case. If this is wrong > we might have to fix it there as well and we need a consistent way to determine > whether a Length is Indefinite or not. Yes, we know that gridTrackSize() is wrong. The hasDefiniteXXX methods are not the right way to detect what we want.
We still need some more work, but we're indeed almost there. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1128: LayoutUnit LayoutBlock::marginIntrinsicLogicalWidthForChild(const LayoutBox& child) const I know I've already said this, but this is not a good idea just because you need it in grid... https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:273: } I think it's better to merge SizingOperation and SizingState in a single enum. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:290: } Let's enclose this in debugging guards as it's meant only to be used inside DCHECK I guess. Also perhaps a more descriptive name? sanityCheck looks fine but a little bit too generic. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:391: // TODO (lajava): orthogonal flows is just one of the cases which may require which other cases? https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:434: LayoutUnit availableSpaceForRows = availableLogicalHeight(ExcludeMarginBorderPadding); Better not do this. Calling availableXXX only makes sense for definite logical heights. Also you are not using this value because you're redefining the variable later. So I'll rather keep it as it was before. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:446: availableSpaceForRows = contentLogicalHeight(); Instead you can define this variable right here. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:736: return direction == ForColumns ? child.hasOverrideContainingBlockLogicalWidth() : child.hasOverrideContainingBlockLogicalHeight(); Ups, this is a bug per se. Let's do it separately with a test. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1702: bool LayoutGrid::gridLengthIsIndefinite(const GridLength& length, GridTrackSizingDirection direction) const The name suggests that you're checking the length of the grid container. Let's use a different name because what you're actually checking is the size of a grid track. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1704: return length.isContentSized() || length.isFlex() || !hasDefiniteLogicalSize(direction); This usage of hasDefiniteLogicalSize() is not correct. Note that it checks the logical size of the grid container. Something like this: <div style="display:grid; height: min-content; grid-template-rows: 10px 10px;"> will return false for every single row which is wrong. We should definitely revisit the tests because if they are working then something weird is happening here (or perhaps we don't have enough) I think you don't need that check at all. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1707: LayoutUnit LayoutGrid::assumedRowsBreadthForOrthogonalChild(const LayoutBox& child) const Breadth -> Size https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1709: ASSERT(isOrthogonalChild(child)); DCHECK https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1716: break; This looks wrong, because the final value of gridAreaBreadth will be always its max preferred width. That's correct only when the max preferred width is larger than the size of the tracks the item spans through. Imagine the following example. An item with max preferred with of 20px which spans 3 rows defined like this "grid-template-rows: 10px max-content 50px;" The code above will return 20px as the grid area breadth which is wrong. The max-content row will be 0px because with the other 2 we have more than enough for the max preferred width of the item. So the grid area breadth will be 60px and not 20px. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1718: gridAreaBreadth += valueForLength(maxTrackBreadth.length(), LayoutUnit()); If I am not wrong the second value is the maximum which is used to resolve percentages. I'd bet that percentages (against definite heights) are not correctly resolved here and would always be 0. Do you have tests for that? https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1734: const Vector<GridTrack>& tracks = (direction == ForColumns) ? sizingData.columnTracks : sizingData.rowTracks; No need for parentheses.
Wow, thanks @svillar for such an awesome review. You have spotted several issues, which will take time to be solved. I have to think carefully about some of them.
https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1128: LayoutUnit LayoutBlock::marginIntrinsicLogicalWidthForChild(const LayoutBox& child) const On 2016/04/14 08:58:41, svillar wrote: > I know I've already said this, but this is not a good idea just because you need > it in grid... Why? This change seems like a good thing regardless.
https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1128: LayoutUnit LayoutBlock::marginIntrinsicLogicalWidthForChild(const LayoutBox& child) const On 2016/04/14 16:31:18, cbiesinger wrote: > On 2016/04/14 08:58:41, svillar wrote: > > I know I've already said this, but this is not a good idea just because you > need > > it in grid... > > Why? This change seems like a good thing regardless. As I said privately, the change seems good as the child should not change. Just that it was not 100% related to the orthogonality thing. But yeah I won't oppose to this change.
I'm preparing a patch with most of the changes you have suggested, but before submitting it I'd like to continue discussing a bit more some of the issues you commented. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:273: } On 2016/04/14 08:58:42, svillar wrote: > I think it's better to merge SizingOperation and SizingState in a single enum. I'm not sure about this, for several reasons. First, the SizingState enum defines intermediate states of the TrackSizing algorithm; the intrinsic sizing logic is torally unrelated. We can see these states as sub-states of TrackSizing. Second, there is no clear transition between IntrinsicSizeComputation and any of the TrackSizing related states. As a matter of fact, computeIntrinsicLogicalHeight uses the same Sizingdata instance being used during the TrackSizing operation, which implies we have to set the original state back. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:290: } On 2016/04/14 08:58:41, svillar wrote: > Let's enclose this in debugging guards as it's meant only to be used inside > DCHECK I guess. Also perhaps a more descriptive name? sanityCheck looks fine but > a little bit too generic. Done. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:391: // TODO (lajava): orthogonal flows is just one of the cases which may require On 2016/04/14 08:58:41, svillar wrote: > which other cases? Well, the spec states the following on this regard: "This cycle is necessary for cases where the inline size of a grid item depends on the block size of its grid area." Orthogonal flows are just an example of the above scenario; the spec mentions as well wrapped column flex containers and multi-column elements. I wonder whether there are other cases which apply directly to grid fulfilling the mentioned condition. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:434: LayoutUnit availableSpaceForRows = availableLogicalHeight(ExcludeMarginBorderPadding); On 2016/04/14 08:58:41, svillar wrote: > Better not do this. Calling availableXXX only makes sense for definite logical > heights. Also you are not using this value because you're redefining the > variable later. So I'll rather keep it as it was before. Done. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:446: availableSpaceForRows = contentLogicalHeight(); On 2016/04/14 08:58:41, svillar wrote: > Instead you can define this variable right here. Done. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1702: bool LayoutGrid::gridLengthIsIndefinite(const GridLength& length, GridTrackSizingDirection direction) const On 2016/04/14 08:58:41, svillar wrote: > The name suggests that you're checking the length of the grid container. Let's > use a different name because what you're actually checking is the size of a grid > track. Acknowledged. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1704: return length.isContentSized() || length.isFlex() || !hasDefiniteLogicalSize(direction); On 2016/04/14 08:58:42, svillar wrote: > Something like this: > <div style="display:grid; height: min-content; grid-template-rows: 10px 10px;"> > > will return false for every single row which is wrong. We should definitely > revisit the tests because if they are working then something weird is happening > here (or perhaps we don't have enough) I don't think there is anything wrong with current tests, at most, not enough coverage. However, this logic is just intended for the initial state where we don't know yet row's size for computing the orthogonal grid item's inline size. This assumed value will be overwritten later during the second cycle of the tracks sizing algorithm. > > I think you don't need that check at all. I agree. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1707: LayoutUnit LayoutGrid::assumedRowsBreadthForOrthogonalChild(const LayoutBox& child) const On 2016/04/14 08:58:41, svillar wrote: > Breadth -> Size Done. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1709: ASSERT(isOrthogonalChild(child)); On 2016/04/14 08:58:41, svillar wrote: > DCHECK Done. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1716: break; On 2016/04/14 08:58:41, svillar wrote: > This looks wrong, because the final value of gridAreaBreadth will be always its > max preferred width. That's correct only when the max preferred width is larger > than the size of the tracks the item spans through. > > Imagine the following example. An item with max preferred with of 20px which > spans 3 rows defined like this "grid-template-rows: 10px max-content 50px;" > > The code above will return 20px as the grid area breadth which is wrong. The > max-content row will be 0px because with the other 2 we have more than enough > for the max preferred width of the item. So the grid area breadth will be 60px > and not 20px. > I understand your concerns, however, I think that for the case we are using this function bot values are correct. Let me explain why so we can decide later which approach we should follow. The assumed row's size is used to determine the size of the column tracks when the item contributing to such size is orthogonal and we still don't know its block-size because it depends on its inline-size (determined based on the size of the rows track). That's why we only call this function under the following condition: if (direction == ForRows && sizingData.sizingState == GridSizingData::ColumnSizingFirstIteration) In this case, we only want to know whether tracks are definite, so we use such definite size, or not. In the later case, we will just assume we have enough space to laid out item's full content, hence the size of the column will be set as the item's minimum height. As you can see, it doesn't matter if we return 20px or 60px, because the size of the columns track will be the same in both cases. If you prefer, we can use max(max-preferred, sunmOfDefiniteTracksinSpan), https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1718: gridAreaBreadth += valueForLength(maxTrackBreadth.length(), LayoutUnit()); On 2016/04/14 08:58:41, svillar wrote: > If I am not wrong the second value is the maximum which is used to resolve > percentages. I'd bet that percentages (against definite heights) are not > correctly resolved here and would always be 0. Do you have tests for that? Your assumption on the usage of such second argument is correct. Also, you are right that we lack tests for percentage sizes and orthogonal cases. Do you think we need that for this patch ? I mean, it's expectable to provide support for all the cases combining orthogonal flows in a single patch ? Perhaps a TODO here would be enough. What do you think ? https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1734: const Vector<GridTrack>& tracks = (direction == ForColumns) ? sizingData.columnTracks : sizingData.rowTracks; On 2016/04/14 08:58:41, svillar wrote: > No need for parentheses. Done.
Description was changed from ========== [CSS Grid Layout] Handle min-content/max-content with orthogonal flows. Currently there is no support for orthogonal flows in many aspects of the Grid Layout logic. The Grid sizing algorithm should be adapted to this scenario, hence this patch focus on the min-content and max-content functions, used to resolve content based track sizes. BUG=234194 ========== to ========== [css-grid] Handle min-content/max-content with orthogonal flows. Currently there is no support for orthogonal flows in many aspects of the Grid Layout logic. The Grid sizing algorithm should be adapted to this scenario, hence this patch focus on the min-content and max-content functions, used to resolve content based track sizes. BUG=234194 ==========
Thanks for your patience. Hope my comments are useful. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1704: return length.isContentSized() || length.isFlex() || !hasDefiniteLogicalSize(direction); On 2016/05/18 10:31:40, jfernandez wrote: > On 2016/04/14 08:58:42, svillar wrote: > > > Something like this: > > <div style="display:grid; height: min-content; grid-template-rows: 10px > 10px;"> > > > > will return false for every single row which is wrong. We should definitely > > revisit the tests because if they are working then something weird is > happening > > here (or perhaps we don't have enough) > > I don't think there is anything wrong with current tests, at most, not enough > coverage. However, this logic is just intended for the initial state where we > don't know yet row's size for computing the orthogonal grid item's inline size. > This assumed value will be overwritten later during the second cycle of the > tracks sizing algorithm. > > > > > I think you don't need that check at all. > > I agree. BTW you are considering percentages as definite sizes when that is not always the case. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1716: break; On 2016/05/18 10:31:40, jfernandez wrote: > On 2016/04/14 08:58:41, svillar wrote: > > This looks wrong, because the final value of gridAreaBreadth will be always > its > > max preferred width. That's correct only when the max preferred width is > larger > > than the size of the tracks the item spans through. > > > > Imagine the following example. An item with max preferred with of 20px which > > spans 3 rows defined like this "grid-template-rows: 10px max-content 50px;" > > > > The code above will return 20px as the grid area breadth which is wrong. The > > max-content row will be 0px because with the other 2 we have more than enough > > for the max preferred width of the item. So the grid area breadth will be 60px > > and not 20px. > > > > I understand your concerns, however, I think that for the case we are using this > function bot values are correct. Let me explain why so we can decide later which > approach we should follow. > > The assumed row's size is used to determine the size of the column tracks when > the item contributing to such size is orthogonal and we still don't know its > block-size because it depends on its inline-size (determined based on the size > of the rows track). That's why we only call this function under the following > condition: > > if (direction == ForRows && sizingData.sizingState == > GridSizingData::ColumnSizingFirstIteration) If this is the case then we should have some ASSERTs verifying that. > In this case, we only want to know whether tracks are definite, so we use such > definite size, or not. In the later case, we will just assume we have enough > space to laid out item's full content, hence the size of the column will be set > as > the item's minimum height. > > As you can see, it doesn't matter if we return 20px or 60px, because the size of > the columns track will be the same in both cases. > > If you prefer, we can use max(max-preferred, sunmOfDefiniteTracksinSpan), I understand what you mean, but still I prefer the function to return a valid value, if it's computing the area breadth then it should return that, not the item's max inline size. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1718: gridAreaBreadth += valueForLength(maxTrackBreadth.length(), LayoutUnit()); On 2016/05/18 10:31:40, jfernandez wrote: > On 2016/04/14 08:58:41, svillar wrote: > > If I am not wrong the second value is the maximum which is used to resolve > > percentages. I'd bet that percentages (against definite heights) are not > > correctly resolved here and would always be 0. Do you have tests for that? > > Your assumption on the usage of such second argument is correct. Also, you are > right that we lack tests for percentage sizes and orthogonal cases. > > Do you think we need that for this patch ? I mean, it's expectable to provide > support for all the cases combining orthogonal flows in a single patch ? Perhaps > a TODO here would be enough. What do you think ? Yes I expect it to work for all the potential cases. If we cannot do that then perhaps it means that we should split the patch in smaller pieces.
https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1704: return length.isContentSized() || length.isFlex() || !hasDefiniteLogicalSize(direction); On 2016/05/26 08:49:02, svillar wrote: > On 2016/05/18 10:31:40, jfernandez wrote: > > On 2016/04/14 08:58:42, svillar wrote: > > > > > Something like this: > > > <div style="display:grid; height: min-content; grid-template-rows: 10px > > 10px;"> > > > > > > will return false for every single row which is wrong. We should definitely > > > revisit the tests because if they are working then something weird is > > happening > > > here (or perhaps we don't have enough) > > > > I don't think there is anything wrong with current tests, at most, not enough > > coverage. However, this logic is just intended for the initial state where we > > don't know yet row's size for computing the orthogonal grid item's inline > size. > > This assumed value will be overwritten later during the second cycle of the > > tracks sizing algorithm. > > > > > > > > I think you don't need that check at all. > > > > I agree. > > BTW you are considering percentages as definite sizes when that is not always > the case. Note that the GridLength is extracted suing the gridTracksize function, which already deals with percentages, so either they are converted to 'auto', hence content-sized, or they are considered as definite. Anyway, I admit this "isIndefinite" function is not very clear. I'd try to implement something better. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1716: break; On 2016/05/26 08:49:02, svillar wrote: > On 2016/05/18 10:31:40, jfernandez wrote: > > On 2016/04/14 08:58:41, svillar wrote: > > > This looks wrong, because the final value of gridAreaBreadth will be always > > its > > > max preferred width. That's correct only when the max preferred width is > > larger > > > than the size of the tracks the item spans through. > > > > > > Imagine the following example. An item with max preferred with of 20px which > > > spans 3 rows defined like this "grid-template-rows: 10px max-content 50px;" > > > > > > The code above will return 20px as the grid area breadth which is wrong. The > > > max-content row will be 0px because with the other 2 we have more than > enough > > > for the max preferred width of the item. So the grid area breadth will be > 60px > > > and not 20px. > > > > > > > I understand your concerns, however, I think that for the case we are using > this > > function bot values are correct. Let me explain why so we can decide later > which > > approach we should follow. > > > > The assumed row's size is used to determine the size of the column tracks when > > the item contributing to such size is orthogonal and we still don't know its > > block-size because it depends on its inline-size (determined based on the size > > of the rows track). That's why we only call this function under the following > > condition: > > > > if (direction == ForRows && sizingData.sizingState == > > GridSizingData::ColumnSizingFirstIteration) > > If this is the case then we should have some ASSERTs verifying that. I don't think ASSERT that makes sense. One should know when we need to estimate the size, because it's not possible to be determined at this phase of the track sizing algorithm. However, I agree with you on that not being a valid argument for not retuning a precise value, as you suggest in the comment below. > > > In this case, we only want to know whether tracks are definite, so we use such > > definite size, or not. In the later case, we will just assume we have enough > > space to laid out item's full content, hence the size of the column will be > set > > as > > the item's minimum height. > > > > As you can see, it doesn't matter if we return 20px or 60px, because the size > of > > the columns track will be the same in both cases. > > > > If you prefer, we can use max(max-preferred, sunmOfDefiniteTracksinSpan), > > I understand what you mean, but still I prefer the function to return a valid > value, if it's computing the area breadth then it should return that, not the > item's max inline size. I understand your point and will provide a change in that direction. https://codereview.chromium.org/815833005/diff/260001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1718: gridAreaBreadth += valueForLength(maxTrackBreadth.length(), LayoutUnit()); On 2016/05/26 08:49:02, svillar wrote: > On 2016/05/18 10:31:40, jfernandez wrote: > > On 2016/04/14 08:58:41, svillar wrote: > > > If I am not wrong the second value is the maximum which is used to resolve > > > percentages. I'd bet that percentages (against definite heights) are not > > > correctly resolved here and would always be 0. Do you have tests for that? > > > > Your assumption on the usage of such second argument is correct. Also, you are > > right that we lack tests for percentage sizes and orthogonal cases. > > > > Do you think we need that for this patch ? I mean, it's expectable to provide > > support for all the cases combining orthogonal flows in a single patch ? > Perhaps > > a TODO here would be enough. What do you think ? > > Yes I expect it to work for all the potential cases. If we cannot do that then > perhaps it means that we should split the patch in smaller pieces. Ill fix the case of percentage sizes for tracks and provide tests to verify it works as expected with orthogonal flows.
We're almost there. I like how the patch has evolved. https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html (right): https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html:23: } Do we really need this? We stretch by default. https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:254: void stateTransition() Nit: what about nextState() ? https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:404: } I think we don't need this, see my comment bellow. https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1373: m_hasAnyOrthogonalChild = m_hasAnyOrthogonalChild || isOrthogonalChild(*child); That is not enough. Note that you could dynamically add and remove items. Since m_hasAnyOrthogonalChild is only used by repeatTracksSizingIfNeeded() I prefer to keep it outside of the class. It's enough to have a local variable in the layout method. Also the repeatTrackSizingIfNeeded is not really needed. https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1836: if (maxTrackSize.isContentSized() || maxTrackSize.isFlex()) I think this is still not correct for percentages of indefinite sizes. For example if the grid container height is auto and the row is a percentage then you'll call valueForLength() but you should not. We need tests for those indefinite sizes and percentages. https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1853: return assumedRowsSizeForOrthogonalChild(child); So assumedRowsSizeForOrthogonalChild() has an assert for orthogonal childs but you're calling it here for every single child.
I'll provide a new patch with some small changes suggested in the last review. However, most of the comments still need further discussion. https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html (right): https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html:23: } On 2016/06/02 14:39:40, svillar wrote: > Do we really need this? We stretch by default. Yes, we need it because we don't allow (yet) stretch alignment with orthogonal flows. https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:254: void stateTransition() On 2016/06/02 14:39:40, svillar wrote: > Nit: what about nextState() ? Done. https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:404: } On 2016/06/02 14:39:40, svillar wrote: > I think we don't need this, see my comment bellow. I disagree, I think the function adds value, see my detailed reply below. https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1373: m_hasAnyOrthogonalChild = m_hasAnyOrthogonalChild || isOrthogonalChild(*child); On 2016/06/02 14:39:40, svillar wrote: > That is not enough. Note that you could dynamically add and remove items. > I see, I'll change how I identify orthogonality. > Since m_hasAnyOrthogonalChild is only used by repeatTracksSizingIfNeeded() I > prefer to keep it outside of the class. It's enough to have a local variable in > the layout method. Also the repeatTrackSizingIfNeeded is not really needed. I understand, and partially agree, your point. However, I really think the function adds value to the grid layout logic. First, it's intended to encapsulate the state validation, in the same way the computeTrackSizesForDirection function does. On the other hand, it tries to encapsulate the conditions which may trigger these extra cycles of the tracks sizing algorithm. As the comment in the code states, the orthogonality between grid container and items is only one of these cases that would require these extra steps, but there are other. https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1836: if (maxTrackSize.isContentSized() || maxTrackSize.isFlex()) On 2016/06/02 14:39:40, svillar wrote: > I think this is still not correct for percentages of indefinite sizes. For > example if the grid container height is auto and the row is a percentage then > you'll call valueForLength() but you should not. I don't understand what you mean; as far as I know, gridTrackForSize return Length(auto) for the case you describe, doesn't it ? In that case, that GridLength would be content sized, right ? >We need tests for those indefinite sizes and percentages. Aren't all the cases in the grid-track-sizing-with-percentages-and-orthogonal-flows test exactly the ones you are requesting ? https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1853: return assumedRowsSizeForOrthogonalChild(child); On 2016/06/02 14:39:40, svillar wrote: > So assumedRowsSizeForOrthogonalChild() has an assert for orthogonal childs but > you're calling it here for every single child. If we are determining column track's size in the ForRows direction it has to be an orthogonal child.
lgtm https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1836: if (maxTrackSize.isContentSized() || maxTrackSize.isFlex()) On 2016/06/02 21:39:25, jfernandez wrote: > On 2016/06/02 14:39:40, svillar wrote: > > I think this is still not correct for percentages of indefinite sizes. For > > example if the grid container height is auto and the row is a percentage then > > you'll call valueForLength() but you should not. > > I don't understand what you mean; as far as I know, gridTrackForSize return > Length(auto) for the case you describe, doesn't it ? In that case, that > GridLength would be content sized, right ? Yes you're right, sorry for the noise. https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1853: return assumedRowsSizeForOrthogonalChild(child); On 2016/06/02 21:39:25, jfernandez wrote: > On 2016/06/02 14:39:40, svillar wrote: > > So assumedRowsSizeForOrthogonalChild() has an assert for orthogonal childs but > > you're calling it here for every single child. > > If we are determining column track's size in the ForRows direction it has to be > an > orthogonal child. OK. In any case as we depend on the value of an state, and knowing that keeping an state up to date is hard, I'd add a DCHECK(isOrthogonalChild(child)) here which does not harm.
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org, svillar@igalia.com Link to the patchset: https://codereview.chromium.org/815833005/#ps320001 (title: "Reabline some tests with percentage cases.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/815833005/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org, svillar@igalia.com Link to the patchset: https://codereview.chromium.org/815833005/#ps340001 (title: "Removed debug guards.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/815833005/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/06/13 18:00:03, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) On 2016/06/13 18:00:03, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) @christian, it seems that we are doing some weird things with the SubtreeLayoutScope in grid, but I wonder why this is only failing in the max and win bots. The idea we had is to use the subtree scope to ensure the grid item is laid out before using its size to compute auto-sized grid tracks. It's really weird, though, that win and mac bots raise such assertion, since we use the subtree layout scope structure during the grid layout algorithm, so how it's possible the Document is not in PerformLayout state ? Also, why is this crashing only in the orthogonal flow cases ?
On 2016/06/14 10:17:33, jfernandez wrote: > On 2016/06/13 18:00:03, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > On 2016/06/13 18:00:03, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > @christian, it seems that we are doing some weird things with the > SubtreeLayoutScope in grid, but > I wonder why this is only failing in the max and win bots. The idea we had is to > use the subtree > scope to ensure the grid item is laid out before using its size to compute > auto-sized grid tracks. > > It's really weird, though, that win and mac bots raise such assertion, since we > use the subtree > layout scope structure during the grid layout algorithm, so how it's possible > the Document > is not in PerformLayout state ? Also, why is this crashing only in the > orthogonal flow cases ? I don't know why this is only in the orthogonal flow case but here are some points: 1) I previously noticed that grid uses SubtreeLayoutScopes in an unusual way, in that it creates them in various places in the code instead of a single one in layoutBlock that's then passed down 2) In this specific stack trace, there is in fact no layout going on. It is just trying to compute preferred widths. You should not do layout while computing preferred widths!! 2a) this may be mac-specific, as mac has a feature where they need preferred widths in order to size the Chrome window in some cases. But I can't quite tell from the stack trace if that is what's going on 3) I believe the windows failures are unrelated
On 2016/06/14 10:46:23, cbiesinger wrote: > On 2016/06/14 10:17:33, jfernandez wrote: > > On 2016/06/13 18:00:03, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > > > On 2016/06/13 18:00:03, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > > > @christian, it seems that we are doing some weird things with the > > SubtreeLayoutScope in grid, but > > I wonder why this is only failing in the max and win bots. The idea we had is > to > > use the subtree > > scope to ensure the grid item is laid out before using its size to compute > > auto-sized grid tracks. > > > > It's really weird, though, that win and mac bots raise such assertion, since > we > > use the subtree > > layout scope structure during the grid layout algorithm, so how it's possible > > the Document > > is not in PerformLayout state ? Also, why is this crashing only in the > > orthogonal flow cases ? > > I don't know why this is only in the orthogonal flow case but here are some Thanks to your insights, I can answer that question too :) > points: > 1) I previously noticed that grid uses SubtreeLayoutScopes in an unusual way, in > that it creates them in various places in the code instead of a single one in > layoutBlock that's then passed down Yeah, I already read your comment some time ago related to that. We must definitively review this issue ASAP. > 2) In this specific stack trace, there is in fact no layout going on. It is just > trying to compute preferred widths. You should not do layout while computing > preferred widths!! Oh, thanks !!! That's actually the root cause of the problem and I know now why it's happening. For an orthogonal flow case, container width depends on item's height. As item is content-sized, it required to perform a layout in order to compute its height. So, if I understand correctly, we should avoid this layout during intrinsic size computation, even than it will lead to 0px size, right ? > 2a) this may be mac-specific, as mac has a feature where they need preferred > widths in order to size the Chrome window in some cases. But I can't quite tell > from the stack trace if that is what's going on That would explain very well this issue, indeed. > 3) I believe the windows failures are unrelated Yeah, I agree.
> So, if I understand correctly, we should avoid this layout during intrinsic size > computation, even than it will lead to 0px size, right ? Yeah, that's correct, even if the end result is kinda undesirable. We have that issue elsewhere too. We made it somewhat better with a separate layout phase for orthogonal roots (https://bugs.chromium.org/p/chromium/issues/detail?id=550963#c15) but it is still a problem for some cases (wrapping column flexboxes have issues, for example, even without orthogonal flows)
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org, svillar@igalia.com Link to the patchset: https://codereview.chromium.org/815833005/#ps440001 (title: "Don't update container's width after track sizing.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/815833005/440001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/815833005/diff/440001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/815833005/diff/440001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:934: return child.logicalHeight() + child.marginLogicalHeight(); I may be misreading this code, but I think this will return outdated layout information of the child. That's not good -- it means that what this function returns depends on which previous layouts may have happened, which means that the end size depends on the previous state as well. I think you may want this function: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/815833005/diff/280001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1853: return assumedRowsSizeForOrthogonalChild(child); On 2016/06/10 08:08:35, svillar wrote: > On 2016/06/02 21:39:25, jfernandez wrote: > > On 2016/06/02 14:39:40, svillar wrote: > > > So assumedRowsSizeForOrthogonalChild() has an assert for orthogonal childs > but > > > you're calling it here for every single child. > > > > If we are determining column track's size in the ForRows direction it has to > be > > an > > orthogonal child. > > OK. In any case as we depend on the value of an state, and knowing that keeping > an state up to date is hard, I'd add a DCHECK(isOrthogonalChild(child)) here > which does not harm. Done. https://codereview.chromium.org/815833005/diff/440001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/815833005/diff/440001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:934: return child.logicalHeight() + child.marginLogicalHeight(); On 2016/06/21 14:45:46, cbiesinger wrote: > I may be misreading this code, but I think this will return outdated layout > information of the child. That's not good -- it means that what this function > returns depends on which previous layouts may have happened, which means that > the end size depends on the previous state as well. I think you may want this > function: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... This is a good point, but I'm not sure it's what we need here. First, since we have already laid out all the orthogonal grid items (FrameView) I think that during the intrinsic size computation those boxed don't need to be laid out again, hence we can use the previously computed logicalHeight. In case we are not computing the intrinsic width, we will perform a layout on any grid item if its container (the grid area) size has changed; obviously if it needs a layout too. Hence, I think that using the computed logicalHeight is correct for this case. Additionally, if I understand it correctly, computeLogicalHeightWithoutLayout returns just the border and padding size if box's height is not explicitly set. We will have incorrect results for intrinsic and auto sized grid items. If nobody objects, I'll land the patch as it is and will revisit this issue when we find out some case incorrectly rendered because of this logica.
Description was changed from ========== [css-grid] Handle min-content/max-content with orthogonal flows. Currently there is no support for orthogonal flows in many aspects of the Grid Layout logic. The Grid sizing algorithm should be adapted to this scenario, hence this patch focus on the min-content and max-content functions, used to resolve content based track sizes. BUG=234194 ========== to ========== [css-grid] Handle min-content/max-content with orthogonal flows Currently there is no support for orthogonal flows in many aspects of the Grid Layout logic. The Grid sizing algorithm should be adapted to this scenario, hence this patch focus on the min-content and max-content functions, used to resolve content based track sizes. BUG=234194 ==========
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org, svillar@igalia.com Link to the patchset: https://codereview.chromium.org/815833005/#ps460001 (title: "Comments and assertions to make the code clearer.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/815833005/460001
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Handle min-content/max-content with orthogonal flows Currently there is no support for orthogonal flows in many aspects of the Grid Layout logic. The Grid sizing algorithm should be adapted to this scenario, hence this patch focus on the min-content and max-content functions, used to resolve content based track sizes. BUG=234194 ========== to ========== [css-grid] Handle min-content/max-content with orthogonal flows Currently there is no support for orthogonal flows in many aspects of the Grid Layout logic. The Grid sizing algorithm should be adapted to this scenario, hence this patch focus on the min-content and max-content functions, used to resolve content based track sizes. BUG=234194 ==========
Message was sent while issue was closed.
Committed patchset #23 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Handle min-content/max-content with orthogonal flows Currently there is no support for orthogonal flows in many aspects of the Grid Layout logic. The Grid sizing algorithm should be adapted to this scenario, hence this patch focus on the min-content and max-content functions, used to resolve content based track sizes. BUG=234194 ========== to ========== [css-grid] Handle min-content/max-content with orthogonal flows Currently there is no support for orthogonal flows in many aspects of the Grid Layout logic. The Grid sizing algorithm should be adapted to this scenario, hence this patch focus on the min-content and max-content functions, used to resolve content based track sizes. BUG=234194 Committed: https://crrev.com/fe619f7270786d15f9b2de8962835d47b1e7823b Cr-Commit-Position: refs/heads/master@{#401246} ==========
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/fe619f7270786d15f9b2de8962835d47b1e7823b Cr-Commit-Position: refs/heads/master@{#401246}
Message was sent while issue was closed.
On 2016/06/21 21:50:03, jfernandez wrote: > https://codereview.chromium.org/815833005/diff/440001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutGrid.cpp:934: return > child.logicalHeight() + child.marginLogicalHeight(); > On 2016/06/21 14:45:46, cbiesinger wrote: > > I may be misreading this code, but I think this will return outdated layout > > information of the child. That's not good -- it means that what this function > > returns depends on which previous layouts may have happened, which means that > > the end size depends on the previous state as well. I think you may want this > > function: > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > > This is a good point, but I'm not sure it's what we need here. First, since we > have already laid out all the orthogonal grid items (FrameView) I think that > during the intrinsic size computation those boxed don't need to be laid out > again, hence we can use the previously computed logicalHeight. > > In case we are not computing the intrinsic width, we will perform a layout on > any grid item if its container (the grid area) size has changed; obviously if it > needs a layout too. Hence, I think that using the computed logicalHeight is > correct for this case. > > Additionally, if I understand it correctly, computeLogicalHeightWithoutLayout > returns just the border and padding size if box's height is not explicitly set. > We will have incorrect results for intrinsic and auto sized grid items. > > If nobody objects, I'll land the patch as it is and will revisit this issue when > we find out some case incorrectly rendered because of this logica. Fair point. I might still switch to using computeChildPreferredLogicalWidths for this case (it uses logicalHeight if no layout is pending and computeLogicalHeightWithoutLayout otherwise) just to be on the safe side, if used while we're inside of computePreferredLogicalWidths
Message was sent while issue was closed.
On 2016/06/22 18:33:01, cbiesinger wrote: > On 2016/06/21 21:50:03, jfernandez wrote: > > > https://codereview.chromium.org/815833005/diff/440001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/LayoutGrid.cpp:934: return > > child.logicalHeight() + child.marginLogicalHeight(); > > On 2016/06/21 14:45:46, cbiesinger wrote: > > > I may be misreading this code, but I think this will return outdated layout > > > information of the child. That's not good -- it means that what this > function > > > returns depends on which previous layouts may have happened, which means > that > > > the end size depends on the previous state as well. I think you may want > this > > > function: > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > > > > This is a good point, but I'm not sure it's what we need here. First, since we > > have already laid out all the orthogonal grid items (FrameView) I think that > > during the intrinsic size computation those boxed don't need to be laid out > > again, hence we can use the previously computed logicalHeight. > > > > In case we are not computing the intrinsic width, we will perform a layout on > > any grid item if its container (the grid area) size has changed; obviously if > it > > needs a layout too. Hence, I think that using the computed logicalHeight is > > correct for this case. > > > > Additionally, if I understand it correctly, computeLogicalHeightWithoutLayout > > returns just the border and padding size if box's height is not explicitly > set. > > We will have incorrect results for intrinsic and auto sized grid items. > > > > If nobody objects, I'll land the patch as it is and will revisit this issue > when > > we find out some case incorrectly rendered because of this logica. > > Fair point. I might still switch to using computeChildPreferredLogicalWidths for > this case (it uses logicalHeight if no layout is pending and > computeLogicalHeightWithoutLayout otherwise) just to be on the safe side, if > used while we're inside of computePreferredLogicalWidths Yes, I think we definitively want to use computeChildPreferredLogicalWidths somehow. The issue is that I didn't figure out how to do it without changing too much of our laoyut logic for doing so. I'll discuss it with @svillar as soon as possible. |
