[css-grid] Improve intrinsic size computation with orthogonal flows

[css-grid] Improve intrinsic size computation with orthogonal flows

FrameView performs an initial pre-layout of orthogonal items so that
preferred logical widths could be computed even with orthogonal flows. The
problem is that pre-layout is done based on the render tree hierarchy
something that is not valid for grids. The reason why is not valid is
because the containers of grid items are the grid areas not the grid
container. That was causing incorrect intrinsic size computations of grid
containers with orthogonal items.

From now on we skip that pre-layout for orthogonal items and do it on demand
in the track sizing algorithm code once the grid areas (the override
containing blocks) are set. This change gives us much better results in
several cases apart from fixing some others that were plainly wrong (like
orthogonal items with percentage sizes).

BUG=685609
Review-Url: https://codereview.chromium.org/2698663003
Cr-Commit-Position: refs/heads/master@{#460774}
Committed: https://chromium.googlesource.com/chromium/src/+/fe510c12d470f2227b0f7c63b94633618a37c7ab

[svillar, 2017-02-15 18:35:13]

svillar@igalia.com changed reviewers:
+ cbiesinger@chromium.org, jfernandez@igalia.com, rego@igalia.com

[svillar, 2017-02-15 18:35:14]

Adding reviewers

[jfernandez, 2017-02-15 22:49:58]

I think it improves the situation, but we need to analyze more carefully some
cases.

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-orthogonal-flows.html
(right):

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-orthogonal-flows.html:52:
<div class="grid itemsStart contentStart min-content height200"
data-expected-width="40" data-expected-height="200">
This is not correct. Since the items in the first row have 30px and 40px for
first and second column respectively, 70px was the correct value.

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-orthogonal-flows.html:61:
<div class="grid itemsStart contentStart max-content height200"
data-expected-width="160" data-expected-height="200">
This changes just avoids the empty space at the end of the item located at first
row first column (non-orthogonal). I guess is more correct, but in any case
there wasn't any issue with intrinsic size in this case.

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-orthogonal-flows.html:70:
<div class="grid itemsStart contentStart fit-content height200"
data-expected-width="160" data-expected-height="200">
Ditto.

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-orthogonal-flows.html:126:
<div class="grid itemsStart contentStart max-content height200"
data-expected-width="160" data-expected-height="200">
ditto.

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html
(right):

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html:69:
<div class="container">
Should we define a more complex case, which requires the extra cycles of the
tracks sizing algorithm ?

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp (left):

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:315: return
child.logicalHeight() + child.marginLogicalHeight();
Removing this code implies that we will mark the grid item for layout, if the
override size (its virtual containing block) has changed, and indeed perform
such layout in logicalHeightForChild in order to compute the column track's
width. 

For the record, I'm totally fine with it, since we were already doing it in the
FrameView, but just wanted to make it more explicit here to be considered by
other reviewers.

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:357: return
child.logicalHeight() + child.marginLogicalHeight();
Ditto.

[jfernandez, 2017-02-15 23:15:08]

I found another case this patch does not solve; see the last test case in
grid-item-spanning-and-orthogonal-flows.html

[svillar, 2017-02-16 09:16:35]

Thanks for the review!

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-orthogonal-flows.html
(right):

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-orthogonal-flows.html:52:
<div class="grid itemsStart contentStart min-content height200"
data-expected-width="40" data-expected-height="200">
On 2017/02/15 22:49:58, jfernandez wrote:
> This is not correct. Since the items in the first row have 30px and 40px for
> first and second column respectively, 70px was the correct value. 

I think you're confused here, there was no 70px it was 50px and it was wrong
because the first column will get 30px and the second 10px because the grid is
sized under min-content constraints.

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-orthogonal-flows.html:61:
<div class="grid itemsStart contentStart max-content height200"
data-expected-width="160" data-expected-height="200">
On 2017/02/15 22:49:57, jfernandez wrote:
> This changes just avoids the empty space at the end of the item located at
first
> row first column (non-orthogonal). I guess is more correct, but in any case
> there wasn't any issue with intrinsic size in this case.

Why not? The grid is sized under max-content constraints, that implies computing
the intrinsic size of the grid.

Also I don't understand what "just avoids" means :) It's actually fixing the
test case, the grid was wider than expected. Same for the other 3 cases.

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html
(right):

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html:69:
<div class="container">
On 2017/02/15 22:49:58, jfernandez wrote:
> Should we define a more complex case, which requires the extra cycles of the
> tracks sizing algorithm ?

Don't know. I'm adding this because it was the test case reported in the bug.

[svillar, 2017-02-16 09:25:28]

On 2017/02/15 23:15:08, jfernandez wrote:
> I found another case this patch does not solve; see the last test case in
> grid-item-spanning-and-orthogonal-flows.html

Why do you think it's wrong?, I think it's working perfectly. After the first
pass the column is 50px and the row is 100px. Then we rerun the algorithm and
the 20px in the row forces a column of 150px in order to fit the verticalRL
item. That's enough for the horizontal item. That's why the final size of the
tracks are: column 150px and row 100px

[svillar, 2017-02-16 09:25:56]

On 2017/02/16 09:25:28, svillar wrote:
> On 2017/02/15 23:15:08, jfernandez wrote:
> > I found another case this patch does not solve; see the last test case in
> > grid-item-spanning-and-orthogonal-flows.html
> 
> Why do you think it's wrong?, I think it's working perfectly. After the first
> pass the column is 50px and the row is 100px. Then we rerun the algorithm and
> the 20px in the row forces a column of 150px in order to fit the verticalRL
         ^^^^
          100px

[jfernandez, 2017-02-16 11:00:17]

My main concerns are that with this patch we don't get yet what the user would
expect regarding intrinsic size. We would probably need to perform the extra
cycles of the sizing algorithm, as we do in regular layout.

However, I agree that it's more spec compliant and it fixes
some cases, indeed. 

So, this change LGTM and I agree on evaluating the extra
cycles in a follow up patch.

In any case, I think it'd be a good idea to ask other
layout experts about their opinion on allowing layout
of children during intrinsic size computation.

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-orthogonal-flows.html
(right):

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-orthogonal-flows.html:52:
<div class="grid itemsStart contentStart min-content height200"
data-expected-width="40" data-expected-height="200">
On 2017/02/16 09:16:35, svillar wrote:
> On 2017/02/15 22:49:58, jfernandez wrote:
> > This is not correct. Since the items in the first row have 30px and 40px for
> > first and second column respectively, 70px was the correct value. 
> 
> I think you're confused here, there was no 70px it was 50px and it was wrong
> because the first column will get 30px and the second 10px because the grid is
> sized under min-content constraints.

Yes, I meant that we should aim for a better result. The previous value was
based
on the pre-layout using the grid container's height. It's debatable whether this
is more correct or not; although I agree than from the Grid spec point of view
assuming infinite is more spec compliant.

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-orthogonal-flows.html:61:
<div class="grid itemsStart contentStart max-content height200"
data-expected-width="160" data-expected-height="200">
On 2017/02/16 09:16:35, svillar wrote:
> On 2017/02/15 22:49:57, jfernandez wrote:
> > This changes just avoids the empty space at the end of the item located at
> first
> > row first column (non-orthogonal). I guess is more correct, but in any case
> > there wasn't any issue with intrinsic size in this case.
> 
> Why not? The grid is sized under max-content constraints, that implies
computing
> the intrinsic size of the grid.
> 
> Also I don't understand what "just avoids" means :) It's actually fixing the
> test case, the grid was wider than expected. Same for the other 3 cases.

Again, I think we are both right here. The pre-layout caused that the orthogonal
in the first row get a width of 20px, hence we get 170px of available size to
use
in the tracks sizing algorithm during layout. Now that we have got rid of the
pre-layout we get a smaller available size from intrinsic size computation
(160px)
which explains the difference. 

As I commented before, the new results are probably more spec compliant.

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html
(right):

https://codereview.chromium.org/2698663003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html:69:
<div class="container">
On 2017/02/16 09:16:35, svillar wrote:
> On 2017/02/15 22:49:58, jfernandez wrote:
> > Should we define a more complex case, which requires the extra cycles of the
> > tracks sizing algorithm ?
> 
> Don't know. I'm adding this because it was the test case reported in the bug.

Acknowledged.

[svillar, 2017-02-16 12:08:38]

svillar@igalia.com changed reviewers:
+ mstensho@opera.com, rune@opera.com

[svillar, 2017-02-16 12:08:38]

Adding some Opera guys to know their opinions. The grid specifics are not very
important so feel free to skip them in your review.

Our main concern is knowing that what I propose in the patch looks good to you
from the engine POV. So the new thing I'm doing for grid is basically doing some
layout of grid items as part of the preferred width (intrinsic size) computation
of the grid container. We must do that in order to achieve correct results for
orthogonal items. I know that in order not to enter an cycle we cannot layout an
object when computing its preferred width, but this case is different because
we're triggering a layout of a child so there is no cycle there.

[mstensho (USE GERRIT), 2017-02-22 14:07:14]

FrameView::layoutOrthogonalWritingModeRoots() is there to avoid doing layout as
part of intrinsic size computation. That said, the very reason for that
machinery is to get better intrinsic size computation.

I still think it's generally problematic (not sure about the orthogonal flows
case, though - maybe it's okay) to use actual layout output as input to
intrinsic sizing, but whether it's done per object in
FrameView::layoutOrthogonalWritingModeRoots() or you do it on your own in
LayoutGrid shouldn't make much of a difference.

So, lgtm.

[svillar, 2017-02-22 15:43:31]

On 2017/02/22 14:07:14, mstensho wrote:
> FrameView::layoutOrthogonalWritingModeRoots() is there to avoid doing layout
as
> part of intrinsic size computation. That said, the very reason for that
> machinery is to get better intrinsic size computation.
> 
> I still think it's generally problematic (not sure about the orthogonal flows
> case, though - maybe it's okay) to use actual layout output as input to
> intrinsic sizing, but whether it's done per object in
> FrameView::layoutOrthogonalWritingModeRoots() or you do it on your own in
> LayoutGrid shouldn't make much of a difference.

Thanks Morten for the analysis.

We did try to avoid those layouts as much as we could, but the thing is that it
is inherently impossible to get good/correct results when computing the
intrinsic size in presence of orthogonal items without doing a layout. That was
confirmed [1] by spec editors.

Apart from that, for us is specially important to do it in LayoutGrid because
that's the only place with information about grid areas, which are the actual
containing blocks of the grid items (in
FrameView::layoutOrthogonalWritingModeRoots() the layout was assuming the
containing block of grid items were the grid container which is totally wrong).

[1] https://github.com/w3c/csswg-drafts/issues/537

[svillar, 2017-02-22 15:43:43]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2017-02-22 15:44:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 17:59:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 17:59:31]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[svillar, 2017-02-23 09:05:15]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2017-02-23 09:05:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 11:09:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 11:09:02]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[svillar, 2017-02-27 15:02:43]

