Message from 김민성

File-level comment, Patchset 4 (Latest):

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

김민성 (Gerrit)

unread,

Jun 15, 2025, 9:52:50 AM6/15/25

to Anders Hartvoll Ruud, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Anders Hartvoll Ruud

김민성 added 1 comment

Patchset-level comments

김민성 . resolved

Hi @and...@chromium.org, would you `Commit-Queue +1` and review this CL, please?

File-level comment, Patchset 4 (Latest):

Related details

Attention is currently required from:

Anders Hartvoll Ruud

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

김민성 (Gerrit)

unread,

Jun 16, 2025, 2:48:50 AM6/16/25

to Jason Leo, Chromium LUCI CQ, Anders Hartvoll Ruud, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Anders Hartvoll Ruud

김민성 added 1 comment

Patchset-level comments

김민성 . resolved

Oops... I'm sorry for missing some tests. I'll fix and reactive this CL. Thank @cg...@chromium.org for voting `CQ +2`!

File-level comment, Patchset 4 (Latest):

Related details

Attention is currently required from:

Anders Hartvoll Ruud

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jason Leo (Gerrit)

unread,

Jun 16, 2025, 3:21:20 AM6/16/25

to 김민성, Chromium LUCI CQ, Anders Hartvoll Ruud, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from 김민성

Jason Leo added 1 comment

Patchset-level comments

김민성 . resolved

Oops... I'm sorry for missing some tests. I'll fix and reactive this CL. Thank @cg...@chromium.org for voting `CQ +2`!

Jason Leo

You're welcome, feel free to ping me if you need another dry run.

Related details

Attention is currently required from:

김민성

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

김민성 (Gerrit)

unread,

Jun 16, 2025, 4:39:30 AM6/16/25

to Olga Gerchikov, Jason Leo, Chromium LUCI CQ, Anders Hartvoll Ruud, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Anders Hartvoll Ruud

김민성 added 1 comment

Patchset-level comments

김민성 . resolved

Hi, @and...@chromium.org, @cg...@chromium.org. Can I ask how to generate expected images of `third_party/blink/web_tests/css1/text_properties/letter_spacing.html` for every platforms?

Related details

Attention is currently required from:

Anders Hartvoll Ruud

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jason Leo (Gerrit)

unread,

Jun 16, 2025, 4:59:57 AM6/16/25

to 김민성, Olga Gerchikov, Chromium LUCI CQ, Anders Hartvoll Ruud, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Anders Hartvoll Ruud and 김민성

Jason Leo added 1 comment

Patchset-level comments

[1]https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_test_expectations.md#Local-manual-rebaselining
[2]https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_test_expectations.md#Rebaselining-using-try-jobs

김민성 . resolved

Hi, @and...@chromium.org, @cg...@chromium.org. Can I ask how to generate expected images of `third_party/blink/web_tests/css1/text_properties/letter_spacing.html` for every platforms?

Jason Leo

As far as I know, if each platform is the same, you can do this[1] to rebaseline locally, otherwise you may need to use tryjobs[2]. Since you don't have trybot permission, you can also let me to run the cq+1 AND copy the expected image into your local code base of each platform manually.

Related details

Attention is currently required from:

Anders Hartvoll Ruud
김민성

Submit Requirements:

Code-Coverage

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jason Leo (Gerrit)

unread,

Jun 16, 2025, 5:03:07 AM6/16/25

to 김민성, Olga Gerchikov, Chromium LUCI CQ, Anders Hartvoll Ruud, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Anders Hartvoll Ruud and 김민성

Jason Leo added 1 comment

Patchset-level comments

김민성 . resolved

Hi, @and...@chromium.org, @cg...@chromium.org. Can I ask how to generate expected images of `third_party/blink/web_tests/css1/text_properties/letter_spacing.html` for every platforms?

Jason Leo

