Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2361373002:
[css-grid] Clearing override sizes before running grid's layout logic. (Closed)
|
Created:
4 years, 3 months ago by jfernandez Modified:
4 years, 2 months ago CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, svillar, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, Manuel Rego, leviw+renderwatch, jchaffraix+rendering, blink-reviews, eae+blinkwatch Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[css-grid] Clearing override sizes before running grid's layout logic.
Grid's layout logic manages two different override sizes; one it's
designed to implement the grid item's stretching behavior, identified
with the concept of 'overrideContentLogicalSize'; there is another
override size, known as overrideContainingBlockContentLogicalSize,
used to implement the Grid Area abstraction, which will behave as
the actual containing block of any grid item.
During grid's layout logic these override sizes are set according
to the CSS style rules. This affects how the grid container and its
children are going to be sized during layout. Grid Tracks sizing
algorithm depend on these override sizes.
In order to ensure that the tracks sizing algorithm produces the
same results when it's run consecutively several times, we need to
clear these override sizes and performs a layout of the affected grid
items. Otherwise, the affected items will return sizing values which
depend on the override values set in the previous layout, which in
some cases, like orthogonal flows, may change through different runs
of the sizing algorithm.
BUG=628565
Committed: https://crrev.com/45a34a1a8f382bcaa36e8d1c58575f34f2832237
Cr-Commit-Position: refs/heads/master@{#424250}
Patch Set 1 #
Total comments: 6
Patch Set 2 : A new approach, just facing orthogonal flow issues. #
Total comments: 4
Patch Set 3 : Getting back the initial approach, adding some constraints to avoid unneeded relayouts. #
Total comments: 2
Patch Set 4 : Patch for landing. #Patch Set 5 : Patch rebased for landing. #Patch Set 6 : Wrapped comments. #
Messages
Total messages: 28 (10 generated)
Description was changed from ========== [css-grid] Clearing override sizes before running grid's layout logic. The Grid's layout logic manages two different override sizes; one it's designed to implement the grid item's stretching behavior, defined as a global HashMap in the LayoutBox class and identified with the concept of 'overrideContentLogicalSize'; there is another override size used to implement the Grid Area abstraction, which will behave as the actual containing block of any grid item. During th grid's layout logic these override sizes are set according to the CSS style rules. The affect how the grid container and its children are going to be sized doring layout. The Grid Tracks sizing algorithm depend on these override sizes. In order to ensure that the Grid Tracks sizing algorithm produces the same results when it's run several times, we need to clear these override sizes and performs a layout of the affected grid items. If we don't do it, the affected items will return sizing values which depend on the override values set in the previous layout, which in some cases, like orthogonal flows, may change through different runs of the sizing algorithm. BUG=628565 ========== to ========== [css-grid] Clearing override sizes before running grid's layout logic. Grid's layout logic manages two different override sizes; one it's designed to implement the grid item's stretching behavior, identified with the concept of 'overrideContentLogicalSize'; there is another override size, known as overrideContainingBlockContentLogicalSize, used to implement the Grid Area abstraction, which will behave as the actual containing block of any grid item. During grid's layout logic these override sizes are set according to the CSS style rules. This affects how the grid container and its children are going to be sized during layout. Grid Tracks sizing algorithm depend on these override sizes. In order to ensure that the tracks sizing algorithm produces the same results when it's run consecutively several times, we need to clear these override sizes and performs a layout of the affected grid items. Otherwise, the affected items will return sizing values which depend on the override values set in the previous layout, which in some cases, like orthogonal flows, may change through different runs of the sizing algorithm. BUG=628565 ==========
jfernandez@igalia.com changed reviewers: + cbiesinger@chromium.org, rego@igalia.com, svillar@igalia.com
https://codereview.chromium.org/2361373002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2361373002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:464: child->layoutIfNeeded(); Why are you laying out the items even before placing them? I think the algorithm will properly lay them out whenever needed. Apart from that adding that extra layout seems overkill to me. Think about it, one of the grid items might be another grid.
https://codereview.chromium.org/2361373002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2361373002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:463: layoutScope.setChildNeedsLayout(child); (I would use forceChildLayout() instead of this sequence) I agree with Sergio, why force-layout everything here? Better to only layout if you need; I don't think you necessarily need layout for any grid item? Flexbox tries to only call layout when it is really needed and that's important for performance.
https://codereview.chromium.org/2361373002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2361373002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:463: layoutScope.setChildNeedsLayout(child); On 2016/09/23 16:57:09, cbiesinger wrote: > (I would use forceChildLayout() instead of this sequence) > > I agree with Sergio, why force-layout everything here? Better to only layout if > you need; I don't think you necessarily need layout for any grid item? Flexbox > tries to only call layout when it is really needed and that's important for > performance. I understand your point and I agree the current code can be seen as a brute-force approach, but as I commented below, I just wanted to spot the nature of the issue I'm trying to solve. So far, we have only found out cases with orthogonal items, but we are not sure if they are the only ones. Note that the problem happens when there are multiple layouts for the same LayoutTree. There is no way to avoid laid out the orthogonal items after clearing the override sizes if we really want to execute the track sizing algo without depending on previous values. https://codereview.chromium.org/2361373002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:464: child->layoutIfNeeded(); On 2016/09/23 15:42:28, svillar wrote: > Why are you laying out the items even before placing them? I think the algorithm > will properly lay them out whenever needed. Well, that code address issues related to subsequent layouts; it won't be executed the first time we laid out the grid and its children. That's why we check m_isDirty. I'm sure we can find another way of doing this, but first we need to agree on a valid approach to solve the real issue, which is the different behavior of the grid layout logic when it's run multiple times. First of all, bear in mind that we only detected, so far, issues when there are orthogonal grid items involved. We may focus this new code only on those cases, but I wanted first to do it generally, for the shake of discussing the problem. As you know, we indeed need to laid out orthogonal grid items before we perform the actual layout code of the grid container; otherwise we won't get the right values for the grid columns. We we are doing here is to clear the override sizes, both local (for stretch) and related to the grid area (item's containing block). We do this in order to recompute it again (remember that we are processing layout code twice) in a clean way. However, we have already laid out the grid items using the previous grid areas, so we must laid out again after clearing the override sizes; otherwise we will get a different result the next time we process the track sizing algorithm. > > Apart from that adding that extra layout seems overkill to me. Think about it, > one of the grid items might be another grid. Yes, indeed. This would complicate the solution. As I said, we can address just the orthogonal case, but such nested grid item could be orthogonal as well. Hard to say how we should address that case.
Ok, here we go with a totally different approach. I hope it addresses most of the concerns expressed in the reviews. Now we reuse the loop we are already doing to performs a pre-layout of orthogonal flow items. This will be done for any orthogonal item, not just the grid children, but I think there is no problem with that because the override sizes clearing is very cheap now that we have replaced the HashMap for a rareData attribute. There are still doubts regarding whether here are non-orthogonal hit by this issues.
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2361373002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2361373002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:1891: if (toLayoutBox(*root).hasContainingBlockOverrideSize() || toLayoutBox(*root).hasOverrideSize()) { Hmm, so I understand why you're doing this but I think there are two (related) problems: - For each layout pass, you force-layout all orthogonal roots whether they need to be laid out or not. Shouldn't you check .needsLayout() || root->parent().needsLayout()? - You lay out the child but the parent may not be marked for layout. That means that there may not be a full grid layout to lay out the child at the correct size, and the child may remain at the incorrect size! (The size without the override applied) https://codereview.chromium.org/2361373002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2361373002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1243: // TODO (lajava) Shouldn't we implement these functions based on physical direction ?. I don't think this new function needs that comment -- the function signature is independent of logical vs physical size
I guess what I'm wondering is: Why can't you do this force-layout at the point where you are accessing the child's size? I'm not sure exactly where this happens but basically insert a clearOverrideSize(); forceLayout(); just before you access .logicalHeight() of the child? Maybe I misunderstood the actual problem.
On 2016/09/26 11:49:31, cbiesinger wrote: > I guess what I'm wondering is: Why can't you do this force-layout at the point > where you are accessing the child's size? I'm not sure exactly where this > happens but basically insert a clearOverrideSize(); forceLayout(); just before > you access .logicalHeight() of the child? Maybe I misunderstood the actual > problem. The actual problem is that we are getting different values during intrinsic size computation; we can't perform layouts there. That's why blink already does this "early" layout of orthogonal boxes.
https://codereview.chromium.org/2361373002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2361373002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:1891: if (toLayoutBox(*root).hasContainingBlockOverrideSize() || toLayoutBox(*root).hasOverrideSize()) { On 2016/09/26 11:48:32, cbiesinger wrote: > Hmm, so I understand why you're doing this but I think there are two (related) > problems: > - For each layout pass, you force-layout all orthogonal roots whether they need > to be laid out or not. Shouldn't you check .needsLayout() || > root->parent().needsLayout()? For the case I'm trying to solve is not enough to check for needsLayout, since this re-layout is triggered by scrollbar changes which set grid container's preferred width as dirty; grid container will be marked for layout later. > - You lay out the child but the parent may not be marked for layout. That means > that there may not be a full grid layout to lay out the child at the correct > size, and the child may remain at the incorrect size! (The size without the > override applied) Yeah, as I said, for the scenario I'm trying to solve the container will be marked for layout, but it's true that this may not be the case in other situations. I'll have to think about it. https://codereview.chromium.org/2361373002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2361373002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1243: // TODO (lajava) Shouldn't we implement these functions based on physical direction ?. On 2016/09/26 11:48:33, cbiesinger wrote: > I don't think this new function needs that comment -- the function signature is > independent of logical vs physical size Acknowledged.
Summarizing our in-person discussion: The problem is in computeLogicalWidth() because the issue is the calculation of preferred sizes; it has a memory of the last layout which may be wrong now. Let's go back to the first patch set with a few minor improvements, as that seems to be the best way to get a mostly-correct way to fix this. Below a summary of the ideas. https://codereview.chromium.org/2361373002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2361373002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:457: if (!m_gridIsDirty) { && (sizesLogicalWidthToFitContent(style()->width()) || width.isIntrinsic() || minWidth.isIntrinsic() || minHeight.isIntrinsic()) https://codereview.chromium.org/2361373002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:459: if (child->isOutOfFlowPositioned()) || !hasOrthogonalFlow(child)
thanks for your patience and persistence with this patch :) lgtm with some comments below https://codereview.chromium.org/2361373002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2361373002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:456: // result when grid's intrinsic size it's computed again. Please add "in the updateLogicalWidth call below" to the end of this comment (also typo: it's -> is) https://codereview.chromium.org/2361373002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:458: for (auto* child : m_orthogonalChildren) { This is a little subtle because m_orthogonalChildren gets initialized by placeItemsOnGrid which is only called below. But this is correct anyway because we only need to call this on children that previously did go through the grid algorithm (and also, when we compute our intrinsic size we also call placeItemsOnGrid). Could you add a comment explaining this?
lgtm with @cbiesinger nits
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org Link to the patchset: https://codereview.chromium.org/2361373002/#ps80001 (title: "Patch for landing.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for third_party/WebKit/Source/core/layout/LayoutGrid.cpp:
While running git apply --index -3 -p1;
error: patch failed: third_party/WebKit/Source/core/layout/LayoutGrid.cpp:430
Falling back to three-way merge...
Applied patch to 'third_party/WebKit/Source/core/layout/LayoutGrid.cpp' with
conflicts.
U third_party/WebKit/Source/core/layout/LayoutGrid.cpp
Patch: third_party/WebKit/Source/core/layout/LayoutGrid.cpp
Index: third_party/WebKit/Source/core/layout/LayoutGrid.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
index
ec13d1934a0da3df498bbfaa44792491e7bfb277..fd6b86676ab43c13b845be8c7fe6338de6cf636d
100644
--- a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
@@ -430,7 +430,7 @@ void LayoutGrid::repeatTracksSizingIfNeeded(GridSizingData&
sizingData, LayoutUn
// a new cycle of the sizing algorithm; there may be more. In addition, not
all the
// cases with orthogonal flows require this extra cycle; we need a more
specific
// condition to detect whether child's min-content contribution has changed
or not.
- if (m_hasAnyOrthogonalChild) {
+ if (!m_orthogonalChildren.isEmpty()) {
computeTrackSizesForDefiniteSize(ForColumns, sizingData,
availableSpaceForColumns);
computeTrackSizesForDefiniteSize(ForRows, sizingData,
availableSpaceForRows);
}
@@ -452,6 +452,21 @@ void LayoutGrid::layoutBlock(bool relayoutChildren)
LayoutSize previousSize = size();
+ // We need to clear both own and containingBlock override sizes to
ensure we get the same
+ // result when grid's intrinsic size is computed again in the
updateLogicalWidth call bellow.
+ if (sizesLogicalWidthToFitContent(styleRef().logicalWidth()) ||
styleRef().logicalWidth().isIntrinsicOrAuto()) {
+ // We do cache orthogonal items during the placeItemsOnGrid call,
which is executed later. However, we are
+ // only interested on running this logic when we are performing a
relayout, hence we have already cached
+ // the orthogonal items.
+ for (auto* child : m_orthogonalChildren) {
+ if (child->isOutOfFlowPositioned())
+ continue;
+ child->clearOverrideSize();
+ child->clearContainingBlockOverrideSize();
+ child->forceLayout();
+ }
+ }
+
updateLogicalWidth();
m_hasDefiniteLogicalHeight = hasDefiniteLogicalHeight();
@@ -1568,12 +1583,13 @@ void LayoutGrid::placeItemsOnGrid(SizingOperation
sizingOperation)
Vector<LayoutBox*> autoMajorAxisAutoGridItems;
Vector<LayoutBox*> specifiedMajorAxisAutoGridItems;
- m_hasAnyOrthogonalChild = false;
+ m_orthogonalChildren.shrink(0);
for (LayoutBox* child = m_orderIterator.first(); child; child =
m_orderIterator.next()) {
if (child->isOutOfFlowPositioned())
continue;
- m_hasAnyOrthogonalChild = m_hasAnyOrthogonalChild ||
isOrthogonalChild(*child);
+ if (isOrthogonalChild(*child))
+ m_orthogonalChildren.append(child);
GridArea area = cachedGridArea(*child);
if (!area.rows.isIndefinite())
BTW -- now that we reformatted blink, please wrap your comment at 80 characters. git cl format does not currently wrap comments.
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org, svillar@igalia.com Link to the patchset: https://codereview.chromium.org/2361373002/#ps120001 (title: "Wrapped comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Clearing override sizes before running grid's layout logic. Grid's layout logic manages two different override sizes; one it's designed to implement the grid item's stretching behavior, identified with the concept of 'overrideContentLogicalSize'; there is another override size, known as overrideContainingBlockContentLogicalSize, used to implement the Grid Area abstraction, which will behave as the actual containing block of any grid item. During grid's layout logic these override sizes are set according to the CSS style rules. This affects how the grid container and its children are going to be sized during layout. Grid Tracks sizing algorithm depend on these override sizes. In order to ensure that the tracks sizing algorithm produces the same results when it's run consecutively several times, we need to clear these override sizes and performs a layout of the affected grid items. Otherwise, the affected items will return sizing values which depend on the override values set in the previous layout, which in some cases, like orthogonal flows, may change through different runs of the sizing algorithm. BUG=628565 ========== to ========== [css-grid] Clearing override sizes before running grid's layout logic. Grid's layout logic manages two different override sizes; one it's designed to implement the grid item's stretching behavior, identified with the concept of 'overrideContentLogicalSize'; there is another override size, known as overrideContainingBlockContentLogicalSize, used to implement the Grid Area abstraction, which will behave as the actual containing block of any grid item. During grid's layout logic these override sizes are set according to the CSS style rules. This affects how the grid container and its children are going to be sized during layout. Grid Tracks sizing algorithm depend on these override sizes. In order to ensure that the tracks sizing algorithm produces the same results when it's run consecutively several times, we need to clear these override sizes and performs a layout of the affected grid items. Otherwise, the affected items will return sizing values which depend on the override values set in the previous layout, which in some cases, like orthogonal flows, may change through different runs of the sizing algorithm. BUG=628565 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Clearing override sizes before running grid's layout logic. Grid's layout logic manages two different override sizes; one it's designed to implement the grid item's stretching behavior, identified with the concept of 'overrideContentLogicalSize'; there is another override size, known as overrideContainingBlockContentLogicalSize, used to implement the Grid Area abstraction, which will behave as the actual containing block of any grid item. During grid's layout logic these override sizes are set according to the CSS style rules. This affects how the grid container and its children are going to be sized during layout. Grid Tracks sizing algorithm depend on these override sizes. In order to ensure that the tracks sizing algorithm produces the same results when it's run consecutively several times, we need to clear these override sizes and performs a layout of the affected grid items. Otherwise, the affected items will return sizing values which depend on the override values set in the previous layout, which in some cases, like orthogonal flows, may change through different runs of the sizing algorithm. BUG=628565 ========== to ========== [css-grid] Clearing override sizes before running grid's layout logic. Grid's layout logic manages two different override sizes; one it's designed to implement the grid item's stretching behavior, identified with the concept of 'overrideContentLogicalSize'; there is another override size, known as overrideContainingBlockContentLogicalSize, used to implement the Grid Area abstraction, which will behave as the actual containing block of any grid item. During grid's layout logic these override sizes are set according to the CSS style rules. This affects how the grid container and its children are going to be sized during layout. Grid Tracks sizing algorithm depend on these override sizes. In order to ensure that the tracks sizing algorithm produces the same results when it's run consecutively several times, we need to clear these override sizes and performs a layout of the affected grid items. Otherwise, the affected items will return sizing values which depend on the override values set in the previous layout, which in some cases, like orthogonal flows, may change through different runs of the sizing algorithm. BUG=628565 Committed: https://crrev.com/45a34a1a8f382bcaa36e8d1c58575f34f2832237 Cr-Commit-Position: refs/heads/master@{#424250} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/45a34a1a8f382bcaa36e8d1c58575f34f2832237 Cr-Commit-Position: refs/heads/master@{#424250} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
