Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1490093003:
[css-grid] Fix items' height computation in indefinite grids with fr (Closed)
|
Created:
5 years ago by svillar Modified:
5 years ago CC:
chromium-reviews, jfernandez, 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 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[css-grid] Fix items' height computation in indefinite grids with fr
Since master@{#358816} we run a second round of the track sizing algorithm
whenever the height of the grid is indefinite in order to properly compute
row sizes. The available space passed to the track sizing algorithm must not
contain neither borders nor paddings, otherwise it will think that it has
more space available than the existing one. We should use the height of the
content box instead.
BUG=560771, 562985
Committed: https://crrev.com/a29568cbf37aa90d769d34df3350b69e257deea2
Cr-Commit-Position: refs/heads/master@{#362957}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Added data-expected-client-* checks and renamed the test #Patch Set 3 : Patch for landing #
Messages
Total messages: 14 (6 generated)
Description was changed from
==========
[css-grid] Fix items' height computation in indefinite grids with fr
Since master@{#358816} we run a second round of the track sizing algorithm
whenever the height of the grid is indefinite in order to properly compute
row sizes. The available space passed to the track sizing algorithm must not
contain neither borders nor paddings, otherwise it will think that it has
more space available than the existing one. We should use the height of the
content box instead.
BUG=
==========
to
==========
[css-grid] Fix items' height computation in indefinite grids with fr
Since master@{#358816} we run a second round of the track sizing algorithm
whenever the height of the grid is indefinite in order to properly compute
row sizes. The available space passed to the track sizing algorithm must not
contain neither borders nor paddings, otherwise it will think that it has
more space available than the existing one. We should use the height of the
content box instead.
BUG=560771,562985
==========
svillar@igalia.com changed reviewers: + cbiesinger@chromium.org, jfernandez@igalia.com, rego@igalia.com
Sending for review
Added data-expected-client-* checks and renamed the test
Non-owner LGTM. Nice catch! Just a few comments on the test. https://codereview.chromium.org/1490093003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-inside-fr-with-borders.html (right): https://codereview.chromium.org/1490093003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-inside-fr-with-borders.html:9: width: 300px; Wouldn't be nice to have a fixed height too, to check that "1fr" is doing some stuff. https://codereview.chromium.org/1490093003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-inside-fr-with-borders.html:19: width: -webkit-fit-content; Maybe it's better to use "fit-content" class already defined at: css-intrinsic-dimensions/resources/width-keyword-classes.css That's what this patch is doing for all the tests: https://codereview.chromium.org/1488493003/ https://codereview.chromium.org/1490093003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-inside-fr-with-borders.html:24: grid-auto-rows: minmax(0,1fr); Do you really need to use "minmax(0, 1fr);" ? Also using "grid-auto-*" instead of "grid-template-*" seems not needed to me. https://codereview.chromium.org/1490093003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-inside-fr-with-borders.html:28: grid-auto-columns: minmax(0,1fr); Ditto. https://codereview.chromium.org/1490093003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-inside-fr-with-borders.html:33: background: gray; .grid has already a grey background, maybe use a different color here. :-) https://codereview.chromium.org/1490093003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-inside-fr-with-borders.html:37: x { display:block; height:40px; width:40px; } Why not use a <div>? You could remove "display: block;". On top of that, we don't have other tests using <x> so it'd be weird.
Thanks! https://codereview.chromium.org/1490093003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-inside-fr-with-borders.html (right): https://codereview.chromium.org/1490093003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-inside-fr-with-borders.html:9: width: 300px; On 2015/12/03 09:24:43, Manuel Rego wrote: > Wouldn't be nice to have a fixed height too, > to check that "1fr" is doing some stuff. We cannot do that because the problem appears when we have to compute the value of 1fr when the height is indefinite. https://codereview.chromium.org/1490093003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-inside-fr-with-borders.html:19: width: -webkit-fit-content; On 2015/12/03 09:24:43, Manuel Rego wrote: > Maybe it's better to use "fit-content" class already defined at: > css-intrinsic-dimensions/resources/width-keyword-classes.css > > That's what this patch is doing for all the tests: > https://codereview.chromium.org/1488493003/ Acknowledged. https://codereview.chromium.org/1490093003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-inside-fr-with-borders.html:24: grid-auto-rows: minmax(0,1fr); On 2015/12/03 09:24:43, Manuel Rego wrote: > Do you really need to use "minmax(0, 1fr);" ? > > Also using "grid-auto-*" instead of "grid-template-*" > seems not needed to me. Why do you prefer grid-template-* over grid-auto-* ? https://codereview.chromium.org/1490093003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-inside-fr-with-borders.html:33: background: gray; On 2015/12/03 09:24:43, Manuel Rego wrote: > .grid has already a grey background, > maybe use a different color here. :-) Acknowledged. https://codereview.chromium.org/1490093003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-inside-fr-with-borders.html:37: x { display:block; height:40px; width:40px; } On 2015/12/03 09:24:43, Manuel Rego wrote: > Why not use a <div>? > You could remove "display: block;". > > On top of that, we don't have other tests > using <x> so it'd be weird. Acknowledged.
https://codereview.chromium.org/1490093003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-inside-fr-with-borders.html (right): https://codereview.chromium.org/1490093003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-inside-fr-with-borders.html:9: width: 300px; On 2015/12/03 09:35:12, svillar wrote: > On 2015/12/03 09:24:43, Manuel Rego wrote: > > Wouldn't be nice to have a fixed height too, > > to check that "1fr" is doing some stuff. > > We cannot do that because the problem appears when we have to compute the value > of 1fr when the height is indefinite. Ok, I didn't realize about that sorry. https://codereview.chromium.org/1490093003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-inside-fr-with-borders.html:24: grid-auto-rows: minmax(0,1fr); On 2015/12/03 09:35:12, svillar wrote: > > Also using "grid-auto-*" instead of "grid-template-*" > > seems not needed to me. > > Why do you prefer grid-template-* over grid-auto-* ? Not preference, it was just an extra comment but any of them are fine. Sorry for not being clear enough.
The CQ bit was checked by svillar@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from rego@igalia.com Link to the patchset: https://codereview.chromium.org/1490093003/#ps40001 (title: "Patch for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490093003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490093003/40001
Message was sent while issue was closed.
Description was changed from
==========
[css-grid] Fix items' height computation in indefinite grids with fr
Since master@{#358816} we run a second round of the track sizing algorithm
whenever the height of the grid is indefinite in order to properly compute
row sizes. The available space passed to the track sizing algorithm must not
contain neither borders nor paddings, otherwise it will think that it has
more space available than the existing one. We should use the height of the
content box instead.
BUG=560771,562985
==========
to
==========
[css-grid] Fix items' height computation in indefinite grids with fr
Since master@{#358816} we run a second round of the track sizing algorithm
whenever the height of the grid is indefinite in order to properly compute
row sizes. The available space passed to the track sizing algorithm must not
contain neither borders nor paddings, otherwise it will think that it has
more space available than the existing one. We should use the height of the
content box instead.
BUG=560771,562985
==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from
==========
[css-grid] Fix items' height computation in indefinite grids with fr
Since master@{#358816} we run a second round of the track sizing algorithm
whenever the height of the grid is indefinite in order to properly compute
row sizes. The available space passed to the track sizing algorithm must not
contain neither borders nor paddings, otherwise it will think that it has
more space available than the existing one. We should use the height of the
content box instead.
BUG=560771,562985
==========
to
==========
[css-grid] Fix items' height computation in indefinite grids with fr
Since master@{#358816} we run a second round of the track sizing algorithm
whenever the height of the grid is indefinite in order to properly compute
row sizes. The available space passed to the track sizing algorithm must not
contain neither borders nor paddings, otherwise it will think that it has
more space available than the existing one. We should use the height of the
content box instead.
BUG=560771,562985
Committed: https://crrev.com/a29568cbf37aa90d769d34df3350b69e257deea2
Cr-Commit-Position: refs/heads/master@{#362957}
==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a29568cbf37aa90d769d34df3350b69e257deea2 Cr-Commit-Position: refs/heads/master@{#362957} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
