Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1778853002:
[css-grid] Fix height computation for items with intrinsic aspect ratios (Closed)
|
Created:
4 years, 9 months ago by svillar Modified:
4 years, 9 months ago CC:
chromium-reviews, jfernandez, szager+layoutwatch_chromium.org, zoltan1, svillar, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, 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 height computation for items with intrinsic aspect ratios
We were not computing the height properly for items spanning content
or flexible sized tracks having intrinsic aspect ratios because we were
returning always the "natural" size, meaning that on an image of 100x50
we're always returning 50, even if the image had a specified width.
This change allowed us also to remove a bunch of code imported from flexbox
renderer which is actually not really needed in grid.
BUG=592975
Committed: https://crrev.com/f76b5035ab7d677198849bff6cd5c3aa17d78468
Cr-Commit-Position: refs/heads/master@{#382287}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Post-review changes #Patch Set 3 : Reverted unneeded changes #
Total comments: 6
Patch Set 4 : Patch for landing #
Messages
Total messages: 16 (6 generated)
svillar@igalia.com changed reviewers: + cbiesinger@chromium.org, jfernandez@igalia.com, rego@igalia.com
Sending out for review
Sorry but I don't get the relationship between the patch and the description. I don't understand why checking if the item is in a content-sized or flexible track is relevant to fix this. Is it already working fine for fixed-size tracks? IMHO it'd be nice to explain it further in the description. https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio-expected.html (right): https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio-expected.html:18: <div class="child" style="height: 25px; width: 50px;"></div> Nit: Maybe using a check-layout.js test is simpler for this case. We won't need to check the -expected file to see the results. https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio.html (right): https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio.html:13: <p>This test shows how min-width:auto is computed for items with intrinisc aspect ratios</p> Only min-width or both? The patch title talks about height. https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio.html:15: <p>Check that width respects the intrinsic aspect ratio when specifying the height</p> You're not testing here combinations like: min-height: 200px; max-height: 50px; Like you're doing in the 2nd part of the test. https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio.html:17: <img src="resources/100x50.png" style="height: 25px;"/> Nit: Maybe we can rename the picture and call it "blue-100x50.png". https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio.html:33: <p>Check that height respects the intrinsic aspect ratio when specifying the width</p> You're not testing here combinations like: width: 25px; max-width: 40px; width: 100px; max-height: 25px; width: 25px; min-height: 100px; Like you're doing in the first part. https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (left): https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1653: { Shouldn't you remove the method signature in the header? https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1661: LayoutUnit LayoutGrid::childIntrinsicHeight(const LayoutBox& child) const Ditto. https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1669: LayoutUnit LayoutGrid::childIntrinsicWidth(const LayoutBox& child) const Ditto. https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1677: LayoutUnit LayoutGrid::intrinsicLogicalHeightForChild(const LayoutBox& child) const Ditto. https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:689: || (!child.styleRef().logicalHeight().isSpecified() && itemSpansContentSizedOrFlexibleSizedTracks(cachedGridSpan(child, ForRows), ForRows, ContentSizedAndFlexible)); I've a hard time trying to understand the 2nd part of the ||. Maybe we need a comment explaining it. In any case instead of: !child.styleRef().logicalHeight().isSpecified() Maybe it's more explicit: child.styleRef().logicalHeight().isIntrinsicOrAuto() https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.h:189: enum SpanMode { FlexibleOnly, ContentSizedAndFlexible }; Maybe call it SpanTracksMode. Dunno really, but trying to clarify its purpose.
Thanks for the review. https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio.html (right): https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio.html:13: <p>This test shows how min-width:auto is computed for items with intrinisc aspect ratios</p> On 2016/03/14 14:38:09, Manuel Rego wrote: > Only min-width or both? > > The patch title talks about height. Correct, both. https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio.html:15: <p>Check that width respects the intrinsic aspect ratio when specifying the height</p> On 2016/03/14 14:38:09, Manuel Rego wrote: > You're not testing here combinations like: > min-height: 200px; > max-height: 50px; > > Like you're doing in the 2nd part of the test. Acknowledged. https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio.html:17: <img src="resources/100x50.png" style="height: 25px;"/> On 2016/03/14 14:38:09, Manuel Rego wrote: > Nit: Maybe we can rename the picture and call it "blue-100x50.png". Acknowledged. https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio.html:33: <p>Check that height respects the intrinsic aspect ratio when specifying the width</p> On 2016/03/14 14:38:09, Manuel Rego wrote: > You're not testing here combinations like: > width: 25px; max-width: 40px; > width: 100px; max-height: 25px; > width: 25px; min-height: 100px; > > Like you're doing in the first part. Acknowledged. https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (left): https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1653: { On 2016/03/14 14:38:09, Manuel Rego wrote: > Shouldn't you remove the method signature in the header? Ups forgot about that. https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:689: || (!child.styleRef().logicalHeight().isSpecified() && itemSpansContentSizedOrFlexibleSizedTracks(cachedGridSpan(child, ForRows), ForRows, ContentSizedAndFlexible)); On 2016/03/14 14:38:09, Manuel Rego wrote: > I've a hard time trying to understand the 2nd part of the ||. > Maybe we need a comment explaining it. Isn't the function name explicit enough? > In any case instead of: > !child.styleRef().logicalHeight().isSpecified() > Maybe it's more explicit: > child.styleRef().logicalHeight().isIntrinsicOrAuto() OK https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/1778853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.h:189: enum SpanMode { FlexibleOnly, ContentSizedAndFlexible }; On 2016/03/14 14:38:09, Manuel Rego wrote: > Maybe call it SpanTracksMode. > Dunno really, but trying to clarify its purpose. I wasn't able to find a good name. I can change it.
Description was changed from ========== [css-grid] Fix height computation for items with intrinsic aspect ratios We were not computing the height properly for items with intrinsic aspect ratios because we were returning always the "natural" size, meaning that on an image of 100x50 we're always returning 50, even if the image had a specified width. This change allowed us also to remove a bunch of code imported from flexbox renderer which is actually not really needed in grid. BUG=592975 ========== to ========== [css-grid] Fix height computation for items with intrinsic aspect ratios We were not computing the height properly for items spanning content or flexible sized tracks having intrinsic aspect ratios because we were returning always the "natural" size, meaning that on an image of 100x50 we're always returning 50, even if the image had a specified width. This change allowed us also to remove a bunch of code imported from flexbox renderer which is actually not really needed in grid. BUG=592975 ==========
Thanks, with the new description is easier to understand what's doing. Nit: Maybe in the description I'd write: "items in content or flexible sized tracks" instead of: "items spanning content or flexible sized tracks" Non-owner lgtm. Nice clean-up!
Some nits on the test that I forgot to publish before (sorry). https://codereview.chromium.org/1778853002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio.html (right): https://codereview.chromium.org/1778853002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio.html:19: <div class="container min-content" data-expected-width="60" data-expected-height="35"> Nit: dunno if we want to check the size of the grid container, I think you can omit it if you want. https://codereview.chromium.org/1778853002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio.html:28: <img src="resources/blue-100x50.png" style="max-height: 50px;" data-expected-width="100" data-expected-height="50"/> Nit: "max-height: 50px" is not really useful, what about "max-height: 40px"? https://codereview.chromium.org/1778853002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio.html:70: <img src="resources/blue-100x50.png" style="width: 100px; max-height: 25px;" data-expected-width="100" data-expected-height="25"/> Nit: "width: 100px" maybe better "width: 120px".
Thanks. https://codereview.chromium.org/1778853002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio.html (right): https://codereview.chromium.org/1778853002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio.html:19: <div class="container min-content" data-expected-width="60" data-expected-height="35"> On 2016/03/21 12:13:29, Manuel Rego wrote: > Nit: dunno if we want to check the size of the grid container, > I think you can omit it if you want. I do want to check them because as you could see, due to the intrinsic ratio, the size of the container is not just the itemsize+borderpaddingmargin https://codereview.chromium.org/1778853002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio.html:28: <img src="resources/blue-100x50.png" style="max-height: 50px;" data-expected-width="100" data-expected-height="50"/> On 2016/03/21 12:13:29, Manuel Rego wrote: > Nit: "max-height: 50px" is not really useful, > what about "max-height: 40px"? good point https://codereview.chromium.org/1778853002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-automatic-minimum-intrinsic-aspect-ratio.html:70: <img src="resources/blue-100x50.png" style="width: 100px; max-height: 25px;" data-expected-width="100" data-expected-height="25"/> On 2016/03/21 12:13:29, Manuel Rego wrote: > Nit: "width: 100px" maybe better "width: 120px". Another good point.
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/1778853002/#ps60001 (title: "Patch for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1778853002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1778853002/60001
lgtm
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Fix height computation for items with intrinsic aspect ratios We were not computing the height properly for items spanning content or flexible sized tracks having intrinsic aspect ratios because we were returning always the "natural" size, meaning that on an image of 100x50 we're always returning 50, even if the image had a specified width. This change allowed us also to remove a bunch of code imported from flexbox renderer which is actually not really needed in grid. BUG=592975 ========== to ========== [css-grid] Fix height computation for items with intrinsic aspect ratios We were not computing the height properly for items spanning content or flexible sized tracks having intrinsic aspect ratios because we were returning always the "natural" size, meaning that on an image of 100x50 we're always returning 50, even if the image had a specified width. This change allowed us also to remove a bunch of code imported from flexbox renderer which is actually not really needed in grid. BUG=592975 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Fix height computation for items with intrinsic aspect ratios We were not computing the height properly for items spanning content or flexible sized tracks having intrinsic aspect ratios because we were returning always the "natural" size, meaning that on an image of 100x50 we're always returning 50, even if the image had a specified width. This change allowed us also to remove a bunch of code imported from flexbox renderer which is actually not really needed in grid. BUG=592975 ========== to ========== [css-grid] Fix height computation for items with intrinsic aspect ratios We were not computing the height properly for items spanning content or flexible sized tracks having intrinsic aspect ratios because we were returning always the "natural" size, meaning that on an image of 100x50 we're always returning 50, even if the image had a specified width. This change allowed us also to remove a bunch of code imported from flexbox renderer which is actually not really needed in grid. BUG=592975 Committed: https://crrev.com/f76b5035ab7d677198849bff6cd5c3aa17d78468 Cr-Commit-Position: refs/heads/master@{#382287} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f76b5035ab7d677198849bff6cd5c3aa17d78468 Cr-Commit-Position: refs/heads/master@{#382287} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
