Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1943633002:
[css-flexbox] Add test case for definite widths (Closed)
|
Created:
4 years, 7 months ago by cbiesinger Modified:
4 years, 7 months ago CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews, dgrogan Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[css-flexbox] Add test case for definite widths
Widths should always be definite, and this testcase
tests for that.
BUG=604346
Committed: https://crrev.com/9822f23e09df78a8a15380fbce9659397f83ee68
Cr-Commit-Position: refs/heads/master@{#392669}
Patch Set 1 #Patch Set 2 : with test #
Total comments: 5
Patch Set 3 : fixed nits #Patch Set 4 : test case only #
Messages
Total messages: 31 (13 generated)
cbiesinger@chromium.org changed reviewers: + mstensho@opera.com, rego@igalia.com
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/1943633002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1943633002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Sorry but I don't know properly flexbox spec, could you point me to where the spec says that the size of the stretched flex item is definite? Thanks. :-) Maybe we've a similar issue in Grid Layout, dunno. https://codereview.chromium.org/1943633002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/css3/flexbox/bug604346.html (right): https://codereview.chromium.org/1943633002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/css3/flexbox/bug604346.html:10: width: 100%; Do we need a similar test to check this for height? Just wondering if we should test this case: + Positioned +-- Flexbox (row) +-- Flexbox (column) +-- Item (height: 100%) https://codereview.chromium.org/1943633002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/css3/flexbox/bug604346.html:23: You should see no red Nit: Missing dot at the end. We usually wrap this in a paragraph.
https://codereview.chromium.org/1943633002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/css3/flexbox/bug604346.html (right): https://codereview.chromium.org/1943633002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/css3/flexbox/bug604346.html:30: XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX Trailing whitespace here and elsewhere. https://codereview.chromium.org/1943633002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1943633002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4178: static bool logicalWidthIsResolvable(const LayoutBox& layoutBox) Not for this CL, but I think this function should be refactored. It's hard to follow the logic. First we walk the ancestry until we find something interesting. Then there's pretty much duplicated code that examines the interesting object and returns true or false. https://codereview.chromium.org/1943633002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4193: if (isStretchingColumnFlexItem(box)) logicalWidthIsResolvable(), isn't that about the size being definite or not? I.e. https://drafts.csswg.org/css-sizing-3/#definite ? Some flexbox extensions to the definition of "definite size" here: https://drafts.csswg.org/css-flexbox/#definite-sizes But all widths and heights of flexboxes and flex items in the test case are auto, so there's nothing definite here, is there? Reduced test: <p>There should be no red or yellow here.</p> <div id="stf" style="float:left; background:yellow;"> <div id="outerFlex" style="display:flex; flex-direction:column;"> <div id="innerFlex" style="display:flex; background:red;"> <div id="elm" style="width:100%; background:green;">xxxxx</div> </div> </div> <div style="width:20em;"></div> </div> Without the patch, I see that #innerFlex gets the correct width. It's a flex item in #outerFlex, and its width is the cross size, which by default is stretched. #elm is a flex item in #innerFlex, and its main size is 100%. Its containing block is #innerFlex, so why not just set it to 100% of the width of #innerFlex? Why ask the ancestry for permission? Fear of cyclic dependencies?
I fixed the nits. The spec text in question is here - https://drafts.csswg.org/css-flexbox/#algo-stretch: If the flex item has align-self: stretch, redo layout for its contents, treating this used size as its definite cross size so that percentage-sized children can be resolved. This is also linked from case 3 of the definite size section that you mentioned. So, even with an "auto" size, you can still have a definite main size as a flex item. (This is not necessarily new - it's also true for most width: auto cases) The flexbox layout algorithm has to check explicitly for a definite height in a few places, because the spec requires it in a few places, for example https://drafts.csswg.org/css-flexbox/#hypothetical-main-size We can't just let the item size itself to 100% of its parent in such a straightforward way, because in the general case it may have to get flexed. So basically we have to compute its actual size, figure out whether to flex it, and then force its size to whatever we computed (setOverrideLogicalContentWidth, in this case). But that's actually not exactly the case in question, it's just because we need to determine whether it's definite for other reasons. I actually dislike logicalWidthIsResolvable also. I'm not even sure now in which cases it does/should return false, since I haven't found a case for regular blocks where we treat percentages as indefinite for widths. I thought I had a case, but my testcase was broken. Maybe this function should do something like check containingBlockLogicalWidthForContent() for -1 -- that's closer to what we do for heights, and reuses the code we use for regular width computations. But as you say, would be a different patch...
Oh, and for heights I believe we have sufficient testing in css3/flexbox/definite-cross-sizes.html. Since we don't have the same code duplication this problem does not come up.
The actual bug has been fixed by reverting the original change that broke it. However, I'd still like to land the test case. Please take a look!
On 2016/05/04 22:16:51, cbiesinger wrote: > I fixed the nits. > > The spec text in question is here - > https://drafts.csswg.org/css-flexbox/#algo-stretch: > If the flex item has align-self: stretch, redo layout for its contents, > treating > this used size as its definite cross size so that percentage-sized children > can > be resolved. > This is also linked from case 3 of the definite size section that you mentioned. > So, even with an "auto" size, you can still have a definite main size as a flex > item. (This is not necessarily new - it's also true for most width: auto cases) Sorry, I really didn't see point 3 last time I read this section of the spec. I must have read it with my beer glasses on, or someone snuck in point 3 during the past days. :) That definitely (no pun intended) suggests that your code changes were in accordance with the spec. Test lgtm. Might want to give it a name that describes the situation it's testing rather than a bug number, but I'm fine either way. But you need to update the description, since you're no longer fixing anything.
Description was changed from ========== [css-flexbox] Correctly handle indefinite widths Some widths are indefinite (floats, some positioned boxes) but are actually definite when a flex item is being stretched. Check for this case in logicalWidthIsResolvable BUG=604346 ========== to ========== [css-flexbox] Add test case for definite widths Widths should always be definite, and this testcase tests for that. BUG=604346 ==========
The CQ bit was checked by cbiesinger@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1943633002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1943633002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by cbiesinger@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1943633002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1943633002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by cbiesinger@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1943633002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1943633002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by cbiesinger@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1943633002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1943633002/60001
Message was sent while issue was closed.
Description was changed from ========== [css-flexbox] Add test case for definite widths Widths should always be definite, and this testcase tests for that. BUG=604346 ========== to ========== [css-flexbox] Add test case for definite widths Widths should always be definite, and this testcase tests for that. BUG=604346 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [css-flexbox] Add test case for definite widths Widths should always be definite, and this testcase tests for that. BUG=604346 ========== to ========== [css-flexbox] Add test case for definite widths Widths should always be definite, and this testcase tests for that. BUG=604346 Committed: https://crrev.com/9822f23e09df78a8a15380fbce9659397f83ee68 Cr-Commit-Position: refs/heads/master@{#392669} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9822f23e09df78a8a15380fbce9659397f83ee68 Cr-Commit-Position: refs/heads/master@{#392669} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
