[masonry] Implement Grid Axis gaps (Main Gaps) for GridLanes [chromium/src : main]
Javier Contreras (Gerrit)
Javier Contreras voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Celeste Pan
- Yanling Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Celeste Pan (Gerrit)
Celeste Pan added 13 comments![]( "Open in Gerrit")
Parent: 8977a266 ([Reference Target] Ship to Stable)Is this a functional change? If it is, can you remove tests from TestExpectations that this change covers?
//For my understanding, is this saying that even in the row direction for grid-lanes, we'll be using rows as the main gaps?
// content edges.nit: this sentence was pretty confusing to me when I read it through, and it took me a couple reads; something like "If the input `track_collection` only has collect lines, this method will return safely with no gaps and zero content edges".
// so each visited line is a gap line.nit: formatting issue
if (!track_collection.HasNonCollapsedLine()) {
return gap_data;
}Do you want to be setting `gap_data.track_count` to `line_positions.size() - 1` if all the lines are collapsed? I'm not sure if that means you should put this above the setting of `gap_data.track_count`.
// outer edge is excluded naturally by the `line_index < end_line`For my understanding, if a range is from line 0-1, then we don't keep track of gaps for line 0 or line 1.
Then if the next range if from line 1-2, then we do keep track of line 1, but not line 2?
LayoutUnit(line_positions[line_index] - gutter_size / 2.0f)});Per the chromium style guide, can you either add an inline parameter comment or assign the 2.0f to a variable?
void BuildMainGaps(const GridLayoutTrackCollection& grid_axis_tracks);It might be nice to have a comment just describing that the main gaps are the gaps parallel to the stacking axis, correlated with the tracks in the grid-axis, and perhaps a link from the spec. Just because it might not be immediately clear to someone.
// Builds gap-decoration geometry and related state for `GridLanes`"grid-lanes", since it might be misleading and someone may think there is a class called "GridLanes".
const GapGeometry* gap_geometry_ = nullptr;Should this maybe be optional? Like the other variables that we aren't necessarily using.
!InvolvedInBlockFragmentation(container_builder_)) {Are we not doing gap decoration if fragmentation is involved, or is it something we plan to implement later? If we plan to implement it later, can you add a tracking TODO comment?
// Keep this geometry off the fragment until Grid Lanes paint and spanner
// support are ready to consume it.Also, it doesn't seem like this is describing any of the code above -- is this supposed to be a TODO?
// if we are not in a multicol, grid, Grid Lanes, or flex context.nit: 'grid lanes" to match the format of the other layouts you have int he comment.
Related details
- Javier Contreras
- Yanling Wang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Contreras (Gerrit)
Javier Contreras added 4 comments![]( "Open in Gerrit")
Parent: 8977a266 ([Reference Target] Ship to Stable)Is this a functional change? If it is, can you remove tests from TestExpectations that this change covers?
This CL only deals with the layout side for MainGaps, there is no paint behavior wired in so there is no effect on any tests in TestExpectations
For my understanding, is this saying that even in the row direction for grid-lanes, we'll be using rows as the main gaps?
That is only for grid, this is a grid-only function, and for grid rows are always main gaps.
// so each visited line is a gap line.Javier Contrerasnit: formatting issue
Done
// Keep this geometry off the fragment until Grid Lanes paint and spanner
// support are ready to consume it.Also, it doesn't seem like this is describing any of the code above -- is this supposed to be a TODO?
Its only there because there is where we would actually commit the geometry to the fragment for when we want to wire in the paint side. Will add as TODO.
Related details
- Celeste Pan
- Yanling Wang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Contreras (Gerrit)
Javier Contreras added 9 comments![]( "Open in Gerrit")
// content edges.Javier Contrerasnit: this sentence was pretty confusing to me when I read it through, and it took me a couple reads; something like "If the input `track_collection` only has collect lines, this method will return safely with no gaps and zero content edges".
Done
if (!track_collection.HasNonCollapsedLine()) {
return gap_data;
}Do you want to be setting `gap_data.track_count` to `line_positions.size() - 1` if all the lines are collapsed? I'm not sure if that means you should put this above the setting of `gap_data.track_count`.
Yes, we do this intentionally. This preserves existing Grid behavior (from where we moved this logic). The count represents the full index space used by Grid’s segment-state aggregators which include collapsed tracks
// outer edge is excluded naturally by the `line_index < end_line`For my understanding, if a range is from line 0-1, then we don't keep track of gaps for line 0 or line 1.
Then if the next range if from line 1-2, then we do keep track of line 1, but not line 2?
yes correct
LayoutUnit(line_positions[line_index] - gutter_size / 2.0f)});Per the chromium style guide, can you either add an inline parameter comment or assign the 2.0f to a variable?
Done
void BuildMainGaps(const GridLayoutTrackCollection& grid_axis_tracks);It might be nice to have a comment just describing that the main gaps are the gaps parallel to the stacking axis, correlated with the tracks in the grid-axis, and perhaps a link from the spec. Just because it might not be immediately clear to someone.
Done
// Builds gap-decoration geometry and related state for `GridLanes`"grid-lanes", since it might be misleading and someone may think there is a class called "GridLanes".
Done
const GapGeometry* gap_geometry_ = nullptr;Should this maybe be optional? Like the other variables that we aren't necessarily using.
In this case I would say probably not. A pointer already has the "nullable"/"exists" behavior that a std::optional has and the optional will end up using a little bit more memory.
!InvolvedInBlockFragmentation(container_builder_)) {Are we not doing gap decoration if fragmentation is involved, or is it something we plan to implement later? If we plan to implement it later, can you add a tracking TODO comment?
we do support gap decor in fragmentation, just will be implemented later.
// if we are not in a multicol, grid, Grid Lanes, or flex context.Javier Contrerasnit: 'grid lanes" to match the format of the other layouts you have int he comment.
Done
Related details
- Celeste Pan
- Yanling Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yanling Wang (Gerrit)
Yanling Wang added 2 comments![]( "Open in Gerrit")
// Grid Lanes does not construct cross gaps yet, so the content edges arenit: maybe start this comment with a todo? same as below.
grid-lanes-direction: row;nit: this can be removed. Since only `grid-template-rows` is set, the default `normal` keyword already resolves the track direction to rows.
Related details
- Celeste Pan
- Javier Contreras
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Contreras (Gerrit)
Javier Contreras added 2 comments![]( "Open in Gerrit")
// Grid Lanes does not construct cross gaps yet, so the content edges arenit: maybe start this comment with a todo? same as below.
Done
grid-lanes-direction: row;nit: this can be removed. Since only `grid-template-rows` is set, the default `normal` keyword already resolves the track direction to rows.
- Celeste Pan
- Yanling Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Celeste Pan (Gerrit)
Celeste Pan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Celeste Pan
| Code-Review | +1 |
1 comment
row-gap: 12px;Doesn't have to be this change, but could you in the future add a test where you don't specify row-gap, just "gap", and ensure we're still picking up the right gap size?
Related details
- Javier Contreras
- Yanling Wang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yanling Wang (Gerrit)
Yanling Wang voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Javier Contreras
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Contreras (Gerrit)
Javier Contreras added 1 comment![]( "Open in Gerrit")
row-gap: 12px;Doesn't have to be this change, but could you in the future add a test where you don't specify row-gap, just "gap", and ensure we're still picking up the right gap size?
- Alison Maher
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 12 comments![]( "Open in Gerrit")
for (const auto& gap : row_gap_data_.gaps) {
gap_geometry_->AddMainGap(gap.center_offset);
}Can we pass the gap geo into the ComputeGridTrackGapData helper so we don't need to loop back over to add the gaps and can add as we go? Same question elsewhere
// A collection with no non-collapsed lines returns no gaps and zero-valuednit: is it worth noting what happens with collapsed cases, or is that handled elsewhere?
#include "third_party/blink/renderer/platform/wtf/vector.h"I'm actually surprised this is needed. Calling it out to double check, but totally ok to leave if nothing else is including this
// Track ranges are half-open, but each range visits its leading line as the
// preceding gap. Thus [0, 1) skips outer line 0 and emits nothing, while
// [1, 2) emits interior line 1; `< end_line` excludes outer line 2.
// Collapsed ranges were skipped above.nit: The original comment for this was slightly easier for me to follow:
"// The first non-collapsed range's leading line is the outer edge of
// the grid content, so it's not a gap and we skip it. The trailing
// outer edge is excluded naturally by the `line_index < end_line`
// bound on the inner loop below."
class CORE_EXPORT GridLanesGapAccumulator {As a separate change, it might make sense to move grid's gap accumulator to its own file at this point to match.
// third_party/blink/renderer/core/layout/gap/README.md` for Blink'sNon-blocking for this change, but do you plan to update the README with how things will work in grid lanes (for main should be as simple as look at grid, but I guess cross gaps will end up being fairly unique)
void GridLanesGapAccumulator::BuildMainGaps(Any code we can share with https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/grid/grid_layout_algorithm.cc;l=1519;drc=b251030f22d6810f6755262d2182316f60552d60
Not sure if it would be helpful to have this based on the grid accumulator to get some of it for free, and then override the cross gaps portion
if (gap_geometry_->GetMainDirection() == kForColumns) {Similar question here about reusing with https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/grid/grid_layout_algorithm.cc;l=1672;drc=b251030f22d6810f6755262d2182316f60552d60 if possible
#include "third_party/blink/renderer/platform/runtime_enabled_features.h"nit: This is another one that may be worth double checking isn't already included by something else, but completely ok to keep if not
const auto& border_scrollbar_padding = BorderScrollbarPadding();This can be moved up earlier to be used in other spots of the code
is_for_columns ? block_size - border_scrollbar_padding.block_endWill this always be the right value? What if items overflow the container?
gap_geometry_ = gap_accumulator->Finalize(stacking_content_start,Would it make sense to call this FinalizeGapGeometry similar to what this is called in grid
Related details
- Javier Contreras
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Contreras (Gerrit)
Javier Contreras added 12 comments![]( "Open in Gerrit")
for (const auto& gap : row_gap_data_.gaps) {
gap_geometry_->AddMainGap(gap.center_offset);
}Can we pass the gap geo into the ComputeGridTrackGapData helper so we don't need to loop back over to add the gaps and can add as we go? Same question elsewhere
We intentionally make it geo-agnostic because Grid lanes and Grid consume the track data slightly differently right now, so we wouldnt be able to just straight up move this there and have grid and grid lanes add their gaps the same way. We could maybe then pass a parameter for the container type and branch on that, or a callback per container type. These seem like overkill though for an extra linear pass. What do you think?
// A collection with no non-collapsed lines returns no gaps and zero-valuednit: is it worth noting what happens with collapsed cases, or is that handled elsewhere?
With collapsed cases we emit no gaps for those. Or do you mean something else?
#include "third_party/blink/renderer/platform/wtf/vector.h"I'm actually surprised this is needed. Calling it out to double check, but totally ok to leave if nothing else is including this
good catch!
// Track ranges are half-open, but each range visits its leading line as the
// preceding gap. Thus [0, 1) skips outer line 0 and emits nothing, while
// [1, 2) emits interior line 1; `< end_line` excludes outer line 2.
// Collapsed ranges were skipped above.nit: The original comment for this was slightly easier for me to follow:
"// The first non-collapsed range's leading line is the outer edge of
// the grid content, so it's not a gap and we skip it. The trailing
// outer edge is excluded naturally by the `line_index < end_line`
// bound on the inner loop below."
Done
As a separate change, it might make sense to move grid's gap accumulator to its own file at this point to match.
Will do in a separate change.
// third_party/blink/renderer/core/layout/gap/README.md` for Blink'sNon-blocking for this change, but do you plan to update the README with how things will work in grid lanes (for main should be as simple as look at grid, but I guess cross gaps will end up being fairly unique)
yes will update there in a separate CL.
Any code we can share with https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/grid/grid_layout_algorithm.cc;l=1519;drc=b251030f22d6810f6755262d2182316f60552d60
Not sure if it would be helpful to have this based on the grid accumulator to get some of it for free, and then override the cross gaps portion
There is some but not a lot: Grid fixes rows as main direction, aggregates spanning-item states (which will be done ver differently in grid lanes), and finalizes both axes.
In Grid-lanes selects its main direction dynamically,similar to flex, and also we currently don't have cross gaps, but those will also be very different from Grid. We'd need to refactor the grid one heavily so we can use grid lanes with it as well, and there wouldnt be that much for us to reuse.
if (gap_geometry_->GetMainDirection() == kForColumns) {Similar question here about reusing with https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/grid/grid_layout_algorithm.cc;l=1672;drc=b251030f22d6810f6755262d2182316f60552d60 if possible
Similarly, there is a lot of grid-only logic there (specifically about ranges) so ther's not that much we can reuse to be honest. Definitely not enough to warrant subclassing, honestly outside of conceptually the grid-axis gaps, not much will be shared between grid and grid lanes, since cross gaps are so different and the way we deal with spanners will be very different as well.
As far as I could tell, the most we could easily reuse was what i refactored already in this CL.
#include "third_party/blink/renderer/platform/runtime_enabled_features.h"nit: This is another one that may be worth double checking isn't already included by something else, but completely ok to keep if not
Done
const auto& border_scrollbar_padding = BorderScrollbarPadding();This can be moved up earlier to be used in other spots of the code
Done
is_for_columns ? block_size - border_scrollbar_padding.block_endWill this always be the right value? What if items overflow the container?
Done
gap_geometry_ = gap_accumulator->Finalize(stacking_content_start,Would it make sense to call this FinalizeGapGeometry similar to what this is called in grid
- Alison Maher
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 2 comments![]( "Open in Gerrit")
for (const auto& gap : row_gap_data_.gaps) {
gap_geometry_->AddMainGap(gap.center_offset);
}Javier ContrerasCan we pass the gap geo into the ComputeGridTrackGapData helper so we don't need to loop back over to add the gaps and can add as we go? Same question elsewhere
We intentionally make it geo-agnostic because Grid lanes and Grid consume the track data slightly differently right now, so we wouldnt be able to just straight up move this there and have grid and grid lanes add their gaps the same way. We could maybe then pass a parameter for the container type and branch on that, or a callback per container type. These seem like overkill though for an extra linear pass. What do you think?
Chatted offline for a way to make this potentially doable
// A collection with no non-collapsed lines returns no gaps and zero-valuedJavier Contrerasnit: is it worth noting what happens with collapsed cases, or is that handled elsewhere?
With collapsed cases we emit no gaps for those. Or do you mean something else?
Oh that was a double negative, reading it again I guess what is there is fine. Thought it was commenting about the non-collapsed scenario, and figured we may want to mention the collapsed scenario too.
Might be worth updating to say "A collection with only collapsed tracks returns..."
Related details
- Javier Contreras
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Contreras (Gerrit)
Javier Contreras added 2 comments![]( "Open in Gerrit")
for (const auto& gap : row_gap_data_.gaps) {
gap_geometry_->AddMainGap(gap.center_offset);
}Javier ContrerasCan we pass the gap geo into the ComputeGridTrackGapData helper so we don't need to loop back over to add the gaps and can add as we go? Same question elsewhere
Alison MaherWe intentionally make it geo-agnostic because Grid lanes and Grid consume the track data slightly differently right now, so we wouldnt be able to just straight up move this there and have grid and grid lanes add their gaps the same way. We could maybe then pass a parameter for the container type and branch on that, or a callback per container type. These seem like overkill though for an extra linear pass. What do you think?
Chatted offline for a way to make this potentially doable
Done
// A collection with no non-collapsed lines returns no gaps and zero-valuedJavier Contrerasnit: is it worth noting what happens with collapsed cases, or is that handled elsewhere?
Alison MaherWith collapsed cases we emit no gaps for those. Or do you mean something else?
Oh that was a double negative, reading it again I guess what is there is fine. Thought it was commenting about the non-collapsed scenario, and figured we may want to mention the collapsed scenario too.
Might be worth updating to say "A collection with only collapsed tracks returns..."
- Alison Maher
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher voted Code-Review+1![]( "Open in Gerrit")
- Javier Contreras
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Contreras (Gerrit)
Javier Contreras voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[masonry] Implement Grid Axis gaps (Main Gaps) for GridLanes
This CL implements basic grid axis gaps for grid lanes, and lays out the
groundwork for grid lanes gap decorations. Follow up CLs will deal with
spanners, painting, cross gaps etc. This CL only deals with the Layout
side of the implementation.
In order to achieve this we repurpose a utility that Grid used for the
same purpose and we move it to a utils file. It builds the `MainGaps` in
O(# of tracks) time.
- M third_party/blink/renderer/core/css/css_gap_decoration_property_utils.cc
- M third_party/blink/renderer/core/layout/build.gni
- M third_party/blink/renderer/core/layout/gap/gap_geometry.cc
- M third_party/blink/renderer/core/layout/gap/gap_geometry.h
- M third_party/blink/renderer/core/layout/grid/grid_layout_algorithm.cc
- M third_party/blink/renderer/core/layout/grid/grid_layout_utils.cc
- M third_party/blink/renderer/core/layout/grid/grid_layout_utils.h
- A third_party/blink/renderer/core/layout/grid_lanes/grid_lanes_gap_accumulator.cc
- A third_party/blink/renderer/core/layout/grid_lanes/grid_lanes_gap_accumulator.h
- M third_party/blink/renderer/core/layout/grid_lanes/grid_lanes_layout_algorithm.cc
- M third_party/blink/renderer/core/layout/grid_lanes/grid_lanes_layout_algorithm.h
- M third_party/blink/renderer/core/layout/grid_lanes/grid_lanes_layout_algorithm_test.cc
- M third_party/blink/renderer/core/style/computed_style.h
- M third_party/blink/renderer/core/style/computed_style_test.cc
Code-Review: +1 by Alison Maher, +1 by Yanling Wang, +1 by Celeste Pan
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |