Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
706903003:
[CSS Grid Layout] Partial implementation of justify-content for Grid. (Closed)
|
Created:
6 years, 1 month ago by jfernandez Modified:
6 years, 1 month ago CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, jfernandez, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, Manuel Rego, svillar, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
Description[CSS Grid Layout] Partial implementation of justify-content for Grid.
Partial implementation of Content Distribution alignment defined by
the justify-content CSS property. There is no support yet for overflow
and all the content-distribution values use for now the corresponding
fallback content-position value.
Both content-position and content-distribution values are implemented
in both horizontal and vertical modes.
BUG=249451
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185747
Patch Set 1 #Patch Set 2 : content-distribution values always fallback. #Patch Set 3 : Using cells instead of items. #Patch Set 4 : Removed unneeded childPosition variable. #
Total comments: 16
Patch Set 5 : Applied suggested changes. #
Total comments: 8
Patch Set 6 : Applied additional suggested changes. #
Total comments: 10
Patch Set 7 : Support for RTL direction. #Patch Set 8 : Don't assert flex tracks exhaust the availableFreeSpace #
Messages
Total messages: 21 (6 generated)
jfernandez@igalia.com changed reviewers: + esprehn@chromium.org, jchaffraix@chromium.org
Patchset #2 (id:20001) has been deleted
I've removed the logic to deal with content-distribution values because for the time being, they will always fallback to the corresponding content-position value. There is an ongoing discussion about this topic, though, so I decided to add some FIXMEs to retake this logic if necessary int he future.
jfernandez@igalia.com changed reviewers: + leviw@chromium.org
https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1504: LayoutUnit static contentDistributionOffset(LayoutUnit availableFreeSpace, ContentPosition& fallbackPosition, ContentDistributionType distribution, unsigned numberOfItems) Note that numberOfItems is not great as it could refer to the number of *grid* items. https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1545: LayoutUnit offset = contentDistributionOffset(availableFreeSpace, position, distribution, numberOfItems); This function is stubbed so it's hard to see what purpose it serves. The 'auto' resolution should be its own function as it would make the code more readable and remove the need for the mostly stubbed function. https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1556: return availableFreeSpace; I think this should account for the children's size or else we won't have the content truly justified as we want (think of center which doesn't seem useful). https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1578: default: We seems to be missing one case ContentPositionAuto so it would be better to just implement it. The compiler would automatically detect new cases for us if we did that.
https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1504: LayoutUnit static contentDistributionOffset(LayoutUnit availableFreeSpace, ContentPosition& fallbackPosition, ContentDistributionType distribution, unsigned numberOfItems) On 2014/11/13 01:37:24, Julien Chaffraix - PST wrote: > Note that numberOfItems is not great as it could refer to the number of *grid* > items. Well, I tried to use the term defined in the Box Alignment spec, which was intended to be generic for good reasons. It's true that in the case of grid layout. where we decided to interpret the alignment subject as tracks, instead of grid items which is used n the rest of the alignment properties, it might be confusing. I'm not sure what to do; from one hand, think is useful to use the same terms and the reference specification document, but I agree in this particular case it may lead to confusion. Perhaps 'numberOfTracks' would be better in this case. https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1545: LayoutUnit offset = contentDistributionOffset(availableFreeSpace, position, distribution, numberOfItems); On 2014/11/13 01:37:24, Julien Chaffraix - PST wrote: > This function is stubbed so it's hard to see what purpose it serves. The 'auto' > resolution should be its own function as it would make the code more readable > and remove the need for the mostly stubbed function. Acknowledged. https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1556: return availableFreeSpace; On 2014/11/13 01:37:24, Julien Chaffraix - PST wrote: > I think this should account for the children's size or else we won't have the > content truly justified as we want (think of center which doesn't seem useful). I don't understand what you mean here; content alignment should not alter the grid item's size. As far as I understand the spec, it just changes the position of the grid tracks to the available empty space is redistributed. In the case of <content-position> values such distribution happens outside the grid tracks boundaries, while <content-distribution> affects both, external and internal boundaries. There is only one exception, the 'stretch' <content-distribution> value, which is under discussion in in the w3c mailing list with many doubts. Actually, grid is supposed to fallaback all the content-distribution values. I'm working on a possible approach, though, which may lead to a change in the specification to include those values in certain cases. https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1578: default: On 2014/11/13 01:37:24, Julien Chaffraix - PST wrote: > We seems to be missing one case ContentPositionAuto so it would be better to > just implement it. The compiler would automatically detect new cases for us if > we did that. Auto value is resolved by the StyleAdjuster. It could be kept, though, when <content-distribution> values are used instead. What we can do, though, is to move the fallback position resolution to the StyleAdjuster as well; I'll think about it if you agree.
https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1504: LayoutUnit static contentDistributionOffset(LayoutUnit availableFreeSpace, ContentPosition& fallbackPosition, ContentDistributionType distribution, unsigned numberOfItems) On 2014/11/13 12:22:35, jfernandez wrote: > On 2014/11/13 01:37:24, Julien Chaffraix - PST wrote: > > Note that numberOfItems is not great as it could refer to the number of *grid* > > items. > > Well, I tried to use the term defined in the Box Alignment spec, which was > intended to be generic for good reasons. It's true that in the case of grid > layout. where we decided to interpret the alignment subject as tracks, instead > of grid items which is used n the rest of the alignment properties, it might be > confusing. > > I'm not sure what to do; from one hand, think is useful to use the same terms > and the reference specification document, but I agree in this particular case it > may lead to confusion. Perhaps 'numberOfTracks' would be better in this case. numberOfTracks is a good name, even better numberOfGridTrack https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1556: return availableFreeSpace; On 2014/11/13 12:22:35, jfernandez wrote: > On 2014/11/13 01:37:24, Julien Chaffraix - PST wrote: > > I think this should account for the children's size or else we won't have the > > content truly justified as we want (think of center which doesn't seem > useful). > > I don't understand what you mean here; content alignment should not alter the > grid item's size. As far as I understand the spec, it just changes the position > of the grid tracks to the available empty space is redistributed. In the case of > <content-position> values such distribution happens outside the grid tracks > boundaries, while <content-distribution> affects both, external and internal > boundaries. I didn't mean as we should change the grid items' sizes. If you want to justify your grid items, you need to account for their sizes, else the position is going to be wrong. Available width / height only return the content width / height and don't remove any space taken by the children. > There is only one exception, the 'stretch' <content-distribution> value, which > is under discussion in in the w3c mailing list with many doubts. Actually, grid > is supposed to fallaback all the content-distribution values. I'm working on a > possible approach, though, which may lead to a change in the specification to > include those values in certain cases. ACK! https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1578: default: On 2014/11/13 12:22:35, jfernandez wrote: > On 2014/11/13 01:37:24, Julien Chaffraix - PST wrote: > > We seems to be missing one case ContentPositionAuto so it would be better to > > just implement it. The compiler would automatically detect new cases for us if > > we did that. > > Auto value is resolved by the StyleAdjuster. It could be kept, though, when > <content-distribution> values are used instead. What we can do, though, is to > move the fallback position resolution to the StyleAdjuster as well; I'll think > about it if you agree. I don't think StyleAdjuster would work as the computed value should be the specified value. The comment was just about how this logic is structured not about changing it completely. Having the explicitly missing value would make the code more future-proof.
https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1504: LayoutUnit static contentDistributionOffset(LayoutUnit availableFreeSpace, ContentPosition& fallbackPosition, ContentDistributionType distribution, unsigned numberOfItems) On 2014/11/13 16:43:31, Julien Chaffraix - PST wrote: > On 2014/11/13 12:22:35, jfernandez wrote: > > On 2014/11/13 01:37:24, Julien Chaffraix - PST wrote: > > > Note that numberOfItems is not great as it could refer to the number of > *grid* > > > items. > > > > Well, I tried to use the term defined in the Box Alignment spec, which was > > intended to be generic for good reasons. It's true that in the case of grid > > layout. where we decided to interpret the alignment subject as tracks, instead > > of grid items which is used n the rest of the alignment properties, it might > be > > confusing. > > > > I'm not sure what to do; from one hand, think is useful to use the same terms > > and the reference specification document, but I agree in this particular case > it > > may lead to confusion. Perhaps 'numberOfTracks' would be better in this case. > > numberOfTracks is a good name, even better numberOfGridTrack Done. https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1556: return availableFreeSpace; On 2014/11/13 16:43:31, Julien Chaffraix - PST wrote: > On 2014/11/13 12:22:35, jfernandez wrote: > > On 2014/11/13 01:37:24, Julien Chaffraix - PST wrote: > > > I think this should account for the children's size or else we won't have > the > > > content truly justified as we want (think of center which doesn't seem > > useful). > > > > I don't understand what you mean here; content alignment should not alter the > > grid item's size. As far as I understand the spec, it just changes the > position > > of the grid tracks to the available empty space is redistributed. In the case > of > > <content-position> values such distribution happens outside the grid tracks > > boundaries, while <content-distribution> affects both, external and internal > > boundaries. > > I didn't mean as we should change the grid items' sizes. If you want to justify > your grid items, you need to account for their sizes, else the position is going > to be wrong. Available width / height only return the content width / height and > don't remove any space taken by the children. > I'm not sure whether you notice that availableSpaceForColumns is initialized to availableLogicalWidth and then passed to the computeUsedBreadthOfGridTracks method, which indeed remove the space taken by the children (in case of contentSized tracks) or the tracks themselves, which is in the end what we want to align with these properties. Alignment specs state that for Grids the alignment subject is the grid boundaries, which I understand it's are the grid tracks. Except in the cases where grid item's size overflows, I think it does not affect to the content alignment logic. Overflow should be handled by the 'overflow' keyword, as it happens with the other alignment properties. Other than that I think we should do nothing about overflow, regarding alignment. > > There is only one exception, the 'stretch' <content-distribution> value, which > > is under discussion in in the w3c mailing list with many doubts. Actually, > grid > > is supposed to fallaback all the content-distribution values. I'm working on a > > possible approach, though, which may lead to a change in the specification to > > include those values in certain cases. > > ACK! https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1578: default: On 2014/11/13 16:43:31, Julien Chaffraix - PST wrote: > On 2014/11/13 12:22:35, jfernandez wrote: > > On 2014/11/13 01:37:24, Julien Chaffraix - PST wrote: > > > We seems to be missing one case ContentPositionAuto so it would be better to > > > just implement it. The compiler would automatically detect new cases for us > if > > > we did that. > > > > Auto value is resolved by the StyleAdjuster. It could be kept, though, when > > <content-distribution> values are used instead. What we can do, though, is to > > move the fallback position resolution to the StyleAdjuster as well; I'll think > > about it if you agree. > > I don't think StyleAdjuster would work as the computed value should be the > specified value. The comment was just about how this logic is structured not > about changing it completely. Having the explicitly missing value would make the > code more future-proof. Acknowledged.
https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1556: return availableFreeSpace; On 2014/11/14 15:32:32, jfernandez wrote: > On 2014/11/13 16:43:31, Julien Chaffraix - PST wrote: > > On 2014/11/13 12:22:35, jfernandez wrote: > > > On 2014/11/13 01:37:24, Julien Chaffraix - PST wrote: > > > > I think this should account for the children's size or else we won't have > > the > > > > content truly justified as we want (think of center which doesn't seem > > > useful). > > > > > > I don't understand what you mean here; content alignment should not alter > the > > > grid item's size. As far as I understand the spec, it just changes the > > position > > > of the grid tracks to the available empty space is redistributed. In the > case > > of > > > <content-position> values such distribution happens outside the grid tracks > > > boundaries, while <content-distribution> affects both, external and internal > > > boundaries. > > > > I didn't mean as we should change the grid items' sizes. If you want to > justify > > your grid items, you need to account for their sizes, else the position is > going > > to be wrong. Available width / height only return the content width / height > and > > don't remove any space taken by the children. > > > > I'm not sure whether you notice that availableSpaceForColumns is initialized to > availableLogicalWidth and then passed to the computeUsedBreadthOfGridTracks > method, which indeed remove the space taken by the children (in case of > contentSized tracks) or the tracks themselves, which is in the end what we want > to align with these properties. I missed that. With that fact, the logic makes more sense. > Alignment specs state that for Grids the alignment subject is the grid > boundaries, which I understand it's are the grid tracks. Except in the cases > where grid item's size overflows, I think it does not affect to the content > alignment logic. Overflow should be handled by the 'overflow' keyword, as it > happens with the other alignment properties. Other than that I think we should > do nothing about overflow, regarding alignment. AFAICT we don't support the 'overflow' value yet but that's fine. We should have FIXME about it though. > > > > > > There is only one exception, the 'stretch' <content-distribution> value, > which > > > is under discussion in in the w3c mailing list with many doubts. Actually, > > grid > > > is supposed to fallaback all the content-distribution values. I'm working on > a > > > possible approach, though, which may lead to a change in the specification > to > > > include those values in certain cases. > > > > ACK! https://codereview.chromium.org/706903003/diff/100001/Source/core/rendering/R... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/706903003/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:511: // Flexible tracks will exhaust the availableLogicalSpace. You're making one assumption here, which is that our rounding isn't off, which is a big one. RenderTable has a similar logic and I am glad we have some ASSERT to check this out. On top of it, it seems really artificial to unconditionally override this value instead of letting the grid items take their share of it. https://codereview.chromium.org/706903003/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1520: default: Why do we need a default? Your reflex should be to omit default as much as possible and explicitly state the cases (unless there is way too many cases). https://codereview.chromium.org/706903003/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1558: } This function is really artificial as I pointed out in a previous review. It would be way more natural to introduce the minimal amount of code and then refactor to introduce the extra scaffolding once you need it. https://codereview.chromium.org/706903003/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1599: default: I have not seen an explanation for keeping the 'default' case.
https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/706903003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderGrid.cpp:1556: return availableFreeSpace; On 2014/11/18 16:14:36, Julien Chaffraix - PST wrote: > On 2014/11/14 15:32:32, jfernandez wrote: > > On 2014/11/13 16:43:31, Julien Chaffraix - PST wrote: > > > On 2014/11/13 12:22:35, jfernandez wrote: > > > > On 2014/11/13 01:37:24, Julien Chaffraix - PST wrote: > > > > > I think this should account for the children's size or else we won't > have > > > the > > > > > content truly justified as we want (think of center which doesn't seem > > > > useful). > > > > > > > > I don't understand what you mean here; content alignment should not alter > > the > > > > grid item's size. As far as I understand the spec, it just changes the > > > position > > > > of the grid tracks to the available empty space is redistributed. In the > > case > > > of > > > > <content-position> values such distribution happens outside the grid > tracks > > > > boundaries, while <content-distribution> affects both, external and > internal > > > > boundaries. > > > > > > I didn't mean as we should change the grid items' sizes. If you want to > > justify > > > your grid items, you need to account for their sizes, else the position is > > going > > > to be wrong. Available width / height only return the content width / height > > and > > > don't remove any space taken by the children. > > > > > > > I'm not sure whether you notice that availableSpaceForColumns is initialized > to > > availableLogicalWidth and then passed to the computeUsedBreadthOfGridTracks > > method, which indeed remove the space taken by the children (in case of > > contentSized tracks) or the tracks themselves, which is in the end what we > want > > to align with these properties. > > I missed that. With that fact, the logic makes more sense. > > > Alignment specs state that for Grids the alignment subject is the grid > > boundaries, which I understand it's are the grid tracks. Except in the cases > > where grid item's size overflows, I think it does not affect to the content > > alignment logic. Overflow should be handled by the 'overflow' keyword, as it > > happens with the other alignment properties. Other than that I think we should > > do nothing about overflow, regarding alignment. > > AFAICT we don't support the 'overflow' value yet but that's fine. We should have > FIXME about it though. > > > > > > > > > > There is only one exception, the 'stretch' <content-distribution> value, > > which > > > > is under discussion in in the w3c mailing list with many doubts. Actually, > > > grid > > > > is supposed to fallaback all the content-distribution values. I'm working > on > > a > > > > possible approach, though, which may lead to a change in the specification > > to > > > > include those values in certain cases. > > > > > > ACK! > Done. https://codereview.chromium.org/706903003/diff/100001/Source/core/rendering/R... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/706903003/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:511: // Flexible tracks will exhaust the availableLogicalSpace. On 2014/11/18 16:14:36, Julien Chaffraix - PST wrote: > You're making one assumption here, which is that our rounding isn't off, which > is a big one. RenderTable has a similar logic and I am glad we have some ASSERT > to check this out. > > On top of it, it seems really artificial to unconditionally override this value > instead of letting the grid items take their share of it. Acknowledged. https://codereview.chromium.org/706903003/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1520: default: On 2014/11/18 16:14:36, Julien Chaffraix - PST wrote: > Why do we need a default? Your reflex should be to omit default as much as > possible and explicitly state the cases (unless there is way too many cases). My idea of the default option in these cases was to explicitly stating that any other option will be invalid, crashing in debug because of the assert and let it safely fallback in release mode. Anyway, I agree that I'd be simpler to just omit it. https://codereview.chromium.org/706903003/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1558: } On 2014/11/18 16:14:36, Julien Chaffraix - PST wrote: > This function is really artificial as I pointed out in a previous review. It > would be way more natural to introduce the minimal amount of code and then > refactor to introduce the extra scaffolding once you need it. Acknowledged. https://codereview.chromium.org/706903003/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1599: default: On 2014/11/18 16:14:36, Julien Chaffraix - PST wrote: > I have not seen an explanation for keeping the 'default' case. Done.
lgtm https://codereview.chromium.org/706903003/diff/120001/LayoutTests/fast/css-gr... File LayoutTests/fast/css-grid-layout/grid-justify-content.html (right): https://codereview.chromium.org/706903003/diff/120001/LayoutTests/fast/css-gr... LayoutTests/fast/css-grid-layout/grid-justify-content.html:154: <div class="grid justifyContentStart" data-expected-width="200" data-expected-height="300"> We would need a direction="ltr" for justify-content: start and end. https://codereview.chromium.org/706903003/diff/120001/LayoutTests/fast/css-gr... LayoutTests/fast/css-grid-layout/grid-justify-content.html:227: <div class="grid justifyContentSpaceBetweenLeft" data-expected-width="200" data-expected-height="300"> You should mention that this is not passing. Better even would be to land failing expectations so that we know this needs to be implemented and documents our behavior. https://codereview.chromium.org/706903003/diff/120001/Source/core/rendering/R... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/706903003/diff/120001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1517: case ContentDistributionSpaceEvenly: Those 3 values will need to be implemented too. Unfortunately a single start position will not do to make them work. Better to put a FIXME about it for now. https://codereview.chromium.org/706903003/diff/120001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1538: We need a FIXME for supporting the <overflow-position> property. https://codereview.chromium.org/706903003/diff/120001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1566: // FIXME: Implement the previous values. For now, we always start align. This is probably blocked on implementing baselines correctly for grid (crbug.com/234191).
https://codereview.chromium.org/706903003/diff/120001/LayoutTests/fast/css-gr... File LayoutTests/fast/css-grid-layout/grid-justify-content.html (right): https://codereview.chromium.org/706903003/diff/120001/LayoutTests/fast/css-gr... LayoutTests/fast/css-grid-layout/grid-justify-content.html:154: <div class="grid justifyContentStart" data-expected-width="200" data-expected-height="300"> On 2014/11/20 16:31:02, Julien Chaffraix - PST wrote: > We would need a direction="ltr" for justify-content: start and end. Done. https://codereview.chromium.org/706903003/diff/120001/LayoutTests/fast/css-gr... LayoutTests/fast/css-grid-layout/grid-justify-content.html:227: <div class="grid justifyContentSpaceBetweenLeft" data-expected-width="200" data-expected-height="300"> On 2014/11/20 16:31:02, Julien Chaffraix - PST wrote: > You should mention that this is not passing. Better even would be to land > failing expectations so that we know this needs to be implemented and documents > our behavior. Actually the specification states that all those values should fallback to their corresponding <content-posistion> value. This test provides cases for both, default and user defined <content-posistion> fallback. For the time being, and this is what could change in the future, all <content-distribution> values must fallback for Grids, so I think the test is correct as it is. It should be changed as soon as the specs introduce the expected behavior for these values on grid containers. https://codereview.chromium.org/706903003/diff/120001/Source/core/rendering/R... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/706903003/diff/120001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1517: case ContentDistributionSpaceEvenly: On 2014/11/20 16:31:03, Julien Chaffraix - PST wrote: > Those 3 values will need to be implemented too. Unfortunately a single start > position will not do to make them work. Better to put a FIXME about it for now. This function just resolved the default <content-position> fallback for each <content-distribution> value, according to the specs. It was stated that if a <content-position> is provided it will be used as fallback instead of the default one. Hence, no FIXME is needed here because it's implemented following to the specs. https://codereview.chromium.org/706903003/diff/120001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1538: On 2014/11/20 16:31:03, Julien Chaffraix - PST wrote: > We need a FIXME for supporting the <overflow-position> property. Acknowledged. https://codereview.chromium.org/706903003/diff/120001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1566: // FIXME: Implement the previous values. For now, we always start align. On 2014/11/20 16:31:02, Julien Chaffraix - PST wrote: > This is probably blocked on implementing baselines correctly for grid > (crbug.com/234191). Acknowledged.
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/706903003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/32005)
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/706903003/160001
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://src.chromium.org/viewvc/blink?view=rev&revision=185747 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
