Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
148293008:
[CSS Grid Layout] Add support to place items using named grid lines (Closed)
|
Created:
6 years, 10 months ago by svillar Modified:
6 years, 7 months ago CC:
blink-reviews, dglazkov+blink, apavlov+blink_chromium.org, ed+blinkwatch_opera.com, darktears Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
Description[CSS Grid Layout] Add support to place items using named grid lines
Currently our code assumes that a <custom-ident> in
grid-{column|row}-{start|end} and grid-{column|row} is always a grid area.
That's wrong because the <custom-ident> could be also a explicitly named
grid line.
Our resolution code is not correct either. This patch fixes it so it now
matches the spec which means that:
* first we try to match any existing grid area.
* if there is a named grid line with the name <custom-ident>-{start|end} for
grid-{column|row}-{start|end} defined before the grid area then we use it
instead of the grid area.
* otherwise if there is a named grid line we resolve to the first such line.
* otherwise we treat it as 'auto'.
BUG=337779
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174021
Patch Set 1 #
Total comments: 13
Patch Set 2 : Applied Julien's suggestions #Patch Set 3 : Back to smart pointers #Patch Set 4 : Improved readability of adjustNamedGridItemPosition #
Total comments: 2
Patch Set 5 : New approach #
Total comments: 3
Patch Set 6 : Refactored code and added comments #Patch Set 7 : Added some more test cases for special grid area names #
Total comments: 4
Patch Set 8 : Create implicit lines in StyleBuilder #Patch Set 9 : Rebased patch #
Total comments: 14
Patch Set 10 : Implementation following specs #
Total comments: 3
Patch Set 11 : Patch for landing #Patch Set 12 : Patch for landing v2. Rebased #Patch Set 13 : Patch for landing v3. Fix for Debug bot #Messages
Total messages: 39 (0 generated)
ping :)
https://codereview.chromium.org/148293008/diff/1/Source/core/css/resolver/Sty... File Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/148293008/diff/1/Source/core/css/resolver/Sty... Source/core/css/resolver/StyleAdjuster.cpp:396: const GridCoordinate gridAreaCoordinates = gridAreaMap.get(gridAreaName); It should be a reference, we don't need a copy here. https://codereview.chromium.org/148293008/diff/1/Source/core/css/resolver/Sty... Source/core/css/resolver/StyleAdjuster.cpp:399: // the grid coordinate to get the end line index. We have a helper function that does something very similar so I would rather use it here: GridPosition::adjustGridPositionForSide https://codereview.chromium.org/148293008/diff/1/Source/core/css/resolver/Sty... Source/core/css/resolver/StyleAdjuster.cpp:418: // The StyleBuilder always treats <ident> as a named grid area. We must decide here if they are going to be resolved <ident> is not <custom-ident> so let's try to match the specification in the new comments, adding FIXMEs where we need to convert <indent> to <custom-ident> in the parsing. https://codereview.chromium.org/148293008/diff/1/Source/core/css/resolver/Sty... Source/core/css/resolver/StyleAdjuster.cpp:420: OwnPtr<GridPosition> adjustedPosition; I don't understand why we use OwnPtr here instead of just a plain GridPosition? That should simplify the code and the caller. https://codereview.chromium.org/148293008/diff/1/Source/core/css/resolver/Sty... Source/core/css/resolver/StyleAdjuster.cpp:424: const bool hasEndSuffix = namedGridAreaOrGridLine.endsWith("-end"); I don't think this logic is right. We are asked to try to match against <custom-ident>-start / <custom-ident>-end. I don't see where we are asked to check if the value ends by -start / -end. I have to munch over this as it seems super gross from an API perspective.
https://codereview.chromium.org/148293008/diff/1/Source/core/css/resolver/Sty... File Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/148293008/diff/1/Source/core/css/resolver/Sty... Source/core/css/resolver/StyleAdjuster.cpp:396: const GridCoordinate gridAreaCoordinates = gridAreaMap.get(gridAreaName); On 2014/02/12 21:55:31, Julien Chaffraix - PST wrote: > It should be a reference, we don't need a copy here. yep https://codereview.chromium.org/148293008/diff/1/Source/core/css/resolver/Sty... Source/core/css/resolver/StyleAdjuster.cpp:399: // the grid coordinate to get the end line index. On 2014/02/12 21:55:31, Julien Chaffraix - PST wrote: > We have a helper function that does something very similar so I would rather use > it here: > > GridPosition::adjustGridPositionForSide Oh indeed, I forgot about that https://codereview.chromium.org/148293008/diff/1/Source/core/css/resolver/Sty... Source/core/css/resolver/StyleAdjuster.cpp:424: const bool hasEndSuffix = namedGridAreaOrGridLine.endsWith("-end"); On 2014/02/12 21:55:31, Julien Chaffraix - PST wrote: > I don't think this logic is right. We are asked to try to match against > <custom-ident>-start / <custom-ident>-end. I don't see where we are asked to > check if the value ends by -start / -end. Which specific logic do you think is wrong? I'm using the hasStartSuffix and hasEndSuffix to check if the author is using the implicit line names for a named grid area. We need to know that because the syntax allows: - named grid areas - implicitly named grid lines - explicitly named grid lines > I have to munch over this as it seems super gross from an API perspective.
https://codereview.chromium.org/148293008/diff/1/Source/core/css/resolver/Sty... File Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/148293008/diff/1/Source/core/css/resolver/Sty... Source/core/css/resolver/StyleAdjuster.cpp:424: const bool hasEndSuffix = namedGridAreaOrGridLine.endsWith("-end"); On 2014/02/13 10:13:39, svillar wrote: > On 2014/02/12 21:55:31, Julien Chaffraix - PST wrote: > > I don't think this logic is right. We are asked to try to match against > > <custom-ident>-start / <custom-ident>-end. I don't see where we are asked to > > check if the value ends by -start / -end. > > Which specific logic do you think is wrong? I'm using the hasStartSuffix and > hasEndSuffix to check if the author is using the implicit line names for a named > grid area. We need to know that because the syntax allows: > - named grid areas > - implicitly named grid lines > - explicitly named grid lines The way you handle the named grid line doesn't follow what the prose says. With grid-row-start: area : 1. If "area" is a valid named area 1.1. If "area-start" is a valid named line, then place the grid item in the first named line. 1.2. Else place the grid item in the start line of the named grid area "area". 2. If "area" is a named line then place the grid item in the first named line. 3. We cannot resolve the property, it is equivalent to "auto". https://codereview.chromium.org/148293008/diff/1/Source/core/css/resolver/Sty... Source/core/css/resolver/StyleAdjuster.cpp:431: if (gridAreaMap.contains(gridAreaName)) { Tab pointed out that in the new specification, named areas are not a primitive concept anymore and thus we could handle them by generating and storing the appropriate named lines. This would remove a lot of complexity in the resolution code.
Damn, looks like I had totally forgotten to send the comments. https://codereview.chromium.org/148293008/diff/1/Source/core/css/resolver/Sty... File Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/148293008/diff/1/Source/core/css/resolver/Sty... Source/core/css/resolver/StyleAdjuster.cpp:424: const bool hasEndSuffix = namedGridAreaOrGridLine.endsWith("-end"); On 2014/02/13 19:32:01, Julien Chaffraix - PST wrote: > On 2014/02/13 10:13:39, svillar wrote: > > On 2014/02/12 21:55:31, Julien Chaffraix - PST wrote: > > > I don't think this logic is right. We are asked to try to match against > > > <custom-ident>-start / <custom-ident>-end. I don't see where we are asked to > > > check if the value ends by -start / -end. > > > > Which specific logic do you think is wrong? I'm using the hasStartSuffix and > > hasEndSuffix to check if the author is using the implicit line names for a > named > > grid area. We need to know that because the syntax allows: > > - named grid areas > > - implicitly named grid lines > > - explicitly named grid lines > > The way you handle the named grid line doesn't follow what the prose says. With > grid-row-start: area : > > 1. If "area" is a valid named area > 1.1. If "area-start" is a valid named line, then place the grid item in the > first named line. > 1.2. Else place the grid item in the start line of the named grid area > "area". > 2. If "area" is a named line then place the grid item in the first named line. > 3. We cannot resolve the property, it is equivalent to "auto". I think the code works like the algorithm above but it's a little bit different because it covers more cases. For example, the author may specify: grid-column: a-start; so you cannot just run the above algorithm using "a-start" as the name of the area. You need to extract "a" and check if that's an area name. There are some other particularities, for example, if in the example above you decide to resolve a-start to a named area, you have to replace the original position from the parser (which is a namedgridarea called "a-start") by another one suitable for the renderer (a namedgridarea called "a") because the renderer will look in the named areas map using the name specified in the position, so it will never find the area "a-start" but the area called "a". The code has also some checks for other stuff like for example, the above specification should be expanded, according to the spec, to grid-column: a-start / a-start; Should we have a named area called "a", this code will resolve to: grid-column: "start line of area a" / auto because the code looks for "-start" only for column-start|row-start (same for -end in column-end|row-end). As a final note, it's very important to know that this method _adjusts_ the position, so in some cases it returns null which means that the original position is left untouched. That's the case of what you mention in 1.2 for example. We could change it and always replace the original position if you think that improves the readability of the code. https://codereview.chromium.org/148293008/diff/1/Source/core/css/resolver/Sty... Source/core/css/resolver/StyleAdjuster.cpp:431: if (gridAreaMap.contains(gridAreaName)) { On 2014/02/13 19:32:01, Julien Chaffraix - PST wrote: > Tab pointed out that in the new specification, named areas are not a primitive > concept anymore and thus we could handle them by generating and storing the > appropriate named lines. This would remove a lot of complexity in the resolution > code. That's right, I wonder how that would affect the auto-placement algorithm tough.
Applied Julien's suggestions
https://codereview.chromium.org/148293008/diff/1/Source/core/css/resolver/Sty... File Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/148293008/diff/1/Source/core/css/resolver/Sty... Source/core/css/resolver/StyleAdjuster.cpp:420: OwnPtr<GridPosition> adjustedPosition; On 2014/02/12 21:55:31, Julien Chaffraix - PST wrote: > I don't understand why we use OwnPtr here instead of just a plain GridPosition? > That should simplify the code and the caller. Note that I need to use a pointer here, because the function can return a new GridPosition or not depending on if we have to modify the currently existing one. By using the OwnPtr/PassOwnPtr stuff we avoid the handling of a raw pointer.
https://codereview.chromium.org/148293008/diff/130001/Source/core/css/resolve... File Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/148293008/diff/130001/Source/core/css/resolve... Source/core/css/resolver/StyleAdjuster.cpp:415: bool isStartSide = side == ColumnStartSide || side == RowStartSide; This could probably be an helper function (not really needed for this patch). https://codereview.chromium.org/148293008/diff/130001/Source/core/css/resolve... Source/core/css/resolver/StyleAdjuster.cpp:423: if (isStartSide && !hasStartSuffix) I think I get why we need this now, thanks for the explanation. It's crazy from my perspective that we need such code though. It makes the implementation complicated by obfuscating the intent of the code. It would be cleaner just to generate implicit named lines from the named areas. That should give us the named line resolution for free.
On 2014/03/05 01:12:34, Julien Chaffraix - PST wrote: > https://codereview.chromium.org/148293008/diff/130001/Source/core/css/resolve... > File Source/core/css/resolver/StyleAdjuster.cpp (right): > > https://codereview.chromium.org/148293008/diff/130001/Source/core/css/resolve... > Source/core/css/resolver/StyleAdjuster.cpp:415: bool isStartSide = side == > ColumnStartSide || side == RowStartSide; > This could probably be an helper function (not really needed for this patch). > > https://codereview.chromium.org/148293008/diff/130001/Source/core/css/resolve... > Source/core/css/resolver/StyleAdjuster.cpp:423: if (isStartSide && > !hasStartSuffix) > I think I get why we need this now, thanks for the explanation. It's crazy from > my perspective that we need such code though. It makes the implementation > complicated by obfuscating the intent of the code. > > It would be cleaner just to generate implicit named lines from the named areas. > That should give us the named line resolution for free. That would still require some of the checks for the suffixes. Also it isn't totally clear to me were to do that creation, there are different possibilities and all of them have some issues. In any case I think that my original approach of doing it in the StyleAdjuster is wrong, because I've realized that I'd lost the specified value which is the one we should return in getComputedStyle(). The specs now mention "treat" and not "compute".
Moved the map of grid line names grom the style to the renderer
The new change looks much better, thanks! https://codereview.chromium.org/148293008/diff/150001/Source/core/rendering/R... File Source/core/rendering/RenderGrid.cpp (left): https://codereview.chromium.org/148293008/diff/150001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1083: // 'auto' and span depend on the opposite position for resolution (e.g. grid-row: auto / 1 or grid-column: span 3 / "myHeader"). I liked this comment as it explained why we shouldn't see the other types when doing a pure style-based resolution. https://codereview.chromium.org/148293008/diff/150001/Source/core/rendering/R... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/148293008/diff/150001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1095: NamedGridAreaMap::const_iterator areaIter = style()->namedGridArea().find(position.namedGridLine()); I think it would be worth a FIXME / comment about inserting the named maps as their implicit named grid lines. https://codereview.chromium.org/148293008/diff/150001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1110: ASSERT(adjustedPosition.integerPosition()); This really looks like we should move it to a function. With that, we would simplify the code above a tiny bit (by removing the need for adjustedPosition in the non-named grid area case).
On 2014/03/20 18:14:41, Julien Chaffraix - PST wrote: > The new change looks much better, thanks! Uploaded a new version with the suggested changes.
Added some more test cases for special grid area names
ping owners :)
https://codereview.chromium.org/148293008/diff/180001/Source/core/rendering/R... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/148293008/diff/180001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:250: } I really don't understand what moving this code to RenderGrid buys us apart from an extra iteration and sort. I am probably missing something though so an explanation would be welcome.
Thanks for the review. https://codereview.chromium.org/148293008/diff/180001/Source/core/rendering/R... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/148293008/diff/180001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:250: } On 2014/03/31 23:36:21, Julien Chaffraix - PST wrote: > I really don't understand what moving this code to RenderGrid buys us apart from > an extra iteration and sort. I am probably missing something though so an > explanation would be welcome. So the idea would be to remove the map of grid lines from the style and build it in the renderer, as it will become a kind of intermediate data structure used only there. The ordered map will remain in the style as it'll be used to create the output for getComputedStyle(). Note that, leaving the code in the StyleBuilder would be worst as we'd need to recreate the whole map whenever one of the properties change (either that or checking one by one the lines/areas that have changed, adding/removing implicit lines, etc... which would be much more complicated).
We should just split the reverse map move to a follow-up change as I think the core change is straightforward and sane but we get side-tracked by the unneeded refactoring. https://codereview.chromium.org/148293008/diff/180001/Source/core/rendering/R... File Source/core/rendering/RenderGrid.cpp (right): https://codereview.chromium.org/148293008/diff/180001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:250: } On 2014/04/01 08:00:42, svillar wrote: > On 2014/03/31 23:36:21, Julien Chaffraix - PST wrote: > > I really don't understand what moving this code to RenderGrid buys us apart > from > > an extra iteration and sort. I am probably missing something though so an > > explanation would be welcome. > > So the idea would be to remove the map of grid lines from the style and build it > in the renderer, as it will become a kind of intermediate data structure used > only there. The ordered map will remain in the style as it'll be used to create > the output for getComputedStyle(). I see the point of splitting the 2 maps as only one is relevant to the computed style but it seems artificial to so from my perspective. > Note that, leaving the code in the StyleBuilder would be worst as we'd need to > recreate the whole map whenever one of the properties change (either that or > checking one by one the lines/areas that have changed, adding/removing implicit > lines, etc... which would be much more complicated). I don't really see the issue with having to recompute this map: we have to recompute the ordered map in these cases anyway. Also until we have users of grid, it's difficult to make a performance case so I would like to go with the more simple solution. https://codereview.chromium.org/148293008/diff/180001/Source/core/rendering/R... Source/core/rendering/RenderGrid.cpp:1012: String lineName = position.namedGridLine(); Wouldn't passing this String directly to isNonExistentNamedLineOrArea avoid the redundant ASSERT above?
As agreed, I moved the addition of the implicit lines to the StyleBuilder. Apart from that I added a new extra test to check that dynamic changes in grid columns/rows/areas are properly handled.
On 2014/04/10 18:35:53, svillar wrote: > As agreed, I moved the addition of the implicit lines to the StyleBuilder. > > Apart from that I added a new extra test to check that dynamic changes in grid > columns/rows/areas are properly handled. Julien, Ojan... this is becoming too old :), time for another review? (I'd probably have to rebase it after the last week changes before landing anyway)
I think we should make our assumptions more in the new code as we are aligning ourselves with the specification. https://codereview.chromium.org/148293008/diff/220001/Source/core/rendering/s... File Source/core/rendering/style/GridResolvedPosition.cpp (right): https://codereview.chromium.org/148293008/diff/220001/Source/core/rendering/s... Source/core/rendering/style/GridResolvedPosition.cpp:138: NamedGridAreaMap::const_iterator areaIter = gridContainerStyle.namedGridArea().find(position.namedGridLine()); I think this should be a FIXME: if we inserted grid lines for grid areas as the specification states, we could follow the exact algorithm from the specification, which would be less confusing. https://codereview.chromium.org/148293008/diff/220001/Source/core/rendering/s... Source/core/rendering/style/GridResolvedPosition.cpp:145: return adjustGridPositionForSide(lineIter->value[0], side); Shouldn't we pick the smallest between the grid-area and the found grid-line? https://codereview.chromium.org/148293008/diff/220001/Source/core/rendering/s... Source/core/rendering/style/GridResolvedPosition.cpp:152: return resolveExplicitPositionFromStyle(gridContainerStyle, adjustedPosition, side); Why do we need to override the integer position? This would pick the second line, not the first. We don't really discriminate between <custom-ident> and <integer> && <custom-ident>? so it seems like we can't handle this case correctly as the specification says to treat it differently depending on the previous cases. https://codereview.chromium.org/148293008/diff/220001/Source/core/rendering/s... Source/core/rendering/style/GridResolvedPosition.cpp:154: I think we should check for the named grid line in this case too.
Thanks for the review! Unless I'm missing something, I think most of the comments don't really apply to the current patch (it's easy to get confused as we iterated many times over it). https://codereview.chromium.org/148293008/diff/220001/Source/core/rendering/s... File Source/core/rendering/style/GridResolvedPosition.cpp (right): https://codereview.chromium.org/148293008/diff/220001/Source/core/rendering/s... Source/core/rendering/style/GridResolvedPosition.cpp:138: NamedGridAreaMap::const_iterator areaIter = gridContainerStyle.namedGridArea().find(position.namedGridLine()); On 2014/05/08 18:48:05, Julien Chaffraix - PST wrote: > I think this should be a FIXME: if we inserted grid lines for grid areas as the > specification states, we could follow the exact algorithm from the > specification, which would be less confusing. I quite don't get this comment. We *do* insert grid lines for grid areas in the StyleBuilder. https://codereview.chromium.org/148293008/diff/220001/Source/core/rendering/s... Source/core/rendering/style/GridResolvedPosition.cpp:145: return adjustGridPositionForSide(lineIter->value[0], side); On 2014/05/08 18:48:05, Julien Chaffraix - PST wrote: > Shouldn't we pick the smallest between the grid-area and the found grid-line? Indeed that's a bug https://codereview.chromium.org/148293008/diff/220001/Source/core/rendering/s... Source/core/rendering/style/GridResolvedPosition.cpp:152: return resolveExplicitPositionFromStyle(gridContainerStyle, adjustedPosition, side); On 2014/05/08 18:48:05, Julien Chaffraix - PST wrote: > Why do we need to override the integer position? This would pick the second > line, not the first. We're converting a NamedGridAreaPosition into a explicit position because as there is no named area with that name, we want to treat it as any other explicit grid line name. Regarding picking the second line, I am not sure why you say this, remember that 0 makes a <integer> && <custom-ident>? declaration invalid. > We don't really discriminate between <custom-ident> and <integer> && > <custom-ident>? so it seems like we can't handle this case correctly as the > specification says to treat it differently depending on the previous cases. I think we properly do that, but not in this function. We handle that case in resolveNamedGridLinePositionFromStyle() which is called by resolveExplicitPositionFromStyle(). https://codereview.chromium.org/148293008/diff/220001/Source/core/rendering/s... Source/core/rendering/style/GridResolvedPosition.cpp:154: On 2014/05/08 18:48:05, Julien Chaffraix - PST wrote: > I think we should check for the named grid line in this case too. Again, resolveExplicitPositionFromStyle() calls resolveNamedGridLinePositionFromStyle() for named grid lines. https://codereview.chromium.org/148293008/diff/220001/Source/core/rendering/s... Source/core/rendering/style/GridResolvedPosition.cpp:154: On 2014/05/08 18:48:05, Julien Chaffraix - PST wrote: > I think we should check for the named grid line in this case too. Same comment as above we handle that case in resolveNamedGridLinePositionFromStyle() which is called by resolveExplicitPositionFromStyle(). Remember that named grid lines are also explicit positions.
I take back one of my comments, the "bug" is not a bug, the code is correct see bellow. https://codereview.chromium.org/148293008/diff/220001/Source/core/rendering/s... File Source/core/rendering/style/GridResolvedPosition.cpp (right): https://codereview.chromium.org/148293008/diff/220001/Source/core/rendering/s... Source/core/rendering/style/GridResolvedPosition.cpp:145: return adjustGridPositionForSide(lineIter->value[0], side); On 2014/05/09 11:14:39, svillar wrote: > On 2014/05/08 18:48:05, Julien Chaffraix - PST wrote: > > Shouldn't we pick the smallest between the grid-area and the found grid-line? > > Indeed that's a bug Errr, no the code is correct. The reason why is correct is because the NamedGridLinesMap already contains the implicit grid lines, so by returning [0] we ensure that the result is always the right one (we don't mind if it's an implicit or an explicitly named grid line)
We talked about this with Sergio and the plan forward is to follow the specification closely. Here are the notes from our discussion: 09:50 <jchaffrai> OK, that's why I don't understand why we can't use the specification's algorithm for <custom-ident> 09:50 <jchaffrai> 1. if there is a named line with the name ''<custom-ident>-start (for grid-*-start) / <custom-ident>-end'' (for grid-*-end), contributes the first such line to the grid item’s placement. 09:50 <jchaffrai> 2. Otherwise, if there is a named line with the specified name, contributes the first such line to the grid item’s placement. 09:50 <jchaffrai> 3. Otherwise, the property contributes nothing (just as if auto were specified). [...] 10:23 <jchaffrai> so here is how I would handle this 10:24 <jchaffrai> split the code path between NamedGridAreaPosition and ExplicitPosition 10:24 <jchaffrai> (like don't try to share any code between the 2) 10:24 <jchaffrai> for NamedGridAreaPosition, just apply the 3 steps from the specification 10:25 <jchaffrai> step 3. is not possible so return the first grid lines with a giant FIXME 10:26 < svillar> but that split won't fix the auto fallback 10:26 < svillar> ah ok :) 10:26 <jchaffrai> the rest of your code (StyleAdjuster) is awesome so no need to touch it 10:26 <jchaffrai> for the 'auto' case, I think we will need to change the way we handle resolution in a deeper way We also mentioned that NamedGridAreaPosition is now misnamed and came from a previous version of the specification. https://codereview.chromium.org/148293008/diff/220001/Source/core/css/resolve... File Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/148293008/diff/220001/Source/core/css/resolve... Source/core/css/resolver/StyleBuilderCustom.cpp:1854: NamedGridLinesMap namedGridRowLines = state.style()->namedGridRowLines(); If you used NamedGridLinesMap& for both lines above, you wouldn't need to do a set below. It would also remove the need for a copy if you end up not modifying the grid lines. https://codereview.chromium.org/148293008/diff/220001/Source/core/rendering/s... File Source/core/rendering/style/GridResolvedPosition.cpp (right): https://codereview.chromium.org/148293008/diff/220001/Source/core/rendering/s... Source/core/rendering/style/GridResolvedPosition.cpp:138: NamedGridAreaMap::const_iterator areaIter = gridContainerStyle.namedGridArea().find(position.namedGridLine()); On 2014/05/09 11:14:39, svillar wrote: > On 2014/05/08 18:48:05, Julien Chaffraix - PST wrote: > > I think this should be a FIXME: if we inserted grid lines for grid areas as > the > > specification states, we could follow the exact algorithm from the > > specification, which would be less confusing. > > I quite don't get this comment. We *do* insert grid lines for grid areas in the > StyleBuilder. OK, I missed that important piece of information. https://codereview.chromium.org/148293008/diff/220001/Source/core/rendering/s... Source/core/rendering/style/GridResolvedPosition.cpp:145: return adjustGridPositionForSide(lineIter->value[0], side); On 2014/05/09 14:40:14, svillar wrote: > On 2014/05/09 11:14:39, svillar wrote: > > On 2014/05/08 18:48:05, Julien Chaffraix - PST wrote: > > > Shouldn't we pick the smallest between the grid-area and the found > grid-line? > > > > Indeed that's a bug > > Errr, no the code is correct. The reason why is correct is because the > NamedGridLinesMap already contains the implicit grid lines, so by returning [0] > we ensure that the result is always the right one (we don't mind if it's an > implicit or an explicitly named grid line) I agree. Sorry for the noise.
https://codereview.chromium.org/148293008/diff/220001/Source/core/css/resolve... File Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/148293008/diff/220001/Source/core/css/resolve... Source/core/css/resolver/StyleBuilderCustom.cpp:1854: NamedGridLinesMap namedGridRowLines = state.style()->namedGridRowLines(); On 2014/05/09 17:37:54, Julien Chaffraix - CET wrote: > If you used NamedGridLinesMap& for both lines above, you wouldn't need to do a > set below. It would also remove the need for a copy if you end up not modifying > the grid lines. Hmm but the style returns a constant reference so I wouldn't be allowed to modify it, would I?
I finally decided to limit the amount of changes against master so I brought the switch(position) back and replaced the NamedGridLine part with the new code
lgtm https://codereview.chromium.org/148293008/diff/230001/LayoutTests/fast/css-gr... File LayoutTests/fast/css-grid-layout/named-grid-lines-with-named-grid-areas-dynamic-get-set.html (right): https://codereview.chromium.org/148293008/diff/230001/LayoutTests/fast/css-gr... LayoutTests/fast/css-grid-layout/named-grid-lines-with-named-grid-areas-dynamic-get-set.html:148: <div>This test checks that.</div> I think you're missing some words here. https://codereview.chromium.org/148293008/diff/230001/Source/core/css/resolve... File Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/148293008/diff/230001/Source/core/css/resolve... Source/core/css/resolver/StyleBuilderCustom.cpp:1854: NamedGridLinesMap namedGridRowLines = state.style()->namedGridRowLines(); You're right about RenderStyle returning a constant reference, we could expose a mutableNamedGridColumnLines() or just do the copy. I have a slight preference for the latter as it's more efficient and compact but I won't push either way. https://codereview.chromium.org/148293008/diff/230001/Source/core/rendering/s... File Source/core/rendering/style/GridResolvedPosition.cpp (right): https://codereview.chromium.org/148293008/diff/230001/Source/core/rendering/s... Source/core/rendering/style/GridResolvedPosition.cpp:142: NamedGridLinesMap::const_iterator explicitLineIter = gridLineNames.find(namedGridLine); Nit: I would put the specification's wording with the code.
The CQ bit was checked by svillar@igalia.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/svillar@igalia.com/148293008/250001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/core/css/resolver/StyleBuilderCustom.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/css/resolver/StyleBuilderCustom.cpp
Hunk #1 FAILED at 1049.
Hunk #2 FAILED at 1738.
Hunk #3 FAILED at 1762.
Hunk #4 FAILED at 1822.
4 out of 4 hunks FAILED -- saving rejects to file
Source/core/css/resolver/StyleBuilderCustom.cpp.rej
Patch: Source/core/css/resolver/StyleBuilderCustom.cpp
Index: Source/core/css/resolver/StyleBuilderCustom.cpp
diff --git a/Source/core/css/resolver/StyleBuilderCustom.cpp
b/Source/core/css/resolver/StyleBuilderCustom.cpp
index
d859c5ef4b42b8ecc27482eac5f08725642f02ec..a70e7be3d856b20d92b6d55aee21780e5d93809c
100644
--- a/Source/core/css/resolver/StyleBuilderCustom.cpp
+++ b/Source/core/css/resolver/StyleBuilderCustom.cpp
@@ -1049,6 +1049,22 @@ if (isInitial) { \
return; \
}
+static void createImplicitNamedGridLinesFromGridArea(const NamedGridAreaMap&
namedGridAreas, NamedGridLinesMap& namedGridLines, GridTrackSizingDirection
direction)
+{
+ NamedGridAreaMap::const_iterator end = namedGridAreas.end();
+ for (NamedGridAreaMap::const_iterator it = namedGridAreas.begin(); it !=
end; ++it) {
+ GridSpan areaSpan = direction == ForRows ? it->value.rows :
it->value.columns;
+
+ NamedGridLinesMap::AddResult startResult = namedGridLines.add(it->key +
"-start", Vector<size_t>());
+
startResult.storedValue->value.append(areaSpan.resolvedInitialPosition.toInt());
+ std::sort(startResult.storedValue->value.begin(),
startResult.storedValue->value.end());
+
+ NamedGridLinesMap::AddResult endResult = namedGridLines.add(it->key +
"-end", Vector<size_t>());
+
endResult.storedValue->value.append(areaSpan.resolvedFinalPosition.toInt() + 1);
+ std::sort(endResult.storedValue->value.begin(),
endResult.storedValue->value.end());
+ }
+}
+
static GridLength createGridTrackBreadth(CSSPrimitiveValue* primitiveValue,
const StyleResolverState& state)
{
if (primitiveValue->getValueID() == CSSValueMinContent)
@@ -1738,6 +1754,11 @@ void StyleBuilder::oldApplyProperty(CSSPropertyID id,
StyleResolverState& state,
OrderedNamedGridLines orderedNamedGridLines;
if (!createGridTrackList(value, trackSizes, namedGridLines,
orderedNamedGridLines, state))
return;
+
+ const NamedGridAreaMap& namedGridAreas =
state.style()->namedGridArea();
+ if (!namedGridAreas.isEmpty())
+ createImplicitNamedGridLinesFromGridArea(namedGridAreas,
namedGridLines, ForColumns);
+
state.style()->setGridTemplateColumns(trackSizes);
state.style()->setNamedGridColumnLines(namedGridLines);
state.style()->setOrderedNamedGridColumnLines(orderedNamedGridLines);
@@ -1762,6 +1783,11 @@ void StyleBuilder::oldApplyProperty(CSSPropertyID id,
StyleResolverState& state,
OrderedNamedGridLines orderedNamedGridLines;
if (!createGridTrackList(value, trackSizes, namedGridLines,
orderedNamedGridLines, state))
return;
+
+ const NamedGridAreaMap& namedGridAreas =
state.style()->namedGridArea();
+ if (!namedGridAreas.isEmpty())
+ createImplicitNamedGridLinesFromGridArea(namedGridAreas,
namedGridLines, ForRows);
+
state.style()->setGridTemplateRows(trackSizes);
state.style()->setNamedGridRowLines(namedGridLines);
state.style()->setOrderedNamedGridRowLines(orderedNamedGridLines);
@@ -1822,7 +1848,16 @@ void StyleBuilder::oldApplyProperty(CSSPropertyID id,
StyleResolverState& state,
}
CSSGridTemplateAreasValue* gridTemplateAreasValue =
toCSSGridTemplateAreasValue(value);
- state.style()->setNamedGridArea(gridTemplateAreasValue->gridAreaMap());
+ const NamedGridAreaMap& newNamedGridAreas =
gridTemplateAreasValue->gridAreaMap();
+
+ NamedGridLinesMap namedGridColumnLines =
state.style()->namedGridColumnLines();
+ NamedGridLinesMap namedGridRowLines =
state.style()->namedGridRowLines();
+ createImplicitNamedGridLinesFromGridArea(newNamedGridAreas,
namedGridColumnLines, ForColumns);
+ createImplicitNamedGridLinesFromGridArea(newNamedGridAreas,
namedGridRowLines, ForRows);
+ state.style()->setNamedGridColumnLines(namedGridColumnLines);
+ state.style()->setNamedGridRowLines(namedGridRowLines);
+
+ state.style()->setNamedGridArea(newNamedGridAreas);
state.style()->setNamedGridAreaRowCount(gridTemplateAreasValue->rowCount());
state.style()->setNamedGridAreaColumnCount(gridTemplateAreasValue->columnCount());
return;
On 2014/05/12 13:43:26, I haz the power (commit-bot) wrote: > Failed to apply patch for Source/core/css/resolver/StyleBuilderCustom.cpp: > While running patch -p1 --forward --force --no-backup-if-mismatch; > patching file Source/core/css/resolver/StyleBuilderCustom.cpp Hmm the rebase is not trivial, I'll do it tomorrow.
The CQ bit was checked by svillar@igalia.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/svillar@igalia.com/148293008/270001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/7338) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6588) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/7065)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/7350)
The CQ bit was checked by svillar@igalia.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/svillar@igalia.com/148293008/290001
Message was sent while issue was closed.
Change committed as 174021 |