Mac can call computeIntrinsicLogicalWidths outside f the layout process meaning
that we should not mark items for layout in those cases.

[mstensho (USE GERRIT), 2017-03-17 09:15:44]

On 2017/02/27 15:02:43, svillar wrote:
> Mac can call computeIntrinsicLogicalWidths outside f the layout process
meaning
> that we should not mark items for layout in those cases.

When and why does Mac do that? The trybot results have been deleted. And what
harm does it do to mark them for layout? Also, it just became clear to me that
marking for layout as part of calculating intrinsic sizes is mighty weird. But
this CL isn't really introducing that (just causing more of it), so fine.

[svillar, 2017-03-17 16:03:14]

On 2017/03/17 09:15:44, mstensho wrote:
> On 2017/02/27 15:02:43, svillar wrote:
> > Mac can call computeIntrinsicLogicalWidths outside f the layout process
> meaning
> > that we should not mark items for layout in those cases.
> 
> When and why does Mac do that? The trybot results have been deleted. And what
> harm does it do to mark them for layout? Also, it just became clear to me that
> marking for layout as part of calculating intrinsic sizes is mighty weird. But
> this CL isn't really introducing that (just causing more of it), so fine.

Regarding when & why the sequence is the following:
content::RenderViewImpl::didUpdateLayout()
content::RenderViewImpl::CheckPreferredSize()
WebViewImpl::WebViewImpl::contentsPreferredMinimumSize()
document->layoutViewItem().minPreferredLogicalWidth()
which eventually ends up calling layout() in GridTrackSizingAlgorithm.cpp for
orthogonal grid items

