Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
614263005:
[CSS Grid Layout] overflow-position keyword for align and justify properties. (Closed)
|
Created:
6 years, 2 months ago by jfernandez Modified:
6 years, 2 months ago CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, jfernandez, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, Manuel Rego, rune+blink, svillar, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
Description[CSS Grid Layout] overflow-position keyword for align and justify properties.
When the alignment subject is larger than the alignment container, it will
overflow. Some alignment modes, if honored in this situation, may cause data
loss; an overflow alignment mode can be explicitly specified to avoid this.
BUG=249451, 376823
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184309
Patch Set 1 #
Total comments: 12
Patch Set 2 : Applied suggested changes. #
Total comments: 20
Patch Set 3 : Better comments and code refactoring. #Patch Set 4 : Default overflow can't be resolved in the adjuster. #
Messages
Total messages: 17 (6 generated)
jfernandez@igalia.com changed reviewers: + cbiesinger@chromium.org, jchaffraix@chromium.org, timloh@chromium.org
fyi I haven't worked in core/rendering enough to review these, removing myself from the reviewers list.
https://codereview.chromium.org/614263005/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/614263005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderGrid.cpp:1182: static void resolveJustification(const RenderStyle* parentStyle, const RenderStyle* childStyle, ItemPosition& justify, OverflowAlignment &overflow) Style: The & should be next to the enum => OverflowAlignment& overflow https://codereview.chromium.org/614263005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderGrid.cpp:1196: if (overflow == OverflowAlignmentDefault) It's super weird to have to do this conversion here. It's a cleaner design to have StyleAdjuster take care of it. It would also remove the need for the out-parameters. https://codereview.chromium.org/614263005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderGrid.cpp:1288: if ((childLogicalHeight > rowHeight) && (overflow == OverflowAlignmentTrue || overflow == OverflowAlignmentDefault)) Can we really get overflow == OverflowAlignmentDefault as we eliminate it when resolving the justification / alignment? https://codereview.chromium.org/614263005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderGrid.cpp:1290: return rowPosition + std::max<LayoutUnit>(0, rowHeight - childLogicalHeight); I think this needs a comment why it's correct (it took me a while to assess that it was, I was going to say we should use return startOfRowForChild). https://codereview.chromium.org/614263005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderGrid.cpp:1299: // FIXME: This should account for the grid item's <overflow-position>. We should remove this FIXME as it doesn't apply to startOfRowForChild. Same for startOfColumnForChild. https://codereview.chromium.org/614263005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderGrid.cpp:1315: if ((childLogicalHeight > rowHeight) && (overflow == OverflowAlignmentTrue || overflow == OverflowAlignmentDefault)) Unneeded parenthesis around the first clause (it applies to all the new if's in the *rowpositionforchild).
Applied suggested changes. https://codereview.chromium.org/614263005/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/614263005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderGrid.cpp:1182: static void resolveJustification(const RenderStyle* parentStyle, const RenderStyle* childStyle, ItemPosition& justify, OverflowAlignment &overflow) On 2014/10/20 16:59:11, Julien Chaffraix - PST wrote: > Style: The & should be next to the enum => OverflowAlignment& overflow Done. https://codereview.chromium.org/614263005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderGrid.cpp:1196: if (overflow == OverflowAlignmentDefault) On 2014/10/20 16:59:10, Julien Chaffraix - PST wrote: > It's super weird to have to do this conversion here. It's a cleaner design to > have StyleAdjuster take care of it. > Yes, I agree. I fact, the same happens with the alignSelf/justifySelf properties but unfortunately, it wont work for every case. The reason why this resolveAlignment methods are still needed is because of the wrapper renders and other elements manipulating the Render Tree. The StyleResolver logic happens before having a RenderTree, so even that most of the cases are covered, I've found several tests rasing assertions in debug mode when using only the StyleAdjuster for those properties. However, I'll implement the overflow resolution in the StyleAdjuster and give it a try. https://codereview.chromium.org/614263005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderGrid.cpp:1288: if ((childLogicalHeight > rowHeight) && (overflow == OverflowAlignmentTrue || overflow == OverflowAlignmentDefault)) On 2014/10/20 16:59:11, Julien Chaffraix - PST wrote: > Can we really get overflow == OverflowAlignmentDefault as we eliminate it when > resolving the justification / alignment? I was thinking on other calls to this method which might happen before the justification /alignment resolution. I agree it might be overprotected logic; anyway, such code will be changed in the next patch and no need for this anymore. https://codereview.chromium.org/614263005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderGrid.cpp:1290: return rowPosition + std::max<LayoutUnit>(0, rowHeight - childLogicalHeight); On 2014/10/20 16:59:11, Julien Chaffraix - PST wrote: > I think this needs a comment why it's correct (it took me a while to assess that > it was, I was going to say we should use return startOfRowForChild). Done. https://codereview.chromium.org/614263005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderGrid.cpp:1299: // FIXME: This should account for the grid item's <overflow-position>. On 2014/10/20 16:59:11, Julien Chaffraix - PST wrote: > We should remove this FIXME as it doesn't apply to startOfRowForChild. Same for > startOfColumnForChild. Acknowledged. https://codereview.chromium.org/614263005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderGrid.cpp:1315: if ((childLogicalHeight > rowHeight) && (overflow == OverflowAlignmentTrue || overflow == OverflowAlignmentDefault)) On 2014/10/20 16:59:11, Julien Chaffraix - PST wrote: > Unneeded parenthesis around the first clause (it applies to all the new if's in > the *rowpositionforchild). Done.
jfernandez@igalia.com changed reviewers: - timloh@chromium.org
jfernandez@igalia.com changed reviewers: + ojan@chromium.org
lgtm https://codereview.chromium.org/614263005/diff/20001/LayoutTests/fast/css-gri... File LayoutTests/fast/css-grid-layout/grid-align-justify-overflow.html (right): https://codereview.chromium.org/614263005/diff/20001/LayoutTests/fast/css-gri... LayoutTests/fast/css-grid-layout/grid-align-justify-overflow.html:28: .cell { cell1 -> cellOverflowWidth cell2 -> cellOverflowHeight cell -> cellWithNoOverflow https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1180: ASSERT(overflow == OverflowAlignmentDefault); I don't think this is the ASSERT you were thinking of :) https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1190: // It overflows through the alignment container's 'start' edge (may cause data loss). I think we could improve this comment as I had a hard time understanding what it meant. Here is some suggestion: // If we overflow our alignment container and overflow is 'true', we ignore the overflow and just return the value regardless (which may cause data loss as we overflow the start edge). // If we overflow and overflow is 'safe', this will correctly clamp us and return the 'start' edge. https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1193: return columnPosition + std::max<LayoutUnit>(0, columnWidth - childLogicalWidth); I think it would be a lot more readable with the following logic: LayoutUnit offsetFromColumnPosition = columnWidth - childLogicalWidth; // If overflow is 'safe', we have to make sure we don't overflow the 'start' edge (potentially cause some data loss as the overflow is unreachable). if (overflow == OverflowAlignmentSafe) offsetFromColumnPosition = clampTo<LayoutUnit>(offsetFromColumnPosition, 0); // Note you can keep the std::max, I just have a fancy with clampTo these days return columnPosition + offsetFromColumnPosition; https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1215: ASSERT(overflow == OverflowAlignmentDefault); Same comment about the ASSERT. https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1226: return columnPosition + std::max<LayoutUnit>(0, columnWidth - childLogicalWidth) / 2; Same general comments. https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1314: ASSERT(overflow == OverflowAlignmentDefault); Ditto. https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1327: return rowPosition + std::max<LayoutUnit>(0, rowHeight - childLogicalHeight); Ditto. https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1344: ASSERT(overflow == OverflowAlignmentDefault); Ditto. https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1356: return rowPosition + std::max<LayoutUnit>(0, rowHeight - childLogicalHeight) / 2; Ditto.
Trying the CQ on the new patch with the last suggestions. https://codereview.chromium.org/614263005/diff/20001/LayoutTests/fast/css-gri... File LayoutTests/fast/css-grid-layout/grid-align-justify-overflow.html (right): https://codereview.chromium.org/614263005/diff/20001/LayoutTests/fast/css-gri... LayoutTests/fast/css-grid-layout/grid-align-justify-overflow.html:28: .cell { On 2014/10/23 00:57:24, Julien Chaffraix - PST wrote: > cell1 -> cellOverflowWidth > cell2 -> cellOverflowHeight > cell -> cellWithNoOverflow Done. https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1180: ASSERT(overflow == OverflowAlignmentDefault); On 2014/10/23 00:57:25, Julien Chaffraix - PST wrote: > I don't think this is the ASSERT you were thinking of :) Done. https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1190: // It overflows through the alignment container's 'start' edge (may cause data loss). On 2014/10/23 00:57:25, Julien Chaffraix - PST wrote: > I think we could improve this comment as I had a hard time understanding what it > meant. Here is some suggestion: > > // If we overflow our alignment container and overflow is 'true', we ignore the > overflow and just return the value regardless (which may cause data loss as we > overflow the start edge). > > // If we overflow and overflow is 'safe', this will correctly clamp us and > return the 'start' edge. Acknowledged. https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1193: return columnPosition + std::max<LayoutUnit>(0, columnWidth - childLogicalWidth); On 2014/10/23 00:57:25, Julien Chaffraix - PST wrote: > I think it would be a lot more readable with the following logic: > > LayoutUnit offsetFromColumnPosition = columnWidth - childLogicalWidth; > // If overflow is 'safe', we have to make sure we don't overflow the 'start' > edge (potentially cause some data loss as the overflow is unreachable). > if (overflow == OverflowAlignmentSafe) > offsetFromColumnPosition = clampTo<LayoutUnit>(offsetFromColumnPosition, 0); > // Note you can keep the std::max, I just have a fancy with clampTo these days > return columnPosition + offsetFromColumnPosition; Done. https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1215: ASSERT(overflow == OverflowAlignmentDefault); On 2014/10/23 00:57:25, Julien Chaffraix - PST wrote: > Same comment about the ASSERT. Done. https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1226: return columnPosition + std::max<LayoutUnit>(0, columnWidth - childLogicalWidth) / 2; On 2014/10/23 00:57:25, Julien Chaffraix - PST wrote: > Same general comments. Done. https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1314: ASSERT(overflow == OverflowAlignmentDefault); On 2014/10/23 00:57:25, Julien Chaffraix - PST wrote: > Ditto. Done. https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1327: return rowPosition + std::max<LayoutUnit>(0, rowHeight - childLogicalHeight); On 2014/10/23 00:57:25, Julien Chaffraix - PST wrote: > Ditto. Done. https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1344: ASSERT(overflow == OverflowAlignmentDefault); On 2014/10/23 00:57:25, Julien Chaffraix - PST wrote: > Ditto. Done. https://codereview.chromium.org/614263005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1356: return rowPosition + std::max<LayoutUnit>(0, rowHeight - childLogicalHeight) / 2; On 2014/10/23 00:57:24, Julien Chaffraix - PST wrote: > Ditto. Done.
The CQ bit was checked by jfernandez@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/614263005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)
In the new patch I've removed the StyleAdjuster logic to resolve the default overflow value, since it'd affect the computed value. Instead, we assume it will be handled as 'true', according to the specs, for grid. The FlexibleBox implementation must do the same.
The CQ bit was checked by jfernandez@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/614263005/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 184309 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
