Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1639723003:
[css-flexbox] Use correct aspect ratio for min-size: auto (Closed)
|
Created:
4 years, 10 months ago by cbiesinger Modified:
4 years, 10 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[css-flexbox] Use correct aspect ratio for min-size: auto
https://codereview.chromium.org/1421423005 caused this regression. We never
handled aspect ratio correctly in flexbox (that's bug 249112), but that patch
made our handling worse because we now distort and enlarge images when
a cross axis size is specified that's bigger than the image's intrinsic size.
This was tested by
imported/csswg-test/css-flexbox-1/flex-minimum-height-flex-items-008.xht which
we now pass. I didn't realize the significance of failing that test at the time :(
BUG=581361, 581535
R=leviw@chromium.org
Committed: https://crrev.com/eba8fa267c94a76faff64836a8c72a683c1a538e
Cr-Commit-Position: refs/heads/master@{#372000}
Patch Set 1 #
Total comments: 8
Patch Set 2 : more tests & a bugfix #
Total comments: 2
Patch Set 3 : remove empty loine #Patch Set 4 : now also handling min/max #
Total comments: 4
Patch Set 5 : review comments #
Messages
Total messages: 36 (14 generated)
The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639723003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639723003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1639723003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right): https://codereview.chromium.org/1639723003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:638: crossSize = child.styleRef().height(); Do we have tests for both these codepaths? https://codereview.chromium.org/1639723003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:649: if (isHorizontalFlow()) Ditto. https://codereview.chromium.org/1639723003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:679: bool LayoutFlexibleBox::crossAxisLengthIsDefinite(const LayoutBox& child, const Length& length) const Damn, it's back :(
With more tests, an additional bugfix, and a slight refactoring for computeMainSizeFromAspectRatio (for easier use in bug 249112) https://codereview.chromium.org/1639723003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right): https://codereview.chromium.org/1639723003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:638: crossSize = child.styleRef().height(); On 2016/01/27 04:53:56, leviw wrote: > Do we have tests for both these codepaths? Complicated question... Short answer, yes, flex-minimum-{width,height}-flex-items-008.xht test the two cases (here and below). Long answer, we already passed the width version because of how we compute the min-content logical width vs the height. The width takes the aspect ratio into account, the height doesn't. Your question made me realize that we have a bug in the contentSize calculation. I've updated the code to fix that bug, and marked some tests as failing, and added a test for that. https://codereview.chromium.org/1639723003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:679: bool LayoutFlexibleBox::crossAxisLengthIsDefinite(const LayoutBox& child, const Length& length) const On 2016/01/27 04:53:56, leviw wrote: > Damn, it's back :( Hm? What do you mean?
With more tests, an additional bugfix, and a slight refactoring for computeMainSizeFromAspectRatio (for easier use in bug 249112)
The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639723003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639723003/20001
LGTM :) https://codereview.chromium.org/1639723003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right): https://codereview.chromium.org/1639723003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:638: crossSize = child.styleRef().height(); On 2016/01/27 at 21:42:56, cbiesinger wrote: > On 2016/01/27 04:53:56, leviw wrote: > > Do we have tests for both these codepaths? > > Complicated question... > > Short answer, yes, flex-minimum-{width,height}-flex-items-008.xht test the two cases (here and below). > > Long answer, we already passed the width version because of how we compute the min-content logical width vs the height. The width takes the aspect ratio into account, the height doesn't. > > Your question made me realize that we have a bug in the contentSize calculation. I've updated the code to fix that bug, and marked some tests as failing, and added a test for that. Word :) https://codereview.chromium.org/1639723003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:679: bool LayoutFlexibleBox::crossAxisLengthIsDefinite(const LayoutBox& child, const Length& length) const On 2016/01/27 at 21:42:56, cbiesinger wrote: > On 2016/01/27 04:53:56, leviw wrote: > > Damn, it's back :( > > Hm? What do you mean? Is this not a method that existed previously? https://codereview.chromium.org/1639723003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right): https://codereview.chromium.org/1639723003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:703: This seems like a weird empty line </whitespace-jerk>
https://codereview.chromium.org/1639723003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right): https://codereview.chromium.org/1639723003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:679: bool LayoutFlexibleBox::crossAxisLengthIsDefinite(const LayoutBox& child, const Length& length) const On 2016/01/27 23:18:45, leviw wrote: > On 2016/01/27 at 21:42:56, cbiesinger wrote: > > On 2016/01/27 04:53:56, leviw wrote: > > > Damn, it's back :( > > > > Hm? What do you mean? > > Is this not a method that existed previously? I don't *believe* so but I'm actually not sure... https://codereview.chromium.org/1639723003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right): https://codereview.chromium.org/1639723003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:703: On 2016/01/27 23:18:45, leviw wrote: > This seems like a weird empty line </whitespace-jerk> Oh oops. Removed.
The CQ bit was checked by cbiesinger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org Link to the patchset: https://codereview.chromium.org/1639723003/#ps40001 (title: "remove empty loine")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639723003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639723003/40001
The CQ bit was unchecked by cbiesinger@chromium.org
I just realized that this does not take min/max-width into account but it should. Let me fix that.
Description was changed from ========== [css-flexbox] Use correct aspect ratio for min-size: auto https://codereview.chromium.org/1421423005 caused this regression. We never handled aspect ratio correctly in flexbox (that's bug 249112), but that patch made our handling worse because we now distort and enlarge images when a cross axis size is specified that's bigger than the image's intrinsic size. This was tested by imported/csswg-test/css-flexbox-1/flex-minimum-height-flex-items-008.xht which we now pass. I didn't realize the significance of failing that test at the time :( BUG=581361 R=leviw@chromium.org ========== to ========== [css-flexbox] Use correct aspect ratio for min-size: auto https://codereview.chromium.org/1421423005 caused this regression. We never handled aspect ratio correctly in flexbox (that's bug 249112), but that patch made our handling worse because we now distort and enlarge images when a cross axis size is specified that's bigger than the image's intrinsic size. This was tested by imported/csswg-test/css-flexbox-1/flex-minimum-height-flex-items-008.xht which we now pass. I didn't realize the significance of failing that test at the time :( BUG=581361,581535 R=leviw@chromium.org ==========
The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run
Levi - this now also fixes http://crbug.com/581535. It's easier to merge to 49 as a single patch. Could you take another look? Thank you!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639723003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639723003/60001
https://codereview.chromium.org/1639723003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right): https://codereview.chromium.org/1639723003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1009: if ((crossMax.isFixed() || crossMax.hasPercent()) && crossAxisLengthIsDefinite(child, crossMax)) { You check these same values in three places now. How about giving this a name? https://codereview.chromium.org/1639723003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h (right): https://codereview.chromium.org/1639723003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h:160: LayoutUnit adjustChildSizeForAspectRatioCrossAxisMinAndMax(const LayoutBox& child, LayoutUnit childSize); Whoa... That's a mouthful.
The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639723003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639723003/80001
PTAL! https://codereview.chromium.org/1639723003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right): https://codereview.chromium.org/1639723003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1009: if ((crossMax.isFixed() || crossMax.hasPercent()) && crossAxisLengthIsDefinite(child, crossMax)) { On 2016/01/28 00:37:59, leviw wrote: > You check these same values in three places now. How about giving this a name? Fair point. I decided to just only allow fixed and percent in crossAxisLengthIsDefinite for now. That will have the same effect right now. We can reconsider if we use that function in another place that might need to support these other values sooner. https://codereview.chromium.org/1639723003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h (right): https://codereview.chromium.org/1639723003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h:160: LayoutUnit adjustChildSizeForAspectRatioCrossAxisMinAndMax(const LayoutBox& child, LayoutUnit childSize); On 2016/01/28 00:37:59, leviw wrote: > Whoa... That's a mouthful. Couldn't come up with a better name :/
I like this better. LGTM.
The CQ bit was unchecked by cbiesinger@google.com
The CQ bit was checked by cbiesinger@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639723003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639723003/80001
Message was sent while issue was closed.
Description was changed from ========== [css-flexbox] Use correct aspect ratio for min-size: auto https://codereview.chromium.org/1421423005 caused this regression. We never handled aspect ratio correctly in flexbox (that's bug 249112), but that patch made our handling worse because we now distort and enlarge images when a cross axis size is specified that's bigger than the image's intrinsic size. This was tested by imported/csswg-test/css-flexbox-1/flex-minimum-height-flex-items-008.xht which we now pass. I didn't realize the significance of failing that test at the time :( BUG=581361,581535 R=leviw@chromium.org ========== to ========== [css-flexbox] Use correct aspect ratio for min-size: auto https://codereview.chromium.org/1421423005 caused this regression. We never handled aspect ratio correctly in flexbox (that's bug 249112), but that patch made our handling worse because we now distort and enlarge images when a cross axis size is specified that's bigger than the image's intrinsic size. This was tested by imported/csswg-test/css-flexbox-1/flex-minimum-height-flex-items-008.xht which we now pass. I didn't realize the significance of failing that test at the time :( BUG=581361,581535 R=leviw@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [css-flexbox] Use correct aspect ratio for min-size: auto https://codereview.chromium.org/1421423005 caused this regression. We never handled aspect ratio correctly in flexbox (that's bug 249112), but that patch made our handling worse because we now distort and enlarge images when a cross axis size is specified that's bigger than the image's intrinsic size. This was tested by imported/csswg-test/css-flexbox-1/flex-minimum-height-flex-items-008.xht which we now pass. I didn't realize the significance of failing that test at the time :( BUG=581361,581535 R=leviw@chromium.org ========== to ========== [css-flexbox] Use correct aspect ratio for min-size: auto https://codereview.chromium.org/1421423005 caused this regression. We never handled aspect ratio correctly in flexbox (that's bug 249112), but that patch made our handling worse because we now distort and enlarge images when a cross axis size is specified that's bigger than the image's intrinsic size. This was tested by imported/csswg-test/css-flexbox-1/flex-minimum-height-flex-items-008.xht which we now pass. I didn't realize the significance of failing that test at the time :( BUG=581361,581535 R=leviw@chromium.org Committed: https://crrev.com/eba8fa267c94a76faff64836a8c72a683c1a538e Cr-Commit-Position: refs/heads/master@{#372000} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/eba8fa267c94a76faff64836a8c72a683c1a538e Cr-Commit-Position: refs/heads/master@{#372000}
Message was sent while issue was closed.
phistuck@gmail.com changed reviewers: + phistuck@gmail.com
Message was sent while issue was closed.
Should object-fit and object-position affect the result?
Message was sent while issue was closed.
On 2016/01/28 15:20:13, PhistucK wrote: > Should object-fit and object-position affect the result? I don't think so. The flexbox spec makes no mention of it. Feel free to email www-style though. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
