Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
842193004:
[css-grid] Handle alignment with orthogonal flows (Closed)
|
Created:
5 years, 11 months ago by jfernandez Modified:
4 years, 5 months ago CC:
blink-reviews, blink-reviews-rendering, Dominik Röttsches, eae+blinkwatch, jchaffraix+rendering, jfernandez, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, Manuel Rego, svillar, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@orthogonal-flows Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[CSS Grid Layout] Handle alignment with orthogonal flows
Now that grid sizing and positioning issues wrt orthogonal flows have
been clarified in the last spec draft, we can adapt now our alignment
logic to work with orthogonal flows.
Even though basic alignment would work with orthogonal flows with
this patch, we still doesn't allow stretching in that case. I'll provide a
patch for that feature since it's a complex logic and better have an
isolated change.
BUG=556171, 445742, 376823, 249451
Committed: https://crrev.com/af5c2165bff01ba473fe28d31dd2a15d789ef08e
Cr-Commit-Position: refs/heads/master@{#406557}
Patch Set 1 #Patch Set 2 : Implemented logic for self-start/end values. #
Total comments: 10
Patch Set 3 : Applied suggested changes. #Patch Set 4 : Patch rebased. #
Total comments: 21
Patch Set 5 : Applied suggested changes. #Patch Set 6 : Removed the new stretching logic, accidentally added in previous patch. #Patch Set 7 : Added the TODO comment suggested. #
Total comments: 11
Patch Set 8 : Applied additional changes. #Messages
Total messages: 27 (9 generated)
jfernandez@igalia.com changed reviewers: + esprehn@chromium.org, jchaffraix@chromium.org, leviw@chromium.org
This issue depends on the one about sizing and positioning in orthogonal flows https://codereview.chromium.org/815833005/
https://codereview.chromium.org/842193004/diff/20001/LayoutTests/fast/css-gri... File LayoutTests/fast/css-grid-layout/grid-item-alignment-with-orthogonal-flows-vertical-lr.html (right): https://codereview.chromium.org/842193004/diff/20001/LayoutTests/fast/css-gri... LayoutTests/fast/css-grid-layout/grid-item-alignment-with-orthogonal-flows-vertical-lr.html:60: <div class="firstRowFirstColumn directionLTR horizontalTB end" data-offset-x="20" data-offset-y="10" data-expected-width="130" data-expected-height="10">XXXXXXXXXXXXX</div> 'direction' is inherited by default so directionLTR on all grid items is not needed. https://codereview.chromium.org/842193004/diff/20001/LayoutTests/fast/css-gri... LayoutTests/fast/css-grid-layout/grid-item-alignment-with-orthogonal-flows-vertical-lr.html:63: <div class="secondRowSecondColumn directionLTR horizontalTB right" data-offset-x="150" data-offset-y="60" data-expected-width="70" data-expected-height="10">XXXXXXX</div> Is this result really correct? I would have expected this child to be right-flushed in the column-axis direction thus leading to data-offset-x="230" (150 + (150 - 70)) and start-aligned in the row-axis direction leading to data-offset-y="20". https://codereview.chromium.org/842193004/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/842193004/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1347: case RightToLeftWritingMode: Seems like we could think of it in terms of isFlippedBlocksWritingMode(): if (isFlippedBlocksWritingMode(child.style()->writingMode())) { return style->isLeftToRightDirection() ? startOfColumnForChild(child) : endOfColumnForChild(child); } else { .... } https://codereview.chromium.org/842193004/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1358: if (hasOrthogonalWritingMode) { // See comment above about the block-axis? https://codereview.chromium.org/842193004/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1726: LayoutUnit childBreadth = hasOrthogonalWritingMode ? child.logicalHeight(): child.logicalWidth(); Style: Space before and after ':'.
Added new patch with the suggested changes. https://codereview.chromium.org/842193004/diff/20001/LayoutTests/fast/css-gri... File LayoutTests/fast/css-grid-layout/grid-item-alignment-with-orthogonal-flows-vertical-lr.html (right): https://codereview.chromium.org/842193004/diff/20001/LayoutTests/fast/css-gri... LayoutTests/fast/css-grid-layout/grid-item-alignment-with-orthogonal-flows-vertical-lr.html:60: <div class="firstRowFirstColumn directionLTR horizontalTB end" data-offset-x="20" data-offset-y="10" data-expected-width="130" data-expected-height="10">XXXXXXXXXXXXX</div> On 2015/01/26 10:42:06, Julien Chaffraix - AEST wrote: > 'direction' is inherited by default so directionLTR on all grid items is not > needed. Done. https://codereview.chromium.org/842193004/diff/20001/LayoutTests/fast/css-gri... LayoutTests/fast/css-grid-layout/grid-item-alignment-with-orthogonal-flows-vertical-lr.html:63: <div class="secondRowSecondColumn directionLTR horizontalTB right" data-offset-x="150" data-offset-y="60" data-expected-width="70" data-expected-height="10">XXXXXXX</div> On 2015/01/26 10:42:06, Julien Chaffraix - AEST wrote: > Is this result really correct? I would have expected this child to be > right-flushed in the column-axis direction thus leading to data-offset-x="230" > (150 + (150 - 70)) and start-aligned in the row-axis direction leading to > data-offset-y="20". Well, I could be wrong, but since both left/right values are relative to the grid container's edges, which is vertical-lr, right would be bottom for the property working in the inline-direction, while the property working in the block-direction would default to start in both cases. So the grid item would be aligned to bottom/left in case of right and top/left in case of left. https://codereview.chromium.org/842193004/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/842193004/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1347: case RightToLeftWritingMode: On 2015/01/26 10:42:07, Julien Chaffraix - AEST wrote: > Seems like we could think of it in terms of isFlippedBlocksWritingMode(): > > if (isFlippedBlocksWritingMode(child.style()->writingMode())) { > return style->isLeftToRightDirection() ? startOfColumnForChild(child) : > endOfColumnForChild(child); > } else { > .... > } Acknowledged. https://codereview.chromium.org/842193004/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1358: if (hasOrthogonalWritingMode) { On 2015/01/26 10:42:07, Julien Chaffraix - AEST wrote: > // See comment above about the block-axis? Done. https://codereview.chromium.org/842193004/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1726: LayoutUnit childBreadth = hasOrthogonalWritingMode ? child.logicalHeight(): child.logicalWidth(); On 2015/01/26 10:42:06, Julien Chaffraix - AEST wrote: > Style: Space before and after ':'. Done.
Description was changed from ========== [CSS Grid Layout] Handle alignment with orthogonal flows. Now that grid sizing and positioning issues wrt orhtogonal flows have been clarified in the last spec draft, we can adapt now our alignment logic to work with orthogonal flows. BUG=234194 ========== to ========== [CSS Grid Layout] Handle alignment with orthogonal flows. Now that grid sizing and positioning issues wrt orhtogonal flows have been clarified in the last spec draft, we can adapt now our alignment logic to work with orthogonal flows. BUG=556171, 445742, 376823, 249451 ==========
Description was changed from ========== [CSS Grid Layout] Handle alignment with orthogonal flows. Now that grid sizing and positioning issues wrt orhtogonal flows have been clarified in the last spec draft, we can adapt now our alignment logic to work with orthogonal flows. BUG=556171, 445742, 376823, 249451 ========== to ========== [CSS Grid Layout] Handle alignment with orthogonal flows Now that grid sizing and positioning issues wrt orhtogonal flows have been clarified in the last spec draft, we can adapt now our alignment logic to work with orthogonal flows. BUG=556171, 445742, 376823, 249451 ==========
https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1676: return child.size().height() > containingBlockContentHeight; Don't you need to add the margin-top to the child's size here? https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1682: return child.size().width() > containingBlockContentWidth; same question here
https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1676: return child.size().height() > containingBlockContentHeight; On 2016/06/23 18:35:12, cbiesinger wrote: > Don't you need to add the margin-top to the child's size here? Umm, that's a good question. The issue is that we weren't doing so before this changes. I'm just adapting the code to consider an orthogonal flow scenario. Not sure where I should fix that issue in this patch, or perhaps doing a different patch after filing a new bug with its proper test case. I'll start investigating this a I'll file the bug if I finally detect the error in current trunk. https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1682: return child.size().width() > containingBlockContentWidth; On 2016/06/23 18:35:12, cbiesinger wrote: > same question here Done.
https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1676: return child.size().height() > containingBlockContentHeight; On 2016/06/23 18:35:12, cbiesinger wrote: > Don't you need to add the margin-top to the child's size here? Umm, that's a good question. The issue is that we weren't doing so before this changes. I'm just adapting the code to consider an orthogonal flow scenario. Not sure where I should fix that issue in this patch, or perhaps doing a different patch after filing a new bug with its proper test case. I'll start investigating this a I'll file the bug if I finally detect the error in current trunk. https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1682: return child.size().width() > containingBlockContentWidth; On 2016/06/23 18:35:12, cbiesinger wrote: > same question here Done.
Amazing patch. We're pretty close. https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1162: return containingBlock()->isHorizontalWritingMode() ? hasOverrideContainingBlockLogicalHeight() : hasOverrideContainingBlockLogicalWidth(); Aren't logical height and width flipped here? If so this means that either some tests are wrong or that we lack tests. https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1734: || childOverflowingContainingBlockWidth(*child)) I think we lack a test for this change. https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2099: // Aligns the alignment subject to be flush with the edge of the alignment container "Aligns the alignment subject to be flush" don't understand that part of the sentence. https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2131: // Centers the alignment subject within its alignment container. The other comments are great but this is not really needed. https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2172: return hasSameDirection ? GridAxisStart : GridAxisEnd; Isn't this exactly the same code as in columnAxisPositionForChild ? We should not have these pieces of important code duplicated. https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2184: return hasSameDirection ? GridAxisEnd : GridAxisStart; Ditto. https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2194: // Centers the alignment subject within its alignment container. Ditto comment. https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.h:169: bool childOverflowingContainingBlockHeight(const LayoutBox&) const; These two names suggest that the functions are returning width and height respectively. Methods returning bools are typically prefixed by is or has.
Submitted a new patch for a new review after applying the suggested changes. https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1162: return containingBlock()->isHorizontalWritingMode() ? hasOverrideContainingBlockLogicalHeight() : hasOverrideContainingBlockLogicalWidth(); On 2016/07/13 09:00:40, svillar wrote: > Aren't logical height and width flipped here? Yes, they are. Good catch !!! > If so this means that either some tests are wrong or that we lack tests. We actually have tests for that, but the issue this mistake causes is just that all the items are considered as overflowing their area. This only would cause a performance regression, but the layout tests we currently have would pass anyway. Anyway, I'll try to figure out whether we could need additional tests cases. https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1734: || childOverflowingContainingBlockWidth(*child)) On 2016/07/13 09:00:40, svillar wrote: > I think we lack a test for this change. Done. https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2099: // Aligns the alignment subject to be flush with the edge of the alignment container On 2016/07/13 09:00:40, svillar wrote: > "Aligns the alignment subject to be flush" don't understand that part of the > sentence. It's what the Alignment spec literally states at: https://drafts.csswg.org/css-align/#positional-values It means it should move the "Alignment Subject" (grid item) to the "Alignment Container" 's (grid area) edge corresponding to the grid item's start edge. https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2131: // Centers the alignment subject within its alignment container. On 2016/07/13 09:00:40, svillar wrote: > The other comments are great but this is not really needed. Done. https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2172: return hasSameDirection ? GridAxisStart : GridAxisEnd; On 2016/07/13 09:00:40, svillar wrote: > Isn't this exactly the same code as in columnAxisPositionForChild ? No, It's not the same. Alignment along column-axis (block-axis) does not depend on the inline flow drection (direction property). Instead, it only depends on the block flow direction (writing-mode property). > We should not have these pieces of important code duplicated. I don't think they are. We could think in further refactoring of this logic, but If so, it should be part of a different patch. https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2184: return hasSameDirection ? GridAxisEnd : GridAxisStart; On 2016/07/13 09:00:40, svillar wrote: > Ditto. Already replied above. https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2194: // Centers the alignment subject within its alignment container. On 2016/07/13 09:00:40, svillar wrote: > Ditto comment. Done. https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.h:169: bool childOverflowingContainingBlockHeight(const LayoutBox&) const; On 2016/07/13 09:00:40, svillar wrote: > These two names suggest that the functions are returning width and height > respectively. Methods returning bools are typically prefixed by is or has. Acknowledged.
https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/842193004/diff/60001/third_party/WebKit/Sourc... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1676: return child.size().height() > containingBlockContentHeight; On 2016/06/24 12:59:15, jfernandez wrote: > On 2016/06/23 18:35:12, cbiesinger wrote: > > Don't you need to add the margin-top to the child's size here? > > Umm, that's a good question. The issue is that we weren't doing so before this > changes. I'm just adapting the code to consider an orthogonal flow scenario. > > Not sure where I should fix that issue in this patch, or perhaps doing a > different patch after filing a new bug with its proper test case. I'll start > investigating this a I'll file the bug if I finally detect the error in current > trunk. Indeed there's a bug here, I've just reported it: https://bugs.chromium.org/p/chromium/issues/detail?id=628155 Maybe it's worth add a TODO in the code pointing to the issue.
Description was changed from ========== [CSS Grid Layout] Handle alignment with orthogonal flows Now that grid sizing and positioning issues wrt orhtogonal flows have been clarified in the last spec draft, we can adapt now our alignment logic to work with orthogonal flows. BUG=556171, 445742, 376823, 249451 ========== to ========== [CSS Grid Layout] Handle alignment with orthogonal flows Now that grid sizing and positioning issues wrt orthogonal flows have been clarified in the last spec draft, we can adapt now our alignment logic to work with orthogonal flows. Even though basic alignment would work with orthogonal flows with this patch, we still doesn't allow stretching in that case. I'll provide a patch for that feature since it's a complex logic and better have an isolated change. BUG=556171, 445742, 376823, 249451 ==========
Submitted a new patch for review.
https://codereview.chromium.org/842193004/diff/120001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/842193004/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1144: LayoutUnit LayoutBox::overrideContainingBlockContentWidth() const By the way, I wonder if we should store the override cb width/height as the actual width/height instead of the logical one, and remove the logical accessors. The logical cb w/h is hard to reason about since for most purposes you need them relative to the child's writing mode. But at any rate -- unrelated to this bug.
Awesome patch. LGTM Consider doing the refactorings I mention. https://codereview.chromium.org/842193004/diff/120001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/842193004/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1144: LayoutUnit LayoutBox::overrideContainingBlockContentWidth() const On 2016/07/19 17:39:36, cbiesinger wrote: > By the way, I wonder if we should store the override cb width/height as the > actual width/height instead of the logical one, and remove the logical > accessors. The logical cb w/h is hard to reason about since for most purposes > you need them relative to the child's writing mode. But at any rate -- unrelated > to this bug. FWIW I agree with Christian. https://codereview.chromium.org/842193004/diff/120001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/842193004/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1860: if (oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth || (oldOverrideContainingBlockContentLogicalHeight != overrideContainingBlockContentLogicalHeight && child->hasRelativeLogicalHeight())) Why don't we need to force a layout for orthogonal children? https://codereview.chromium.org/842193004/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1888: || isChildOverflowingContainingBlockWidth(*child)) One line. https://codereview.chromium.org/842193004/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2274: I think it'd be a good idea to store in a bool the value of child.style()->isLeftToRightDirection(). That'll ease a bit reading the code. https://codereview.chromium.org/842193004/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2285: } I definitely think we should move this code block to an utility method and use it in the four places we're using it now (it will need some parameter)
https://codereview.chromium.org/842193004/diff/120001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/842193004/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1144: LayoutUnit LayoutBox::overrideContainingBlockContentWidth() const On 2016/07/20 09:23:36, svillar wrote: > On 2016/07/19 17:39:36, cbiesinger wrote: > > By the way, I wonder if we should store the override cb width/height as the > > actual width/height instead of the logical one, and remove the logical > > accessors. The logical cb w/h is hard to reason about since for most purposes > > you need them relative to the child's writing mode. But at any rate -- > unrelated > > to this bug. > > FWIW I agree with Christian. I think it'd make sense, so I'll add a TODO about it, so we can figure out how it impacts our current logic. https://codereview.chromium.org/842193004/diff/120001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/842193004/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1860: if (oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth || (oldOverrideContainingBlockContentLogicalHeight != overrideContainingBlockContentLogicalHeight && child->hasRelativeLogicalHeight())) On 2016/07/20 09:23:36, svillar wrote: > Why don't we need to force a layout for orthogonal children? Sincerely, that condition has been added in r401246 as a defensive strategy to face certain corner cases derived from orthogonal flows scenarios. I couldn't find any case, yet, needing that extra layout just for the mere fact of being orthogonal to the grid container. https://codereview.chromium.org/842193004/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1888: || isChildOverflowingContainingBlockWidth(*child)) On 2016/07/20 09:23:36, svillar wrote: > One line. Done. https://codereview.chromium.org/842193004/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2274: On 2016/07/20 09:23:36, svillar wrote: > I think it'd be a good idea to store in a bool the value of > child.style()->isLeftToRightDirection(). That'll ease a bit reading the code. Acknowledged. https://codereview.chromium.org/842193004/diff/120001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2285: } On 2016/07/20 09:23:36, svillar wrote: > I definitely think we should move this code block to an utility method and use > it in the four places we're using it now (it will need some parameter) This logic, and the similar one implemented in rowAxisPositionForChild, uses either grid container's or child's writing mode and inline direction to determine the final position, since sefl-start/end values depend on both elements. I think we'd lose some context if we extract the logic from the function where it's used. However, I understand your point and will add a TODO to evaluate it and, if convenient, implement such refactoring as part of a different patch. I thin we can use that opportunity to apply other refactoring, so we can rely on the LayoutBox functions for checking out orthogonality and inline flow direction instead of implementing new ones in the Grid Layout code.
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from svillar@igalia.com Link to the patchset: https://codereview.chromium.org/842193004/#ps140001 (title: "Applied additional changes.")
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 Layout] Handle alignment with orthogonal flows Now that grid sizing and positioning issues wrt orthogonal flows have been clarified in the last spec draft, we can adapt now our alignment logic to work with orthogonal flows. Even though basic alignment would work with orthogonal flows with this patch, we still doesn't allow stretching in that case. I'll provide a patch for that feature since it's a complex logic and better have an isolated change. BUG=556171, 445742, 376823, 249451 ========== to ========== [CSS Grid Layout] Handle alignment with orthogonal flows Now that grid sizing and positioning issues wrt orthogonal flows have been clarified in the last spec draft, we can adapt now our alignment logic to work with orthogonal flows. Even though basic alignment would work with orthogonal flows with this patch, we still doesn't allow stretching in that case. I'll provide a patch for that feature since it's a complex logic and better have an isolated change. BUG=556171, 445742, 376823, 249451 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [CSS Grid Layout] Handle alignment with orthogonal flows Now that grid sizing and positioning issues wrt orthogonal flows have been clarified in the last spec draft, we can adapt now our alignment logic to work with orthogonal flows. Even though basic alignment would work with orthogonal flows with this patch, we still doesn't allow stretching in that case. I'll provide a patch for that feature since it's a complex logic and better have an isolated change. BUG=556171, 445742, 376823, 249451 ========== to ========== [CSS Grid Layout] Handle alignment with orthogonal flows Now that grid sizing and positioning issues wrt orthogonal flows have been clarified in the last spec draft, we can adapt now our alignment logic to work with orthogonal flows. Even though basic alignment would work with orthogonal flows with this patch, we still doesn't allow stretching in that case. I'll provide a patch for that feature since it's a complex logic and better have an isolated change. BUG=556171, 445742, 376823, 249451 Committed: https://crrev.com/af5c2165bff01ba473fe28d31dd2a15d789ef08e Cr-Commit-Position: refs/heads/master@{#406557} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/af5c2165bff01ba473fe28d31dd2a15d789ef08e Cr-Commit-Position: refs/heads/master@{#406557} |
