Support contain:size;
<rdar://problem/75956816>
Created attachment 424901 [details] size-containment-WIP
Created attachment 424903 [details] WIP-patch-for-size-containment
Created attachment 425756 [details] Patch
Comment on attachment 425756 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425756&action=review > Source/WebCore/ChangeLog:8 > + This patch brings initial support of CSS contain:size. According to https://www.w3.org/TR/css-contain-2/#containment-size, Probably better to refer to contain-1, could also be a footnote ([1]). In general these lines should be kept short IIUC. > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1152 > + if (!m_strategy->isComputingSizeContainment()) { The strategy stuff probably needs explaining in the Changelog. > Source/WebCore/rendering/RenderBlock.cpp:1078 > + downcast<RenderBlockFlow>(*this).adjustSizeContainmentChildForPagination(r, r.logicalTop()); The pagination probably needs explaining in the Changelog. > Source/WebCore/rendering/RenderBox.cpp:2880 > + auto borderAndPading = RenderBox::borderBefore() + RenderBox::paddingBefore() + RenderBox::borderAfter() + RenderBox::paddingAfter(); There is borderAndPaddingBefore and borderAndPaddingAfter. *borderAndPadding* > Source/WebCore/rendering/RenderMultiColumnSet.cpp:240 > + if (columnCount() <= computedColumnCount() /*&& m_minSpaceShortage <= 0*/) { For the final patch we should have no code in comments. > Source/WebCore/rendering/RenderMultiColumnSet.cpp:245 > + if (forcedBreaksCount() >= computedColumnCount() /*&& m_minSpaceShortage <= 0*/) { Ditto. > Source/WebCore/rendering/RenderMultiColumnSet.cpp:257 > + return m_computedColumnHeight + sizeContainmentShortage; // So bail out rather than looping infinitely. The sizeContainmentShortage probably also needs ChangeLog explanation.
Comment on attachment 425756 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425756&action=review Hi Rob, Thanks for the review! >> Source/WebCore/ChangeLog:8 >> + This patch brings initial support of CSS contain:size. According to https://www.w3.org/TR/css-contain-2/#containment-size, > > Probably better to refer to contain-1, could also be a footnote ([1]). In general these lines should be kept short IIUC. Done, thanks! >> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1152 >> + if (!m_strategy->isComputingSizeContainment()) { > > The strategy stuff probably needs explaining in the Changelog. Yeah, will explain in the latest version:) >> Source/WebCore/rendering/RenderBlock.cpp:1078 >> + downcast<RenderBlockFlow>(*this).adjustSizeContainmentChildForPagination(r, r.logicalTop()); > > The pagination probably needs explaining in the Changelog. Ditto. >> Source/WebCore/rendering/RenderBox.cpp:2880 >> + auto borderAndPading = RenderBox::borderBefore() + RenderBox::paddingBefore() + RenderBox::borderAfter() + RenderBox::paddingAfter(); > > There is borderAndPaddingBefore and borderAndPaddingAfter. *borderAndPadding* There is a slight difference for fieldset. borderAndPaddingBefore add extra intrinsicBorderForFieldset for fieldset. but size containment doesn't need that. I will add a comment in the new patch. >> Source/WebCore/rendering/RenderMultiColumnSet.cpp:240 >> + if (columnCount() <= computedColumnCount() /*&& m_minSpaceShortage <= 0*/) { > > For the final patch we should have no code in comments. Ah, done! Thanks! >> Source/WebCore/rendering/RenderMultiColumnSet.cpp:257 >> + return m_computedColumnHeight + sizeContainmentShortage; // So bail out rather than looping infinitely. > > The sizeContainmentShortage probably also needs ChangeLog explanation. Done!
Created attachment 425987 [details] Patch
Comment on attachment 425987 [details] Patch Hi, I think this patch is ready for review. Thanks!
Looks good to me, but I think it should be reviewed by someone who’s been working in this area recently.
Created attachment 426439 [details] Patch
(In reply to Darin Adler from comment #9) > Looks good to me, but I think it should be reviewed by someone who’s been > working in this area recently. Hi Darin, Thanks! I added Simon to the cc list:) The new patch is now rebased on the code which supports contain:layout. Luckily we have 3 more tests passed in this patch. imported/w3c/web-platform-tests/css/css-contain/contain-animation-001.html imported/w3c/web-platform-tests/css/css-contain/contain-layout-size-003.html imported/w3c/web-platform-tests/css/css-contain/contain-strict-003.html
Created attachment 426537 [details] Patch
Created attachment 426543 [details] Patch
Comment on attachment 426543 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426543&action=review Nice work. I've mostly focused of the parts I know the most like grid. > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1186 > for (auto trackIndex : m_contentSizedTracksIndex) { So, if I understood this correctly, this loop is the only thing you have to do for the case of computing content:size. It that's the case then I'd do the following: 1- move the loop into a lambda inside this method. 2- at the beginning of the method do something like if (m_strategy->isComputingSizeContainment()) { execTheCallback(); return; } That would allow you to keep the method almost untouched (except for the lambda) and there would be no need to change indentation and all that. > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1247 > + m_sizingState = ColumnSizingSecondIteration; I am not sure I understand this change. I guess you basically compute content:size as if nothing had happened, so the first iteration of row computing could happen. My question is, why not adding a new state to the state machine? Something that would happen after RowSizingFirstIteration? > Source/WebCore/rendering/RenderBox.cpp:2880 > + // borderAndPaddingLogicalHeight contains intrinsicBorderForFieldset() which add extra border for fieldset, so we can't use it directly. Are you sure this is correct? I don't see any intrinsicBorderForFieldset() in the computation of borderAndPaddingLogicalHeight() > Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:211 > + } Same comment here as the method in grid. I'd create a lambda with the code bellow that just assigns the maxLogicalWidth and then adds the scrollbarwith and then at the beginning of the method if (shouldApplySizeContainment(*this)) { callLambda(); return; } .... body of the method untouched ... callLambda() > Source/WebCore/rendering/RenderFlexibleBox.cpp:141 > + } Ditto for this method. If the only thing you have to do is to add the scrollbarWidth to the min and max for the case of size containment, then move it to a lambda and if (shouldApplySizeContainment()) { lambda(); return; } ... body of the method untouched ... lambda() > Source/WebCore/rendering/RenderGrid.cpp:467 > + return 0; I am not sure I understand this change. Auto-fit is basically like auto-fill except that empty columns collapse. The fact that they collapse do not mean that they don't exist. But here we're saying that there are no auto-fit tracks. Maybe I am missing something?
Comment on attachment 426543 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426543&action=review Hi Sergio, Thanks for the review! >> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1186 >> for (auto trackIndex : m_contentSizedTracksIndex) { > > So, if I understood this correctly, this loop is the only thing you have to do for the case of computing content:size. It that's the case then I'd do the following: > 1- move the loop into a lambda inside this method. > 2- at the beginning of the method do something like > if (m_strategy->isComputingSizeContainment()) { > execTheCallback(); > return; > } > > That would allow you to keep the method almost untouched (except for the lambda) and there would be no need to change indentation and all that. Done! Yeah, a lambda function makes it more readable, the code is put to resolveIntrinsicTrackSizesFromChildren(). `sizeTrackToFitNonSpanningItem(span, *gridItem, track);` changes track sizes, so we also need to put this loop into the lambda. And we need to expand tack.growthLimit() to track.baseSize() if tack.growthLimit() is infinity which is handled at the end of this function, so instead of retrun early, it will be changed to if (!m_strategy->isComputingSizeContainment()) resolveIntrinsicTrackSizesFromChildren(); >> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1247 >> + m_sizingState = ColumnSizingSecondIteration; > > I am not sure I understand this change. I guess you basically compute content:size as if nothing had happened, so the first iteration of row computing could happen. My question is, why not adding a new state to the state machine? Something that would happen after RowSizingFirstIteration? Yes, in RenderGrid::layoutBlock it computes track height twice if shouldApplySizeContainment(*this). * computeTrackSizesForIndefiniteSize(m_trackSizingAlgorithm, ForRows); To compute trackBasedLogicalHeight as if there's no content. trackBasedLogicalHeight is used in the second time. * computeTrackSizesForDefiniteSize(ForRows, trackBasedLogicalHeight); Using trackBasedLogicalHeight computed by the first time to distribute the track sizes. So the state stays in RowSizingFirstIteration twice. I will add a new state RowSizingExtraIterationForSizeContainment to indicate that is the extra try for size containment. >> Source/WebCore/rendering/RenderBox.cpp:2880 >> + // borderAndPaddingLogicalHeight contains intrinsicBorderForFieldset() which add extra border for fieldset, so we can't use it directly. > > Are you sure this is correct? I don't see any intrinsicBorderForFieldset() in the computation of borderAndPaddingLogicalHeight() Yes, borderAndPaddingLogicalHeight calls borderBefore() and borderAfter(). In RenderBlock::borderBefore(), it adds intrinsicBorderForFieldset(). I'll point out it is specific to RenderBlock in the comment. >> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:211 >> + } > > Same comment here as the method in grid. I'd create a lambda with the code bellow that just assigns the maxLogicalWidth and then adds the scrollbarwith and then at the beginning of the method > if (shouldApplySizeContainment(*this)) { > callLambda(); > return; > } > .... body of the method untouched ... > callLambda() Done! Put the code to computeIntrinsicLogicalWidthsFromChildren. And also we need to handle the scrollbar width at the end of function, the code is now looked like this: if (!shouldApplySizeContainment(*this)) computeIntrinsicLogicalWidthsFromChildren(); >> Source/WebCore/rendering/RenderFlexibleBox.cpp:141 >> + } > > Ditto for this method. If the only thing you have to do is to add the scrollbarWidth to the min and max for the case of size containment, then move it to a lambda and > if (shouldApplySizeContainment()) { > lambda(); > return; > } > ... body of the method untouched ... > lambda() Done! Create a lambda function computeIntrinsicLogicalWidthsFromChildren. And it looks like this now: if (!shouldApplySizeContainment(*this)) computeIntrinsicLogicalWidthsFromChildren(); >> Source/WebCore/rendering/RenderGrid.cpp:467 >> + return 0; > > I am not sure I understand this change. Auto-fit is basically like auto-fill except that empty columns collapse. The fact that they collapse do not mean that they don't exist. But here we're saying that there are no auto-fit tracks. Maybe I am missing something? Hmmm, make sense. I think I should check size containment in computeEmptyTracksForAutoRepeat() instead.
Created attachment 427668 [details] Patch
Created attachment 427740 [details] Patch
(In reply to cathiechen from comment #15) > Comment on attachment 426543 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=426543&action=review > > Hi Sergio, > Thanks for the review! > > >> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1186 > >> for (auto trackIndex : m_contentSizedTracksIndex) { > > > > So, if I understood this correctly, this loop is the only thing you have to do for the case of computing content:size. It that's the case then I'd do the following: > > 1- move the loop into a lambda inside this method. > > 2- at the beginning of the method do something like > > if (m_strategy->isComputingSizeContainment()) { > > execTheCallback(); > > return; > > } > > > > That would allow you to keep the method almost untouched (except for the lambda) and there would be no need to change indentation and all that. > > Done! Yeah, a lambda function makes it more readable, the code is put to > resolveIntrinsicTrackSizesFromChildren(). > `sizeTrackToFitNonSpanningItem(span, *gridItem, track);` changes track > sizes, so we also need to put this loop into the lambda. > And we need to expand tack.growthLimit() to track.baseSize() if > tack.growthLimit() is infinity which is handled at the end of this function, > so instead of retrun early, it will be changed to > if (!m_strategy->isComputingSizeContainment()) > resolveIntrinsicTrackSizesFromChildren(); > Sorry, I misunderstood this! The new patch puts the code handle infinity growthLimit into a lambda instead:)
Comment on attachment 427740 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427740&action=review I've reviewed the grid and flex parts and they both look great to me now. > Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:190 > + auto handleScrollbarWidth = [&]() { Nit: addScrollbarWidth ? > Source/WebCore/rendering/RenderFlexibleBox.cpp:86 > + auto handleScrollbarWidth = [&]() { Nit: maybe addScrollbarWidth?
Comment on attachment 427740 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427740&action=review >> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:190 >> + auto handleScrollbarWidth = [&]() { > > Nit: addScrollbarWidth ? Done, thanks! >> Source/WebCore/rendering/RenderFlexibleBox.cpp:86 >> + auto handleScrollbarWidth = [&]() { > > Nit: maybe addScrollbarWidth? Done!
Created attachment 427744 [details] Patch
Comment on attachment 427744 [details] Patch This is one of those "hard to review" patches. it's really difficult to tell if it covers all the cases or whether it does not cause any unexpected states in the renderers. I even added "contain: size" to LFC to have a better understanding but it did not really help. Guess we'll just need to deal with the potential fallout from this.
Comment on attachment 427744 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427744&action=review > Source/WebCore/rendering/RenderBox.cpp:2865 > + auto borderAndPading = RenderBox::borderBefore() + RenderBox::paddingBefore() + RenderBox::borderAfter() + RenderBox::paddingAfter(); Can use borderAndPaddingBefore/borderAndPaddingAfter here.
Created attachment 428256 [details] Patch
(In reply to Rob Buis from comment #23) > Comment on attachment 427744 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427744&action=review > > > Source/WebCore/rendering/RenderBox.cpp:2865 > > + auto borderAndPading = RenderBox::borderBefore() + RenderBox::paddingBefore() + RenderBox::borderAfter() + RenderBox::paddingAfter(); > > Can use borderAndPaddingBefore/borderAndPaddingAfter here. Thanks, Rob! The borderAndPadding* interfaces eventually call RenderBlock::borderBefore which addes intrinsicBorderForFieldset(). I've added more detailed explanation in the comments.
(In reply to zalan from comment #22) > Comment on attachment 427744 [details] > Patch > > This is one of those "hard to review" patches. it's really difficult to tell > if it covers all the cases or whether it does not cause any unexpected > states in the renderers. I even added "contain: size" to LFC to have a > better understanding but it did not really help. Guess we'll just need to > deal with the potential fallout from this. Hi zalan, Thanks a lot for doing that! I think I can keep eyes on the follow-up bugs if any.
Zalan Bujtas found in /Volumes/Data/worker/Commit-Queue/build/LayoutTests/ChangeLog does not appear to be a valid reviewer according to contributors.json. /Volumes/Data/worker/Commit-Queue/build/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Created attachment 428264 [details] Patch
Committed r277321 (237580@main): <https://commits.webkit.org/237580@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 428264 [details].