As far as I know, if each platform is the same, you can do this[1] to rebaseline locally, otherwise you may need to use tryjobs[2]. Since you don't have trybot permission, you can also let me to run the cq+1 AND copy the expected image into your local code base of each platform manually.

[1]https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_test_expectations.md#Local-manual-rebaselining
[2]https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_test_expectations.md#Rebaselining-using-try-jobs

Jason Leo

You can click the checks tab to see all checks result, and click one of them, you'll see there's `actual_image`, that is the new baseline you want to replace.

satisfied_requirement

unsatisfied_requirement

open

diffy

김민성 (Gerrit)

unread,

Jun 16, 2025, 5:21:11 AM6/16/25

to Olga Gerchikov, Jason Leo, Chromium LUCI CQ, Anders Hartvoll Ruud, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Anders Hartvoll Ruud and Jason Leo

김민성 added 1 comment

Patchset-level comments

김민성 . resolved

Hi, @and...@chromium.org, @cg...@chromium.org. Can I ask how to generate expected images of `third_party/blink/web_tests/css1/text_properties/letter_spacing.html` for every platforms?

Jason Leo

As far as I know, if each platform is the same, you can do this[1] to rebaseline locally, otherwise you may need to use tryjobs[2]. Since you don't have trybot permission, you can also let me to run the cq+1 AND copy the expected image into your local code base of each platform manually.

[1]https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_test_expectations.md#Local-manual-rebaselining
[2]https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_test_expectations.md#Rebaselining-using-try-jobs

Jason Leo

You can click the checks tab to see all checks result, and click one of them, you'll see there's `actual_image`, that is the new baseline you want to replace.

김민성

Thanks for prompt reply and explanation kindly!
I understand. The each expected result looks different per platform. Would you vote cq+1, please?

File-level comment, Patchset 6 (Latest):

Related details

Attention is currently required from:

Anders Hartvoll Ruud
Jason Leo

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

김민성 (Gerrit)

unread,

Jun 17, 2025, 6:41:03 AM6/17/25

to Jason Leo, Olga Gerchikov, Chromium LUCI CQ, Anders Hartvoll Ruud, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, mac-r...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Anders Hartvoll Ruud and Jason Leo

김민성 added 1 comment

Patchset-level comments

김민성 . resolved

I've updated this CL. Would you vote `cq +1` and review this, please?

File-level comment, Patchset 7 (Latest):

Related details

Attention is currently required from:

Anders Hartvoll Ruud
Jason Leo

Submit Requirements:

Code-Coverage

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

김민성 (Gerrit)

unread,

Jun 17, 2025, 9:47:22 AM6/17/25

to Jason Leo, Olga Gerchikov, Chromium LUCI CQ, Anders Hartvoll Ruud, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, mac-r...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Anders Hartvoll Ruud and Jason Leo

김민성 added 1 comment

Patchset-level comments

김민성 . resolved

Hi @cg...@chromium.org. Would you vote cq+1, please? I've fixed the test by removing redundant affected baselines.

Related details

Attention is currently required from:

Anders Hartvoll Ruud
Jason Leo

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Anders Hartvoll Ruud (Gerrit)

unread,

Jun 18, 2025, 2:56:22 AM6/18/25

to 김민성, Jason Leo, Olga Gerchikov, Chromium LUCI CQ, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, mac-r...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from 김민성

Anders Hartvoll Ruud added 1 comment

Commit Message

To achieve this, the type of 'letter-spacing' in FontDescription is
changed from float to Length, allowing proper inheritance and improved
compatibility with computed style resolution.

Anders Hartvoll Ruud . unresolved

I'm not sure why need to keep percentages around on the computed style?

https://www.w3.org/TR/css-text-4/#letter-spacing-property says percentages resolve against the computed font-size, which means we can and should resolve them earlier.

I see that we have a WPT with e.g. the following assertion:

```
test_computed_value("letter-spacing", "110%");
```

I don't see how this can be correct. @ko...@chromium.org: thoughts on this?