The harm is not marking for layout, but actually _doing_ the layout. Note that
there is an ASSERT in SubtreeLayoutScope for
m_root.document.view()->isInPerformLayout() which is obviously false at that
phase of execution.

Note that this CL is not only preventing orthogonal grid items from being early
laid out, but I'm also removing some code in the GridTrackSizingAlgorithm which
was preventing the calls to layout() for those items in some situations.

[svillar, 2017-03-30 14:12:25]

The CQ bit was checked by svillar@igalia.com

[svillar, 2017-03-30 14:12:25]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com,
jfernandez@igalia.com
Link to the patchset: https://codereview.chromium.org/2698663003/#ps40001
(title: "Patch for landing")

[commit-bot: I haz the power, 2017-03-30 14:12:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 15:45:09]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490883145463680,
"parent_rev": "f9a212dec0fec22e21d7f66ad1aa9414ed1dc068", "commit_rev":
"fe510c12d470f2227b0f7c63b94633618a37c7ab"}

[commit-bot: I haz the power, 2017-03-30 15:46:57]

Description was changed from

==========
[css-grid] Improve intrinsic size computation with orthogonal flows

FrameView performs an initial pre-layout of orthogonal items so that
preferred logical widths could be computed even with orthogonal flows. The
problem is that pre-layout is done based on the render tree hierarchy
something that is not valid for grids. The reason why is not valid is
because the containers of grid items are the grid areas not the grid
container. That was causing incorrect intrinsic size computations of grid
containers with orthogonal items.

