Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1383003002:
[css-grid] Fix definite/indefinite size detection for abspos elements (Closed)
|
Created:
5 years, 2 months ago by Manuel Rego Modified:
4 years, 8 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 definite/indefinite size detection for abspos elements
We were considering that any abspos element has a definite size, and
that's not true. That's only true if the offset properties in that
dimension (left and right or top and bottom) are non-auto.
Fixed LayoutBox::hasDefiniteLogicalWidth|Height() to check this
properly.
This has been confirmed by the CSS WG in the following thread:
https://lists.w3.org/Archives/Public/www-style/2015Nov/0074.html
BUG=538513
TEST=fast/css-grid-layout/positioned-grid-container-percentage-tracks.html
Committed: https://crrev.com/b75b9bdaa3ede6fe0230ae822885d6fb38ed514b
Cr-Commit-Position: refs/heads/master@{#386045}
Patch Set 1 #
Total comments: 9
Patch Set 2 : New version #
Total comments: 2
Messages
Total messages: 23 (8 generated)
rego@igalia.com changed reviewers: + cbiesinger@chromium.org, mstensho@opera.com, svillar@igalia.com
This is an alternative patch to CL: https://codereview.chromium.org/1081433005/
Actually maybe we don't need these functions anymore. I've managed to cook a patch for https://codereview.chromium.org/1317643005/ without the need for these methods that we all agree are a bit weird.
https://codereview.chromium.org/1383003002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1383003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:2404: computeLogicalHeight(logicalHeight(), 0, computedValues); I don't think this is the right place to do this. Surely we already handle correct height calculation when top and bottom are non-auto. Something else must be wrong. https://codereview.chromium.org/1383003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4272: if (box->isOutOfFlowPositioned() && box->style()->logicalWidth().isAuto() && !box->style()->logicalLeft().isAuto() && !box->style()->logicalRight().isAuto()) The box->style()->logicalWidth().isAuto() confuses me. Is it necessary? Sure, if all three (width, left, right) are auto, we have an over-constrained situation, but does it matter here? https://codereview.chromium.org/1383003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4298: if (isOutOfFlowPositioned() && logicalHeight.isAuto() && !style()->logicalTop().isAuto() && !style()->logicalBottom().isAuto()) Same question as in logicalWidthIsResolvable().
https://codereview.chromium.org/1383003002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1383003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:2404: computeLogicalHeight(logicalHeight(), 0, computedValues); On 2015/10/02 09:08:50, mstensho wrote: > I don't think this is the right place to do this. Surely we already handle > correct height calculation when top and bottom are non-auto. Something else must > be wrong. Yes, this is handled in computePositionedLogicalHeight: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Why is this change needed here? https://codereview.chromium.org/1383003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4272: if (box->isOutOfFlowPositioned() && box->style()->logicalWidth().isAuto() && !box->style()->logicalLeft().isAuto() && !box->style()->logicalRight().isAuto()) I too am skeptical that this is correct... do you have a concrete testcase that this change fixes? https://codereview.chromium.org/1383003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4298: if (isOutOfFlowPositioned() && logicalHeight.isAuto() && !style()->logicalTop().isAuto() && !style()->logicalBottom().isAuto()) As I wrote in the other patch: I don't believe that's a correct interpretation of the CSS spec. At least, https://drafts.csswg.org/css-sizing-3/#definite does not indicate that this would be a definite size?
Please continue working on this CL or close it.
My advice will be to fix hasDefiniteLogicalXXX to return the right value for abspos elements and leave the grid changes for a separate patch.
Description was changed from ========== [css-grid] Fix definite/indefinite size detection for abspos elements We were considering that any abspos element has a definite size, and that's not true. That's only true if the offset properties in that dimension (left and right or top and bottom) are non-auto. Fixed LayoutBox::hasDefiniteLogicalWidth|Height() to check this properly. On top of that, the patch is modifying LayoutBox::computeContentAndScrollbarLogicalHeightUsing() for the case of abspos elements, in order to fix the behavior of percentage rows in absolutely positioned grid containers. BUG=538513, 476840 TEST=fast/css-grid-layout/positioned-grid-container-percentage-tracks.html ========== to ========== [css-grid] Fix definite/indefinite size detection for abspos elements We were considering that any abspos element has a definite size, and that's not true. That's only true if the offset properties in that dimension (left and right or top and bottom) are non-auto. Fixed LayoutBox::hasDefiniteLogicalWidth|Height() to check this properly. On top of that, the patch is modifying LayoutBox::computeContentAndScrollbarLogicalHeightUsing() for the case of abspos elements, in order to fix the behavior of percentage rows in absolutely positioned grid containers. BUG=538513, 476840 TEST=fast/css-grid-layout/positioned-grid-container-percentage-tracks.html ==========
mstensho@opera.com changed reviewers: - mstensho@opera.com
Description was changed from ========== [css-grid] Fix definite/indefinite size detection for abspos elements We were considering that any abspos element has a definite size, and that's not true. That's only true if the offset properties in that dimension (left and right or top and bottom) are non-auto. Fixed LayoutBox::hasDefiniteLogicalWidth|Height() to check this properly. On top of that, the patch is modifying LayoutBox::computeContentAndScrollbarLogicalHeightUsing() for the case of abspos elements, in order to fix the behavior of percentage rows in absolutely positioned grid containers. BUG=538513, 476840 TEST=fast/css-grid-layout/positioned-grid-container-percentage-tracks.html ========== to ========== [css-grid] Fix definite/indefinite size detection for abspos elements We were considering that any abspos element has a definite size, and that's not true. That's only true if the offset properties in that dimension (left and right or top and bottom) are non-auto. Fixed LayoutBox::hasDefiniteLogicalWidth|Height() to check this properly. This has been confirmed by the CSS WG in the following thread: https://lists.w3.org/Archives/Public/www-style/2015Nov/0074.html BUG=538513 TEST=fast/css-grid-layout/positioned-grid-container-percentage-tracks.html ==========
rego@igalia.com changed reviewers: + mstensho@opera.com
Finally I found time to come back to this issue. Sorry for the long wait. It seems we now do things better in LayoutGrid so I don't need any weird code, only fix properly the hasDefinitiveSize methods. The patch is simpler now and it's fixing the test. https://codereview.chromium.org/1383003002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1383003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:2404: computeLogicalHeight(logicalHeight(), 0, computedValues); It seems this is not needed anymore, so I'm removing this change. https://codereview.chromium.org/1383003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4272: if (box->isOutOfFlowPositioned() && box->style()->logicalWidth().isAuto() && !box->style()->logicalLeft().isAuto() && !box->style()->logicalRight().isAuto()) Yeah, we don't need to check if width is auto too, I've removed that condition. https://codereview.chromium.org/1383003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4298: if (isOutOfFlowPositioned() && logicalHeight.isAuto() && !style()->logicalTop().isAuto() && !style()->logicalBottom().isAuto()) Again I've removed here the condition to check if height is auto. Tab confirmed that this is a definitive size: https://lists.w3.org/Archives/Public/www-style/2015Nov/0074.html
No expectation file? And the bots are green?? :) I think you need to use linux_chromium_rel_ng and similar trybots, rather than linux_blink_rel (those aren't normally used anymore, after Chromium devoured Blink). https://codereview.chromium.org/1383003002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1383003002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4148: if (box->isOutOfFlowPositioned() && !box->style()->logicalLeft().isAuto() && !box->style()->logicalRight().isAuto()) maybe isFixed() would be a better choice than !isAuto(). I'm thinking about percentages... https://codereview.chromium.org/1383003002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:4174: if (isOutOfFlowPositioned() && !style()->logicalTop().isAuto() && !style()->logicalBottom().isAuto()) Ditto.
On 2016/04/08 09:06:35, mstensho wrote: > No expectation file? And the bots are green?? :) I think you need to use > linux_chromium_rel_ng and similar trybots, rather than linux_blink_rel (those > aren't normally used anymore, after Chromium devoured Blink). It's a Test Harness check-layout test (check-layout-th.js), and these tests don't need the -expected file. There're a lot of them in the grid folder. > https://codereview.chromium.org/1383003002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > https://codereview.chromium.org/1383003002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutBox.cpp:4148: if > (box->isOutOfFlowPositioned() && !box->style()->logicalLeft().isAuto() && > !box->style()->logicalRight().isAuto()) > maybe isFixed() would be a better choice than !isAuto(). I'm thinking about > percentages... Mmmm, I'm not sure if I get this question. If we use percentage for top/bottom, we'll be in a definitive size again. Maybe I should use isSpecified() instead of !isAuto()? Probably they're equivalent in this case.
On 2016/04/08 09:38:03, Manuel Rego wrote: > On 2016/04/08 09:06:35, mstensho wrote: > > No expectation file? And the bots are green?? :) I think you need to use > > linux_chromium_rel_ng and similar trybots, rather than linux_blink_rel (those > > aren't normally used anymore, after Chromium devoured Blink). > > It's a Test Harness check-layout test (check-layout-th.js), > and these tests don't need the -expected file. > There're a lot of them in the grid folder. Right, I missed that. I just saw that the previous patch set had an expectation file, while the new set didn't. > https://codereview.chromium.org/1383003002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > > > > https://codereview.chromium.org/1383003002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/LayoutBox.cpp:4148: if > > (box->isOutOfFlowPositioned() && !box->style()->logicalLeft().isAuto() && > > !box->style()->logicalRight().isAuto()) > > maybe isFixed() would be a better choice than !isAuto(). I'm thinking about > > percentages... > > Mmmm, I'm not sure if I get this question. > If we use percentage for top/bottom, we'll be in a definitive size again. > Maybe I should use isSpecified() instead of !isAuto()? > Probably they're equivalent in this case. https://drafts.csswg.org/css-sizing-3/#definite "A size that can be determined without measuring content" <div id="relpos" style="position:relative;"> <div id="abspos" style="position:absolute; top:10%; bottom:10%;"></div> line </div> We need to measure content (the height of the line in our case) to resolve the 'top' and 'bottom' of #abspos. Oh, but the spec does also say "Additionally, the size of the containing block of an absolutely positioned element is always definite with respect to that element". Anyway, I think the code is fine as it is, then. lgtm
The CQ bit was checked by rego@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1383003002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1383003002/20001
On 2016/04/08 09:48:47, mstensho wrote: > On 2016/04/08 09:38:03, Manuel Rego wrote: > > On 2016/04/08 09:06:35, mstensho wrote: > > > No expectation file? And the bots are green?? :) I think you need to use > > > linux_chromium_rel_ng and similar trybots, rather than linux_blink_rel > (those > > > aren't normally used anymore, after Chromium devoured Blink). > > > > It's a Test Harness check-layout test (check-layout-th.js), > > and these tests don't need the -expected file. > > There're a lot of them in the grid folder. > > Right, I missed that. I just saw that the previous patch set had an expectation > file, while the new set didn't. Yeah, I changed it from the first version and forgot to notify (sorry). > https://codereview.chromium.org/1383003002/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > > > > > > > > https://codereview.chromium.org/1383003002/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/layout/LayoutBox.cpp:4148: if > > > (box->isOutOfFlowPositioned() && !box->style()->logicalLeft().isAuto() && > > > !box->style()->logicalRight().isAuto()) > > > maybe isFixed() would be a better choice than !isAuto(). I'm thinking about > > > percentages... > > > > Mmmm, I'm not sure if I get this question. > > If we use percentage for top/bottom, we'll be in a definitive size again. > > Maybe I should use isSpecified() instead of !isAuto()? > > Probably they're equivalent in this case. > > https://drafts.csswg.org/css-sizing-3/#definite > > "A size that can be determined without measuring content" > > <div id="relpos" style="position:relative;"> > <div id="abspos" style="position:absolute; top:10%; bottom:10%;"></div> > line > </div> > > We need to measure content (the height of the line in our case) to resolve the > 'top' and 'bottom' of #abspos. > > Oh, but the spec does also say "Additionally, the size of the containing block > of an absolutely positioned element is always definite with respect to that > element". > > Anyway, I think the code is fine as it is, then. > > lgtm Thanks for the review.
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Fix definite/indefinite size detection for abspos elements We were considering that any abspos element has a definite size, and that's not true. That's only true if the offset properties in that dimension (left and right or top and bottom) are non-auto. Fixed LayoutBox::hasDefiniteLogicalWidth|Height() to check this properly. This has been confirmed by the CSS WG in the following thread: https://lists.w3.org/Archives/Public/www-style/2015Nov/0074.html BUG=538513 TEST=fast/css-grid-layout/positioned-grid-container-percentage-tracks.html ========== to ========== [css-grid] Fix definite/indefinite size detection for abspos elements We were considering that any abspos element has a definite size, and that's not true. That's only true if the offset properties in that dimension (left and right or top and bottom) are non-auto. Fixed LayoutBox::hasDefiniteLogicalWidth|Height() to check this properly. This has been confirmed by the CSS WG in the following thread: https://lists.w3.org/Archives/Public/www-style/2015Nov/0074.html BUG=538513 TEST=fast/css-grid-layout/positioned-grid-container-percentage-tracks.html ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Fix definite/indefinite size detection for abspos elements We were considering that any abspos element has a definite size, and that's not true. That's only true if the offset properties in that dimension (left and right or top and bottom) are non-auto. Fixed LayoutBox::hasDefiniteLogicalWidth|Height() to check this properly. This has been confirmed by the CSS WG in the following thread: https://lists.w3.org/Archives/Public/www-style/2015Nov/0074.html BUG=538513 TEST=fast/css-grid-layout/positioned-grid-container-percentage-tracks.html ========== to ========== [css-grid] Fix definite/indefinite size detection for abspos elements We were considering that any abspos element has a definite size, and that's not true. That's only true if the offset properties in that dimension (left and right or top and bottom) are non-auto. Fixed LayoutBox::hasDefiniteLogicalWidth|Height() to check this properly. This has been confirmed by the CSS WG in the following thread: https://lists.w3.org/Archives/Public/www-style/2015Nov/0074.html BUG=538513 TEST=fast/css-grid-layout/positioned-grid-container-percentage-tracks.html Committed: https://crrev.com/b75b9bdaa3ede6fe0230ae822885d6fb38ed514b Cr-Commit-Position: refs/heads/master@{#386045} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b75b9bdaa3ede6fe0230ae822885d6fb38ed514b Cr-Commit-Position: refs/heads/master@{#386045}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1954683002/ by rego@igalia.com. The reason for reverting is: This is breaking how percentages work in Flexbox and Grid compared to regular Blocks. There're some discussion ongoing on the CSS WG to verify the expected behavior, so we're reverting this for now until we've a final resolution.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