Related details

Attention is currently required from:

김민성

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

김민성 (Gerrit)

unread,

Jun 18, 2025, 3:11:47 AM6/18/25

to Jason Leo, Olga Gerchikov, Chromium LUCI CQ, Anders Hartvoll Ruud, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, mac-r...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Anders Hartvoll Ruud

김민성 added 1 comment

Commit Message

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/css-text/letter-spacing/letter-spacing-percent-001.html;l=1-24

To achieve this, the type of 'letter-spacing' in FontDescription is
changed from float to Length, allowing proper inheritance and improved
compatibility with computed style resolution.

Anders Hartvoll Ruud . unresolved

I'm not sure why need to keep percentages around on the computed style?
https://www.w3.org/TR/css-text-4/#letter-spacing-property says percentages resolve against the computed font-size, which means we can and should resolve them earlier.
I see that we have a WPT with e.g. the following assertion:
```
test_computed_value("letter-spacing", "110%");
```
I don't see how this can be correct. @ko...@chromium.org: thoughts on this?

김민성

I think the `letter-spacing` should be computed when child doesn't have `letter-spacing` but parent has, and their font size is different.

The `letter-spacing-percent-001.html` test will be failed if we don't keep the percentages.
Here is failure cases.
```
<style type='text/css'>
	div { font-size: 20px; line-height: 1; color: blue; }
	small { font-size: 50%; }
</style>

<div style="letter-spacing: 10%">ABC123().*$いろはx x&nbsp;x፡x་x
	<small>ABC123().*$いろはx x&nbsp;x፡x་x</small></div>
	
...
	
<div style="letter-spacing: 10%; font-size: 0.1em"><div style="font-size: 20px">ABC123().*$いろはx x&nbsp;x፡x་x
	<small>ABC123().*$いろはx x&nbsp;x፡x་x</small></div></div>
```

Related details

Attention is currently required from:

Anders Hartvoll Ruud

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Anders Hartvoll Ruud (Gerrit)

unread,

Jun 18, 2025, 3:15:06 AM6/18/25

to 김민성, Jason Leo, Olga Gerchikov, Chromium LUCI CQ, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, mac-r...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from 김민성

Anders Hartvoll Ruud added 1 comment

Commit Message

To achieve this, the type of 'letter-spacing' in FontDescription is
changed from float to Length, allowing proper inheritance and improved
compatibility with computed style resolution.

Anders Hartvoll Ruud . unresolved

I'm not sure why need to keep percentages around on the computed style?
https://www.w3.org/TR/css-text-4/#letter-spacing-property says percentages resolve against the computed font-size, which means we can and should resolve them earlier.
I see that we have a WPT with e.g. the following assertion:
```
test_computed_value("letter-spacing", "110%");
```
I don't see how this can be correct. @ko...@chromium.org: thoughts on this?

김민성

I think the `letter-spacing` should be computed when child doesn't have `letter-spacing` but parent has, and their font size is different.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/css-text/letter-spacing/letter-spacing-percent-001.html;l=1-24
The `letter-spacing-percent-001.html` test will be failed if we don't keep the percentages.
Here is failure cases.
```
<style type='text/css'>
	div { font-size: 20px; line-height: 1; color: blue; }
	small { font-size: 50%; }
</style>
<div style="letter-spacing: 10%">ABC123().*$いろはx x&nbsp;x፡x་x
	<small>ABC123().*$いろはx x&nbsp;x፡x་x</small></div>
	
...
	
<div style="letter-spacing: 10%; font-size: 0.1em"><div style="font-size: 20px">ABC123().*$いろはx x&nbsp;x፡x་x
	<small>ABC123().*$いろはx x&nbsp;x፡x་x</small></div></div>
```

Anders Hartvoll Ruud

Never mind, the spec says "Percentages inherit intact" in a note, I just missed it. You're right.

Related details

Attention is currently required from:

김민성

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

김민성 (Gerrit)

unread,

Jun 18, 2025, 3:31:49 AM6/18/25

to Jason Leo, Olga Gerchikov, Chromium LUCI CQ, Anders Hartvoll Ruud, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, mac-r...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Anders Hartvoll Ruud

김민성 added 1 comment

Commit Message

To achieve this, the type of 'letter-spacing' in FontDescription is
changed from float to Length, allowing proper inheritance and improved
compatibility with computed style resolution.

Anders Hartvoll Ruud . resolved

I'm not sure why need to keep percentages around on the computed style?
https://www.w3.org/TR/css-text-4/#letter-spacing-property says percentages resolve against the computed font-size, which means we can and should resolve them earlier.
I see that we have a WPT with e.g. the following assertion:
```
test_computed_value("letter-spacing", "110%");
```
I don't see how this can be correct. @ko...@chromium.org: thoughts on this?

김민성

I think the `letter-spacing` should be computed when child doesn't have `letter-spacing` but parent has, and their font size is different.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/css-text/letter-spacing/letter-spacing-percent-001.html;l=1-24
The `letter-spacing-percent-001.html` test will be failed if we don't keep the percentages.
Here is failure cases.
```
<style type='text/css'>
	div { font-size: 20px; line-height: 1; color: blue; }
	small { font-size: 50%; }
</style>
<div style="letter-spacing: 10%">ABC123().*$いろはx x&nbsp;x፡x་x
	<small>ABC123().*$いろはx x&nbsp;x፡x་x</small></div>
	
...
	
<div style="letter-spacing: 10%; font-size: 0.1em"><div style="font-size: 20px">ABC123().*$いろはx x&nbsp;x፡x་x
	<small>ABC123().*$いろはx x&nbsp;x፡x་x</small></div></div>
```

Anders Hartvoll Ruud

Never mind, the spec says "Percentages inherit intact" in a note, I just missed it. You're right.

김민성

Thanks for clarify!

File-level comment, Patchset 7 (Latest):

Related details

Attention is currently required from:

Anders Hartvoll Ruud

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Anders Hartvoll Ruud (Gerrit)

unread,

Jun 18, 2025, 3:35:40 AM6/18/25

to 김민성, Jason Leo, Olga Gerchikov, Chromium LUCI CQ, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, mac-r...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from 김민성

Anders Hartvoll Ruud added 5 comments

Patchset-level comments

Anders Hartvoll Ruud . resolved

Looks pretty reasonable overall.

File third_party/blink/renderer/core/css/properties/css_parsing_utils.cc

Line 8412, Patchset 7 (Latest):// TODO(Minseong): Merge ParseLetterSpacing and ParseWordSpacing if percentage

Anders Hartvoll Ruud . unresolved

Please use `TODO(crbug.com/ABC)`.

File third_party/blink/renderer/core/css/resolver/style_builder_converter.cc

Line 2529, Patchset 7 (Latest):// TODO(Minseong): Merge ConvertLetterSpacing and ConvertWordSpacing if

Anders Hartvoll Ruud . unresolved

crbug

Line 2538, Patchset 7 (Latest):

  const CSSPrimitiveValue& primitive_value = To<CSSPrimitiveValue>(value);
  if (RuntimeEnabledFeatures::CSSLetterSpacingPercentageEnabled() &&
      primitive_value.IsPercentage()) {
    return Length::Percent(
        primitive_value.ComputePercentage(AdjustedZoomConversionData(state)));
  }
  if (RuntimeEnabledFeatures::CSSLetterSpacingPercentageEnabled() &&
      primitive_value.IsCalculated()) {
    const auto* calculation_value =
        To<CSSMathFunctionValue>(primitive_value)
            .ToCalcValue(AdjustedZoomConversionData(state));
    if (primitive_value.HasPercentage()) {
      return Length(calculation_value);
    }
    float computed_font_size =
        state.StyleBuilder().GetFontDescription().ComputedSize();
    return Length::Fixed(ValueForLength(Length(calculation_value),
                                        LayoutUnit(computed_font_size)));
  }
  return Length::Fixed(
      primitive_value.ComputeLength<float>(state.CssToLengthConversionData()));

Anders Hartvoll Ruud . unresolved

Shouldn't we just be using `StyleBuilderConverter::ConvertLength` for all of this?

File third_party/blink/renderer/platform/fonts/font_description_test.cc

Line 653, Patchset 7 (Latest): PixelsAndPercent(20.0, 50.0, /* has_explicit_pixels */ true,

Anders Hartvoll Ruud . unresolved

We should use this style so that clang-tidy can understand it:

```suggestion
      PixelsAndPercent(20.0, 50.0, /*has_explicit_pixels=*/ true,
```

Related details

Attention is currently required from:

김민성

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

김민성 (Gerrit)

unread,

Jun 18, 2025, 3:59:12 AM6/18/25

to Jason Leo, Olga Gerchikov, Chromium LUCI CQ, Anders Hartvoll Ruud, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, mac-r...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Anders Hartvoll Ruud

김민성 added 5 comments

Patchset-level comments

김민성 . resolved

Thanks for your review! I've updated this.

File third_party/blink/renderer/core/css/properties/css_parsing_utils.cc

Line 8412, Patchset 7:// TODO(Minseong): Merge ParseLetterSpacing and ParseWordSpacing if percentage

Anders Hartvoll Ruud . resolved

Please use `TODO(crbug.com/ABC)`.

김민성

Done

File third_party/blink/renderer/core/css/resolver/style_builder_converter.cc

Line 2529, Patchset 7:// TODO(Minseong): Merge ConvertLetterSpacing and ConvertWordSpacing if

Anders Hartvoll Ruud . resolved

crbug

김민성

Done

Line 2538, Patchset 7: const CSSPrimitiveValue& primitive_value = To<CSSPrimitiveValue>(value);


  if (RuntimeEnabledFeatures::CSSLetterSpacingPercentageEnabled() &&
      primitive_value.IsPercentage()) {
    return Length::Percent(
        primitive_value.ComputePercentage(AdjustedZoomConversionData(state)));
  }
  if (RuntimeEnabledFeatures::CSSLetterSpacingPercentageEnabled() &&
      primitive_value.IsCalculated()) {
    const auto* calculation_value =
        To<CSSMathFunctionValue>(primitive_value)
            .ToCalcValue(AdjustedZoomConversionData(state));
    if (primitive_value.HasPercentage()) {
      return Length(calculation_value);
    }
    float computed_font_size =
        state.StyleBuilder().GetFontDescription().ComputedSize();
    return Length::Fixed(ValueForLength(Length(calculation_value),
                                        LayoutUnit(computed_font_size)));
  }
  return Length::Fixed(
      primitive_value.ComputeLength<float>(state.CssToLengthConversionData()));

Anders Hartvoll Ruud . resolved

Shouldn't we just be using `StyleBuilderConverter::ConvertLength` for all of this?

김민성

Oh, yes we can use. Thanks!

File third_party/blink/renderer/platform/fonts/font_description_test.cc

Line 653, Patchset 7: PixelsAndPercent(20.0, 50.0, /* has_explicit_pixels */ true,

Anders Hartvoll Ruud . resolved

We should use this style so that clang-tidy can understand it:
```suggestion
      PixelsAndPercent(20.0, 50.0, /*has_explicit_pixels=*/ true,
```

김민성

Done

Related details

Attention is currently required from:

Anders Hartvoll Ruud

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Anders Hartvoll Ruud (Gerrit)

unread,

Jun 18, 2025, 4:37:04 AM6/18/25

to 김민성, Jason Leo, Olga Gerchikov, Chromium LUCI CQ, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, mac-r...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from 김민성

Anders Hartvoll Ruud voted Code-Review+1

Code-Review

+1

Related details

Attention is currently required from:

김민성

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jason Leo (Gerrit)

unread,

Jun 18, 2025, 4:44:38 AM6/18/25

to 김민성, Anders Hartvoll Ruud, Olga Gerchikov, Chromium LUCI CQ, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, mac-r...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from 김민성

Jason Leo voted and added 1 comment

Votes added by Jason Leo

Code-Review	+1
Commit-Queue	+2

1 comment

Patchset-level comments

Jason Leo . resolved

LGTM

Related details

Attention is currently required from:

김민성

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Jason Leo (Gerrit)

unread,

Jun 18, 2025, 4:58:11 AM6/18/25

to 김민성, Anders Hartvoll Ruud, Olga Gerchikov, Chromium LUCI CQ, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, mac-r...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from 김민성

Jason Leo added 1 comment

Patchset-level comments

Jason Leo . unresolved

Just a reminder: do we need a I2S process for this? @and...@chromium.org

Related details

Attention is currently required from:

김민성

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Anders Hartvoll Ruud (Gerrit)

unread,

Jun 18, 2025, 5:04:10 AM6/18/25

to 김민성, Jason Leo, Olga Gerchikov, Chromium LUCI CQ, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, mac-r...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Jason Leo and 김민성

Anders Hartvoll Ruud added 1 comment

Patchset-level comments

Jason Leo . resolved

Just a reminder: do we need a I2S process for this? @and...@chromium.org

Anders Hartvoll Ruud

Not to land this CL specifically, since it's behind a flag with status:experimental.

But yeah, to set status:stable, we do need an approved I2S.

Related details

Attention is currently required from:

Jason Leo
김민성

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jason Leo (Gerrit)

unread,

Jun 18, 2025, 5:05:37 AM6/18/25

to 김민성, Anders Hartvoll Ruud, Olga Gerchikov, Chromium LUCI CQ, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, mac-r...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from 김민성

Jason Leo voted and added 1 comment

Votes added by Jason Leo

Code-Review	+1
Commit-Queue	+2

1 comment

Patchset-level comments

Jason Leo . resolved

Just a reminder: do we need a I2S process for this? @and...@chromium.org

Anders Hartvoll Ruud

Not to land this CL specifically, since it's behind a flag with status:experimental.
But yeah, to set status:stable, we do need an approved I2S.

Jason Leo

Got you

Related details

Attention is currently required from:

김민성

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Jun 18, 2025, 6:25:44 AM6/18/25

to 김민성, Jason Leo, Anders Hartvoll Ruud, Olga Gerchikov, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, mac-r...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

Support percentage values for letter-spacing

This CL implements support for percentage values in the 'letter-spacing'
property, as defined in the CSS Text Module Level 4 spec.



To achieve this, the type of 'letter-spacing' in FontDescription is
changed from float to Length, allowing proper inheritance and improved
compatibility with computed style resolution.

Note: This CL does not implement percentage handling for 'word-spacing'.
Spec: https://www.w3.org/TR/css-text-4/#letter-spacing-property

Bug: 327740939

Change-Id: I9e9fd7ba01289755ec264aa8fb9ea7b5a9416714

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6636283

Reviewed-by: Jason Leo <cg...@chromium.org>

Commit-Queue: Jason Leo <cg...@chromium.org>

Reviewed-by: Anders Hartvoll Ruud <and...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1475486}

Files:

M third_party/blink/renderer/build/scripts/core/style/templates/computed_style_initial_values.h.tmpl
M third_party/blink/renderer/core/css/css_properties.json5
M third_party/blink/renderer/core/css/css_property_equality.cc
M third_party/blink/renderer/core/css/properties/css_parsing_utils.cc
M third_party/blink/renderer/core/css/properties/css_parsing_utils.h
M third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc
M third_party/blink/renderer/core/css/resolver/style_adjuster.cc
M third_party/blink/renderer/core/css/resolver/style_builder_converter.cc
M third_party/blink/renderer/core/css/resolver/style_builder_converter.h
M third_party/blink/renderer/core/html/html_permission_element.cc
M third_party/blink/renderer/core/layout/svg/layout_svg_inline_text.cc
M third_party/blink/renderer/core/style/computed_style.h
M third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d_state.cc
M third_party/blink/renderer/platform/exported/web_font_description.cc
M third_party/blink/renderer/platform/fonts/fallback_list_composite_key_test.cc
M third_party/blink/renderer/platform/fonts/font_description.cc
M third_party/blink/renderer/platform/fonts/font_description.h
M third_party/blink/renderer/platform/fonts/font_description_test.cc
M third_party/blink/renderer/platform/fonts/shaping/harfbuzz_shaper_test.cc
M third_party/blink/renderer/platform/fonts/shaping/shape_result_test.cc
M third_party/blink/renderer/platform/geometry/length.cc
M third_party/blink/renderer/platform/geometry/length.h
M third_party/blink/renderer/platform/runtime_enabled_features.json5
M third_party/blink/web_tests/TestExpectations
M third_party/blink/web_tests/css1/text_properties/letter_spacing.html
M third_party/blink/web_tests/css2.1/t1604-c542-letter-sp-01-b-a.html
M third_party/blink/web_tests/css3/calc/letter-spacing.html
D third_party/blink/web_tests/external/wpt/css/css-text/parsing/letter-spacing-computed-expected.txt
D third_party/blink/web_tests/external/wpt/css/css-text/parsing/letter-spacing-valid-expected.txt
D third_party/blink/web_tests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/letter-spacing-expected.txt
D third_party/blink/web_tests/external/wpt/css/css-values/animations/calc-interpolation-expected.txt
D third_party/blink/web_tests/external/wpt/css/css-values/calc-letter-spacing-expected.txt
D third_party/blink/web_tests/external/wpt/css/css-values/hypot-pow-sqrt-serialize-expected.txt
M third_party/blink/web_tests/platform/linux/css1/text_properties/letter_spacing-expected.png
M third_party/blink/web_tests/platform/mac/css1/text_properties/letter_spacing-expected.png
M third_party/blink/web_tests/platform/win/css1/text_properties/letter_spacing-expected.png
D third_party/blink/web_tests/platform/win10/css1/text_properties/letter_spacing-expected.png

Change size: L

Delta: 37 files changed, 160 insertions(+), 118 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Anders Hartvoll Ruud, +1 by Jason Leo

File-level comment, Patchset 1 (Latest):

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

luci-bisection@appspot.gserviceaccount.com (Gerrit)

unread,

Jun 18, 2025, 11:09:50 AM6/18/25

to 김민성, Chromium LUCI CQ, Jason Leo, Anders Hartvoll Ruud, Olga Gerchikov, Fredrik Söderquist, Dirk Schulze, Stephen Chenney, AyeAye, Alexis Menard, chromium...@chromium.org, mac-r...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, pdr+svgw...@chromium.org, fserb...@chromium.org, blink-rev...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, blink-revie...@chromium.org, blink-reviews-p...@chromium.org, jmedle...@chromium.org, drott+bl...@chromium.org, blink-revi...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

luci-bi...@appspot.gserviceaccount.com has created a revert of this change

Open original change

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

김민성 (Gerrit)

unread,

Jun 18, 2025, 5:56:10 PM6/18/25

to Anders Hartvoll Ruud, Jason Leo, Alexis Menard, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Olga Gerchikov, Stephen Chenney, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, mac-r...@chromium.org, pdr+svgw...@chromium.org, zol...@webkit.org

Attention needed from Anders Hartvoll Ruud and Jason Leo

김민성 added 1 comment

Patchset-level comments

김민성 . resolved

Hi, would you review this, please?
Previously, I mistakenly placed the win-10 image under the win directory, which led to incorrect results. This has been fixed.