From now on we skip that pre-layout for orthogonal items and do it on demand
in the track sizing algorithm code once the grid areas (the override
containing blocks) are set. This change gives us much better results in
several cases apart from fixing some others that were plainly wrong (like
orthogonal items with percentage sizes).

BUG=685609
==========

to

==========
[css-grid] Improve intrinsic size computation with orthogonal flows

FrameView performs an initial pre-layout of orthogonal items so that
preferred logical widths could be computed even with orthogonal flows. The
problem is that pre-layout is done based on the render tree hierarchy
something that is not valid for grids. The reason why is not valid is
because the containers of grid items are the grid areas not the grid
container. That was causing incorrect intrinsic size computations of grid
containers with orthogonal items.

From now on we skip that pre-layout for orthogonal items and do it on demand
in the track sizing algorithm code once the grid areas (the override
containing blocks) are set. This change gives us much better results in
several cases apart from fixing some others that were plainly wrong (like
orthogonal items with percentage sizes).

BUG=685609

Review-Url: https://codereview.chromium.org/2698663003
Cr-Commit-Position: refs/heads/master@{#460774}
Committed:
https://chromium.googlesource.com/chromium/src/+/fe510c12d470f2227b0f7c63b946...
==========

[commit-bot: I haz the power, 2017-03-30 15:46:58]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/fe510c12d470f2227b0f7c63b946...

[emircan, 2017-03-30 22:13:45]

On 2017/03/30 15:46:58, commit-bot: I haz the power wrote:
> Committed patchset #3 (id:40001) as
>
https://chromium.googlesource.com/chromium/src/+/fe510c12d470f2227b0f7c63b946...

Can you take alook to see if this is related to the issues on Win chromium bots?
https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/33395