[Gap Decorations]: Setup paint for simple gap cases [chromium/src : main]
Kevin Babbitt (Gerrit)
Kevin Babbitt added 3 comments![]( "Open in Gerrit")
BoxSide BoxFragmentPainter::BoxSideFromGridDirection(Kevin BabbittWould it make sense for this to live in the anonymous namespace?
+1, or if the anonymous namespace doesn't make sense for some reason, can it be made `static`?
LayoutUnit row_width = fragment_rect.size.width;Is this correct, even as a placeholder? Looking at `PaintGridColumnGaps()` I would expect this to be the decoration thickness, and if we're using a placeholder value rather than the property value I'd expect that to be `1`.
LogicalOffset gap_position(row_left, row_top);
LogicalSize gap_size(row_width, row_height);
LogicalRect gap_logical(gap_position, gap_size);
PhysicalRect gap_rect = converter.ToPhysical(gap_logical);
gap_rect.offset += paint_rect.offset;
gfx::Rect snapped_rule = ToPixelSnappedRect(gap_rect);
BoxBorderPainter::DrawBoxSide(paint_info.context, snapped_rule, box_side,
default_color_for_row,
default_rule_style_for_row, auto_dark_mode);Kevin BabbittI wonder if we could pull some of this out to be in a helper between row and column specific versions
+1
Related details
- Kurt Catti-Schmidt
- 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 19 comments![]( "Open in Gerrit")
[Gap Decorations]: Simple paint caseSam Davis Omekaranit: "Setup paint for simple gap cases"
Done
// `PaintGridColumnGaps` method because currently row gaps are treated as
// fragmentable which would require painting logic to be quite different.Sam Davis OmekaraSince we aren't currently plumbing this info for fragmenation, is this true yet?
Although we aren't plumbing the info yet, I think it is true today that in grid: "row gaps are fragmentable". But taking a closer at the change(with insights from your other comments), I think I can consolidate both into one method.
BoxSide BoxFragmentPainter::BoxSideFromGridDirection(Would it make sense for this to live in the anonymous namespace?
Absolutely!!
if (style.IsHorizontalWritingMode()) {Sam Davis OmekaraShould we add tests for different writing modes?
Not a bad idea, but I haven't fully implemented all the logic to handle other writing modes yet. So maybe in a future change.
Haven't looked too deeply, but I suspect offsets/size calculation will change based on writing mode like: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/paint/box_fragment_painter.cc;l=1521?q=box_fragment_painter&ss=chromium
if (GetPhysicalFragment().IsGrid()) {Sam Davis Omekaranit: we could combine this with the if-statement above
I think ultimately(before this CL lands), this code block wouldn't even live here. It will live somewhere in `BoxFrgamnetPainter::PaintBoxDecorationBackgroundWithRectImpl`[1]. I think Ian suggested after Border painting but still need his confirmation.
const GapFragmentData::GapGeometry* gap_geometry =
box_fragment_.GapGeometry();
CHECK(gap_geometry);
PaintGridRowGaps(paint_info, paint_rect, gap_geometry->rows);
PaintGridColumnGaps(paint_info, paint_rect, gap_geometry->columns);Sam Davis OmekaraI wonder if we should move this to its own method. Or even potentially considering creating a painter class specifically for gap decorations, similar to the table related painters above.
Moved to its own method. Open to creating a class if that's needed but like I mentioned in an earlier comment this code block will most likely sit somewhere in `BoxFrgamnetPainter::PaintBoxDecorationBackgroundWithRectImpl` and a method should suffice there.
// Below logic, for offsets and size would change once we start painting based
// on gap intersection pairs.Sam Davis Omekaranit: I'm not sure I fully follow what this means
Taken out but I wanted to point out a good chunk of the painting logic for offsets and size would change when we incorporate painting based on the intersection points as spec'd.
LogicalOffset gap_position(column_left, column_top);Sam Davis Omekaranit: I might add a line above this one
Done
LogicalRect gap_logical(gap_position, gap_size);Sam Davis Omekaranit: I might add a line below this one
Done
BoxBorderPainter::DrawBoxSide(paint_info.context, snapped_rule, box_side,Sam Davis Omekaranit: could plug `ToPixelSnappedRect(gap_rect)` in directly
Done
PaintAutoDarkMode(style, DarkModeFilter::ElementRole::kBackground));Sam Davis Omekaranit: I might add an extra line after this line
Done
// TODO(crbug.com/357648037): Using default values, this should be retrievedSam Davis Omekaranit: "hard coded"
Done
// TODO(crbug.com/357648037): Using default values, this should be retrievedSam Davis Omekara"These values"
Done
// TODO(crbug.com/357648037): Using default values, this should be retrievedSam Davis Omekaranit: "."
Done
PhysicalRect fragment_rect = box_fragment_.LocalRect();Sam Davis Omekaranit: I might add an extra line before this line
Done
LayoutUnit row_width = fragment_rect.size.width;Is this correct, even as a placeholder? Looking at `PaintGridColumnGaps()` I would expect this to be the decoration thickness, and if we're using a placeholder value rather than the property value I'd expect that to be `1`.
I see your point; but the `rule_thickness` will apply to the `row_height` not `row_width` since rows flow in the block direction. From conversations with Ian, I think for now we want to paint full row gaps since it'll be hard to determine the "center" when row gaps are fragmented. So this logic paints decorations in the entire row gap.
LogicalOffset gap_position(row_left, row_top);Sam Davis Omekaranit: I'd probably add some extra spacing similar to comments in the column method
Done
LogicalOffset gap_position(row_left, row_top);
LogicalSize gap_size(row_width, row_height);
LogicalRect gap_logical(gap_position, gap_size);
PhysicalRect gap_rect = converter.ToPhysical(gap_logical);
gap_rect.offset += paint_rect.offset;
gfx::Rect snapped_rule = ToPixelSnappedRect(gap_rect);
BoxBorderPainter::DrawBoxSide(paint_info.context, snapped_rule, box_side,
default_color_for_row,
default_rule_style_for_row, auto_dark_mode);Kevin BabbittI wonder if we could pull some of this out to be in a helper between row and column specific versions
+1
Ended up consolidating everything to one function that handles offsets based of the track direction. Let me know if this is sufficient!
return ColumnRuleWidth().GetLegacyValue() && !ColumnRuleIsTransparent() &&
BorderStyleIsVisible(ColumnRuleStyle().GetLegacyValue());Sam Davis OmekaraSince this is the same logic as in the last part of `HasColumnRule()`, we could move that to a helper to reduce repeated logic
Attempted this in an earlier patch but`HasColumnRule()` needs the `column` property defined to return true [1]--this doesn't apply to grid/flex. Do you think we should augment `HasColumnRule()` by looking at the `display` property?
Related details
- Alison Maher
- Kevin Babbitt
- Kurt Catti-Schmidt
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")
// Below logic, for offsets and size would change once we start painting based
// on gap intersection pairs.Sam Davis Omekaranit: I'm not sure I fully follow what this means
Taken out but I wanted to point out a good chunk of the painting logic for offsets and size would change when we incorporate painting based on the intersection points as spec'd.
Done
Sam Davis Omekara (Gerrit)
Sam Davis Omekara added 1 comment![]( "Open in Gerrit")
// TODO(crbug.com/357648037): Only using one value, should update this andSam Davis Omekaranit: "We are currently only painting gaps with a single color, but we should update this to paint all values potentially set by the author."
Also might be worth noting this in the CL description.
Done
Sam Davis Omekara (Gerrit)
Sam Davis Omekara added 1 comment![]( "Open in Gerrit")
if (RuntimeEnabledFeatures::CSSGapDecorationEnabled() &&Painting gap decorations after borders. Should add a TODO that this might change depending on the resolution on the paint order issue.
Alison Maher (Gerrit)
Alison Maher added 6 comments![]( "Open in Gerrit")
void PaintGapDecorations(const PaintInfo&, const PhysicalRect& paint_rect);
void PaintGridGaps(GridTrackSizingDirection track_direction,Can we make both of these private?
if (GetPhysicalFragment().IsGrid()) {nit: we could combine this with the if-statement above
I think ultimately(before this CL lands), this code block wouldn't even live here. It will live somewhere in `BoxFrgamnetPainter::PaintBoxDecorationBackgroundWithRectImpl`[1]. I think Ian suggested after Border painting but still need his confirmation.
I was just noting that we could get rid of the nested if-statements, but it looks like that has been cleaned up otherwise, so resolving.
if (GetPhysicalFragment().IsGrid()) {I would suggest reversing this check to the following:
```
if (!GetPhysicalFragment().IsGrid()) {
return
}
...
```
const GapFragmentData::GapGeometry* gap_geometry =
box_fragment_.GapGeometry();
CHECK(gap_geometry);
PaintGridRowGaps(paint_info, paint_rect, gap_geometry->rows);
PaintGridColumnGaps(paint_info, paint_rect, gap_geometry->columns);Sam Davis OmekaraI wonder if we should move this to its own method. Or even potentially considering creating a painter class specifically for gap decorations, similar to the table related painters above.
Moved to its own method. Open to creating a class if that's needed but like I mentioned in an earlier comment this code block will most likely sit somewhere in `BoxFrgamnetPainter::PaintBoxDecorationBackgroundWithRectImpl` and a method should suffice there.
Acknowledged
LogicalOffset gap_position(row_left, row_top);
LogicalSize gap_size(row_width, row_height);
LogicalRect gap_logical(gap_position, gap_size);
PhysicalRect gap_rect = converter.ToPhysical(gap_logical);
gap_rect.offset += paint_rect.offset;
gfx::Rect snapped_rule = ToPixelSnappedRect(gap_rect);
BoxBorderPainter::DrawBoxSide(paint_info.context, snapped_rule, box_side,
default_color_for_row,
default_rule_style_for_row, auto_dark_mode);Kevin BabbittI wonder if we could pull some of this out to be in a helper between row and column specific versions
Sam Davis Omekara+1
Ended up consolidating everything to one function that handles offsets based of the track direction. Let me know if this is sufficient!
Acknowledged
return ColumnRuleWidth().GetLegacyValue() && !ColumnRuleIsTransparent() &&
BorderStyleIsVisible(ColumnRuleStyle().GetLegacyValue());Sam Davis OmekaraSince this is the same logic as in the last part of `HasColumnRule()`, we could move that to a helper to reduce repeated logic
Attempted this in an earlier patch but`HasColumnRule()` needs the `column` property defined to return true [1]--this doesn't apply to grid/flex. Do you think we should augment `HasColumnRule()` by looking at the `display` property?
My thinking was to pull just `ColumnRuleWidth().GetLegacyValue() && !ColumnRuleIsTransparent() && BorderStyleIsVisible(ColumnRuleStyle().GetLegacyValue());` out and then have each of the methods call a helper that calculates just that. That way we don't have to duplicate the same logic. We can probably do the same for row, which would be shared between flex and grid.
But combining them and checking the display type could work, too and may end up being cleaner.
Related details
- Kevin Babbitt
- Kurt Catti-Schmidt
- 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 4 comments![]( "Open in Gerrit")
void PaintGapDecorations(const PaintInfo&, const PhysicalRect& paint_rect);
void PaintGridGaps(GridTrackSizingDirection track_direction,Can we make both of these private?
Done
if (GetPhysicalFragment().IsGrid()) {Sam Davis OmekaraI would suggest reversing this check to the following:
```
if (!GetPhysicalFragment().IsGrid()) {
return
}
...
```
Done. Also added a note for flexbox implementation in future
if (RuntimeEnabledFeatures::CSSGapDecorationEnabled() &&Painting gap decorations after borders. Should add a TODO that this might change depending on the resolution on the paint order issue.
Done
return ColumnRuleWidth().GetLegacyValue() && !ColumnRuleIsTransparent() &&
BorderStyleIsVisible(ColumnRuleStyle().GetLegacyValue());Sam Davis OmekaraSince this is the same logic as in the last part of `HasColumnRule()`, we could move that to a helper to reduce repeated logic
Alison MaherAttempted this in an earlier patch but`HasColumnRule()` needs the `column` property defined to return true [1]--this doesn't apply to grid/flex. Do you think we should augment `HasColumnRule()` by looking at the `display` property?
My thinking was to pull just `ColumnRuleWidth().GetLegacyValue() && !ColumnRuleIsTransparent() && BorderStyleIsVisible(ColumnRuleStyle().GetLegacyValue());` out and then have each of the methods call a helper that calculates just that. That way we don't have to duplicate the same logic. We can probably do the same for row, which would be shared between flex and grid.
But combining them and checking the display type could work, too and may end up being cleaner.
Okay, just did that. I included a check for just grid for now, will augment for flex box, masonry as we progress.
Related details
- Alison Maher
- Kevin Babbitt
- Kurt Catti-Schmidt
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 3 comments![]( "Open in Gerrit")
Votes added by Alison Maher
| Code-Review | +1 |
3 comments
// borders, this is likely to change following the resolution of the paintnit: ". T"
// order issue for gap decorations.nit: do we have a github issue to link to for this?
CSS Gap Decorations: column and row gaps are painted.nit: might be worth adding a bit to the title for these to indicate what specifically is being tested (for example, "with dotted rule styling")
Related details
- Kevin Babbitt
- Kurt Catti-Schmidt
- Sam Davis Omekara
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Grogan (Gerrit)
David Grogan added 1 comment![]( "Open in Gerrit")
rule_thickness = LayoutUnit(0);drive-by nit: Remove 0, `LayoutUnit`'s default constructor does that for you and omitting 0 is minutely faster.
(Though I think you can just omit this entire line)
David Grogan (Gerrit)
David Grogan added 1 comment![]( "Open in Gerrit")
rule_thickness = LayoutUnit(0);drive-by nit: Remove 0, `LayoutUnit`'s default constructor does that for you and omitting 0 is minutely faster.
(Though I think you can just omit this entire line)
Oh, I see that this code has a short life expectancy, so whatever.
Sam Davis Omekara (Gerrit)
Sam Davis Omekara added 4 comments![]( "Open in Gerrit")
rule_thickness = LayoutUnit(0);David Grogandrive-by nit: Remove 0, `LayoutUnit`'s default constructor does that for you and omitting 0 is minutely faster.
(Though I think you can just omit this entire line)
Oh, I see that this code has a short life expectancy, so whatever.
Still useful to know. Thank you!
// borders, this is likely to change following the resolution of the paintSam Davis Omekaranit: ". T"
Done
nit: do we have a github issue to link to for this?
no we don't. not yet
nit: might be worth adding a bit to the title for these to indicate what specifically is being tested (for example, "with dotted rule styling")
Done
Related details
- Kevin Babbitt
- Kurt Catti-Schmidt
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 2 comments![]( "Open in Gerrit")
LayoutUnit row_width = fragment_rect.size.width;Sam Davis OmekaraIs this correct, even as a placeholder? Looking at `PaintGridColumnGaps()` I would expect this to be the decoration thickness, and if we're using a placeholder value rather than the property value I'd expect that to be `1`.
I see your point; but the `rule_thickness` will apply to the `row_height` not `row_width` since rows flow in the block direction. From conversations with Ian, I think for now we want to paint full row gaps since it'll be hard to determine the "center" when row gaps are fragmented. So this logic paints decorations in the entire row gap.
Acknowledged
if (!SpecifiesColumns() && Display() != EDisplay::kGrid) [[likely]] {Does this impact column rules on multicol containers?
Related details
- Kurt Catti-Schmidt
- 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")
if (!SpecifiesColumns() && Display() != EDisplay::kGrid) [[likely]] {Does this impact column rules on multicol containers?
No, the behavior for multicol containers remains unchanged. The check removes the requirement of having to specify the `columns` property for grid containers.
Related details
- Kevin Babbitt
- Kurt Catti-Schmidt
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kevin Babbitt
| Code-Review | +1 |
1 comment
if (!SpecifiesColumns() && Display() != EDisplay::kGrid) [[likely]] {Sam Davis OmekaraDoes this impact column rules on multicol containers?
No, the behavior for multicol containers remains unchanged. The check removes the requirement of having to specify the `columns` property for grid containers.
- Kurt Catti-Schmidt
- Sam Davis Omekara
Code-Coverage
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/50318.
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
- Kurt Catti-Schmidt
- Sam Davis Omekara
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt voted and added 3 comments![]( "Open in Gerrit")
Votes added by Kurt Catti-Schmidt
| Code-Review | +1 |
3 comments
return style_.HasColumnRule();Is this going to be expanded over time? If so, add a TODO comment.
WritingModeConverter converter(style.GetWritingDirection(),
box_fragment_.Size());Does this need to be declared up here? Looks like it's not used until the end of the method.
PhysicalRect local_rect = box_fragment_.LocalRect();const
Related details
- 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 3 comments![]( "Open in Gerrit")
return style_.HasColumnRule();Is this going to be expanded over time? If so, add a TODO comment.
Oh we decided to consolidate into that singular function `HasColumnRule()`
See:
https://chromium-review.googlesource.com/c/chromium/src/+/6101656/comment/972c7f25_c5718c14/
WritingModeConverter converter(style.GetWritingDirection(),
box_fragment_.Size());Does this need to be declared up here? Looks like it's not used until the end of the method.
It's used inside the loop, so we'll probably want it before the loop. Also, from the codebase this seems to be defined at the top of the function; See:
PhysicalRect local_rect = box_fragment_.LocalRect();- Ian Kilpatrick
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 |
Related details
- Ian Kilpatrick
Code-Coverage
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 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/paint/box_fragment_painter.cc
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
Change information
[Gap Decorations]: Setup paint for simple gap cases
This CL uses the gap geometry from crrev.com/c/6092603 to paint gap
decorations within grid. The change begins by introducing a
`ShouldPaintGapDecorations` method in the `BoxDecorationData` class.
This method affects the `ShouldPaint` method, ensuring that gap
decorations are painted when necessary.
Next, two methods, `PaintColumnGaps` and `PaintRowGaps`, are
implemented in the `BoxFragmentPainter` class. These methods use the
stored offsets to construct rects, which are passed to appropriate
paint ops.
Future changes will preliminarily handle row fragmentation and
update the painting logic to use gap intersection pairs, as outlined in
the spec: https://drafts.csswg.org/css-gaps-1/#break.
- M third_party/blink/renderer/core/layout/physical_box_fragment.cc
- M third_party/blink/renderer/core/paint/box_decoration_data.h
- M third_party/blink/renderer/core/paint/box_fragment_painter.cc
- M third_party/blink/renderer/core/paint/box_fragment_painter.h
- M third_party/blink/renderer/core/style/computed_style.h
- A third_party/blink/web_tests/external/wpt/css/css-gaps/tentative/grid/grid-gap-decorations-001.html
- A third_party/blink/web_tests/external/wpt/css/css-gaps/tentative/grid/grid-gap-decorations-002-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-gaps/tentative/grid/grid-gap-decorations-002.html
- A third_party/blink/web_tests/external/wpt/css/css-gaps/tentative/grid/grid-gap-decorations-003-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-gaps/tentative/grid/grid-gap-decorations-003.html
- A third_party/blink/web_tests/external/wpt/css/css-gaps/tentative/grid/grid-gap-decorations-004-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-gaps/tentative/grid/grid-gap-decorations-004.html
- A third_party/blink/web_tests/external/wpt/css/css-gaps/tentative/grid/grid-gap-decorations-005-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-gaps/tentative/grid/grid-gap-decorations-005.html
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. |
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/50318
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |