[Gap Decorations]: Build Gap Intersection Points [chromium/src : main]
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kurt Catti-Schmidt
| Code-Review | +1 |
1 comment
const auto container_end = cross_tracks[cross_tracks.size() - 1];Is it possible for `cross_tracks` to be size 0? If so, these array accesses could be out-of-range.
Might be worth adding `DCHECK_GT(cross_tracks(), 0);` before these calls just to be safe. If it's possible to hit, you'll need an `if` statement that returns early.
Related details
- Alison Maher
- Kevin Babbitt
- Sam Davis Omekara
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 5 comments![]( "Open in Gerrit")
[Gap Decorations]: Build Gap Intersection PointsHigh level question: will gap intersections be used in stead of gap geometry, or do we need both?
I somewhat figured the intersection points would inform the gap geometry we end up storing as opposed to the other way around
// TODO(samomekarajr): Potential optimization. Consider if the list of
// intersection points needs to be stored for each `GapBoundary`. We can use
// two lists on the `GapGeometry` to store the intersection points for the
// entire grid. This will reduce the memory overhead. Intersection points
// are used to paint gap decorations. An intersection point occurs:
// 1. At the center of an intersection between a gap and the container edge.
// 2. At the center of an intersection between gaps in different directions.nit: I might restructure this to move the TODO downward like so:
```
// Intersection points are used to paint gap decorations. An
// intersection point occurs:
// 1. At the center of an intersection between a gap and the container edge.
// 2. At the center of an intersection between gaps in different directions.
//
// TODO(samomekarajr): Potential optimization. Consider if the list of
// intersection points needs to be stored for each `GapBoundary`. We can use
// two lists on the `GapGeometry` to store the intersection points for the
// entire grid. This will reduce the memory overhead.
```
Also might be worth a link to spec for future reference
// TODO(samomekarajr): Potential optimization to consider here. CurrentlyCan be done in a follow up, but this sounds like a nice optimization
for (auto& gap : gap_geometry->GetGapBoundaries(track_direction)) {Is it possible to compute this info when we build up the gap geometry in `BuildGapGeometry()` instead of looping through them later?
TEST_F(GridLayoutAlgorithmTest, GridLayoutAlgorithmGapIntersections) {It may be interesting to have items that span, as well, to test that scenario out
Related details
Sam Davis Omekara (Gerrit)
Sam Davis Omekara added 5 comments![]( "Open in Gerrit")
[Gap Decorations]: Build Gap Intersection PointsHigh level question: will gap intersections be used in stead of gap geometry, or do we need both?
I somewhat figured the intersection points would inform the gap geometry we end up storing as opposed to the other way around
Hmm, I didn't quite see it that way. Here's the high-level design as I envision it:
1. GapGeometry: A container that holds gaps.
2. GapBoundary: Defines the start and end boundaries for each gap within the container.
3. GapIntersections: Points along each gap boundary where the gap intersects with either the container edges or other gaps.
// TODO(samomekarajr): Potential optimization. Consider if the list of
// intersection points needs to be stored for each `GapBoundary`. We can use
// two lists on the `GapGeometry` to store the intersection points for the
// entire grid. This will reduce the memory overhead. Intersection points
// are used to paint gap decorations. An intersection point occurs:
// 1. At the center of an intersection between a gap and the container edge.
// 2. At the center of an intersection between gaps in different directions.nit: I might restructure this to move the TODO downward like so:
```
// Intersection points are used to paint gap decorations. An
// intersection point occurs:
// 1. At the center of an intersection between a gap and the container edge.
// 2. At the center of an intersection between gaps in different directions.
//
// TODO(samomekarajr): Potential optimization. Consider if the list of
// intersection points needs to be stored for each `GapBoundary`. We can use
// two lists on the `GapGeometry` to store the intersection points for the
// entire grid. This will reduce the memory overhead.
```Also might be worth a link to spec for future reference
Done
const auto container_end = cross_tracks[cross_tracks.size() - 1];Is it possible for `cross_tracks` to be size 0? If so, these array accesses could be out-of-range.
Might be worth adding `DCHECK_GT(cross_tracks(), 0);` before these calls just to be safe. If it's possible to hit, you'll need an `if` statement that returns early.
I think it's safe to say cross_tracks is always greater than 0.
for (auto& gap : gap_geometry->GetGapBoundaries(track_direction)) {Is it possible to compute this info when we build up the gap geometry in `BuildGapGeometry()` instead of looping through them later?
Correct me if I'm wrong but this involves computing intersections on the fly without necessarily looking at the gaps on the cross axis.
If this is right, I was actually thinking about this along with my suggestion of having the intersections reside on the container `GapGeometry` rather than each `GapBoundary`. The idea of intersections residing in `GapGeometry` might work for grid and not flexbox since things don't align as well-defined as grids.
I have updated the code to compute on the fly but still populate to each gap boundary. A heads up is that for flex box I anticipate that we would have to construct gaps first then compute the intersections because we wouldn't know where they are going to lie.
TEST_F(GridLayoutAlgorithmTest, GridLayoutAlgorithmGapIntersections) {It may be interesting to have items that span, as well, to test that scenario out
This wouldn't change anything about the intersection points (at least for grid). The intersection points for spanning and non spanning items are the same. The difference would be which intersection points we choose to draw from depending on the `*-rule-break` property. The next change will begin handling this.
See intersections in this example: https://drafts.csswg.org/css-gaps-1/#example-8907e768
Related details
- Alison Maher
- Kevin Babbitt
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Babbitt (Gerrit)
Kevin Babbitt added 1 comment![]( "Open in Gerrit")
TEST_F(GridLayoutAlgorithmTest, GridLayoutAlgorithmGapIntersections) {Sam Davis OmekaraIt may be interesting to have items that span, as well, to test that scenario out
This wouldn't change anything about the intersection points (at least for grid). The intersection points for spanning and non spanning items are the same. The difference would be which intersection points we choose to draw from depending on the `*-rule-break` property. The next change will begin handling this.
See intersections in this example: https://drafts.csswg.org/css-gaps-1/#example-8907e768
This wouldn't change anything about the intersection points (at least for grid).
Shouldn't it though? Reference: https://drafts.csswg.org/css-gaps-1/#example-217ec7b6
Note that there is no intersection point between items 2 and 6 in the spot where both items span the same gap. Same for items 4 and 7. I would also expect a 2x2 spanner to suppress the gap intersection point in its "middle".
Related details
- Alison Maher
- Sam Davis Omekara
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Davis Omekara (Gerrit)
Sam Davis Omekara added 1 comment![]( "Open in Gerrit")
TEST_F(GridLayoutAlgorithmTest, GridLayoutAlgorithmGapIntersections) {Sam Davis OmekaraIt may be interesting to have items that span, as well, to test that scenario out
Kevin BabbittThis wouldn't change anything about the intersection points (at least for grid). The intersection points for spanning and non spanning items are the same. The difference would be which intersection points we choose to draw from depending on the `*-rule-break` property. The next change will begin handling this.
See intersections in this example: https://drafts.csswg.org/css-gaps-1/#example-8907e768
This wouldn't change anything about the intersection points (at least for grid).
Shouldn't it though? Reference: https://drafts.csswg.org/css-gaps-1/#example-217ec7b6
Note that there is no intersection point between items 2 and 6 in the spot where both items span the same gap. Same for items 4 and 7. I would also expect a 2x2 spanner to suppress the gap intersection point in its "middle".
Yes, I believe we are on the same page. The data structure being built occurs during the layout phase, and my implementation is strictly based on the definition of an intersection point as described here:
```
A gap intersection point is defined to exist in each of the following locations:
- The center of an intersection between a gap and the content edge of the container.
- The center of an intersection between gaps in different directions.
- ```
Note that there is no mention of spanning items.
The suppression of intersection points will be governed by the `*-rule-break` property, which I envision as occurring during the paint phase. In my view, suppression depends on whether we choose to draw from that intersection point or not, similar to [1]:
```
...
If break is spanning-item, and a line segment from the start to the first item in endpoints, with the same width as the gap, does not intersect a child item in the container.
...
```
So while these are intersection points during layout, they are not considered valid intersection points when constructing pairs (i.e., during the paint phase).
When the `*-rule-break` is set to `none`, every intersection point constructed during layout is considered a valid intersection point.
I think we are aligned in our understanding: we suppress or filter out intersection points during the paint phase. Please let me know if this is clear, and I am open to any suggestions you may have.
[1]: https://drafts.csswg.org/css-gaps-1/#determine-pairs-of-gap-decoration-endpoints
Related details
- Alison Maher
- Kevin Babbitt
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Davis Omekara (Gerrit)
Also, forgot to mention my thinking is roughly shaped by how multi-col handles column spanners today [1]
Alison Maher (Gerrit)
Alison Maher added 6 comments![]( "Open in Gerrit")
[Gap Decorations]: Build Gap Intersection PointsSam Davis OmekaraHigh level question: will gap intersections be used in stead of gap geometry, or do we need both?
I somewhat figured the intersection points would inform the gap geometry we end up storing as opposed to the other way around
Hmm, I didn't quite see it that way. Here's the high-level design as I envision it:
1. GapGeometry: A container that holds gaps.
2. GapBoundary: Defines the start and end boundaries for each gap within the container.
3. GapIntersections: Points along each gap boundary where the gap intersects with either the container edges or other gaps.
Maybe this will be clearer from later changes, but I think what isn't clear to me is why we need to store both the boundaries and the intersection points. I kind of would have thought the intersection points may have been enough to paint the gap decorations without needing both pieces of information.
// Fragment 1: Fragment 2:
// +-----------------+ +-------------------+
// | Item 1| |Item 2 | | |
// | | | | | Gap End |
// |_______| |_______| |________ ________|
// | Gap Start | | Item 3 | | Item 4 |
// | | | | | |
// +-----------------+ +-------------------+nit: with the WG resolution, this will no longer be possible, but can be adjusted in a separate change.
// Constructs gap geometry for Gap Decorations. Each gap boundary isLikely worth adding to the comment about the intersections piece happening here.
HeapVector<LayoutUnit>& intersections,Should this be called `intersection_points` for consistency
for (auto& gap : gap_geometry->GetGapBoundaries(track_direction)) {Sam Davis OmekaraIs it possible to compute this info when we build up the gap geometry in `BuildGapGeometry()` instead of looping through them later?
Correct me if I'm wrong but this involves computing intersections on the fly without necessarily looking at the gaps on the cross axis.
If this is right, I was actually thinking about this along with my suggestion of having the intersections reside on the container `GapGeometry` rather than each `GapBoundary`. The idea of intersections residing in `GapGeometry` might work for grid and not flexbox since things don't align as well-defined as grids.
I have updated the code to compute on the fly but still populate to each gap boundary. A heads up is that for flex box I anticipate that we would have to construct gaps first then compute the intersections because we wouldn't know where they are going to lie.
Acknowledged
Yeah I think we don't necessarily need to handle supression here, but still may be worth adding some tests around what initial intersection points are created when there are spanners mixed in, as well, since those don't line up with the non-spanners and is an interesting edge case to make sure we have the expected intersections.
Or, for example, as case like https://drafts.csswg.org/css-gaps-1/#example-883084bc would be interesting
Related details
- Kevin Babbitt
- Sam Davis Omekara
Kevin Babbitt (Gerrit)
Kevin Babbitt added 1 comment![]( "Open in Gerrit")
I pushed an [update](https://github.com/w3c/csswg-drafts/commit/f8b28b2155b69d07cf1fef3c6a169cdf83cdd59c) to the spec text to reflect my original intent, which is what was illustrated in the examples. But I don't think I fully understood Sam Davis's comment about suppressing intersection points during paint until after I made that push.
I could revert that update and instead change the algorithm for pairing gap intersection points, and change the examples to illustrate gap intersection points at every intersection. Personally, though, I find the concept of a "gap intersection point" more intuitive as it is with the update, i.e. when it only includes spots where there's a visible intersection between gaps. I'm open to feedback on that if others feel differently, or if it would significantly complicate implementation for some reason.
Related details
- Sam Davis Omekara
Sam Davis Omekara (Gerrit)
Sam Davis Omekara added 5 comments![]( "Open in Gerrit")
[Gap Decorations]: Build Gap Intersection PointsSam Davis OmekaraHigh level question: will gap intersections be used in stead of gap geometry, or do we need both?
I somewhat figured the intersection points would inform the gap geometry we end up storing as opposed to the other way around
Alison MaherHmm, I didn't quite see it that way. Here's the high-level design as I envision it:
1. GapGeometry: A container that holds gaps.
2. GapBoundary: Defines the start and end boundaries for each gap within the container.
3. GapIntersections: Points along each gap boundary where the gap intersects with either the container edges or other gaps.
Maybe this will be clearer from later changes, but I think what isn't clear to me is why we need to store both the boundaries and the intersection points. I kind of would have thought the intersection points may have been enough to paint the gap decorations without needing both pieces of information.
Yeah, I think it'll be clearer. If we were storing the (inline, block) for intersection points there won't be any need for the gap boundaries; but in my opinion (or maybe because it's early on) Having them belong to a gap boundary makes things easier to reason about. It could be a potential optimization in the future. Added this to a TODO.
// Fragment 1: Fragment 2:
// +-----------------+ +-------------------+
// | Item 1| |Item 2 | | |
// | | | | | Gap End |
// |_______| |_______| |________ ________|
// | Gap Start | | Item 3 | | Item 4 |
// | | | | | |
// +-----------------+ +-------------------+nit: with the WG resolution, this will no longer be possible, but can be adjusted in a separate change.
Will prepare a separate change to adjust this.
// Constructs gap geometry for Gap Decorations. Each gap boundary isLikely worth adding to the comment about the intersections piece happening here.
Done
Should this be called `intersection_points` for consistency
Done
TEST_F(GridLayoutAlgorithmTest, GridLayoutAlgorithmGapIntersections) {Followed up with Kevin offline and had a fruitful conversation concerning where things are headed. Also added the test for spanners. Closing thread for now. Feel free to open back up if need be.
Related details
- Alison Maher
- Kevin Babbitt
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Babbitt (Gerrit)
Kevin Babbitt voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kevin Babbitt
| Code-Review | +1 |
1 comment
// block_offset), eliminating the need for the GaoBoundary data structure.```suggestion
// block_offset), eliminating the need for the GapBoundary data structure.
```
Related details
- Alison Maher
- Sam Davis Omekara
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alison Maher
| Code-Review | +1 |
1 comment
// `intersection_points`, which will be used to deetermine pairs for paintingnit: "determine"
Related details
- Sam Davis Omekara
Sam Davis Omekara (Gerrit)
Sam Davis Omekara added 2 comments![]( "Open in Gerrit")
// block_offset), eliminating the need for the GaoBoundary data structure.```suggestion
// block_offset), eliminating the need for the GapBoundary data structure.
```
Done
// `intersection_points`, which will be used to deetermine pairs for paintingSam Davis Omekaranit: "determine"
Done
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Davis Omekara (Gerrit)
Sam Davis Omekara voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
18 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/layout/gap_fragment_data.h
Insertions: 1, Deletions: 1.
@@ -56,7 +56,7 @@
// use two lists on the `GapGeometry` to store the intersection points for
// the entire grid. This might reduce the memory overhead. Also consider
// storing the intersection points as a list of pairs (inline_offset,
- // block_offset), eliminating the need for the GaoBoundary data structure.
+ // block_offset), eliminating the need for the GapBoundary data structure.
HeapVector<LayoutUnit> intersection_points;
};
```
```
The name of the file: third_party/blink/renderer/core/layout/grid/grid_layout_algorithm.h
Insertions: 1, Deletions: 1.
@@ -266,7 +266,7 @@
// For column gaps, the offsets correspond to inline coordinates;
// for row gaps, they correspond to block coordinates. The first track,
// midpoint of each gap boundary, and last track are stored in
- // `intersection_points`, which will be used to deetermine pairs for painting
+ // `intersection_points`, which will be used to determine pairs for painting
// gap decorations.
void BuildGapGeometry(GridTrackSizingDirection track_direction,
const GridLayoutData& layout_data,
```
Change information
[Gap Decorations]: Build Gap Intersection Points
This CL introduces the construction of gap intersection points for gaps
present in a grid. According to the spec[1], gap decorations are painted
relative to pairs of gap intersection points and are defined to exist
at: A) The center of an intersection between a gap and the content edge
of the container. B) The center of an intersection between gaps in
different directions.
Future changes will include the parsing of the `*rule-break` property
and the `*rule-outset` property. Following this, the painting code will
be updated to use the intersection pairs.
[1]: https://drafts.csswg.org/css-gaps-1/#layout-painting
- M third_party/blink/renderer/core/layout/gap_fragment_data.h
- M third_party/blink/renderer/core/layout/grid/grid_layout_algorithm.cc
- M third_party/blink/renderer/core/layout/grid/grid_layout_algorithm.h
- M third_party/blink/renderer/core/layout/grid/grid_layout_algorithm_test.cc
Code-Review: +1 by Alison Maher, +1 by Kevin Babbitt, +1 by Kurt Catti-Schmidt
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |