[gap-decorations] Update multicol gap decorations based on new def [chromium/src : main]
Javier Contreras (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Alison Maher
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 24 comments![]( "Open in Gerrit")
2. Instead of having `CrossGap`s end at only at spanners, we instead simply havenit: extra "at"
GapToRangesMap column_gaps_to_blocked_row_ranges_map_;This could use a comment
using GapToRangesMap = HashMap<wtf_size_t,
Vector<TrackRange>,
blink::IntWithZeroKeyHashTraits<int>>;Would it potentially make sense for this to live in GapGeometry instead (if, for example, it becomes useful for some other case in the future)
Or I see later on that it already is. Can we just use that one instead?
std::move(column_gaps_to_blocked_row_ranges_map_));Similar comment to one I think I made before, but creating a separate data structure rather than adding them directly to the gap geo as we go seems unnecessary. Can we add a TODO to look into cleaning that up for multicol (and other cases)
ResetRangeOfCrossGapsBeforeCurrentMainGap();Will this always be called after CommitRangeOfCrossGapsBeforeCurrentMainGap()? If so, it might make sense to move this into CommitRangeOfCrossGapsBeforeCurrentMainGap() instead
// version. We add a cross gap for every column in a row, after the"the start of"
// first column."since there is no gap before the first column"
/*spanner_main_gap_type=*/SpannerMainGapType::kStart);nit: since the type and variable name match, it should be ok to remove this comment
/*spanner_main_gap_type=*/SpannerMainGapType::kEnd);nit: same comment as above
// gap before range to be that one of the main gap before this one. Thisnit: "one" not needed
main_gaps_.back().SetRangeOfCrossGapsBefore(
main_gaps_[main_gaps_.size() - 2].RangeOfCrossGapsBefore());
CHECK(main_gaps_.back().RangeOfCrossGapsBefore().IsValid());If we hit a spanner, could we just check if the last main gap was a spanner, and if so, just update the end offset to the stored spanner main gap, creating one consecutive spanner for groups of spanners?
// the two main gaps added above)nit: missing punctuation
wtf_size_t spanner_index = main_gap_running_index_ + 1;Should we do a CHECK that this is actually an end spanner main gap
and that this index is also less than main_gaps_.size()
// The intersection at an end spanner main gap must still be added to
// the vector, so we can paint behind spanners with `rule-break: none`.
wtf_size_t spanner_index = main_gap_running_index_ + 1;
intersections.push_back(main_gaps_[spanner_index].GetGapOffset());I think there is still an open question on what happens if a spanner takes up space in more than one row, what happens in that case. Do we paint the gap consecutively through the two rows (even though in general, we see the two rows as having separate decorations)
// In order to support painting behind spanners with `rule-break: none`
// in cases where we have a sequence of spanners back to back, we need
// to add the offsets of any spanners that are adjacent to the current
// spanner. When spanner main gaps have the same `CrossGapBeforeRange`,
// it means they are back to back with no columns in between.As noted earlier, I wonder if we can store these as one large spanner, such that later, we only have to deal with one conceptual spanner for each group of spanners
// Only for multicol:nit: this is implied so can be removed
// There are rare edge cases where there are spanners in sequence without
// any columns in between. When this happens, we must make sure we advance
// the `main_gap_running_index_` past this sequence.
if (main_gap_after_consecutive_spanners_index_.has_value()) {
main_gap_running_index_ =
main_gap_after_consecutive_spanners_index_.value();
}
if (main_gap_running_index_ == main_gaps.size()) {
main_gap_running_index_ = kNotFound;
return content_block_end_;
}
CHECK_LE(main_gap_running_index_, main_gaps.size());
if (main_gaps[main_gap_running_index_].IsEndSpannerMainGap()) {
// Main gaps placed at the end of spanners don't have any cross gaps
// associated with them, so we skip them.
++main_gap_running_index_;
if (main_gap_running_index_ == main_gaps.size()) {
main_gap_running_index_ = kNotFound;
return content_block_end_;
}
}Same comments as earlier, I wonder if we can simplify this if we consider these as one large spanner rather than separate spanners
CrossGap::EdgeIntersectionState cross_gap_edge_state =
GetCrossGaps()[gap_index].GetEdgeIntersectionState();Can we set this in multicol and just rely on this for both, rather than having separate logic for each layout type?
// TODO(crbug.com/446616449): This might change soon, and if so we will need
// to revisit this.Same comment as earlier, what if a spanner is large enough to cross into two rows?
// `GenerateCrossIntersectionList`, which is not accounted for when the
// `column_gaps_to_blocked_row_ranges_` is created.Can you expand on this comment? It makes me wonder if we can just account for this in column_gaps_to_blocked_row_ranges_, but I'm not sure what that would mean/look like.
wtf_size_t multicol_adjustment =
GetContainerType() == ContainerType::kMultiColumn ? 1 : 0;Since this will always be the same, this could be a static var
// rather than at the middle, so we must adjustnit: was there meant to be more to this comment?
Related details
- Javier Contreras
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Contreras (Gerrit)
Javier Contreras added 23 comments![]( "Open in Gerrit")
2. Instead of having `CrossGap`s end at only at spanners, we instead simply haveJavier Contrerasnit: extra "at"
Done
GapToRangesMap column_gaps_to_blocked_row_ranges_map_;This could use a comment
Done
using GapToRangesMap = HashMap<wtf_size_t,
Vector<TrackRange>,
blink::IntWithZeroKeyHashTraits<int>>;Would it potentially make sense for this to live in GapGeometry instead (if, for example, it becomes useful for some other case in the future)
Or I see later on that it already is. Can we just use that one instead?
we'd need to include the gap_geometry.h file here, which to me seems like overkill just for an alias.
std::move(column_gaps_to_blocked_row_ranges_map_));Similar comment to one I think I made before, but creating a separate data structure rather than adding them directly to the gap geo as we go seems unnecessary. Can we add a TODO to look into cleaning that up for multicol (and other cases)
Done
(row_gap_size_));nit: parens no longer needed
Done
ResetRangeOfCrossGapsBeforeCurrentMainGap();Will this always be called after CommitRangeOfCrossGapsBeforeCurrentMainGap()? If so, it might make sense to move this into CommitRangeOfCrossGapsBeforeCurrentMainGap() instead
Yes good idea, it wasnt the case before but it is now
// version. We add a cross gap for every column in a row, after theJavier Contreras"the start of"
Done
// first column."since there is no gap before the first column"
Done
/*spanner_main_gap_type=*/SpannerMainGapType::kStart);nit: since the type and variable name match, it should be ok to remove this comment
Done
/*spanner_main_gap_type=*/SpannerMainGapType::kEnd);nit: same comment as above
Done
// gap before range to be that one of the main gap before this one. ThisJavier Contrerasnit: "one" not needed
Done
main_gaps_.back().SetRangeOfCrossGapsBefore(
main_gaps_[main_gaps_.size() - 2].RangeOfCrossGapsBefore());
CHECK(main_gaps_.back().RangeOfCrossGapsBefore().IsValid());If we hit a spanner, could we just check if the last main gap was a spanner, and if so, just update the end offset to the stored spanner main gap, creating one consecutive spanner for groups of spanners?
Resolved
// the two main gaps added above)Javier Contrerasnit: missing punctuation
Done
wtf_size_t spanner_index = main_gap_running_index_ + 1;Should we do a CHECK that this is actually an end spanner main gap
and that this index is also less than main_gaps_.size()
Done
// The intersection at an end spanner main gap must still be added to
// the vector, so we can paint behind spanners with `rule-break: none`.
wtf_size_t spanner_index = main_gap_running_index_ + 1;
intersections.push_back(main_gaps_[spanner_index].GetGapOffset());I think there is still an open question on what happens if a spanner takes up space in more than one row, what happens in that case. Do we paint the gap consecutively through the two rows (even though in general, we see the two rows as having separate decorations)
Discussed offline, will file an issue for the case where the spanner pushes content back but we still have a row behind the spanner.
// In order to support painting behind spanners with `rule-break: none`
// in cases where we have a sequence of spanners back to back, we need
// to add the offsets of any spanners that are adjacent to the current
// spanner. When spanner main gaps have the same `CrossGapBeforeRange`,
// it means they are back to back with no columns in between.As noted earlier, I wonder if we can store these as one large spanner, such that later, we only have to deal with one conceptual spanner for each group of spanners
Done
// Only for multicol:nit: this is implied so can be removed
Done
// There are rare edge cases where there are spanners in sequence without
// any columns in between. When this happens, we must make sure we advance
// the `main_gap_running_index_` past this sequence.
if (main_gap_after_consecutive_spanners_index_.has_value()) {
main_gap_running_index_ =
main_gap_after_consecutive_spanners_index_.value();
}
if (main_gap_running_index_ == main_gaps.size()) {
main_gap_running_index_ = kNotFound;
return content_block_end_;
}
CHECK_LE(main_gap_running_index_, main_gaps.size());
if (main_gaps[main_gap_running_index_].IsEndSpannerMainGap()) {
// Main gaps placed at the end of spanners don't have any cross gaps
// associated with them, so we skip them.
++main_gap_running_index_;
if (main_gap_running_index_ == main_gaps.size()) {
main_gap_running_index_ = kNotFound;
return content_block_end_;
}
}Same comments as earlier, I wonder if we can simplify this if we consider these as one large spanner rather than separate spanners
Done
CrossGap::EdgeIntersectionState cross_gap_edge_state =
GetCrossGaps()[gap_index].GetEdgeIntersectionState();Can we set this in multicol and just rely on this for both, rather than having separate logic for each layout type?
I have updated the code to use the edge state. However, we still need special logic for multicolumn because the gaps are not equivalent between flex and mc (for instance flex cross gaps will always only have 2 intersections, multicol's may have 3).
I updated the unit tests to expect the correct edge state now, but this also uncovered a bug that we were accidentally passing some outset related tests. Now we are failing them (but we have the correct edge state now), but that is ok for now because the outset property will change (with the inset related changes and the fact that the intersections are no longer at the middle of the row gap). I've added these tests as failing until this issue is resolved and in the spec https://github.com/w3c/csswg-drafts/issues/12784, https://github.com/w3c/csswg-drafts/issues/12848
// TODO(crbug.com/446616449): This might change soon, and if so we will need
// to revisit this.Same comment as earlier, what if a spanner is large enough to cross into two rows?
See my response to earlier comment for this.
// `GenerateCrossIntersectionList`, which is not accounted for when the
// `column_gaps_to_blocked_row_ranges_` is created.Can you expand on this comment? It makes me wonder if we can just account for this in column_gaps_to_blocked_row_ranges_, but I'm not sure what that would mean/look like.
Still working on fixing this, but the rest is ready to review.
wtf_size_t multicol_adjustment =
GetContainerType() == ContainerType::kMultiColumn ? 1 : 0;Since this will always be the same, this could be a static var
Done
// rather than at the middle, so we must adjustnit: was there meant to be more to this comment?
- Alison Maher
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lkaye Handley (Gerrit)
Lkaye Handley added 1 comment![]( "Open in Gerrit")
- Alison Maher
- Javier Contreras
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
CrossGap::EdgeIntersectionState cross_gap_edge_state =
GetCrossGaps()[gap_index].GetEdgeIntersectionState();Javier ContrerasCan we set this in multicol and just rely on this for both, rather than having separate logic for each layout type?
I have updated the code to use the edge state. However, we still need special logic for multicolumn because the gaps are not equivalent between flex and mc (for instance flex cross gaps will always only have 2 intersections, multicol's may have 3).
I updated the unit tests to expect the correct edge state now, but this also uncovered a bug that we were accidentally passing some outset related tests. Now we are failing them (but we have the correct edge state now), but that is ok for now because the outset property will change (with the inset related changes and the fact that the intersections are no longer at the middle of the row gap). I've added these tests as failing until this issue is resolved and in the spec https://github.com/w3c/csswg-drafts/issues/12784, https://github.com/w3c/csswg-drafts/issues/12848
Disregard the previous comment. After some more thinking and discussing with Sam, we should in fact be treating all intersections as `kBoth` (all intersections in multicol should be edges) given where we are placing the intersections now (before row gaps) and given the ongoing discussion in the linked issues above. We can reconsider this later based on what we decide for `inset` etc.
// `GenerateCrossIntersectionList`, which is not accounted for when the
// `column_gaps_to_blocked_row_ranges_` is created.Javier ContrerasCan you expand on this comment? It makes me wonder if we can just account for this in column_gaps_to_blocked_row_ranges_, but I'm not sure what that would mean/look like.
Still working on fixing this, but the rest is ready to review.
Turns out with the new design of where we place the intersections, we don't need the gap.
Related details
- Alison Maher
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 9 comments![]( "Open in Gerrit")
// in between, each spanner accounting for 2 `MainGap`s). In such cases, weIs this still accurate?
// If we have consecutive spanners with no content in between, we removethenit: missing space
}
if (!spanner_placed_after_spanner_with_no_content_in_between) {This can be simplified to an else
CHECK_GE(main_gaps_.size(), 2u);Will it always be 2? (if there are rows or other spanners earlier on?)
// enables us to carry this range forward to any spanner main gaps that are
// back to back (with no columns in between).Is this still needed, since any back to back spanners are now considered contiguous?
// In multicol, a gap of intersections will be spanner adjacent if and only if
// there are 3 intersections in the gap, and we are at the middle
// intersection. This is because all multicol CrossGaps will have only 2
// intersections, except if they are adjacent to a spanner, in which case they
// will have 3 intersections: One at the start of the gap, one at the start of
// the spanner, and one at the end of the spanner. The middle intersection is
// the one that is spanner adjacent.What if there are two spanners at separate locations, won't there be more than 3?
if (cross_gap_edge_state == CrossGap::EdgeIntersectionState::kBoth) {If they are all considered both, should this be a CHECK_EQ instead and just always return true?
IsTrackCovered(track_direction, primary_index, secondary_index - 1)) {Why can't we use this same logic for multicol?
return intersections.size() == 3 && intersection_index == 1;As mentioned elsewhere, what happens if there is more than one (non-contiguous) spanner in a row
Related details
- Javier Contreras
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
// in between, each spanner accounting for 2 `MainGap`s). In such cases, weJavier ContrerasIs this still accurate?
It is. We will end up removing one of those main gaps later, but we still need to carry the range forward as we do.
// If we have consecutive spanners with no content in between, we removetheJavier Contrerasnit: missing space
Done
}
if (!spanner_placed_after_spanner_with_no_content_in_between) {This can be simplified to an else
Done
Will it always be 2? (if there are rows or other spanners earlier on?)
There can be more, but its a CHECK_GE so it will be fine.
// enables us to carry this range forward to any spanner main gaps that are
// back to back (with no columns in between).Is this still needed, since any back to back spanners are now considered contiguous?
its still needed to set the range, but i updated the comment
// In multicol, a gap of intersections will be spanner adjacent if and only if
// there are 3 intersections in the gap, and we are at the middle
// intersection. This is because all multicol CrossGaps will have only 2
// intersections, except if they are adjacent to a spanner, in which case they
// will have 3 intersections: One at the start of the gap, one at the start of
// the spanner, and one at the end of the spanner. The middle intersection is
// the one that is spanner adjacent.What if there are two spanners at separate locations, won't there be more than 3?
Not really. We create a cross gap for each column in each row of columns (refer to picture in resources). The intersections related to each of those cross gaps (C1...) can have at most 3 intersections (if the cross gap has a spanner after it), or two (if it doesn't have a spanner after it).
I expanded upon this in the README and you can use the picture for reference.
if (cross_gap_edge_state == CrossGap::EdgeIntersectionState::kBoth) {If they are all considered both, should this be a CHECK_EQ instead and just always return true?
Done
IsTrackCovered(track_direction, primary_index, secondary_index - 1)) {Why can't we use this same logic for multicol?
Because for a given multicol (say for example the one in the picture) in which if it was a grid it would have 2 Cross Gaps and 2 row gaps, in multicol we end up having way more cross gaps than we do "visual columns". We could try to retrofit by adding back the additional map and tracking ranges in that map. This way though we don't need to use any other structures or data.
return intersections.size() == 3 && intersection_index == 1;As mentioned elsewhere, what happens if there is more than one (non-contiguous) spanner in a row
- Alison Maher
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Davis Omekara (Gerrit)
Sam Davis Omekara added 5 comments![]( "Open in Gerrit")
`CrossGap`s end at both spanners and row gaps, and start right after.nit: "... at all main gaps (spanners and row gaps)"
bool IsGapSpannerAdjacent(GridTrackSizingDirection direction,This is called IsGapSpannerAdjacent. So shouldn't it be only checking num of intersections?
using GapToRangesMap =Should this be renamed back since we don't end up needing in this CL?
bool GapGeometry::IsGapSpannerAdjacent(Ahh, I see what this is doing now. So I'll restructure it this way:
IsGapSpannerAdjacent should only take in intersections. But the comment should hint that it's *_only_* used multi col cross gaps, when the main gap below it is a spanner gap. I think Adjacent could also mean side by side too if I'm not mistaken ... so maybe `IsMainGapBelowSpanner` might be clearer ...
Then when we use this `IsMainGapBelowSpanner` , we can check secondary index == 1 to mark as BlockedAfter...
const LayoutUnit center =Can this knowledge belong to GapGeometry::GetGapOffset().
Related details
- Alison Maher
- Javier Contreras
Code-Coverage
Code-Owners
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 4 comments![]( "Open in Gerrit")
Votes added by Alison Maher
| Code-Review | +1 |
4 comments
CHECK_GE(main_gaps_.size(), 2u);Javier ContrerasWill it always be 2? (if there are rows or other spanners earlier on?)
There can be more, but its a CHECK_GE so it will be fine.
Ah missed GE, thanks for the clarification
// The only situation where we would expect the range to not be valid is if
// there are spanners placed back-to-back with no content in between. As far
// as GapDecorations is related, in these cases we "condense" the sequence
// of consecutive spanners into a single spanner, by having only two
// MainGaps (a start and an end) represent the entire sequence of spanners.
bool spanner_placed_after_spanner_with_no_content_in_between =
!range_of_cross_gaps_before_current_main_gap_.IsValid();
// If we have consecutive spanners with no content in between, we remove the
// last end spanner main gap, and the new spanner's end will become the end
// of the big condensed spanner.
if (spanner_placed_after_spanner_with_no_content_in_between) {
main_gaps_.pop_back();
} else {
// As described in third_party/blink/renderer/core/layout/gap/README.md,
// we create a `MainGap` at the spanner's block start offset when we have
// a spanner, and another `MainGap` at the spanner's block end offset.
main_gaps_.emplace_back(offset.block_offset, SpannerMainGapType::kStart);
}
// A spanner ends the current range of cross gaps, since it leads to the
// introduction of new `CrossGap`s.
CommitRangeOfCrossGapsBeforeCurrentMainGap();
main_gaps_.emplace_back(offset.block_offset + logical_fragment.BlockSize(),
SpannerMainGapType::kEnd);
CHECK_GE(main_gaps_.size(), 2u);
// Even though this last `MainGap` is a spanner end gap, we set its cross
// gap before range to be that of the main gap before this one.
main_gaps_.back().SetRangeOfCrossGapsBefore(
main_gaps_[main_gaps_.size() - 2].RangeOfCrossGapsBefore());
CHECK(main_gaps_.back().RangeOfCrossGapsBefore().IsValid());nit: I wonder if we should move to a helper now that this block is getting a bit lengthy
}
if (GetContainerType() == ContainerType::kMultiColumn) {nit: this can be an else if
if (track_direction == kForColumns) {
return intersections.size() == 3 && intersection_index == 1;Don't we keep track of gaps that are spanners or not? Can we use that instead?
Related details
- Javier Contreras
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Contreras (Gerrit)
Javier Contreras added 8 comments![]( "Open in Gerrit")
`CrossGap`s end at both spanners and row gaps, and start right after.nit: "... at all main gaps (spanners and row gaps)"
Done
nit: I wonder if we should move to a helper now that this block is getting a bit lengthy
Done
bool IsGapSpannerAdjacent(GridTrackSizingDirection direction,This is called IsGapSpannerAdjacent. So shouldn't it be only checking num of intersections?
Done
Should this be renamed back since we don't end up needing in this CL?
Done
}
if (GetContainerType() == ContainerType::kMultiColumn) {nit: this can be an else if
Done
Ahh, I see what this is doing now. So I'll restructure it this way:
IsGapSpannerAdjacent should only take in intersections. But the comment should hint that it's *_only_* used multi col cross gaps, when the main gap below it is a spanner gap. I think Adjacent could also mean side by side too if I'm not mistaken ... so maybe `IsMainGapBelowSpanner` might be clearer ...
Then when we use this `IsMainGapBelowSpanner` , we can check secondary index == 1 to mark as BlockedAfter...
Its checking if a CrossGap is adjacent to a spanner (above it/ ends at a spanner) but you are right, naming should be better to reflect this. Updated.
if (track_direction == kForColumns) {
return intersections.size() == 3 && intersection_index == 1;Don't we keep track of gaps that are spanners or not? Can we use that instead?
We keep track of which MainGaps are spanners yes, but we don't have a way of relating a CrossGap to a MainGap. This method is meant to tell if a set of cross gap intersections (not `CrossGap` per se) are above a spanner.
Can this knowledge belong to GapGeometry::GetGapOffset().
As per our offline discussion, moving there but renaming function to make it clear that we are not just getting the gap offset.
Related details
- Alison Maher
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
| 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
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Davis Omekara (Gerrit)
Sam Davis Omekara voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Javier Contreras
Code-Coverage
Code-Owners
Code-Review
| 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
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/55068.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
Code-Coverage
Code-Owners
Code-Review
| 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
[gap-decorations] Update multicol gap decorations based on new def
This CL updates the implementation for GapDecorations in multicol. Based
on some discussions in https://github.com/w3c/csswg-drafts/issues/12784,
and the definition of gap intersections
https://drafts.csswg.org/css-gaps-1/#layout-painting we have to update
how we compute and store the information to generate the intersections
for gap decorations in multicol.
The main new behavior is that intersections in the column gap happen
right before/after where the column gap intersects any row gaps.
Previously this happened at the middle of where these two gaps met.
There are couple of tweaks needed for this, which overall at the end of
the day reduces the code complexity of painting multicol gap
decorations.
1. Instead of having ONE `MainGap` being generated by a spanner, we now
generate TWO, one at the start of the spanner and one at the end.
2. Instead of having `CrossGap`s end only at spanners, we instead simply
have `CrossGap`s end at all main gaps (spanners and row gaps), and start
right after. This way, we can easily generate the intersections there,
which is what the discussion seems to have settled on.
3. Using the two above, along with a transient map which maps from an
intersection to a `MainGap`, we can now determine in constant time
whether a given intersection is `blocked before/after` or if it should
be treated like an Edge intersection, which enables us to fix the rule
breaking behavior that was broken for multicol.
This also enables us to get rid of a lot of the complex logic of
advancing indices of spanner main gaps when generating the intersections
for multicol.
This CL also makes two more changes:
a) Some of the expectation files had to be updated to match the new
behavior.
b) We add a test case of a multicol container with multiple spanners
c) `column-height-009.html` test is now technically incorrect given the
dicussion at https://github.com/w3c/csswg-drafts/issues/12784, so this
CL updates the expectation for it.
This brings the behavior to exactly what we had with the original
GapDecorations, before the optimized pipeline.
- M third_party/blink/renderer/core/layout/column_layout_algorithm.cc
- M third_party/blink/renderer/core/layout/column_layout_algorithm.h
- M third_party/blink/renderer/core/layout/column_layout_algorithm_test.cc
- M third_party/blink/renderer/core/layout/gap/README.md
- 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/gap/main_gap.h
- M third_party/blink/renderer/core/layout/gap/resources/multicol-mc.png
- M third_party/blink/renderer/core/paint/gap_decorations_painter.cc
- M third_party/blink/web_tests/TestExpectations
- M third_party/blink/web_tests/external/wpt/css/css-gaps/multicol/multicol-gap-decorations-004-ref.html
- M third_party/blink/web_tests/external/wpt/css/css-gaps/multicol/multicol-gap-decorations-007-ref.html
- M third_party/blink/web_tests/external/wpt/css/css-gaps/multicol/multicol-gap-decorations-008-ref.html
- M third_party/blink/web_tests/external/wpt/css/css-gaps/multicol/multicol-gap-decorations-009-ref.html
- M third_party/blink/web_tests/external/wpt/css/css-gaps/multicol/multicol-gap-decorations-009.html
- M third_party/blink/web_tests/external/wpt/css/css-gaps/multicol/multicol-gap-decorations-010-ref.html
- M third_party/blink/web_tests/external/wpt/css/css-gaps/multicol/multicol-gap-decorations-011-ref.html
- M third_party/blink/web_tests/external/wpt/css/css-gaps/multicol/multicol-gap-decorations-012-ref.html
- M third_party/blink/web_tests/external/wpt/css/css-gaps/multicol/multicol-gap-decorations-014-ref.html
- M third_party/blink/web_tests/external/wpt/css/css-gaps/multicol/multicol-gap-decorations-017-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-gaps/multicol/multicol-gap-decorations-020-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-gaps/multicol/multicol-gap-decorations-020.html
- M third_party/blink/web_tests/external/wpt/css/css-multicol/column-height-009-ref.html
Code-Review: +1 by Alison Maher, +1 by Sam Davis Omekara
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/55068
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |