[gap-decorations] Base interpolation for *-rule-width [chromium/src : main]

0 views
Skip to first unread message

Javier Contreras (Gerrit)

unread,
Jun 17, 2025, 6:26:40 PMJun 17
to Sam Davis Omekara, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Sam Davis Omekara

Javier Contreras added 1 comment

File third_party/blink/web_tests/external/wpt/css/css-multicol/animation/column-rule-width-interpolation.html
Line 55, Patchset 9 (Parent): {at: 0, expect: '0px'},
Javier Contreras . unresolved

this feels like this test case was wrong to begin with. If it is inheriting from the parent, why would it start out at 0px rather than at 30px, like the parent does?

Open in Gerrit

Related details

Attention is currently required from:
  • Sam Davis Omekara
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
Gerrit-Change-Number: 6643003
Gerrit-PatchSet: 9
Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
Gerrit-CC: Alexis Menard <alexis...@intel.com>
Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>
Gerrit-Comment-Date: Tue, 17 Jun 2025 22:26:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Sam Davis Omekara (Gerrit)

unread,
Jun 19, 2025, 4:56:02 AMJun 19
to Javier Contreras, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Javier Contreras

Sam Davis Omekara added 10 comments

Patchset-level comments
File-level comment, Patchset 10 (Latest):
Sam Davis Omekara . resolved

Looks good overall tbh, few things to clarify here and there but things looking good fr

File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
Line 401, Patchset 10 (Latest):using CSSColumnRuleColorInterpolationType =
Sam Davis Omekara . unresolved

The typedefs used here seem repeated for both Column/Row. Is it possible to have just two rather than four (i.e. `CSSRuleColorInterpolationType` && `CSSRuleWidthInterpolationType`).

Line 199, Patchset 10 (Latest):// int specializations (column-rule-width & row-rule-width)
Sam Davis Omekara . unresolved

Guessing StyleColor specialization doesn't need the other `Maybe*` funcs

Line 171, Patchset 10 (Latest):CSSGapDataListInterpolationType<StyleColor>::GetGapDataListForProperty(
Sam Davis Omekara . unresolved

Question: Should this be called `GetDataListForProperty` or just `GetProperty`, I ask because this isn't really getting the underlying data list and right after this is called we then call `GetGapDataList()` on the returned val.

So maybe `GetProperty()` or keep as `GetDataListForProperty()` but it returns `property.GetDataList`

Line 165, Patchset 10 (Latest): result.push_back(gap_data.GetValue());
Sam Davis Omekara . unresolved

Would also add a `TODO` that says this is expected to fail for repeaters for now.

Line 157, Patchset 10 (Latest): if (property.PropertyID() == CSSPropertyID::kColumnRuleColor) {
Sam Davis Omekara . unresolved

nit (if you want), but block could be neater as per:

```
rule_color = property.PropertyID() == CSSPropertyID::kColumnRuleColor ?
style.ColumnRuleColor() : style.RowRuleColor();
for (const auto& gap_data : rule_color.GetGapDataList()) {
result.push_back(gap_data.GetValue());
}

```

File third_party/blink/renderer/core/animation/interpolation_types_map.cc
Line 173, Patchset 10 (Latest): applicable_types->push_back(
Sam Davis Omekara . unresolved

nit (if you want): Instead of breaking switch cases into `CSSLength*` -> `CSSGap*` -> `CSSLength*)`, we can just pull out the Rule width properties and have them at the npttp, similar to aspect ratio, grid ...

File third_party/blink/web_tests/external/wpt/css/css-gaps/animation/webkit-column-rule-width-interpolation.html
File-level comment, Patchset 10 (Latest):
Sam Davis Omekara . unresolved

I wonder if we'd want to keep this duplicated in both places rather than moving to gap decor folder ... Not sure though

File third_party/blink/web_tests/external/wpt/css/css-multicol/animation/column-rule-width-interpolation.html
Line 55, Patchset 9 (Parent): {at: 0, expect: '0px'},
Javier Contreras . unresolved

this feels like this test case was wrong to begin with. If it is inheriting from the parent, why would it start out at 0px rather than at 30px, like the parent does?

Sam Davis Omekara

I ran into this with a patch I had but I don't think we should run into this for this change. Let's sync offline about this...

https://chromium-review.googlesource.com/c/chromium/src/+/6490963/6/third_party/blink/web_tests/external/wpt/css/css-multicol/animation/column-rule-width-interpolation.html

But essentially it should start from 0px because although it inherited from the parent, the computed value should be 0px because style is none. That logic shouldn't change in this change.

Line 55, Patchset 9 (Parent): {at: 0, expect: '0px'},
Javier Contreras . unresolved

this feels like this test case was wrong to begin with. If it is inheriting from the parent, why would it start out at 0px rather than at 30px, like the parent does?

Sam Davis Omekara

This is because of the 0px when none behavior (*I think*)

Open in Gerrit

Related details

Attention is currently required from:
  • Javier Contreras
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
Gerrit-Change-Number: 6643003
Gerrit-PatchSet: 10
Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
Gerrit-CC: Alexis Menard <alexis...@intel.com>
Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
Gerrit-Attention: Javier Contreras <javi...@microsoft.com>
Gerrit-Comment-Date: Thu, 19 Jun 2025 08:55:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Javier Contreras <javi...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Javier Contreras (Gerrit)

unread,
Jun 20, 2025, 6:20:49 PMJun 20
to AyeAye Python Dispatcher, Sam Davis Omekara, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Sam Davis Omekara

Javier Contreras added 7 comments

File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
Line 401, Patchset 10:using CSSColumnRuleColorInterpolationType =
Sam Davis Omekara . resolved

The typedefs used here seem repeated for both Column/Row. Is it possible to have just two rather than four (i.e. `CSSRuleColorInterpolationType` && `CSSRuleWidthInterpolationType`).

Javier Contreras

I ended up not using them anywhere so just removing.

Line 199, Patchset 10:// int specializations (column-rule-width & row-rule-width)
Sam Davis Omekara . resolved

Guessing StyleColor specialization doesn't need the other `Maybe*` funcs

Javier Contreras

It does, they're just not implemented in this patch.

Line 171, Patchset 10:CSSGapDataListInterpolationType<StyleColor>::GetGapDataListForProperty(
Sam Davis Omekara . resolved

Question: Should this be called `GetDataListForProperty` or just `GetProperty`, I ask because this isn't really getting the underlying data list and right after this is called we then call `GetGapDataList()` on the returned val.

So maybe `GetProperty()` or keep as `GetDataListForProperty()` but it returns `property.GetDataList`

Javier Contreras

Done

Line 165, Patchset 10: result.push_back(gap_data.GetValue());
Sam Davis Omekara . resolved

Would also add a `TODO` that says this is expected to fail for repeaters for now.

Javier Contreras

Done

Line 157, Patchset 10: if (property.PropertyID() == CSSPropertyID::kColumnRuleColor) {
Sam Davis Omekara . resolved

nit (if you want), but block could be neater as per:

```
rule_color = property.PropertyID() == CSSPropertyID::kColumnRuleColor ?
style.ColumnRuleColor() : style.RowRuleColor();
for (const auto& gap_data : rule_color.GetGapDataList()) {
result.push_back(gap_data.GetValue());
}

```

Javier Contreras

Done

File third_party/blink/renderer/core/animation/interpolation_types_map.cc
Line 173, Patchset 10: applicable_types->push_back(
Sam Davis Omekara . resolved

nit (if you want): Instead of breaking switch cases into `CSSLength*` -> `CSSGap*` -> `CSSLength*)`, we can just pull out the Rule width properties and have them at the npttp, similar to aspect ratio, grid ...

Javier Contreras

yes I was gonna change this but forgot to, thanks for reminder.

File third_party/blink/web_tests/external/wpt/css/css-gaps/animation/webkit-column-rule-width-interpolation.html
Sam Davis Omekara . resolved

I wonder if we'd want to keep this duplicated in both places rather than moving to gap decor folder ... Not sure though

Javier Contreras

I hadn't realized, but these aren't WPT because they make use of a resources file that is only on chromium, so I added my tests to this same folder where the existing tests were before. Otherwise they don't run correctly

Open in Gerrit

Related details

Attention is currently required from:
  • Sam Davis Omekara
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
Gerrit-Change-Number: 6643003
Gerrit-PatchSet: 17
Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
Gerrit-CC: Alexis Menard <alexis...@intel.com>
Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>
Gerrit-Comment-Date: Fri, 20 Jun 2025 22:20:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sam Davis Omekara <samome...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Javier Contreras (Gerrit)

unread,
Jun 23, 2025, 11:28:10 AMJun 23
to AyeAye Python Dispatcher, Sam Davis Omekara, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Sam Davis Omekara

Javier Contreras added 1 comment

File third_party/blink/web_tests/external/wpt/css/css-multicol/animation/column-rule-width-interpolation.html
Line 55, Patchset 9 (Parent): {at: 0, expect: '0px'},
Javier Contreras . unresolved

this feels like this test case was wrong to begin with. If it is inheriting from the parent, why would it start out at 0px rather than at 30px, like the parent does?

Sam Davis Omekara

I ran into this with a patch I had but I don't think we should run into this for this change. Let's sync offline about this...

https://chromium-review.googlesource.com/c/chromium/src/+/6490963/6/third_party/blink/web_tests/external/wpt/css/css-multicol/animation/column-rule-width-interpolation.html

But essentially it should start from 0px because although it inherited from the parent, the computed value should be 0px because style is none. That logic shouldn't change in this change.

Javier Contreras

I did some searching and it does seem like the test was previously wrong, and the value at time `0` should be the parent's value, here are some examples, but there are many more:

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/motion/animation/offset-position-interpolation.html;l=93

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/css-position/animations/right-interpolation.html;l=51

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/css-fonts/animations/font-variation-settings-interpolation.html;l=63

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/css-position/animations/top-interpolation.html;l=56

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/css-grid/animation/grid-template-columns-interpolation.html;l=170

Open in Gerrit

Related details

Attention is currently required from:
  • Sam Davis Omekara
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
Gerrit-Change-Number: 6643003
Gerrit-PatchSet: 17
Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
Gerrit-CC: Alexis Menard <alexis...@intel.com>
Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>
Gerrit-Comment-Date: Mon, 23 Jun 2025 15:27:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Javier Contreras <javi...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Sam Davis Omekara (Gerrit)

unread,
Jun 24, 2025, 1:06:53 PMJun 24
to Javier Contreras, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Javier Contreras

Sam Davis Omekara voted and added 1 comment

Votes added by Sam Davis Omekara

Code-Review+1

1 comment

File third_party/blink/web_tests/external/wpt/css/css-multicol/animation/column-rule-width-interpolation.html
Line 55, Patchset 9 (Parent): {at: 0, expect: '0px'},
Javier Contreras . resolved

this feels like this test case was wrong to begin with. If it is inheriting from the parent, why would it start out at 0px rather than at 30px, like the parent does?

Sam Davis Omekara

I ran into this with a patch I had but I don't think we should run into this for this change. Let's sync offline about this...

https://chromium-review.googlesource.com/c/chromium/src/+/6490963/6/third_party/blink/web_tests/external/wpt/css/css-multicol/animation/column-rule-width-interpolation.html

But essentially it should start from 0px because although it inherited from the parent, the computed value should be 0px because style is none. That logic shouldn't change in this change.

Javier Contreras

I did some searching and it does seem like the test was previously wrong, and the value at time `0` should be the parent's value, here are some examples, but there are many more:

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/motion/animation/offset-position-interpolation.html;l=93

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/css-position/animations/right-interpolation.html;l=51

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/css-fonts/animations/font-variation-settings-interpolation.html;l=63

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/css-position/animations/top-interpolation.html;l=56

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/css-grid/animation/grid-template-columns-interpolation.html;l=170

Sam Davis Omekara

As discussed offline, the examples you've listed don't have the zero if style is hidden behavior. But we can debug to figure out why we're not respecting that restriction in this change.

Open in Gerrit

Related details

Attention is currently required from:
  • Javier Contreras
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
    Gerrit-Change-Number: 6643003
    Gerrit-PatchSet: 17
    Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
    Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
    Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
    Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-Attention: Javier Contreras <javi...@microsoft.com>
    Gerrit-Comment-Date: Tue, 24 Jun 2025 17:06:43 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Kevin Babbitt (Gerrit)

    unread,
    Jun 24, 2025, 6:58:00 PMJun 24
    to Javier Contreras, Sam Davis Omekara, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
    Attention needed from Javier Contreras and Sam Davis Omekara

    Kevin Babbitt added 2 comments

    File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
    Line 32, Patchset 18 (Latest): CHECK(property_id_ == CSSPropertyID::kColumnRuleColor ||
    property_id_ == CSSPropertyID::kColumnRuleWidth ||
    property_id_ == CSSPropertyID::kRowRuleColor ||
    property_id_ == CSSPropertyID::kRowRuleWidth);
    Kevin Babbitt . unresolved

    What about `*-rule-style`? Those should be animatable as discrete values, similar to `border-*-style` or `outline-style`.

    File third_party/blink/renderer/core/css/css_properties.json5
    Line 6602, Patchset 18 (Latest): // TODO(crbug.com/357648037): Turn this on when implementing interpolation for Gap Decorations.
    Kevin Babbitt . unresolved

    Since we're flipping to `true` should we delete this TODO?

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Javier Contreras
    • Sam Davis Omekara
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Owners
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
      Gerrit-Change-Number: 6643003
      Gerrit-PatchSet: 18
      Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
      Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
      Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
      Gerrit-CC: Alexis Menard <alexis...@intel.com>
      Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
      Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>
      Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
      Gerrit-Attention: Javier Contreras <javi...@microsoft.com>
      Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>
      Gerrit-Comment-Date: Tue, 24 Jun 2025 22:57:49 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Sam Davis Omekara (Gerrit)

      unread,
      Jun 24, 2025, 7:04:19 PMJun 24
      to Javier Contreras, Kevin Babbitt, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
      Attention needed from Javier Contreras and Kevin Babbitt

      Sam Davis Omekara added 1 comment

      File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
      Line 32, Patchset 18 (Latest): CHECK(property_id_ == CSSPropertyID::kColumnRuleColor ||
      property_id_ == CSSPropertyID::kColumnRuleWidth ||
      property_id_ == CSSPropertyID::kRowRuleColor ||
      property_id_ == CSSPropertyID::kRowRuleWidth);
      Kevin Babbitt . unresolved

      What about `*-rule-style`? Those should be animatable as discrete values, similar to `border-*-style` or `outline-style`.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Javier Contreras
      • Kevin Babbitt
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Owners
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
      Gerrit-Change-Number: 6643003
      Gerrit-PatchSet: 18
      Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
      Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
      Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
      Gerrit-CC: Alexis Menard <alexis...@intel.com>
      Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
      Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>
      Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
      Gerrit-Attention: Kevin Babbitt <kbab...@microsoft.com>
      Gerrit-Attention: Javier Contreras <javi...@microsoft.com>
      Gerrit-Comment-Date: Tue, 24 Jun 2025 23:04:08 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      Comment-In-Reply-To: Kevin Babbitt <kbab...@microsoft.com>
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Javier Contreras (Gerrit)

      unread,
      Jun 25, 2025, 11:35:10 AMJun 25
      to Kevin Babbitt, Sam Davis Omekara, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
      Attention needed from Kevin Babbitt and Sam Davis Omekara

      Javier Contreras added 2 comments

      File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
      Line 32, Patchset 18: CHECK(property_id_ == CSSPropertyID::kColumnRuleColor ||

      property_id_ == CSSPropertyID::kColumnRuleWidth ||
      property_id_ == CSSPropertyID::kRowRuleColor ||
      property_id_ == CSSPropertyID::kRowRuleWidth);
      Kevin Babbitt . unresolved

      What about `*-rule-style`? Those should be animatable as discrete values, similar to `border-*-style` or `outline-style`.

      Sam Davis Omekara

      Not 100% sure but the style properties in chromium don't seem to have the interpolable field.

      `border`: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_properties.json5;l=2551?q=css_properties.json5&ss=chromium

      `outline`: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_properties.json5;l=4409?q=css_properties.json5&ss=chromium

      `column-rule`:
      https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_properties.json5;l=6571?q=css_properties.json5&ss=chromium

      In mdn[1], it states these should have discrete animation values, but unsure if they are implemented in blink.

      [1]: https://developer.mozilla.org/en-US/docs/Web/CSS/border-style

      File third_party/blink/renderer/core/css/css_properties.json5
      Line 6602, Patchset 18: // TODO(crbug.com/357648037): Turn this on when implementing interpolation for Gap Decorations.
      Kevin Babbitt . resolved

      Since we're flipping to `true` should we delete this TODO?

      Javier Contreras

      Done

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Kevin Babbitt
      • Sam Davis Omekara
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Owners
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
      Gerrit-Change-Number: 6643003
      Gerrit-PatchSet: 19
      Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
      Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
      Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
      Gerrit-CC: Alexis Menard <alexis...@intel.com>
      Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
      Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>
      Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
      Gerrit-Attention: Kevin Babbitt <kbab...@microsoft.com>
      Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>
      Gerrit-Comment-Date: Wed, 25 Jun 2025 15:34:57 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      Comment-In-Reply-To: Kevin Babbitt <kbab...@microsoft.com>
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Sam Davis Omekara (Gerrit)

      unread,
      Jun 25, 2025, 12:20:37 PMJun 25
      to Javier Contreras, Kevin Babbitt, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
      Attention needed from Javier Contreras and Kevin Babbitt

      Sam Davis Omekara voted Code-Review+1

      Code-Review+1
      Open in Gerrit

      Related details

      Attention is currently required from:
      • Javier Contreras
      • Kevin Babbitt
      Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement is not satisfiedCode-Owners
        • requirement satisfiedCode-Review
        • requirement is not satisfiedNo-Unresolved-Comments
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
        Gerrit-Change-Number: 6643003
        Gerrit-PatchSet: 19
        Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
        Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
        Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
        Gerrit-CC: Alexis Menard <alexis...@intel.com>
        Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
        Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>
        Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
        Gerrit-Attention: Kevin Babbitt <kbab...@microsoft.com>
        Gerrit-Attention: Javier Contreras <javi...@microsoft.com>
        Gerrit-Comment-Date: Wed, 25 Jun 2025 16:20:27 +0000
        Gerrit-HasComments: No
        Gerrit-Has-Labels: Yes
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Kevin Babbitt (Gerrit)

        unread,
        Jun 25, 2025, 1:57:46 PMJun 25
        to Javier Contreras, Alison Maher, Sam Davis Omekara, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
        Attention needed from Alison Maher and Javier Contreras

        Kevin Babbitt voted and added 1 comment

        Votes added by Kevin Babbitt

        Code-Review+1

        1 comment

        File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
        Line 32, Patchset 18: CHECK(property_id_ == CSSPropertyID::kColumnRuleColor ||
        property_id_ == CSSPropertyID::kColumnRuleWidth ||
        property_id_ == CSSPropertyID::kRowRuleColor ||
        property_id_ == CSSPropertyID::kRowRuleWidth);
        Kevin Babbitt . unresolved

        What about `*-rule-style`? Those should be animatable as discrete values, similar to `border-*-style` or `outline-style`.

        Sam Davis Omekara

        Not 100% sure but the style properties in chromium don't seem to have the interpolable field.

        `border`: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_properties.json5;l=2551?q=css_properties.json5&ss=chromium

        `outline`: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_properties.json5;l=4409?q=css_properties.json5&ss=chromium

        `column-rule`:
        https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_properties.json5;l=6571?q=css_properties.json5&ss=chromium

        In mdn[1], it states these should have discrete animation values, but unsure if they are implemented in blink.

        [1]: https://developer.mozilla.org/en-US/docs/Web/CSS/border-style

        Javier Contreras

        Moreover, none of those properties have a corresponding interpolation type [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/interpolation_types_map.cc;drc=a5302fd8428c0287f753b2503b2f06d4550ab307;l=91)

        Kevin Babbitt

        Oh that's interesting, maybe properties that animate as discrete values don't need interpolation logic.

        Can we add WPT coverage validating that `*-rule-style` animates with the correct behavior? I'm okay with doing that in a separate CL, as long as we aren't running into this `CHECK()` when an author specifies an animation on `*-rule-style`.

        Open in Gerrit

        Related details

        Attention is currently required from:
        • Alison Maher
        • Javier Contreras
        Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement is not satisfiedCode-Owners
        • requirement satisfiedCode-Review
        • requirement is not satisfiedNo-Unresolved-Comments
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
        Gerrit-Change-Number: 6643003
        Gerrit-PatchSet: 22
        Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
        Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
        Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
        Gerrit-Reviewer: Kevin Babbitt <kbab...@microsoft.com>
        Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
        Gerrit-CC: Alexis Menard <alexis...@intel.com>
        Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
        Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
        Gerrit-Attention: Javier Contreras <javi...@microsoft.com>
        Gerrit-Attention: Alison Maher <alm...@microsoft.com>
        Gerrit-Comment-Date: Wed, 25 Jun 2025 17:57:36 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: Yes
        Comment-In-Reply-To: Kevin Babbitt <kbab...@microsoft.com>
        Comment-In-Reply-To: Javier Contreras <javi...@microsoft.com>
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Javier Contreras (Gerrit)

        unread,
        Jun 25, 2025, 3:36:09 PMJun 25
        to Kevin Babbitt, Alison Maher, Sam Davis Omekara, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
        Attention needed from Alison Maher

        Javier Contreras added 1 comment

        File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
        Line 32, Patchset 18: CHECK(property_id_ == CSSPropertyID::kColumnRuleColor ||
        property_id_ == CSSPropertyID::kColumnRuleWidth ||
        property_id_ == CSSPropertyID::kRowRuleColor ||
        property_id_ == CSSPropertyID::kRowRuleWidth);
        Kevin Babbitt . resolved

        What about `*-rule-style`? Those should be animatable as discrete values, similar to `border-*-style` or `outline-style`.

        Sam Davis Omekara

        Not 100% sure but the style properties in chromium don't seem to have the interpolable field.

        `border`: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_properties.json5;l=2551?q=css_properties.json5&ss=chromium

        `outline`: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_properties.json5;l=4409?q=css_properties.json5&ss=chromium

        `column-rule`:
        https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_properties.json5;l=6571?q=css_properties.json5&ss=chromium

        In mdn[1], it states these should have discrete animation values, but unsure if they are implemented in blink.

        [1]: https://developer.mozilla.org/en-US/docs/Web/CSS/border-style

        Javier Contreras

        Moreover, none of those properties have a corresponding interpolation type [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/interpolation_types_map.cc;drc=a5302fd8428c0287f753b2503b2f06d4550ab307;l=91)

        Kevin Babbitt

        Oh that's interesting, maybe properties that animate as discrete values don't need interpolation logic.

        Can we add WPT coverage validating that `*-rule-style` animates with the correct behavior? I'm okay with doing that in a separate CL, as long as we aren't running into this `CHECK()` when an author specifies an animation on `*-rule-style`.

        Javier Contreras

        Will do. And yeah, this path will only be hit by the properties specified right now, since thats how I've set it up in the file I linked earlier

        Open in Gerrit

        Related details

        Attention is currently required from:
        • Alison Maher
        Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement is not satisfiedCode-Owners
        • requirement satisfiedCode-Review
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
        Gerrit-Change-Number: 6643003
        Gerrit-PatchSet: 23
        Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
        Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
        Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
        Gerrit-Reviewer: Kevin Babbitt <kbab...@microsoft.com>
        Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
        Gerrit-CC: Alexis Menard <alexis...@intel.com>
        Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
        Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
        Gerrit-Attention: Alison Maher <alm...@microsoft.com>
        Gerrit-Comment-Date: Wed, 25 Jun 2025 19:35:58 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: No
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Alison Maher (Gerrit)

        unread,
        Jun 26, 2025, 12:28:44 PMJun 26
        to Javier Contreras, Robert Flack, Kevin Babbitt, Sam Davis Omekara, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
        Attention needed from Javier Contreras and Robert Flack

        Alison Maher added 12 comments

        Patchset-level comments
        File-level comment, Patchset 24 (Latest):
        Alison Maher . resolved

        Overall, this looks good. Added a few suggestions. +flackr@ for review, as well, who is an owner in the animation directory

        Commit Message
        Line 23, Patchset 24 (Latest):`GapDataList` via templates, one for <int> one for <StyleColor>. We then
        Alison Maher . unresolved

        nit: "and"

        File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
        Line 295, Patchset 24 (Latest): initial_list[index], CssProperty(), 1,
        Alison Maher . unresolved

        nit: This should have a comment to note the var name

        Line 254, Patchset 24 (Latest): // GridTemplatePropertyInterpolationType::MaybeMergeSingles
        Alison Maher . unresolved

        nit: missing punctuation

        Line 252, Patchset 24 (Latest): // Once we can handle repeaters maybe would have to have a condition here to
        Alison Maher . unresolved

        I'd make this a TODO instead

        Line 242, Patchset 24 (Latest): } else if (CssProperty().PropertyID() == CSSPropertyID::kRowRuleWidth) {
        Alison Maher . unresolved

        Should this be just an else with a CHECK that it equals row-rule-width instead, or do we expect any others will need to use this method?

        Line 186, Patchset 24 (Latest): const auto& vals = list.GetGapDataList();
        Alison Maher . unresolved

        nit: I'd call this values instead of vals

        Line 183, Patchset 24 (Latest): int>::MaybeConvertStandardPropertyUnderlyingValue(const ComputedStyle&
        style) const {
        Alison Maher . unresolved

        This feels like it should be re-wrapped in some way

        Line 111, Patchset 24 (Latest): CHECK(initial_list.HasSingleValue());
        Alison Maher . unresolved

        Is this always going to be true? (i.e. if it is a repeater, would that just return the first one essentially?

        File third_party/blink/renderer/core/animation/length_list_property_functions.cc
        Line 92, Patchset 24 (Latest): case CSSPropertyID::kColumnRuleWidth:
        case CSSPropertyID::kRowRuleWidth:
        Alison Maher . unresolved

        Out of curiosity, why do we need this? And do we only need it when the feature flag is enabled?

        File third_party/blink/web_tests/animations/interpolation/row-rule-width-interpolation.html
        Line 9, Patchset 24 (Latest): display: flex;
        Alison Maher . unresolved

        Would it make sense to add tests for grid and multicol, too, just to make sure these are working as expected everywhere?

        Line 42, Patchset 24 (Latest): from: 'initial',
        Alison Maher . unresolved

        I'd suggest adding some cases for the keywords we accept, as well (ex. thin)

        Open in Gerrit

        Related details

        Attention is currently required from:
        • Javier Contreras
        • Robert Flack
        Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement is not satisfiedCode-Owners
          • requirement satisfiedCode-Review
          • requirement is not satisfiedNo-Unresolved-Comments
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: comment
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
          Gerrit-Change-Number: 6643003
          Gerrit-PatchSet: 24
          Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
          Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
          Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
          Gerrit-Reviewer: Kevin Babbitt <kbab...@microsoft.com>
          Gerrit-Reviewer: Robert Flack <fla...@chromium.org>
          Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
          Gerrit-CC: Alexis Menard <alexis...@intel.com>
          Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
          Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
          Gerrit-Attention: Javier Contreras <javi...@microsoft.com>
          Gerrit-Attention: Robert Flack <fla...@chromium.org>
          Gerrit-Comment-Date: Thu, 26 Jun 2025 16:28:32 +0000
          Gerrit-HasComments: Yes
          Gerrit-Has-Labels: No
          satisfied_requirement
          unsatisfied_requirement
          open
          diffy

          Javier Contreras (Gerrit)

          unread,
          Jun 26, 2025, 1:35:58 PMJun 26
          to Robert Flack, Kevin Babbitt, Alison Maher, Sam Davis Omekara, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
          Attention needed from Alison Maher and Robert Flack

          Javier Contreras added 11 comments

          Commit Message
          Line 23, Patchset 24:`GapDataList` via templates, one for <int> one for <StyleColor>. We then
          Alison Maher . resolved

          nit: "and"

          Javier Contreras

          Done

          File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
          Line 295, Patchset 24: initial_list[index], CssProperty(), 1,
          Alison Maher . resolved

          nit: This should have a comment to note the var name

          Javier Contreras

          Done

          Line 254, Patchset 24: // GridTemplatePropertyInterpolationType::MaybeMergeSingles
          Alison Maher . resolved

          nit: missing punctuation

          Javier Contreras

          Done

          Line 252, Patchset 24: // Once we can handle repeaters maybe would have to have a condition here to
          Alison Maher . resolved

          I'd make this a TODO instead

          Javier Contreras

          Done

          Line 242, Patchset 24: } else if (CssProperty().PropertyID() == CSSPropertyID::kRowRuleWidth) {
          Alison Maher . resolved

          Should this be just an else with a CHECK that it equals row-rule-width instead, or do we expect any others will need to use this method?

          Javier Contreras

          Done

          Line 186, Patchset 24: const auto& vals = list.GetGapDataList();
          Alison Maher . resolved

          nit: I'd call this values instead of vals

          Javier Contreras

          Done

          Line 183, Patchset 24: int>::MaybeConvertStandardPropertyUnderlyingValue(const ComputedStyle&
          style) const {
          Alison Maher . resolved

          This feels like it should be re-wrapped in some way

          Javier Contreras

          I've tried, but the formatter keeps wrapping it like this.

          Line 111, Patchset 24: CHECK(initial_list.HasSingleValue());
          Alison Maher . resolved

          Is this always going to be true? (i.e. if it is a repeater, would that just return the first one essentially?

          Javier Contreras

          I believe this will always be true because the `initial` value for both ColumnRuleWidth and RowRuleWidth will never be a repeater, unless I am missing something.

          File third_party/blink/renderer/core/animation/length_list_property_functions.cc
          Line 92, Patchset 24: case CSSPropertyID::kColumnRuleWidth:
          case CSSPropertyID::kRowRuleWidth:
          Alison Maher . resolved

          Out of curiosity, why do we need this? And do we only need it when the feature flag is enabled?

          Javier Contreras

          We need it for when we call `CreateLength` to convert from a `InterpolableLength` to a `Length` in `ApplyStandardPropertyValue`. The flag state doesn't really matter since its not something that is called for those properties outside of this change.

          File third_party/blink/web_tests/animations/interpolation/row-rule-width-interpolation.html
          Line 9, Patchset 24: display: flex;
          Alison Maher . unresolved

          Would it make sense to add tests for grid and multicol, too, just to make sure these are working as expected everywhere?

          Javier Contreras

          It seems to me like that would be a bit unnecessary, since the animation pipeline is completely separate from any sort of container logic, so its completely agnostic to the container type. It seems unnecessary to me to add layout-related tests on a style/animation change. But if you feel strongly about it I can add a grid test.

          There's already [existing](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/animations/interpolation/webkit-column-rule-width-interpolation.html;l=6?q=webkit-column-rule-width&sq=&ss=chromium%2Fchromium%2Fsrc) tests for this in multicol.

          Line 42, Patchset 24: from: 'initial',
          Alison Maher . resolved

          I'd suggest adding some cases for the keywords we accept, as well (ex. thin)

          Javier Contreras

          Done

          Open in Gerrit

          Related details

          Attention is currently required from:
          • Alison Maher
          • Robert Flack
          Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement is not satisfiedCode-Owners
          • requirement satisfiedCode-Review
          • requirement is not satisfiedNo-Unresolved-Comments
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: comment
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
          Gerrit-Change-Number: 6643003
          Gerrit-PatchSet: 26
          Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
          Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
          Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
          Gerrit-Reviewer: Kevin Babbitt <kbab...@microsoft.com>
          Gerrit-Reviewer: Robert Flack <fla...@chromium.org>
          Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
          Gerrit-CC: Alexis Menard <alexis...@intel.com>
          Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
          Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
          Gerrit-Attention: Robert Flack <fla...@chromium.org>
          Gerrit-Attention: Alison Maher <alm...@microsoft.com>
          Gerrit-Comment-Date: Thu, 26 Jun 2025 17:35:45 +0000
          Gerrit-HasComments: Yes
          Gerrit-Has-Labels: No
          Comment-In-Reply-To: Alison Maher <alm...@microsoft.com>
          satisfied_requirement
          unsatisfied_requirement
          open
          diffy

          Alison Maher (Gerrit)

          unread,
          Jun 26, 2025, 1:49:33 PMJun 26
          to Javier Contreras, Robert Flack, Kevin Babbitt, Sam Davis Omekara, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
          Attention needed from Javier Contreras and Robert Flack

          Alison Maher added 6 comments

          File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
          Line 296, Patchset 26 (Latest): initial_list[index], CssProperty(), /* zoom */ 1,
          Alison Maher . unresolved

          `/*zoom=*/1`

          Line 253, Patchset 26 (Latest): // TODO: Once we can handle repeaters maybe would have to have a condition
          Alison Maher . unresolved

          I'd make this a TODO(javiercon) or associate it with the bug

          Line 243, Patchset 26 (Latest): CHECK(CssProperty().PropertyID() == CSSPropertyID::kRowRuleWidth);
          Alison Maher . unresolved

          nit: CHECK_EQ

          Line 111, Patchset 24: CHECK(initial_list.HasSingleValue());
          Alison Maher . resolved

          Is this always going to be true? (i.e. if it is a repeater, would that just return the first one essentially?

          Javier Contreras

          I believe this will always be true because the `initial` value for both ColumnRuleWidth and RowRuleWidth will never be a repeater, unless I am missing something.

          Alison Maher

          Ok interesting, I guess what was confusing to me was that we are calling GetProperty() to get the initial value (which I thought maybe meant it was the initial style for the animation as opposed to what you would get with the initial keyword).

          File third_party/blink/renderer/core/animation/length_list_property_functions.cc
          Line 92, Patchset 24: case CSSPropertyID::kColumnRuleWidth:
          case CSSPropertyID::kRowRuleWidth:
          Alison Maher . unresolved

          Out of curiosity, why do we need this? And do we only need it when the feature flag is enabled?

          Javier Contreras

          We need it for when we call `CreateLength` to convert from a `InterpolableLength` to a `Length` in `ApplyStandardPropertyValue`. The flag state doesn't really matter since its not something that is called for those properties outside of this change.

          Alison Maher

          The flag state doesn't really matter since its not something that is called for those properties outside of this change.

          What do you mean by this? Are you saying we don't ever get here if the flag is disabled?

          File third_party/blink/web_tests/animations/interpolation/row-rule-width-interpolation.html
          Line 9, Patchset 24: display: flex;
          Alison Maher . unresolved

          Would it make sense to add tests for grid and multicol, too, just to make sure these are working as expected everywhere?

          Javier Contreras

          It seems to me like that would be a bit unnecessary, since the animation pipeline is completely separate from any sort of container logic, so its completely agnostic to the container type. It seems unnecessary to me to add layout-related tests on a style/animation change. But if you feel strongly about it I can add a grid test.

          There's already [existing](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/animations/interpolation/webkit-column-rule-width-interpolation.html;l=6?q=webkit-column-rule-width&sq=&ss=chromium%2Fchromium%2Fsrc) tests for this in multicol.

          Alison Maher

          I think it makes sense to add some for grid, then. Even though it shouldn't matter, it is generally safer to have tests for those sorts of combinations in case something breaks in the future in one and not the other for some reason (and hopefully should be fairly straightforward to add)

          Open in Gerrit

          Related details

          Attention is currently required from:
          • Javier Contreras
          • Robert Flack
          Gerrit-Attention: Javier Contreras <javi...@microsoft.com>
          Gerrit-Attention: Robert Flack <fla...@chromium.org>
          Gerrit-Comment-Date: Thu, 26 Jun 2025 17:49:21 +0000
          Gerrit-HasComments: Yes
          Gerrit-Has-Labels: No
          Comment-In-Reply-To: Javier Contreras <javi...@microsoft.com>
          Comment-In-Reply-To: Alison Maher <alm...@microsoft.com>
          satisfied_requirement
          unsatisfied_requirement
          open
          diffy

          Javier Contreras (Gerrit)

          unread,
          Jun 26, 2025, 2:21:57 PMJun 26
          to Robert Flack, Kevin Babbitt, Alison Maher, Sam Davis Omekara, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
          Attention needed from Alison Maher, Kevin Babbitt, Robert Flack and Sam Davis Omekara

          Javier Contreras added 5 comments

          File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
          Line 296, Patchset 26: initial_list[index], CssProperty(), /* zoom */ 1,
          Alison Maher . resolved

          `/*zoom=*/1`

          Javier Contreras

          Done

          Line 253, Patchset 26: // TODO: Once we can handle repeaters maybe would have to have a condition
          Alison Maher . resolved

          I'd make this a TODO(javiercon) or associate it with the bug

          Javier Contreras

          Done

          Line 243, Patchset 26: CHECK(CssProperty().PropertyID() == CSSPropertyID::kRowRuleWidth);
          Alison Maher . resolved

          nit: CHECK_EQ

          Javier Contreras

          Done

          File third_party/blink/renderer/core/animation/length_list_property_functions.cc
          Line 92, Patchset 24: case CSSPropertyID::kColumnRuleWidth:
          case CSSPropertyID::kRowRuleWidth:
          Alison Maher . resolved

          Out of curiosity, why do we need this? And do we only need it when the feature flag is enabled?

          Javier Contreras

          We need it for when we call `CreateLength` to convert from a `InterpolableLength` to a `Length` in `ApplyStandardPropertyValue`. The flag state doesn't really matter since its not something that is called for those properties outside of this change.

          Alison Maher

          The flag state doesn't really matter since its not something that is called for those properties outside of this change.

          What do you mean by this? Are you saying we don't ever get here if the flag is disabled?

          Javier Contreras

          Yes correct. With the flag disabled we don't go through this path for column/row-rule-width interpolation.

          File third_party/blink/web_tests/animations/interpolation/row-rule-width-interpolation.html
          Line 9, Patchset 24: display: flex;
          Alison Maher . resolved

          Would it make sense to add tests for grid and multicol, too, just to make sure these are working as expected everywhere?

          Javier Contreras

          It seems to me like that would be a bit unnecessary, since the animation pipeline is completely separate from any sort of container logic, so its completely agnostic to the container type. It seems unnecessary to me to add layout-related tests on a style/animation change. But if you feel strongly about it I can add a grid test.

          There's already [existing](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/animations/interpolation/webkit-column-rule-width-interpolation.html;l=6?q=webkit-column-rule-width&sq=&ss=chromium%2Fchromium%2Fsrc) tests for this in multicol.

          Alison Maher

          I think it makes sense to add some for grid, then. Even though it shouldn't matter, it is generally safer to have tests for those sorts of combinations in case something breaks in the future in one and not the other for some reason (and hopefully should be fairly straightforward to add)

          Javier Contreras

          Done

          Open in Gerrit

          Related details

          Attention is currently required from:
          • Alison Maher
          • Kevin Babbitt
          • Robert Flack
          • Sam Davis Omekara
          Submit Requirements:
            • requirement satisfiedCode-Coverage
            • requirement is not satisfiedCode-Owners
            • requirement is not satisfiedCode-Review
            Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
            Gerrit-MessageType: comment
            Gerrit-Project: chromium/src
            Gerrit-Branch: main
            Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
            Gerrit-Change-Number: 6643003
            Gerrit-PatchSet: 27
            Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
            Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
            Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
            Gerrit-Reviewer: Kevin Babbitt <kbab...@microsoft.com>
            Gerrit-Reviewer: Robert Flack <fla...@chromium.org>
            Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
            Gerrit-CC: Alexis Menard <alexis...@intel.com>
            Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
            Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
            Gerrit-Attention: Kevin Babbitt <kbab...@microsoft.com>
            Gerrit-Attention: Robert Flack <fla...@chromium.org>
            Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>
            Gerrit-Attention: Alison Maher <alm...@microsoft.com>
            Gerrit-Comment-Date: Thu, 26 Jun 2025 18:21:45 +0000
            satisfied_requirement
            unsatisfied_requirement
            open
            diffy

            Sam Davis Omekara (Gerrit)

            unread,
            Jun 26, 2025, 2:23:10 PMJun 26
            to Javier Contreras, Robert Flack, Kevin Babbitt, Alison Maher, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
            Attention needed from Alison Maher, Kevin Babbitt and Robert Flack

            Sam Davis Omekara voted and added 1 comment

            Votes added by Sam Davis Omekara

            Code-Review+1

            1 comment

            Patchset-level comments
            Sam Davis Omekara . resolved

            re stamping

            Open in Gerrit

            Related details

            Attention is currently required from:
            • Alison Maher
            • Kevin Babbitt
            • Robert Flack
            Submit Requirements:
              • requirement satisfiedCode-Coverage
              • requirement is not satisfiedCode-Owners
              • requirement satisfiedCode-Review
              Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
              Gerrit-MessageType: comment
              Gerrit-Project: chromium/src
              Gerrit-Branch: main
              Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
              Gerrit-Change-Number: 6643003
              Gerrit-PatchSet: 27
              Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
              Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
              Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
              Gerrit-Reviewer: Kevin Babbitt <kbab...@microsoft.com>
              Gerrit-Reviewer: Robert Flack <fla...@chromium.org>
              Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
              Gerrit-CC: Alexis Menard <alexis...@intel.com>
              Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
              Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
              Gerrit-Attention: Kevin Babbitt <kbab...@microsoft.com>
              Gerrit-Attention: Robert Flack <fla...@chromium.org>
              Gerrit-Attention: Alison Maher <alm...@microsoft.com>
              Gerrit-Comment-Date: Thu, 26 Jun 2025 18:23:00 +0000
              Gerrit-HasComments: Yes
              Gerrit-Has-Labels: Yes
              satisfied_requirement
              unsatisfied_requirement
              open
              diffy

              Robert Flack (Gerrit)

              unread,
              Jun 26, 2025, 3:58:59 PMJun 26
              to Javier Contreras, Kevin Ellis, Sam Davis Omekara, Kevin Babbitt, Alison Maher, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
              Attention needed from Alison Maher, Javier Contreras, Kevin Babbitt and Kevin Ellis

              Robert Flack added 3 comments

              File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
              Line 23, Patchset 27 (Latest):template <typename T>
              Robert Flack . unresolved

              Is this ever constructed with different types? I couldn't find any cases where int wasn't used in the code. If not, it seems like for clarity it would be best to remove the templated type. Also, it seems like if we don't need templated types then we can avoid having the entire implementation in the header which is bad for compile times.

              File third_party/blink/renderer/core/animation/interpolation_types_map.cc
              Line 201, Patchset 27 (Latest): MakeGarbageCollected<CSSGapDataListInterpolationType<int>>(
              Robert Flack . unresolved

              Do we need a custom type here? Can we not directly use CSSLengthListInterpolationType? It seems like that would significantly reduce the support code needed. Or alternately if specialized code is needed a subclass of CSSLengthListInterpolationType like CSSTransformOriginInterpolationType

              File third_party/blink/renderer/core/style/gap_data_list.h
              Line 51, Patchset 27 (Latest): gap_data_list_.emplace_back(GapData<int>(length.IntValue()));
              Robert Flack . unresolved

              Doesn't this lose the type from the lengths? Do you have tests which have mixed units in a single rule and/or different units between rules?

              Open in Gerrit

              Related details

              Attention is currently required from:
              • Alison Maher
              • Javier Contreras
              • Kevin Babbitt
              • Kevin Ellis
              Submit Requirements:
                • requirement satisfiedCode-Coverage
                • requirement is not satisfiedCode-Owners
                • requirement satisfiedCode-Review
                • requirement is not satisfiedNo-Unresolved-Comments
                Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
                Gerrit-MessageType: comment
                Gerrit-Project: chromium/src
                Gerrit-Branch: main
                Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
                Gerrit-Change-Number: 6643003
                Gerrit-PatchSet: 27
                Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
                Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
                Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
                Gerrit-Reviewer: Kevin Babbitt <kbab...@microsoft.com>
                Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
                Gerrit-Reviewer: Robert Flack <fla...@chromium.org>
                Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
                Gerrit-CC: Alexis Menard <alexis...@intel.com>
                Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
                Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
                Gerrit-Attention: Kevin Babbitt <kbab...@microsoft.com>
                Gerrit-Attention: Javier Contreras <javi...@microsoft.com>
                Gerrit-Attention: Alison Maher <alm...@microsoft.com>
                Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
                Gerrit-Comment-Date: Thu, 26 Jun 2025 19:58:53 +0000
                Gerrit-HasComments: Yes
                Gerrit-Has-Labels: No
                satisfied_requirement
                unsatisfied_requirement
                open
                diffy

                Javier Contreras (Gerrit)

                unread,
                Jun 26, 2025, 4:10:21 PMJun 26
                to Kevin Ellis, Sam Davis Omekara, Robert Flack, Kevin Babbitt, Alison Maher, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
                Attention needed from Alison Maher, Kevin Babbitt, Kevin Ellis and Robert Flack

                Javier Contreras added 3 comments

                File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
                Line 23, Patchset 27 (Latest):template <typename T>
                Robert Flack . unresolved

                Is this ever constructed with different types? I couldn't find any cases where int wasn't used in the code. If not, it seems like for clarity it would be best to remove the templated type. Also, it seems like if we don't need templated types then we can avoid having the entire implementation in the header which is bad for compile times.

                Javier Contreras

                Not in this CL, but it will be.

                This class will be used for interpolating `ColumnRuleWidth` and `ColumnRuleColor` (as well as the respective `Row` properties). This CL only handles `width` (template `int`) and a follow up CL will handle `color` (template `StyleColor`).

                This CL only handles Width for clarity and to avoid having a massive CL.

                File third_party/blink/renderer/core/animation/interpolation_types_map.cc
                Line 201, Patchset 27 (Latest): MakeGarbageCollected<CSSGapDataListInterpolationType<int>>(
                Robert Flack . unresolved

                Do we need a custom type here? Can we not directly use CSSLengthListInterpolationType? It seems like that would significantly reduce the support code needed. Or alternately if specialized code is needed a subclass of CSSLengthListInterpolationType like CSSTransformOriginInterpolationType

                Javier Contreras
                I believe that a custom type is needed, and the reason for this is that `ColumnRuleWidth` and `RowRuleWidth` have the following [possible syntax](https://drafts.csswg.org/css-gaps-1/#column-row-rule-width), which is as you say, a LengthList (e.g `1px 5px 10px`, but also could be `repeat(5, 10px 10px)` or `10px repeat(3, 5px 6px) 8px`.

                Which is why I think we need the custom type. The [follow up CL](https://chromium-review.googlesource.com/c/chromium/src/+/6655208) implements the repeater logic. It was left off this CL to keep this CL contained and not massive.
                File third_party/blink/renderer/core/style/gap_data_list.h
                Line 51, Patchset 27 (Latest): gap_data_list_.emplace_back(GapData<int>(length.IntValue()));
                Robert Flack . unresolved

                Doesn't this lose the type from the lengths? Do you have tests which have mixed units in a single rule and/or different units between rules?

                Javier Contreras

                I'm not sure if I understand your question. This constructor will only be used to create a GapDataList out of a list of Lengths (we only care about the int value of said lengths).

                Open in Gerrit

                Related details

                Attention is currently required from:
                • Alison Maher
                • Kevin Babbitt
                • Kevin Ellis
                • Robert Flack
                Gerrit-Attention: Robert Flack <fla...@chromium.org>
                Gerrit-Attention: Alison Maher <alm...@microsoft.com>
                Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
                Gerrit-Comment-Date: Thu, 26 Jun 2025 20:10:11 +0000
                Gerrit-HasComments: Yes
                Gerrit-Has-Labels: No
                Comment-In-Reply-To: Robert Flack <fla...@chromium.org>
                satisfied_requirement
                unsatisfied_requirement
                open
                diffy

                Alison Maher (Gerrit)

                unread,
                Jun 27, 2025, 12:22:12 PMJun 27
                to Javier Contreras, Kevin Ellis, Sam Davis Omekara, Robert Flack, Kevin Babbitt, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
                Attention needed from Javier Contreras, Kevin Babbitt, Kevin Ellis and Robert Flack

                Alison Maher voted and added 2 comments

                Votes added by Alison Maher

                Code-Review+1

                2 comments

                File third_party/blink/web_tests/animations/interpolation/rule-width-interpolation-multiple-values-2.html
                Alison Maher . unresolved

                nit: same here, -002.html

                File third_party/blink/web_tests/animations/interpolation/rule-width-interpolation-multiple-values.html
                Alison Maher . unresolved

                nit: I'd rename this to end in -001.html

                Open in Gerrit

                Related details

                Attention is currently required from:
                • Javier Contreras
                • Kevin Babbitt
                • Kevin Ellis
                • Robert Flack
                  Submit Requirements:
                  • requirement satisfiedCode-Coverage
                  • requirement satisfiedCode-Owners
                  • requirement satisfiedCode-Review
                  • requirement is not satisfiedNo-Unresolved-Comments
                  Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
                  Gerrit-MessageType: comment
                  Gerrit-Project: chromium/src
                  Gerrit-Branch: main
                  Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
                  Gerrit-Change-Number: 6643003
                  Gerrit-PatchSet: 27
                  Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
                  Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
                  Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
                  Gerrit-Reviewer: Kevin Babbitt <kbab...@microsoft.com>
                  Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
                  Gerrit-Reviewer: Robert Flack <fla...@chromium.org>
                  Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
                  Gerrit-CC: Alexis Menard <alexis...@intel.com>
                  Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
                  Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
                  Gerrit-Attention: Kevin Babbitt <kbab...@microsoft.com>
                  Gerrit-Attention: Javier Contreras <javi...@microsoft.com>
                  Gerrit-Attention: Robert Flack <fla...@chromium.org>
                  Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
                  Gerrit-Comment-Date: Fri, 27 Jun 2025 16:21:59 +0000
                  Gerrit-HasComments: Yes
                  Gerrit-Has-Labels: Yes
                  satisfied_requirement
                  unsatisfied_requirement
                  open
                  diffy

                  Robert Flack (Gerrit)

                  unread,
                  Jun 27, 2025, 12:22:45 PMJun 27
                  to Javier Contreras, Alison Maher, Kevin Ellis, Sam Davis Omekara, Kevin Babbitt, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
                  Attention needed from Javier Contreras, Kevin Babbitt and Kevin Ellis

                  Robert Flack added 5 comments

                  File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
                  Line 121, Patchset 27 (Latest):void GetList(const CSSProperty& property,
                  Robert Flack . unresolved

                  This is too generic of a name to be in a header file in the blink namespace when it is only specialized for a couple types, especially when it will usually result in generating code with a runtime error. Please try to move this to a corresponding implementation (.cc) file or scope the function in a way that it is not in the blink namespace (e.g. could be a static function of the class).

                  Line 61, Patchset 27 (Latest): // Should not be called, only the specialized types should be used.
                  Robert Flack . unresolved

                  This pattern seems problematic to me - shifting something that should be a compile failure to be a runtime failure. Can we not have pure virtual functions in the generic class so that it is a compile failure to use any type other than those which have specializations implementing them?

                  I.e. this class would not provide an implementation of MaybeConvertNeutral, instead you would derive subclasses which use particular types, e.g.

                  ```
                  class CORE_EXPORT CSSGapLengthListInterpolationType : public CSSGapDataListInterpolationType<int> {
                  /* Implementations of MaybeConvertNeutral and other virtual methods */
                  }
                  ```

                  Alternately, you could exclude the function body and it would be a linker error to use a type which did not have a specialization in the .cc file, however I feel like this would be less obvious.

                  Line 23, Patchset 27 (Latest):template <typename T>
                  Robert Flack . unresolved

                  Is this ever constructed with different types? I couldn't find any cases where int wasn't used in the code. If not, it seems like for clarity it would be best to remove the templated type. Also, it seems like if we don't need templated types then we can avoid having the entire implementation in the header which is bad for compile times.

                  Javier Contreras

                  Not in this CL, but it will be.

                  This class will be used for interpolating `ColumnRuleWidth` and `ColumnRuleColor` (as well as the respective `Row` properties). This CL only handles `width` (template `int`) and a follow up CL will handle `color` (template `StyleColor`).

                  This CL only handles Width for clarity and to avoid having a massive CL.

                  Robert Flack

                  Understood - though does it need to be templated? Given both lengths and colors will be interpolated as InterpolationValue maybe we could use InterpolationValue directly.

                  File third_party/blink/renderer/core/animation/interpolation_types_map.cc
                  Line 201, Patchset 27 (Latest): MakeGarbageCollected<CSSGapDataListInterpolationType<int>>(
                  Robert Flack . unresolved

                  Do we need a custom type here? Can we not directly use CSSLengthListInterpolationType? It seems like that would significantly reduce the support code needed. Or alternately if specialized code is needed a subclass of CSSLengthListInterpolationType like CSSTransformOriginInterpolationType

                  Javier Contreras
                  I believe that a custom type is needed, and the reason for this is that `ColumnRuleWidth` and `RowRuleWidth` have the following [possible syntax](https://drafts.csswg.org/css-gaps-1/#column-row-rule-width), which is as you say, a LengthList (e.g `1px 5px 10px`, but also could be `repeat(5, 10px 10px)` or `10px repeat(3, 5px 6px) 8px`.

                  Which is why I think we need the custom type. The [follow up CL](https://chromium-review.googlesource.com/c/chromium/src/+/6655208) implements the repeater logic. It was left off this CL to keep this CL contained and not massive.
                  Robert Flack

                  Got it, so this has a similar repeating syntax to track lists. Ideally we'd share the code as much as possible (e.g. have a common base class for both grid track lists and column lists) but perhaps the syntaxes are sufficiently different that this is not possible?

                  File third_party/blink/renderer/core/style/gap_data_list.h
                  Line 51, Patchset 27 (Latest): gap_data_list_.emplace_back(GapData<int>(length.IntValue()));
                  Robert Flack . unresolved

                  Doesn't this lose the type from the lengths? Do you have tests which have mixed units in a single rule and/or different units between rules?

                  Javier Contreras

                  I'm not sure if I understand your question. This constructor will only be used to create a GapDataList out of a list of Lengths (we only care about the int value of said lengths).

                  Robert Flack

                  Length has a type_ field, the int value is in the units of the type field. It would be invalid to interpolate between lengths of different types using the int value as is because 1% is not the same length as 1px, but IntValue will return 1 for both. Perhaps in this situation all of the types are already in pixels in which case it would be a good idea to call length.Pixels() instead so that we have the DCHECK that we are in fact dealing with pixels.

                  Open in Gerrit

                  Related details

                  Attention is currently required from:
                  • Javier Contreras
                  • Kevin Babbitt
                  • Kevin Ellis
                  Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
                  Gerrit-Comment-Date: Fri, 27 Jun 2025 16:22:39 +0000
                  Gerrit-HasComments: Yes
                  Gerrit-Has-Labels: No
                  Comment-In-Reply-To: Javier Contreras <javi...@microsoft.com>
                  Comment-In-Reply-To: Robert Flack <fla...@chromium.org>
                  satisfied_requirement
                  unsatisfied_requirement
                  open
                  diffy

                  Javier Contreras (Gerrit)

                  unread,
                  Jun 27, 2025, 3:34:29 PMJun 27
                  to Alison Maher, Kevin Ellis, Sam Davis Omekara, Robert Flack, Kevin Babbitt, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
                  Attention needed from Kevin Babbitt, Kevin Ellis and Robert Flack

                  Javier Contreras added 4 comments

                  File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
                  Line 121, Patchset 27 (Latest):void GetList(const CSSProperty& property,
                  Robert Flack . resolved

                  This is too generic of a name to be in a header file in the blink namespace when it is only specialized for a couple types, especially when it will usually result in generating code with a runtime error. Please try to move this to a corresponding implementation (.cc) file or scope the function in a way that it is not in the blink namespace (e.g. could be a static function of the class).

                  Javier Contreras

                  Fixed in other approach from previous comment, will port over if looks good.

                  Line 61, Patchset 27 (Latest): // Should not be called, only the specialized types should be used.
                  Robert Flack . unresolved

                  This pattern seems problematic to me - shifting something that should be a compile failure to be a runtime failure. Can we not have pure virtual functions in the generic class so that it is a compile failure to use any type other than those which have specializations implementing them?

                  I.e. this class would not provide an implementation of MaybeConvertNeutral, instead you would derive subclasses which use particular types, e.g.

                  ```
                  class CORE_EXPORT CSSGapLengthListInterpolationType : public CSSGapDataListInterpolationType<int> {
                  /* Implementations of MaybeConvertNeutral and other virtual methods */
                  }
                  ```

                  Alternately, you could exclude the function body and it would be a linker error to use a type which did not have a specialization in the .cc file, however I feel like this would be less obvious.

                  Javier Contreras

                  I have implemented the suggestion of having a pure virtual "base" and subclasses that override it in the follow up [CL](https://chromium-review.googlesource.com/c/chromium/src/+/6684748). If the changes there look good to you, I can port them over here. I didnt do it in this one since some people have already signed off on current implementation.

                  Line 23, Patchset 27 (Latest):template <typename T>
                  Robert Flack . unresolved

                  Is this ever constructed with different types? I couldn't find any cases where int wasn't used in the code. If not, it seems like for clarity it would be best to remove the templated type. Also, it seems like if we don't need templated types then we can avoid having the entire implementation in the header which is bad for compile times.

                  Javier Contreras

                  Not in this CL, but it will be.

                  This class will be used for interpolating `ColumnRuleWidth` and `ColumnRuleColor` (as well as the respective `Row` properties). This CL only handles `width` (template `int`) and a follow up CL will handle `color` (template `StyleColor`).

                  This CL only handles Width for clarity and to avoid having a massive CL.

                  Robert Flack

                  Understood - though does it need to be templated? Given both lengths and colors will be interpolated as InterpolationValue maybe we could use InterpolationValue directly.

                  Javier Contreras

                  I think this is an interesting suggestion that could potentially work. We'd still need to find a way to get those `InterpolationValues` to be the right type for when we feed them back to the style, and I fear there might be other gotchas trying to implement it this way. Would it be ok with you if we leave this as a TODO for potential exploration later?

                  File third_party/blink/renderer/core/animation/interpolation_types_map.cc
                  Line 201, Patchset 27 (Latest): MakeGarbageCollected<CSSGapDataListInterpolationType<int>>(
                  Robert Flack . unresolved

                  Do we need a custom type here? Can we not directly use CSSLengthListInterpolationType? It seems like that would significantly reduce the support code needed. Or alternately if specialized code is needed a subclass of CSSLengthListInterpolationType like CSSTransformOriginInterpolationType

                  Javier Contreras
                  I believe that a custom type is needed, and the reason for this is that `ColumnRuleWidth` and `RowRuleWidth` have the following [possible syntax](https://drafts.csswg.org/css-gaps-1/#column-row-rule-width), which is as you say, a LengthList (e.g `1px 5px 10px`, but also could be `repeat(5, 10px 10px)` or `10px repeat(3, 5px 6px) 8px`.

                  Which is why I think we need the custom type. The [follow up CL](https://chromium-review.googlesource.com/c/chromium/src/+/6655208) implements the repeater logic. It was left off this CL to keep this CL contained and not massive.
                  Robert Flack

                  Got it, so this has a similar repeating syntax to track lists. Ideally we'd share the code as much as possible (e.g. have a common base class for both grid track lists and column lists) but perhaps the syntaxes are sufficiently different that this is not possible?

                  Javier Contreras

                  If you are referring to the grid tracklist syntax then yes, they are pretty different since the syntax for that one includes may other different units such as `fr`, `ch`, `auto`.

                  Open in Gerrit

                  Related details

                  Attention is currently required from:
                  • Kevin Babbitt
                  • Kevin Ellis
                  • Robert Flack
                  Gerrit-Attention: Robert Flack <fla...@chromium.org>
                  Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
                  Gerrit-Comment-Date: Fri, 27 Jun 2025 19:34:12 +0000
                  satisfied_requirement
                  unsatisfied_requirement
                  open
                  diffy

                  Robert Flack (Gerrit)

                  unread,
                  Jun 30, 2025, 9:10:27 AMJun 30
                  to Javier Contreras, Alison Maher, Kevin Ellis, Sam Davis Omekara, Kevin Babbitt, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
                  Attention needed from Javier Contreras, Kevin Babbitt and Kevin Ellis

                  Robert Flack added 1 comment

                  File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
                  Line 23, Patchset 27 (Latest):template <typename T>
                  Robert Flack . unresolved

                  Is this ever constructed with different types? I couldn't find any cases where int wasn't used in the code. If not, it seems like for clarity it would be best to remove the templated type. Also, it seems like if we don't need templated types then we can avoid having the entire implementation in the header which is bad for compile times.

                  Javier Contreras

                  Not in this CL, but it will be.

                  This class will be used for interpolating `ColumnRuleWidth` and `ColumnRuleColor` (as well as the respective `Row` properties). This CL only handles `width` (template `int`) and a follow up CL will handle `color` (template `StyleColor`).

                  This CL only handles Width for clarity and to avoid having a massive CL.

                  Robert Flack

                  Understood - though does it need to be templated? Given both lengths and colors will be interpolated as InterpolationValue maybe we could use InterpolationValue directly.

                  Javier Contreras

                  I think this is an interesting suggestion that could potentially work. We'd still need to find a way to get those `InterpolationValues` to be the right type for when we feed them back to the style, and I fear there might be other gotchas trying to implement it this way. Would it be ok with you if we leave this as a TODO for potential exploration later?

                  Robert Flack

                  Alternately, if the only shared code ends up being the repeat extension handling, it may be cleaner to keep the classes concrete and call that as a templated helper function from both of them.

                  Open in Gerrit

                  Related details

                  Attention is currently required from:
                  • Javier Contreras
                  • Kevin Babbitt
                  • Kevin Ellis
                  Gerrit-Attention: Javier Contreras <javi...@microsoft.com>
                  Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
                  Gerrit-Comment-Date: Mon, 30 Jun 2025 13:10:20 +0000
                  satisfied_requirement
                  unsatisfied_requirement
                  open
                  diffy

                  Javier Contreras (Gerrit)

                  unread,
                  Jun 30, 2025, 2:23:26 PMJun 30
                  to Alison Maher, Kevin Ellis, Sam Davis Omekara, Robert Flack, Kevin Babbitt, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
                  Attention needed from Alison Maher, Kevin Babbitt, Kevin Ellis, Robert Flack and Sam Davis Omekara

                  Javier Contreras added 5 comments

                  File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
                  Line 61, Patchset 27: // Should not be called, only the specialized types should be used.
                  Robert Flack . resolved

                  This pattern seems problematic to me - shifting something that should be a compile failure to be a runtime failure. Can we not have pure virtual functions in the generic class so that it is a compile failure to use any type other than those which have specializations implementing them?

                  I.e. this class would not provide an implementation of MaybeConvertNeutral, instead you would derive subclasses which use particular types, e.g.

                  ```
                  class CORE_EXPORT CSSGapLengthListInterpolationType : public CSSGapDataListInterpolationType<int> {
                  /* Implementations of MaybeConvertNeutral and other virtual methods */
                  }
                  ```

                  Alternately, you could exclude the function body and it would be a linker error to use a type which did not have a specialization in the .cc file, however I feel like this would be less obvious.

                  Javier Contreras

                  I have implemented the suggestion of having a pure virtual "base" and subclasses that override it in the follow up [CL](https://chromium-review.googlesource.com/c/chromium/src/+/6684748). If the changes there look good to you, I can port them over here. I didnt do it in this one since some people have already signed off on current implementation.

                  Javier Contreras

                  I have implemented the new appcoach. Let me know of any other suggestions

                  File third_party/blink/renderer/core/animation/interpolation_types_map.cc
                  Line 201, Patchset 27: MakeGarbageCollected<CSSGapDataListInterpolationType<int>>(
                  Robert Flack . resolved

                  Do we need a custom type here? Can we not directly use CSSLengthListInterpolationType? It seems like that would significantly reduce the support code needed. Or alternately if specialized code is needed a subclass of CSSLengthListInterpolationType like CSSTransformOriginInterpolationType

                  Javier Contreras
                  I believe that a custom type is needed, and the reason for this is that `ColumnRuleWidth` and `RowRuleWidth` have the following [possible syntax](https://drafts.csswg.org/css-gaps-1/#column-row-rule-width), which is as you say, a LengthList (e.g `1px 5px 10px`, but also could be `repeat(5, 10px 10px)` or `10px repeat(3, 5px 6px) 8px`.

                  Which is why I think we need the custom type. The [follow up CL](https://chromium-review.googlesource.com/c/chromium/src/+/6655208) implements the repeater logic. It was left off this CL to keep this CL contained and not massive.
                  Robert Flack

                  Got it, so this has a similar repeating syntax to track lists. Ideally we'd share the code as much as possible (e.g. have a common base class for both grid track lists and column lists) but perhaps the syntaxes are sufficiently different that this is not possible?

                  Javier Contreras

                  If you are referring to the grid tracklist syntax then yes, they are pretty different since the syntax for that one includes may other different units such as `fr`, `ch`, `auto`.

                  Javier Contreras

                  Done

                  File third_party/blink/renderer/core/style/gap_data_list.h
                  Line 51, Patchset 27: gap_data_list_.emplace_back(GapData<int>(length.IntValue()));
                  Robert Flack . resolved

                  Doesn't this lose the type from the lengths? Do you have tests which have mixed units in a single rule and/or different units between rules?

                  Javier Contreras

                  I'm not sure if I understand your question. This constructor will only be used to create a GapDataList out of a list of Lengths (we only care about the int value of said lengths).

                  Robert Flack

                  Length has a type_ field, the int value is in the units of the type field. It would be invalid to interpolate between lengths of different types using the int value as is because 1% is not the same length as 1px, but IntValue will return 1 for both. Perhaps in this situation all of the types are already in pixels in which case it would be a good idea to call length.Pixels() instead so that we have the DCHECK that we are in fact dealing with pixels.

                  Javier Contreras

                  Done

                  File third_party/blink/web_tests/animations/interpolation/rule-width-interpolation-multiple-values-2.html
                  File-level comment, Patchset 27:
                  Alison Maher . resolved

                  nit: same here, -002.html

                  Javier Contreras

                  Done

                  File third_party/blink/web_tests/animations/interpolation/rule-width-interpolation-multiple-values.html
                  File-level comment, Patchset 27:
                  Alison Maher . resolved

                  nit: I'd rename this to end in -001.html

                  Javier Contreras

                  Done

                  Open in Gerrit

                  Related details

                  Attention is currently required from:
                  • Alison Maher
                  • Kevin Babbitt
                  • Kevin Ellis
                  • Robert Flack
                  • Sam Davis Omekara
                  Submit Requirements:
                  • requirement satisfiedCode-Coverage
                  • requirement is not satisfiedCode-Owners
                  • requirement is not satisfiedCode-Review
                  • requirement is not satisfiedNo-Unresolved-Comments
                  Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
                  Gerrit-MessageType: comment
                  Gerrit-Project: chromium/src
                  Gerrit-Branch: main
                  Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
                  Gerrit-Change-Number: 6643003
                  Gerrit-PatchSet: 28
                  Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
                  Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
                  Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
                  Gerrit-Reviewer: Kevin Babbitt <kbab...@microsoft.com>
                  Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
                  Gerrit-Reviewer: Robert Flack <fla...@chromium.org>
                  Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
                  Gerrit-CC: Alexis Menard <alexis...@intel.com>
                  Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
                  Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
                  Gerrit-Attention: Kevin Babbitt <kbab...@microsoft.com>
                  Gerrit-Attention: Robert Flack <fla...@chromium.org>
                  Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>
                  Gerrit-Attention: Alison Maher <alm...@microsoft.com>
                  Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
                  Gerrit-Comment-Date: Mon, 30 Jun 2025 18:23:15 +0000
                  Gerrit-HasComments: Yes
                  Gerrit-Has-Labels: No
                  Comment-In-Reply-To: Javier Contreras <javi...@microsoft.com>
                  Comment-In-Reply-To: Robert Flack <fla...@chromium.org>
                  Comment-In-Reply-To: Alison Maher <alm...@microsoft.com>
                  satisfied_requirement
                  unsatisfied_requirement
                  open
                  diffy

                  Robert Flack (Gerrit)

                  unread,
                  Jun 30, 2025, 3:02:19 PMJun 30
                  to Javier Contreras, Alison Maher, Kevin Ellis, Sam Davis Omekara, Kevin Babbitt, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
                  Attention needed from Alison Maher, Javier Contreras, Kevin Babbitt, Kevin Ellis and Sam Davis Omekara

                  Robert Flack added 3 comments

                  File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
                  Line 60, Patchset 28:inline void CSSGapDataListInterpolationType<int>::GetList(
                  Robert Flack . unresolved

                  This can be an override in CSSGapWidthListInterpolationType.

                  Line 37, Patchset 28: NOTREACHED();
                  Robert Flack . unresolved

                  This can also be pure virtual with no implementation. Any implementations of this abstract class should provide an override of this method.

                  File third_party/blink/renderer/core/animation/css_gap_width_list_interpolation_type.h
                  Line 12, Patchset 28:class CORE_EXPORT CSSGapWidthListInterpolationType
                  Robert Flack . unresolved

                  Since this isn't specific to width, it might make more sense to call this CSSGapLengthListInterpolationType.

                  Open in Gerrit

                  Related details

                  Attention is currently required from:
                  • Alison Maher
                  • Javier Contreras
                  • Kevin Babbitt
                  • Kevin Ellis
                  • Sam Davis Omekara
                  Gerrit-Attention: Javier Contreras <javi...@microsoft.com>
                  Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>
                  Gerrit-Attention: Alison Maher <alm...@microsoft.com>
                  Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
                  Gerrit-Comment-Date: Mon, 30 Jun 2025 19:02:09 +0000
                  Gerrit-HasComments: Yes
                  Gerrit-Has-Labels: No
                  satisfied_requirement
                  unsatisfied_requirement
                  open
                  diffy

                  Javier Contreras (Gerrit)

                  unread,
                  Jun 30, 2025, 4:12:03 PMJun 30
                  to Alison Maher, Kevin Ellis, Sam Davis Omekara, Robert Flack, Kevin Babbitt, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
                  Attention needed from Alison Maher, Kevin Babbitt, Kevin Ellis, Robert Flack and Sam Davis Omekara

                  Javier Contreras added 3 comments

                  File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
                  Line 60, Patchset 28:inline void CSSGapDataListInterpolationType<int>::GetList(
                  Robert Flack . unresolved

                  This can be an override in CSSGapWidthListInterpolationType.

                  Javier Contreras

                  Yes but we use it in the `InheritedGapDataListChecker` so we need it to be static and thus can't also be virtual

                  Line 37, Patchset 28: NOTREACHED();
                  Robert Flack . resolved

                  This can also be pure virtual with no implementation. Any implementations of this abstract class should provide an override of this method.

                  Javier Contreras

                  Done

                  File third_party/blink/renderer/core/animation/css_gap_width_list_interpolation_type.h
                  Line 12, Patchset 28:class CORE_EXPORT CSSGapWidthListInterpolationType
                  Robert Flack . unresolved

                  Since this isn't specific to width, it might make more sense to call this CSSGapLengthListInterpolationType.

                  Javier Contreras

                  I see why that makes sense, however, I opted for width because the property we are interpolating is column/row-rule-width, even though under the hood we are using `Length` to represent this.

                  Open in Gerrit

                  Related details

                  Attention is currently required from:
                  • Alison Maher
                  • Kevin Babbitt
                  • Kevin Ellis
                  • Robert Flack
                  • Sam Davis Omekara
                  Submit Requirements:
                  • requirement satisfiedCode-Coverage
                  • requirement is not satisfiedCode-Owners
                  • requirement is not satisfiedCode-Review
                  • requirement is not satisfiedNo-Unresolved-Comments
                  Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
                  Gerrit-MessageType: comment
                  Gerrit-Project: chromium/src
                  Gerrit-Branch: main
                  Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
                  Gerrit-Change-Number: 6643003
                  Gerrit-PatchSet: 30
                  Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
                  Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
                  Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
                  Gerrit-Reviewer: Kevin Babbitt <kbab...@microsoft.com>
                  Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
                  Gerrit-Reviewer: Robert Flack <fla...@chromium.org>
                  Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
                  Gerrit-CC: Alexis Menard <alexis...@intel.com>
                  Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
                  Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
                  Gerrit-Attention: Kevin Babbitt <kbab...@microsoft.com>
                  Gerrit-Attention: Robert Flack <fla...@chromium.org>
                  Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>
                  Gerrit-Attention: Alison Maher <alm...@microsoft.com>
                  Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
                  Gerrit-Comment-Date: Mon, 30 Jun 2025 20:11:54 +0000
                  Gerrit-HasComments: Yes
                  Gerrit-Has-Labels: No
                  Comment-In-Reply-To: Robert Flack <fla...@chromium.org>
                  satisfied_requirement
                  unsatisfied_requirement
                  open
                  diffy

                  Javier Contreras (Gerrit)

                  unread,
                  Jul 1, 2025, 11:21:59 AM (14 days ago) Jul 1
                  to Alison Maher, Kevin Ellis, Sam Davis Omekara, Robert Flack, Kevin Babbitt, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
                  Attention needed from Alison Maher, Kevin Babbitt, Kevin Ellis, Robert Flack and Sam Davis Omekara

                  Javier Contreras added 1 comment

                  File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
                  Line 23, Patchset 27:template <typename T>
                  Robert Flack . unresolved

                  Is this ever constructed with different types? I couldn't find any cases where int wasn't used in the code. If not, it seems like for clarity it would be best to remove the templated type. Also, it seems like if we don't need templated types then we can avoid having the entire implementation in the header which is bad for compile times.

                  Javier Contreras

                  Not in this CL, but it will be.

                  This class will be used for interpolating `ColumnRuleWidth` and `ColumnRuleColor` (as well as the respective `Row` properties). This CL only handles `width` (template `int`) and a follow up CL will handle `color` (template `StyleColor`).

                  This CL only handles Width for clarity and to avoid having a massive CL.

                  Robert Flack

                  Understood - though does it need to be templated? Given both lengths and colors will be interpolated as InterpolationValue maybe we could use InterpolationValue directly.

                  Javier Contreras

                  I think this is an interesting suggestion that could potentially work. We'd still need to find a way to get those `InterpolationValues` to be the right type for when we feed them back to the style, and I fear there might be other gotchas trying to implement it this way. Would it be ok with you if we leave this as a TODO for potential exploration later?

                  Robert Flack

                  Alternately, if the only shared code ends up being the repeat extension handling, it may be cleaner to keep the classes concrete and call that as a templated helper function from both of them.

                  Javier Contreras

                  Yes I think I see what you mean. Could we still leave this as a potential exploration once we have the repeater stuff done so we can see how we could turn it into what you suggest?

                  Gerrit-Comment-Date: Tue, 01 Jul 2025 15:21:49 +0000
                  Gerrit-HasComments: Yes
                  Gerrit-Has-Labels: No
                  satisfied_requirement
                  unsatisfied_requirement
                  open
                  diffy

                  Kevin Babbitt (Gerrit)

                  unread,
                  Jul 1, 2025, 4:32:15 PM (14 days ago) Jul 1
                  to Javier Contreras, Alison Maher, Kevin Ellis, Sam Davis Omekara, Robert Flack, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
                  Attention needed from Alison Maher, Javier Contreras, Kevin Ellis, Robert Flack and Sam Davis Omekara

                  Kevin Babbitt added 2 comments

                  File third_party/blink/renderer/core/animation/css_gap_width_list_interpolation_type.h
                  Line 12, Patchset 28:class CORE_EXPORT CSSGapWidthListInterpolationType
                  Robert Flack . unresolved

                  Since this isn't specific to width, it might make more sense to call this CSSGapLengthListInterpolationType.

                  Javier Contreras

                  I see why that makes sense, however, I opted for width because the property we are interpolating is column/row-rule-width, even though under the hood we are using `Length` to represent this.

                  Kevin Babbitt

                  FWIW, I can envision future applications of this to other gap decoration length lists. For example, I've seen/heard suggestions for more control over outset/offsets, which might necessitate lists similar to what we have for width.

                  I also think CSSGapLengthListInterpolationType would be more consistent with use of "Length" in names of other objects related to interpolation.

                  File third_party/blink/renderer/core/animation/css_gap_width_list_interpolation_type.cc
                  Line 72, Patchset 30 (Latest): state.StyleBuilder().SetRowRuleWidth(GapDataList<int>(result));
                  Kevin Babbitt . unresolved
                  ```suggestion
                  CHECK_EQ(property_id_, CSSPropertyID::kRowRuleWidth);
                  state.StyleBuilder().SetRowRuleWidth(GapDataList<int>(result));
                  ```
                  Open in Gerrit

                  Related details

                  Attention is currently required from:
                  • Alison Maher
                  • Javier Contreras
                  Gerrit-Attention: Javier Contreras <javi...@microsoft.com>
                  Gerrit-Attention: Robert Flack <fla...@chromium.org>
                  Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>
                  Gerrit-Attention: Alison Maher <alm...@microsoft.com>
                  Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
                  Gerrit-Comment-Date: Tue, 01 Jul 2025 20:32:04 +0000
                  satisfied_requirement
                  unsatisfied_requirement
                  open
                  diffy

                  Robert Flack (Gerrit)

                  unread,
                  Jul 2, 2025, 9:10:19 AM (13 days ago) Jul 2
                  to Javier Contreras, Alison Maher, Kevin Ellis, Sam Davis Omekara, Kevin Babbitt, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
                  Attention needed from Alison Maher, Javier Contreras, Kevin Ellis and Sam Davis Omekara

                  Robert Flack added 2 comments

                  File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
                  Line 60, Patchset 28:inline void CSSGapDataListInterpolationType<int>::GetList(
                  Robert Flack . unresolved

                  This can be an override in CSSGapWidthListInterpolationType.

                  Javier Contreras

                  Yes but we use it in the `InheritedGapDataListChecker` so we need it to be static and thus can't also be virtual

                  Robert Flack

                  Ah I see. Given this class is only instantiated / directly referenced from third_party/blink/renderer/core/animation/css_gap_width_list_interpolation_type.cc perhaps we can move the implementation into that cc file as seems to be the case with many of the other CSSConversionChecker implementations: https://source.chromium.org/search?q=file:%5C.cc%20%22:%20public%20CSSInterpolationType::CSSConversionChecker%22&start=1

                  We should aim to only keep things in the header files that people including this need, but this checker can entirely be an implementation detail.

                  File third_party/blink/renderer/core/animation/css_gap_width_list_interpolation_type.h
                  Line 12, Patchset 28:class CORE_EXPORT CSSGapWidthListInterpolationType
                  Robert Flack . unresolved

                  Since this isn't specific to width, it might make more sense to call this CSSGapLengthListInterpolationType.

                  Javier Contreras

                  I see why that makes sense, however, I opted for width because the property we are interpolating is column/row-rule-width, even though under the hood we are using `Length` to represent this.

                  Kevin Babbitt

                  FWIW, I can envision future applications of this to other gap decoration length lists. For example, I've seen/heard suggestions for more control over outset/offsets, which might necessitate lists similar to what we have for width.

                  I also think CSSGapLengthListInterpolationType would be more consistent with use of "Length" in names of other objects related to interpolation.

                  Robert Flack

                  +1 I think it's better for consistency to use the data type rather than the property name.

                  Open in Gerrit

                  Related details

                  Attention is currently required from:
                  • Alison Maher
                  • Javier Contreras
                  • Kevin Ellis
                  • Sam Davis Omekara
                  Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>
                  Gerrit-Attention: Alison Maher <alm...@microsoft.com>
                  Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
                  Gerrit-Comment-Date: Wed, 02 Jul 2025 13:10:12 +0000
                  Gerrit-HasComments: Yes
                  Gerrit-Has-Labels: No
                  Comment-In-Reply-To: Kevin Babbitt <kbab...@microsoft.com>
                  satisfied_requirement
                  unsatisfied_requirement
                  open
                  diffy

                  Kevin Ellis (Gerrit)

                  unread,
                  Jul 2, 2025, 9:55:59 AM (13 days ago) Jul 2
                  to Javier Contreras, Alison Maher, Sam Davis Omekara, Robert Flack, Kevin Babbitt, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
                  Attention needed from Alison Maher, Javier Contreras and Sam Davis Omekara

                  Kevin Ellis added 1 comment

                  File third_party/blink/web_tests/animations/interpolation/row-rule-width-interpolation.html
                  Line 27, Patchset 30 (Latest):assertInterpolation({
                  Kevin Ellis . unresolved

                  Can we instead add as WPT tests. As much as possible, I'd love to see tests that don't depend on blink implementation details (e.g. compositing decisions) migrated to WPT.

                  See third_party/blink/web_tests/external/wpt/css/support/interpolation-testcommon.js
                  third_party/blink/web_tests/external/wpt/css/motion/animation/offset-position-interpolation.html

                  Open in Gerrit

                  Related details

                  Attention is currently required from:
                  • Alison Maher
                  • Javier Contreras
                  • Sam Davis Omekara
                  Gerrit-Comment-Date: Wed, 02 Jul 2025 13:55:53 +0000
                  Gerrit-HasComments: Yes
                  Gerrit-Has-Labels: No
                  satisfied_requirement
                  unsatisfied_requirement
                  open
                  diffy

                  Javier Contreras (Gerrit)

                  unread,
                  Jul 2, 2025, 12:05:07 PM (13 days ago) Jul 2
                  to Alison Maher, Kevin Ellis, Sam Davis Omekara, Robert Flack, Kevin Babbitt, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
                  Attention needed from Alison Maher, Kevin Babbitt, Kevin Ellis, Robert Flack and Sam Davis Omekara

                  Javier Contreras added 6 comments

                  Patchset-level comments
                  File-level comment, Patchset 32 (Latest):
                  Javier Contreras . resolved

                  Added 2 more WPT tests and responded to feedback. PTAL

                  File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
                  Line 60, Patchset 28:inline void CSSGapDataListInterpolationType<int>::GetList(
                  Robert Flack . resolved

                  This can be an override in CSSGapWidthListInterpolationType.

                  Javier Contreras

                  Yes but we use it in the `InheritedGapDataListChecker` so we need it to be static and thus can't also be virtual

                  Robert Flack

                  Ah I see. Given this class is only instantiated / directly referenced from third_party/blink/renderer/core/animation/css_gap_width_list_interpolation_type.cc perhaps we can move the implementation into that cc file as seems to be the case with many of the other CSSConversionChecker implementations: https://source.chromium.org/search?q=file:%5C.cc%20%22:%20public%20CSSInterpolationType::CSSConversionChecker%22&start=1

                  We should aim to only keep things in the header files that people including this need, but this checker can entirely be an implementation detail.

                  Javier Contreras

                  Done

                  Line 23, Patchset 27:template <typename T>
                  Robert Flack . resolved

                  Is this ever constructed with different types? I couldn't find any cases where int wasn't used in the code. If not, it seems like for clarity it would be best to remove the templated type. Also, it seems like if we don't need templated types then we can avoid having the entire implementation in the header which is bad for compile times.

                  Javier Contreras

                  Not in this CL, but it will be.

                  This class will be used for interpolating `ColumnRuleWidth` and `ColumnRuleColor` (as well as the respective `Row` properties). This CL only handles `width` (template `int`) and a follow up CL will handle `color` (template `StyleColor`).

                  This CL only handles Width for clarity and to avoid having a massive CL.

                  Robert Flack

                  Understood - though does it need to be templated? Given both lengths and colors will be interpolated as InterpolationValue maybe we could use InterpolationValue directly.

                  Javier Contreras

                  I think this is an interesting suggestion that could potentially work. We'd still need to find a way to get those `InterpolationValues` to be the right type for when we feed them back to the style, and I fear there might be other gotchas trying to implement it this way. Would it be ok with you if we leave this as a TODO for potential exploration later?

                  Robert Flack

                  Alternately, if the only shared code ends up being the repeat extension handling, it may be cleaner to keep the classes concrete and call that as a templated helper function from both of them.

                  Javier Contreras

                  Yes I think I see what you mean. Could we still leave this as a potential exploration once we have the repeater stuff done so we can see how we could turn it into what you suggest?

                  Javier Contreras

                  Resolving for now.

                  File third_party/blink/renderer/core/animation/css_gap_width_list_interpolation_type.h
                  Line 12, Patchset 28:class CORE_EXPORT CSSGapWidthListInterpolationType
                  Robert Flack . resolved

                  Since this isn't specific to width, it might make more sense to call this CSSGapLengthListInterpolationType.

                  Javier Contreras

                  I see why that makes sense, however, I opted for width because the property we are interpolating is column/row-rule-width, even though under the hood we are using `Length` to represent this.

                  Kevin Babbitt

                  FWIW, I can envision future applications of this to other gap decoration length lists. For example, I've seen/heard suggestions for more control over outset/offsets, which might necessitate lists similar to what we have for width.

                  I also think CSSGapLengthListInterpolationType would be more consistent with use of "Length" in names of other objects related to interpolation.

                  Robert Flack

                  +1 I think it's better for consistency to use the data type rather than the property name.

                  Javier Contreras

                  Done

                  File third_party/blink/renderer/core/animation/css_gap_width_list_interpolation_type.cc
                  Line 72, Patchset 30: state.StyleBuilder().SetRowRuleWidth(GapDataList<int>(result));
                  Kevin Babbitt . resolved
                  ```suggestion
                  CHECK_EQ(property_id_, CSSPropertyID::kRowRuleWidth);
                  state.StyleBuilder().SetRowRuleWidth(GapDataList<int>(result));
                  ```
                  Javier Contreras

                  Done

                  File third_party/blink/web_tests/animations/interpolation/row-rule-width-interpolation.html
                  Line 27, Patchset 30:assertInterpolation({
                  Kevin Ellis . resolved

                  Can we instead add as WPT tests. As much as possible, I'd love to see tests that don't depend on blink implementation details (e.g. compositing decisions) migrated to WPT.

                  See third_party/blink/web_tests/external/wpt/css/support/interpolation-testcommon.js
                  third_party/blink/web_tests/external/wpt/css/motion/animation/offset-position-interpolation.html

                  Javier Contreras

                  Done

                  Open in Gerrit

                  Related details

                  Attention is currently required from:
                  • Alison Maher
                  • Kevin Babbitt
                  • Kevin Ellis
                  • Robert Flack
                  • Sam Davis Omekara
                  Submit Requirements:
                  • requirement satisfiedCode-Coverage
                  • requirement is not satisfiedCode-Owners
                  • requirement is not satisfiedCode-Review
                  Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
                  Gerrit-MessageType: comment
                  Gerrit-Project: chromium/src
                  Gerrit-Branch: main
                  Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
                  Gerrit-Change-Number: 6643003
                  Gerrit-PatchSet: 32
                  Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
                  Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
                  Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
                  Gerrit-Reviewer: Kevin Babbitt <kbab...@microsoft.com>
                  Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
                  Gerrit-Reviewer: Robert Flack <fla...@chromium.org>
                  Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
                  Gerrit-CC: Alexis Menard <alexis...@intel.com>
                  Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
                  Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
                  Gerrit-Attention: Kevin Babbitt <kbab...@microsoft.com>
                  Gerrit-Attention: Robert Flack <fla...@chromium.org>
                  Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>
                  Gerrit-Attention: Alison Maher <alm...@microsoft.com>
                  Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
                  Gerrit-Comment-Date: Wed, 02 Jul 2025 16:04:57 +0000
                  Gerrit-HasComments: Yes
                  Gerrit-Has-Labels: No
                  Comment-In-Reply-To: Kevin Babbitt <kbab...@microsoft.com>
                  Comment-In-Reply-To: Javier Contreras <javi...@microsoft.com>
                  Comment-In-Reply-To: Robert Flack <fla...@chromium.org>
                  Comment-In-Reply-To: Kevin Ellis <kev...@chromium.org>
                  satisfied_requirement
                  unsatisfied_requirement
                  open
                  diffy

                  Robert Flack (Gerrit)

                  unread,
                  Jul 3, 2025, 11:40:47 AM (12 days ago) Jul 3
                  to Javier Contreras, Alison Maher, Kevin Ellis, Sam Davis Omekara, Kevin Babbitt, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
                  Attention needed from Alison Maher, Javier Contreras, Kevin Babbitt, Kevin Ellis and Sam Davis Omekara

                  Robert Flack added 5 comments

                  Patchset-level comments
                  File-level comment, Patchset 34 (Latest):
                  Robert Flack . resolved

                  Just a few more comments, I feel like this is very close. Thanks for working through this.

                  File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
                  Line 35, Patchset 34 (Latest): virtual GapDataList<T> GetProperty(const ComputedStyle& style) const = 0;
                  Robert Flack . unresolved

                  I'm actually not sure I understand the value of having this function in the base class if it's only used by the derived class.

                  It's also the only thing using the template type. Perhaps we could re-add the template type when it's actually needed - e.g. if you later end up needing some shared type-dependent functionality. I'm not actually convinced that we will though - both subclasses could likely call a shared templated helper function to perform the list extension. Note that we could define both subclasses in css_grap_data_list_interpolation_type.cc if it helps them be able to share some templated implementation helper functions.

                  Line 31, Patchset 34 (Latest): property_id_ == CSSPropertyID::kRowRuleWidth);
                  Robert Flack . unresolved

                  Perhaps move this to the derived class and only check for the two relevant properties. We can add the color rules when that class is added later to its constructor.

                  I suppose the counter-argument would be if we implement repeating semantics in this class - that we assert that the repeating semantics actually make sense for the property. However, given we don't have that yet I'm not sure if it matters.

                  File third_party/blink/renderer/core/animation/css_gap_length_list_interpolation_type.h
                  Line 67, Patchset 34 (Latest):class InheritedGapLengthListChecker final
                  Robert Flack . unresolved

                  I don't think any external classes need access to this checker as its functions are only called via the virtual CSSConversionChecker interface. You can move the implementation entirely into the .cc file similar to the examples I linked earlier in https://chromium-review.googlesource.com/c/chromium/src/+/6643003/comment/6a3dbde7_6f4d53be/

                  File third_party/blink/renderer/core/animation/css_gap_length_list_interpolation_type.cc
                  Line 72, Patchset 34 (Latest): CHECK_EQ(property_id_, CSSPropertyID::kRowRuleWidth);
                  Robert Flack . unresolved

                  It seems like it would be better to check at construction that property_id_ is either kColumnRuleWidth or kRowRuleWidth, as there are several places throughout this class where the code assumes that it is one or the other.

                  Open in Gerrit

                  Related details

                  Attention is currently required from:
                  • Alison Maher
                  • Javier Contreras
                  • Kevin Babbitt
                  • Kevin Ellis
                  • Sam Davis Omekara
                  Submit Requirements:
                    • requirement satisfiedCode-Coverage
                    • requirement is not satisfiedCode-Owners
                    • requirement is not satisfiedCode-Review
                    • requirement is not satisfiedNo-Unresolved-Comments
                    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
                    Gerrit-MessageType: comment
                    Gerrit-Project: chromium/src
                    Gerrit-Branch: main
                    Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
                    Gerrit-Change-Number: 6643003
                    Gerrit-PatchSet: 34
                    Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
                    Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
                    Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
                    Gerrit-Reviewer: Kevin Babbitt <kbab...@microsoft.com>
                    Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
                    Gerrit-Reviewer: Robert Flack <fla...@chromium.org>
                    Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
                    Gerrit-CC: Alexis Menard <alexis...@intel.com>
                    Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
                    Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
                    Gerrit-Attention: Kevin Babbitt <kbab...@microsoft.com>
                    Gerrit-Attention: Javier Contreras <javi...@microsoft.com>
                    Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>
                    Gerrit-Attention: Alison Maher <alm...@microsoft.com>
                    Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
                    Gerrit-Comment-Date: Thu, 03 Jul 2025 15:40:34 +0000
                    Gerrit-HasComments: Yes
                    Gerrit-Has-Labels: No
                    satisfied_requirement
                    unsatisfied_requirement
                    open
                    diffy

                    Javier Contreras (Gerrit)

                    unread,
                    Jul 3, 2025, 1:41:33 PM (12 days ago) Jul 3
                    to Alison Maher, Kevin Ellis, Sam Davis Omekara, Robert Flack, Kevin Babbitt, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
                    Attention needed from Alison Maher, Kevin Babbitt, Kevin Ellis, Robert Flack and Sam Davis Omekara

                    Javier Contreras added 4 comments

                    File third_party/blink/renderer/core/animation/css_gap_data_list_interpolation_type.h
                    Line 35, Patchset 34: virtual GapDataList<T> GetProperty(const ComputedStyle& style) const = 0;
                    Robert Flack . resolved

                    I'm actually not sure I understand the value of having this function in the base class if it's only used by the derived class.

                    It's also the only thing using the template type. Perhaps we could re-add the template type when it's actually needed - e.g. if you later end up needing some shared type-dependent functionality. I'm not actually convinced that we will though - both subclasses could likely call a shared templated helper function to perform the list extension. Note that we could define both subclasses in css_grap_data_list_interpolation_type.cc if it helps them be able to share some templated implementation helper functions.

                    Javier Contreras

                    I now very clearly see your vision. I made some changes to my WIP CL with the repeater handling and now we dont need template here, in fact we don't need this class at all.

                    Line 31, Patchset 34: property_id_ == CSSPropertyID::kRowRuleWidth);
                    Robert Flack . resolved

                    Perhaps move this to the derived class and only check for the two relevant properties. We can add the color rules when that class is added later to its constructor.

                    I suppose the counter-argument would be if we implement repeating semantics in this class - that we assert that the repeating semantics actually make sense for the property. However, given we don't have that yet I'm not sure if it matters.

                    Javier Contreras

                    Done

                    File third_party/blink/renderer/core/animation/css_gap_length_list_interpolation_type.h
                    Line 67, Patchset 34:class InheritedGapLengthListChecker final
                    Robert Flack . resolved

                    I don't think any external classes need access to this checker as its functions are only called via the virtual CSSConversionChecker interface. You can move the implementation entirely into the .cc file similar to the examples I linked earlier in https://chromium-review.googlesource.com/c/chromium/src/+/6643003/comment/6a3dbde7_6f4d53be/

                    Javier Contreras

                    Done

                    File third_party/blink/renderer/core/animation/css_gap_length_list_interpolation_type.cc
                    Line 72, Patchset 34: CHECK_EQ(property_id_, CSSPropertyID::kRowRuleWidth);
                    Robert Flack . resolved

                    It seems like it would be better to check at construction that property_id_ is either kColumnRuleWidth or kRowRuleWidth, as there are several places throughout this class where the code assumes that it is one or the other.

                    Javier Contreras

                    Done

                    Open in Gerrit

                    Related details

                    Attention is currently required from:
                    • Alison Maher
                    • Kevin Babbitt
                    • Kevin Ellis
                    • Robert Flack
                    • Sam Davis Omekara
                    Submit Requirements:
                    • requirement satisfiedCode-Coverage
                    • requirement is not satisfiedCode-Owners
                    • requirement is not satisfiedCode-Review
                    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
                    Gerrit-MessageType: comment
                    Gerrit-Project: chromium/src
                    Gerrit-Branch: main
                    Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
                    Gerrit-Change-Number: 6643003
                    Gerrit-PatchSet: 38
                    Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
                    Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
                    Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
                    Gerrit-Reviewer: Kevin Babbitt <kbab...@microsoft.com>
                    Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
                    Gerrit-Reviewer: Robert Flack <fla...@chromium.org>
                    Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
                    Gerrit-CC: Alexis Menard <alexis...@intel.com>
                    Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
                    Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
                    Gerrit-Attention: Kevin Babbitt <kbab...@microsoft.com>
                    Gerrit-Attention: Robert Flack <fla...@chromium.org>
                    Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>
                    Gerrit-Attention: Alison Maher <alm...@microsoft.com>
                    Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
                    Gerrit-Comment-Date: Thu, 03 Jul 2025 17:41:22 +0000
                    Gerrit-HasComments: Yes
                    Gerrit-Has-Labels: No
                    Comment-In-Reply-To: Robert Flack <fla...@chromium.org>
                    satisfied_requirement
                    unsatisfied_requirement
                    open
                    diffy

                    Robert Flack (Gerrit)

                    unread,
                    Jul 3, 2025, 1:51:59 PM (12 days ago) Jul 3
                    to Javier Contreras, Alison Maher, Kevin Ellis, Sam Davis Omekara, Kevin Babbitt, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
                    Attention needed from Alison Maher, Javier Contreras, Kevin Babbitt, Kevin Ellis and Sam Davis Omekara

                    Robert Flack voted and added 2 comments

                    Votes added by Robert Flack

                    Code-Review+1

                    2 comments

                    Patchset-level comments
                    File third_party/blink/renderer/core/animation/css_gap_length_list_interpolation_type.h
                    Line 23, Patchset 38 (Latest): property_id_ = property.GetCSSProperty().PropertyID();
                    Robert Flack . unresolved
                    nit: This should be set in the initializer list, i.e.
                    ```
                    explicit CSSGapLengthListInterpolationType(
                    PropertyHandle property,
                    const PropertyRegistration* registration = nullptr)
                    : CSSInterpolationType(property, registration),
                    property_id_(property.GetCSSProperty().PropertyID()) {
                    ```
                    Open in Gerrit

                    Related details

                    Attention is currently required from:
                    • Alison Maher
                    • Javier Contreras
                    • Kevin Babbitt
                    • Kevin Ellis
                    • Sam Davis Omekara
                    Submit Requirements:
                    • requirement satisfiedCode-Coverage
                    • requirement satisfiedCode-Owners
                    • requirement satisfiedCode-Review
                    • requirement is not satisfiedNo-Unresolved-Comments
                    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
                    Gerrit-MessageType: comment
                    Gerrit-Project: chromium/src
                    Gerrit-Branch: main
                    Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
                    Gerrit-Change-Number: 6643003
                    Gerrit-PatchSet: 38
                    Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
                    Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
                    Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
                    Gerrit-Reviewer: Kevin Babbitt <kbab...@microsoft.com>
                    Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
                    Gerrit-Reviewer: Robert Flack <fla...@chromium.org>
                    Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
                    Gerrit-CC: Alexis Menard <alexis...@intel.com>
                    Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
                    Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
                    Gerrit-Attention: Kevin Babbitt <kbab...@microsoft.com>
                    Gerrit-Attention: Javier Contreras <javi...@microsoft.com>
                    Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>
                    Gerrit-Attention: Alison Maher <alm...@microsoft.com>
                    Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
                    Gerrit-Comment-Date: Thu, 03 Jul 2025 17:51:52 +0000
                    Gerrit-HasComments: Yes
                    Gerrit-Has-Labels: Yes
                    satisfied_requirement
                    unsatisfied_requirement
                    open
                    diffy

                    Javier Contreras (Gerrit)

                    unread,
                    Jul 3, 2025, 2:06:59 PM (12 days ago) Jul 3
                    to Robert Flack, Alison Maher, Kevin Ellis, Sam Davis Omekara, Kevin Babbitt, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
                    Attention needed from Alison Maher, Kevin Babbitt, Kevin Ellis and Sam Davis Omekara

                    Javier Contreras added 1 comment

                    File third_party/blink/renderer/core/animation/css_gap_length_list_interpolation_type.h
                    Line 23, Patchset 38: property_id_ = property.GetCSSProperty().PropertyID();
                    Robert Flack . resolved
                    nit: This should be set in the initializer list, i.e.
                    ```
                    explicit CSSGapLengthListInterpolationType(
                    PropertyHandle property,
                    const PropertyRegistration* registration = nullptr)
                    : CSSInterpolationType(property, registration),
                    property_id_(property.GetCSSProperty().PropertyID()) {
                    ```
                    Javier Contreras

                    Done

                    Open in Gerrit

                    Related details

                    Attention is currently required from:
                    • Alison Maher
                    • Kevin Babbitt
                    • Kevin Ellis
                    • Sam Davis Omekara
                    Submit Requirements:
                    • requirement satisfiedCode-Coverage
                    • requirement satisfiedCode-Owners
                    • requirement satisfiedCode-Review
                    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
                    Gerrit-MessageType: comment
                    Gerrit-Project: chromium/src
                    Gerrit-Branch: main
                    Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
                    Gerrit-Change-Number: 6643003
                    Gerrit-PatchSet: 39
                    Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
                    Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
                    Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
                    Gerrit-Reviewer: Kevin Babbitt <kbab...@microsoft.com>
                    Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
                    Gerrit-Reviewer: Robert Flack <fla...@chromium.org>
                    Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
                    Gerrit-CC: Alexis Menard <alexis...@intel.com>
                    Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
                    Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
                    Gerrit-Attention: Kevin Babbitt <kbab...@microsoft.com>
                    Gerrit-Attention: Sam Davis Omekara <samome...@microsoft.com>
                    Gerrit-Attention: Alison Maher <alm...@microsoft.com>
                    Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
                    Gerrit-Comment-Date: Thu, 03 Jul 2025 18:06:47 +0000
                    satisfied_requirement
                    open
                    diffy

                    Javier Contreras (Gerrit)

                    unread,
                    Jul 3, 2025, 2:32:25 PM (12 days ago) Jul 3
                    to Robert Flack, Alison Maher, Kevin Ellis, Sam Davis Omekara, Kevin Babbitt, AyeAye Python Dispatcher, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
                    Attention needed from Alison Maher, Kevin Babbitt, Kevin Ellis and Sam Davis Omekara

                    Javier Contreras voted Commit-Queue+2

                    Commit-Queue+2
                    Gerrit-Comment-Date: Thu, 03 Jul 2025 18:32:13 +0000
                    Gerrit-HasComments: No
                    Gerrit-Has-Labels: Yes
                    satisfied_requirement
                    open
                    diffy

                    Chromium LUCI CQ (Gerrit)

                    unread,
                    Jul 3, 2025, 2:51:45 PM (12 days ago) Jul 3
                    to Javier Contreras, Robert Flack, Alison Maher, Kevin Ellis, Sam Davis Omekara, Kevin Babbitt, AyeAye Python Dispatcher, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org

                    Chromium LUCI CQ submitted the change with unreviewed changes

                    Unreviewed changes

                    38 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/animation/css_gap_length_list_interpolation_type.h
                    Insertions: 2, Deletions: 2.

                    @@ -19,8 +19,8 @@

                    explicit CSSGapLengthListInterpolationType(
                    PropertyHandle property,
                    const PropertyRegistration* registration = nullptr)
                    -      : CSSInterpolationType(property, registration) {
                    - property_id_ = property.GetCSSProperty().PropertyID();
                    + : CSSInterpolationType(property, registration),
                    + property_id_(property.GetCSSProperty().PropertyID()) {
                    CHECK(property_id_ == CSSPropertyID::kColumnRuleWidth ||
                    property_id_ == CSSPropertyID::kRowRuleWidth);
                    }
                    ```

                    Change information

                    Commit message:
                    [gap-decorations] Base interpolation for *-rule-width

                    This CL implements basic interpolation for row-rule-width and
                    column-rule-width properties for CSSGapDecorations.

                    This CL only implements the basic interpolation for the width
                    properties, upcoming CLs will implement the other template specific
                    functions for StyleColor specializations, and follow up CLs will also
                    implement the remaining logic for dealing with repeaters.

                    The overall structure of interpolation for gap decorations will be as
                    follows:

                    `CSSGapLengthListInterpolationType` handles interpolation for
                    `GapDataList` (for *-rule-width). We then hand off the actual
                    interpolation with an `InterpolableList` of `InterpolableLength`. The
                    followup CL will account for the `repeat` syntax.
                    Bug: 357648037, 419066541
                    Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
                    Commit-Queue: Javier Contreras <javi...@microsoft.com>
                    Reviewed-by: Robert Flack <fla...@chromium.org>
                    Cr-Commit-Position: refs/heads/main@{#1482349}
                    Files:
                    • M third_party/blink/renderer/core/animation/BUILD.gn
                    • A third_party/blink/renderer/core/animation/css_gap_length_list_interpolation_type.cc
                    • A third_party/blink/renderer/core/animation/css_gap_length_list_interpolation_type.h
                    • M third_party/blink/renderer/core/animation/interpolation_types_map.cc
                    • M third_party/blink/renderer/core/animation/length_list_property_functions.cc
                    • M third_party/blink/renderer/core/css/css_properties.json5
                    • M third_party/blink/renderer/core/style/gap_data_list.h
                    • M third_party/blink/web_tests/TestExpectations
                    • A third_party/blink/web_tests/external/wpt/css/css-gaps/animation/gap-decorations-width-neutral-keyframe-001.html
                    • A third_party/blink/web_tests/external/wpt/css/css-gaps/animation/gap-decorations-width-neutral-keyframe-002.html
                    • A third_party/blink/web_tests/external/wpt/css/css-gaps/animation/row-rule-width-interpolation.html
                    • A third_party/blink/web_tests/external/wpt/css/css-gaps/animation/rule-width-interpolation-multiple-values-001.html
                    • A third_party/blink/web_tests/external/wpt/css/css-gaps/animation/rule-width-interpolation-multiple-values-002.html
                    • M third_party/blink/web_tests/external/wpt/web-animations/animation-model/animation-types/accumulation-per-property-001-expected.txt
                    • M third_party/blink/web_tests/external/wpt/web-animations/animation-model/animation-types/addition-per-property-001-expected.txt
                    • M third_party/blink/web_tests/external/wpt/web-animations/animation-model/animation-types/interpolation-per-property-001-expected.txt
                    Change size: L
                    Delta: 16 files changed, 607 insertions(+), 22 deletions(-)
                    Branch: refs/heads/main
                    Submit Requirements:
                    • requirement satisfiedCode-Review: +1 by Robert Flack
                    Open in Gerrit
                    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
                    Gerrit-MessageType: merged
                    Gerrit-Project: chromium/src
                    Gerrit-Branch: main
                    Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
                    Gerrit-Change-Number: 6643003
                    Gerrit-PatchSet: 40
                    Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
                    Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
                    Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
                    Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
                    Gerrit-Reviewer: Kevin Babbitt <kbab...@microsoft.com>
                    Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
                    Gerrit-Reviewer: Robert Flack <fla...@chromium.org>
                    Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
                    Gerrit-CC: Alexis Menard <alexis...@intel.com>
                    Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
                    open
                    diffy
                    satisfied_requirement

                    Blink W3C Test Autoroller (Gerrit)

                    unread,
                    Jul 3, 2025, 3:47:27 PM (12 days ago) Jul 3
                    to Javier Contreras, Chromium LUCI CQ, Robert Flack, Alison Maher, Kevin Ellis, Sam Davis Omekara, Kevin Babbitt, AyeAye Python Dispatcher, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org

                    Message from Blink W3C Test Autoroller

                    The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/53580

                    Open in Gerrit

                    Related details

                    Attention set is empty
                    Submit Requirements:
                    • requirement satisfiedCode-Coverage
                    • requirement satisfiedCode-Owners
                    • requirement satisfiedCode-Review
                    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
                    Gerrit-MessageType: comment
                    Gerrit-Project: chromium/src
                    Gerrit-Branch: main
                    Gerrit-Change-Id: Ia048e4673936031bc644449d240407029c54fcbe
                    Gerrit-Change-Number: 6643003
                    Gerrit-PatchSet: 40
                    Gerrit-Owner: Javier Contreras <javi...@microsoft.com>
                    Gerrit-Reviewer: Alison Maher <alm...@microsoft.com>
                    Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
                    Gerrit-Reviewer: Javier Contreras <javi...@microsoft.com>
                    Gerrit-Reviewer: Kevin Babbitt <kbab...@microsoft.com>
                    Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
                    Gerrit-Reviewer: Robert Flack <fla...@chromium.org>
                    Gerrit-Reviewer: Sam Davis Omekara <samome...@microsoft.com>
                    Gerrit-CC: Alexis Menard <alexis...@intel.com>
                    Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>
                    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
                    Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
                    Gerrit-Comment-Date: Thu, 03 Jul 2025 19:47:20 +0000
                    Gerrit-HasComments: No
                    Gerrit-Has-Labels: No
                    satisfied_requirement
                    open
                    diffy
                    Reply all
                    Reply to author
                    Forward
                    0 new messages