Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2333583002:
[css-grid] Update intrinsic size for the extra sizing alg iteration. (Closed)
|
Created:
4 years, 3 months ago by jfernandez Modified:
3 years, 10 months ago CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, svillar, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, Manuel Rego, leviw+renderwatch, jchaffraix+rendering, blink-reviews, eae+blinkwatch Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[css-grid] Update intrinsic size for the extra sizing alg iteration.
The spec states that in case any grid item has changed its 'min content
contribution' we must repeat the track sizing algorithm.
We were doing so without updating the grid contaner's intrinsic size.
Hence, when the grid container was sized under min-content/max-content
constraints, we were using an incorrect available space for sizing the
column and row tracks.
This patchs ensures that any orthogonal flows item is laid out before
evaluating its 'min size contribution'. Also, if any item has a new
size contribution, we assume the grid container's intrinsic size has
changed, so it's recomputed.
BUG=628565
Patch Set 1 #
Total comments: 5
Messages
Total messages: 22 (4 generated)
Description was changed from ========== [css-grid] Update intrinsic size for the extra sizing alg iteration. The spec states that in case any grid item has changed its 'min content contribution' we must repeat the track sizing algorithm. We were doing so without updating the grid contaner's intrinsic size. Hence, when the grid container was sized under min-content/max-content constraints, we were using an incorrect available space for sizing the column and row tracks. This patchs ensures that any orthogonal flows item is laid out before evaluating its 'min size contribution'. Also, if any item has a new size contribution, we assune the grid container's intrinsic size has changed, so it's recomputed. BUG=628565 ========== to ========== [css-grid] Update intrinsic size for the extra sizing alg iteration. The spec states that in case any grid item has changed its 'min content contribution' we must repeat the track sizing algorithm. We were doing so without updating the grid contaner's intrinsic size. Hence, when the grid container was sized under min-content/max-content constraints, we were using an incorrect available space for sizing the column and row tracks. This patchs ensures that any orthogonal flows item is laid out before evaluating its 'min size contribution'. Also, if any item has a new size contribution, we assune the grid container's intrinsic size has changed, so it's recomputed. BUG=628565 ==========
jfernandez@igalia.com changed reviewers: + cbiesinger@chromium.org, rego@igalia.com, svillar@igalia.com
Description was changed from ========== [css-grid] Update intrinsic size for the extra sizing alg iteration. The spec states that in case any grid item has changed its 'min content contribution' we must repeat the track sizing algorithm. We were doing so without updating the grid contaner's intrinsic size. Hence, when the grid container was sized under min-content/max-content constraints, we were using an incorrect available space for sizing the column and row tracks. This patchs ensures that any orthogonal flows item is laid out before evaluating its 'min size contribution'. Also, if any item has a new size contribution, we assune the grid container's intrinsic size has changed, so it's recomputed. BUG=628565 ========== to ========== [css-grid] Update intrinsic size for the extra sizing alg iteration. The spec states that in case any grid item has changed its 'min content contribution' we must repeat the track sizing algorithm. We were doing so without updating the grid contaner's intrinsic size. Hence, when the grid container was sized under min-content/max-content constraints, we were using an incorrect available space for sizing the column and row tracks. This patchs ensures that any orthogonal flows item is laid out before evaluating its 'min size contribution'. Also, if any item has a new size contribution, we assume the grid container's intrinsic size has changed, so it's recomputed. BUG=628565 ==========
https://codereview.chromium.org/2333583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2333583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:438: if (!minContentContributionChanged) { I don't get this. For example for the first orthogonal child you will never check if the content contribution has changed or not. More tests needed? https://codereview.chromium.org/2333583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:449: } I have many doubts about the above block. It's like a very reduced version of our layoutBlock() method. I am not sure this is correct. As mentioned in private chats I think we should let the generic code call our layout() method again.
@cbiesigner, what do you think ? https://codereview.chromium.org/2333583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2333583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:438: if (!minContentContributionChanged) { On 2016/09/13 13:44:18, svillar wrote: > I don't get this. For example for the first orthogonal child you will never > check if the content contribution has changed or not. More tests needed? Umm, why "first orthogonal child" ? We definitively do it for the first one, and for the rest of orthogonal children until we find one. After that, we don't need to continue checking that because it's enough that just one item has changed its contribution to repeat the whole algorithm. We need to complete the loop, though, because we must laid out all the orthogonal children. I admit that there is room for improvement here, since we probably only need to laid out certain items, but let's be defensive for now. https://codereview.chromium.org/2333583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:449: } On 2016/09/13 13:44:18, svillar wrote: > I have many doubts about the above block. It's like a very reduced version of > our layoutBlock() method. I am not sure this is correct. As mentioned in private > chats I think we should let the generic code call our layout() method again. I understand your concerns. However, notice that we run all the algorithm's pieces which are relevant for this logic. We just do a simplified layout of the items, ignoring the alignment and margin logic. Perhaps the main issue is to trigger the intrinsic size computation in the middle of a layout. The alternative, as we discussed, is to mark the grid container as still needing to be laid out and also its intrinsic sizes as dirty. However, this seems to contradict what SubtreeLayoutSCope is trying to ensure, since we are running this layout logic inside a closed scope. We may need to change that, or perhaps there is a layout API I'm not aware of to schedule a new layout of the grid container as soon the one ongoing one is completed.
jfernandez@igalia.com changed reviewers: + mstensho@opera.com
@mstensho any idea of how to force a relayout of the grid container, so we can avoid to re-compute intrinsic size internally ? As I explained before, we need to do it because some orthogonal children may have changed its min-content contribution.
On 2016/09/16 12:28:09, jfernandez wrote: > @mstensho any idea of how to force a relayout of the grid container, so we can > avoid to re-compute intrinsic size internally ? As I explained before, we need > to do it because some orthogonal children may have changed its min-content > contribution. Don't we have the "orthogonal writing mode roots" machinery in FrameView to deal with min/max width calculation of such objects?
On 2016/09/20 07:37:51, mstensho wrote: > On 2016/09/16 12:28:09, jfernandez wrote: > > @mstensho any idea of how to force a relayout of the grid container, so we can > > avoid to re-compute intrinsic size internally ? As I explained before, we need > > to do it because some orthogonal children may have changed its min-content > > contribution. > > Don't we have the "orthogonal writing mode roots" machinery in FrameView to deal > with min/max width calculation of such objects? Yes, the problem is that grid items have Grid Areas as containing blocks. At the time FrameView executes its logic for orthogonal flows, Grid Areas have not been set, yet. Later, during grid layout, we determine Grid Areas size based on grid items layout. However, once we set the Grid Areas size, some orthogonal grid items may have changed its min-content contribution, so as Grid Layout spec states [1], we should repeat the Grid Areas sizing algorithm with the new min-content contribution. As far as I now, FrameView operates over ANY orthogonal box in the Layout Tree; we don't need that. In any case, the actual issue is related to the grid container's intrinsic size. In order to execute the track sizing algorithm, we need to determine the available size we have for the content-sized tracks. This is determined based on container's intrinsic size. However, orthogonal grid items may have changed container's intrinsic size once they were laid out inside their respective Grid Areas. [1] https://drafts.csswg.org/css-grid/#algo-overview
On 2016/09/20 08:10:04, jfernandez wrote: > On 2016/09/20 07:37:51, mstensho wrote: > > On 2016/09/16 12:28:09, jfernandez wrote: > > > @mstensho any idea of how to force a relayout of the grid container, so we > can > > > avoid to re-compute intrinsic size internally ? As I explained before, we > need > > > to do it because some orthogonal children may have changed its min-content > > > contribution. > > > > Don't we have the "orthogonal writing mode roots" machinery in FrameView to > deal > > with min/max width calculation of such objects? > > Yes, the problem is that grid items have Grid Areas as containing blocks. At the > time > FrameView executes its logic for orthogonal flows, Grid Areas have not been set, > yet. > Later, during grid layout, we determine Grid Areas size based on grid items > layout. > However, once we set the Grid Areas size, some orthogonal grid items may have > changed its min-content contribution, so as Grid Layout spec states [1], we > should > repeat the Grid Areas sizing algorithm with the new min-content contribution. > > As far as I now, FrameView operates over ANY orthogonal box in the Layout Tree; > we don't need that. In any case, the actual issue is related to the grid > container's > intrinsic size. In order to execute the track sizing algorithm, we need to > determine > the available size we have for the content-sized tracks. This is determined > based > on container's intrinsic size. However, orthogonal grid items may have changed > container's intrinsic size once they were laid out inside their respective Grid > Areas. > > [1] https://drafts.csswg.org/css-grid/#algo-overview It seems wrong to me for the size of some ancestor to affect the intrinsic size of an element. Intrinsic sizes are used to determine the size of an ancestor, not the only way around.
On 2016/09/20 11:38:41, mstensho wrote: > On 2016/09/20 08:10:04, jfernandez wrote: > > On 2016/09/20 07:37:51, mstensho wrote: > > > On 2016/09/16 12:28:09, jfernandez wrote: > > > > @mstensho any idea of how to force a relayout of the grid container, so we > > can > > > > avoid to re-compute intrinsic size internally ? As I explained before, we > > need > > > > to do it because some orthogonal children may have changed its min-content > > > > contribution. > > > > > > Don't we have the "orthogonal writing mode roots" machinery in FrameView to > > deal > > > with min/max width calculation of such objects? > > > > Yes, the problem is that grid items have Grid Areas as containing blocks. At > the > > time > > FrameView executes its logic for orthogonal flows, Grid Areas have not been > set, > > yet. > > Later, during grid layout, we determine Grid Areas size based on grid items > > layout. > > However, once we set the Grid Areas size, some orthogonal grid items may have > > changed its min-content contribution, so as Grid Layout spec states [1], we > > should > > repeat the Grid Areas sizing algorithm with the new min-content contribution. > > > > As far as I now, FrameView operates over ANY orthogonal box in the Layout > Tree; > > we don't need that. In any case, the actual issue is related to the grid > > container's > > intrinsic size. In order to execute the track sizing algorithm, we need to > > determine > > the available size we have for the content-sized tracks. This is determined > > based > > on container's intrinsic size. However, orthogonal grid items may have changed > > container's intrinsic size once they were laid out inside their respective > Grid > > Areas. > > > > [1] https://drafts.csswg.org/css-grid/#algo-overview > > It seems wrong to me for the size of some ancestor to affect the intrinsic size > of an element. > > Intrinsic sizes are used to determine the size of an ancestor, not the only way > around. I think I explained it bad, sorry about that. The Grid Layout spec defines some kind of abstraction called Grid Area. This is the actual containing block of the boxes that are children of the grid; aka grid items. When a grid block is sized with min-content or max-content, we use the Grid Areas size as grid's intrinsic size. These grid areas can be also content sized, so their size depend on the grid items' size. Until this point, everything should work as any other box layout model, even with this Grid Areas abstraction acting as actual containing block of grid items. However, when we have orthogonal grid items the Grid Areas sizing algorithm needs to consider some special cases. During grid's layout logic, we need to determine the available size we have to determine the actual size of the different Grid Areas. As I commented before, this available size is the grid's intrinsic size in case of min-content/max-content constraints. When there are orthogonal grid items, we have to face the following issues: 1- FrameView orthogonal flow pre-layout does not consider Grid Areas as actual containing blocks. We can't do much about this, since grid hasn't been laid out yet, so there is no Grid Area at all. 2- When computing column tracks for determining the size of the different Grid Areas, for an orthogonal grid item we use an estimation of its containing block's height (row tracks defining its Grid Area). This will change as soon as we determine the actual size of the mentioned row tracks. As I said, we can't do much about issue 1, but Grid spec address issue 2 by repeating the sizing algorithm (columns and rows) if any grid item have change its min-content contribution. The spec cites orthogonal flow boxes and multicolumn as possible examples of this scenario. We already implemented these extra cycle of the track sizing algorithm, however, we haven't consider how this could affect to the intrinsic size of the grid container, which is sized under min-content/max-content constraints, as I mentioned before.
This code may be correct-ish due to https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... However, I want to advise you against changing the preferred width during layout. The engine is not really designed with that in mind as historically CSS did not require that, and it is slow because it causes relayouts of the ancestor chain. By the way, what happens if you discover that your preferred width has changed again in this second path?
I think I need a (simple) test case. I'm very unfamiliar with grid layout. The test in the bug report is simple enough, but unfortunately it doesn't exhibit any layout problems, as far as I can see here. And this CL only modifies the expectations in some heavy tests. https://codereview.chromium.org/2333583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/2333583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.h:245: Vector<LayoutBox*> m_orthogonalChilds; childs -> children
On 2016/09/20 21:07:37, cbiesinger wrote: > This code may be correct-ish due to > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > umm, I saw that code, which made me thing that it was going to be enough to simply set Grid container's intrinsic width as dirty to trigger a new layout. However, that's not happen. I guess is that because that condition requires intrinsic sizes were not dirty before, which is not the case of the issue I'm trying to solve with this patch. The idea solution, as we internally have discussed, would be to trigger a new layout of the whole grid container, which will have the latest grid area sizes (stored in the overrideContainingBlock attributes). This will require, probably, to recompute intrinsic sizes, but wouldn't need to do it internally as part of the specific grid's layout logic. However, I've been unable so far to do it. Since the grid layout logic is performed inside a SubtreeLayoutScope, we can't simply mark the box as needing layout at the end of the layoutBlock function, because SubtreeLayoutSCope precisely prevents the box of needing layout after executing its own layout logic. I guess I'm missing something, because it seems an action reasonable to request when, for some reason, the layout we have just finished has changed something that needs to do it again. > However, I want to advise you against changing the preferred width during > layout. The engine is not really designed with that in mind as historically CSS > did not require that, and it is slow because it causes relayouts of the ancestor > chain. Yes, we have many doubts about that. I think I should ask the Spec editors about the needed or not to update intrinsic size in these scenarios. They just mention "repeat the sizing alg using the new min-content contributions". Perhaps they just meant that i should use a different "available size" to be used inside the track sizing algorithm, without updating the intrinsic size. >By the way, what happens if you discover that your preferred width has > changed again in this second path? The spec states that the algorithm must be repeated only once, precisely to avoid the infinite loop scenario you are suggesting.
On 2016/09/21 10:32:55, jfernandez wrote: > The idea solution, as we internally have discussed, would be to trigger a > new layout of the whole grid container, which will have the latest grid > area sizes (stored in the overrideContainingBlock attributes). This will > require, probably, to recompute intrinsic sizes, but wouldn't need to > do it internally as part of the specific grid's layout logic. > > However, I've been unable so far to do it. Since the grid layout logic > is performed inside a SubtreeLayoutScope, we can't simply mark the > box as needing layout at the end of the layoutBlock function, because > SubtreeLayoutSCope precisely prevents the box of needing layout > after executing its own layout logic. I guess I'm missing something, > because it seems an action reasonable to request when, for some > reason, the layout we have just finished has changed something that > needs to do it again. Instead of using the "needs layout" machinery, you could set a flag in LayoutGrid if you need relayout, and then detect this situation and have it lay itself out again when you have returned to LayoutGrid::layoutBlock(). See how LayoutBlockFlow::layoutBlock() may call LayoutBlockFlow::layoutBlockFlow() repeatedly for fragmentation. It still seems bad to let preferred widths depend on layout, though.
On 2016/09/20 21:30:29, mstensho wrote: > I think I need a (simple) test case. I'm very unfamiliar with grid layout. The > test in the bug report is simple enough, but unfortunately it doesn't exhibit > any layout problems, as far as I can see here. And this CL only modifies the > expectations in some heavy tests. The test case defined in the big report is perhaps the simplest one we can define. You have to enable web-platform-experimental-features to be able to see the issue properly. The issue is that grid have this abstraction Row as actual container of the grid item, while using a content sized intrinsic size for the grid container. The orthogonal item will cause the tracks being resized, while grid container doesn't notice, regarding its intrinsic size. The layout issue is detected when resizing the results frame, since it will trigger a new layout and intrinsic sizing computation, but this time using the already computed Row Track sizes. What we need to clarify with Spec editors is whether we want the result after doing this extra layout or perhaps we should let the grid tracks overflowing the grid container.
On 2016/09/21 10:47:49, mstensho wrote: > On 2016/09/21 10:32:55, jfernandez wrote: > > The idea solution, as we internally have discussed, would be to trigger a > > new layout of the whole grid container, which will have the latest grid > > area sizes (stored in the overrideContainingBlock attributes). This will > > require, probably, to recompute intrinsic sizes, but wouldn't need to > > do it internally as part of the specific grid's layout logic. > > > > However, I've been unable so far to do it. Since the grid layout logic > > is performed inside a SubtreeLayoutScope, we can't simply mark the > > box as needing layout at the end of the layoutBlock function, because > > SubtreeLayoutSCope precisely prevents the box of needing layout > > after executing its own layout logic. I guess I'm missing something, > > because it seems an action reasonable to request when, for some > > reason, the layout we have just finished has changed something that > > needs to do it again. > > Instead of using the "needs layout" machinery, you could set a flag in > LayoutGrid if you need relayout, and then detect this situation and have it lay > itself out again when you have returned to LayoutGrid::layoutBlock(). See how > LayoutBlockFlow::layoutBlock() may call LayoutBlockFlow::layoutBlockFlow() > repeatedly for fragmentation. > good idea, I'll try. > It still seems bad to let preferred widths depend on layout, though. Yes, that s the key issue we need to get clarified.
On 2016/09/21 11:04:11, jfernandez wrote:
> On 2016/09/20 21:30:29, mstensho wrote:
> > I think I need a (simple) test case. I'm very unfamiliar with grid layout.
The
> > test in the bug report is simple enough, but unfortunately it doesn't
exhibit
> > any layout problems, as far as I can see here. And this CL only modifies the
> > expectations in some heavy tests.
>
> The test case defined in the big report is perhaps the simplest one we can
> define. You have to enable web-platform-experimental-features to be able
> to see the issue properly.
Right! I had forgotten about the flag. Now I can reproduce it. Adding a body {
overflow:hidden; } would make it more reliable, though (had to do that when I
tried to rip it out of jsbin).
On 2016/09/21 12:13:40, mstensho wrote:
> On 2016/09/21 11:04:11, jfernandez wrote:
> > On 2016/09/20 21:30:29, mstensho wrote:
> > > I think I need a (simple) test case. I'm very unfamiliar with grid layout.
> The
> > > test in the bug report is simple enough, but unfortunately it doesn't
> exhibit
> > > any layout problems, as far as I can see here. And this CL only modifies
the
> > > expectations in some heavy tests.
> >
> > The test case defined in the big report is perhaps the simplest one we can
> > define. You have to enable web-platform-experimental-features to be able
> > to see the issue properly.
>
> Right! I had forgotten about the flag. Now I can reproduce it. Adding a body {
> overflow:hidden; } would make it more reliable, though (had to do that when I
> tried to rip it out of jsbin).
yes, the overflow: hidden trick avoid the issues we have with the double layout.
however, the issue to get clarified here is whether or not we should update
the intrinsic size, since orthogonal items have changed its min-content
contribution,
and how to do it in that case.
I think part of this issue must be clarified first by the spec editors, but
there are
also some technical issues derived from updating intrinsic size during layout.
Javier and I discussed this a bit in person yesterday. We've already crossed the bridge with preferred sizes depending on layout when we added the scrollbar logic here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... I now think it wouldn't be too bad to reuse that code for this issue with these caveats: 1) grid will have to make sure to mark the container chain's preferred widths dirty as well, when it calls this function: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... 2) The LayoutBlockFlow code above should also freeze this preferred size change. Perhaps we need a FreezePreferredSizesScope (like the existing FreezeScrollbarsScope), and inside of that we never call setPreferredSizeDirty 2) is important to avoid the risk of infinite loops.
Please close this if it has been abandoned. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
