[word-space-transform] Implement parsing of `word-space-transform` [chromium/src : main]
Minseong Kim (Gerrit)
Minseong Kim added 1 comment![]( "Open in Gerrit")
Hi! Would you review this CL, please?
Related details
- Ian Kilpatrick
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 10 comments![]( "Open in Gerrit")
for each one of thos keywords.Please fix this WARNING reported by Spellchecker: "thos" is a possible misspelling of "those" or "this".
To bypass Spellchecker, ...
"thos" is a possible misspelling of "those" or "this".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
valid_for_first_letter: true,Does this make sense for ::first-letter? Does the spec list it?
valid_for_marker: true,I think the spec for ::marker has an explicit list for which properties apply. Is this included?
valid_for_page_context: true,Doesn't sound correct?
valid_for_permission_element: true,This is definitely suspicious.
If you add the syntax as a comment, the code is easier to read:
`// none | [ space | ideographic-space ] && auto-phrase?`
list->Append(*auto_phrase);Since you're adding the values as you go, the serialization won't be canonicalized when serializing declaration values, which I think the spec demands in this case:
"If certain component values can appear in any order without changing the meaning of the value (a pattern typically represented by a double bar || in the value syntax), reorder the component values to use the canonical order of component values as given in the property definition table."
I suggest creating the list value after the loop, always with the auto-phrase at the end.
} else if (word_space_transform & kWordSpaceTransformIdeographicSpace) {This could be a pure `else` because of the DCHECK above. Also, if the `else` failed here, you would get a crash in the dereference below.
How about just:
```
CSSValueID space_keyword = (word_space_transform & kWordSpaceTransformSpace) ? CSSValueID::kSpace : CSSValueID::kWordSpaceTransformIdeographicSpace;
CSSValue* space_value = CSSIdentifiedValue::Create(space_keyword);
```
Member<void*> pointers[12];This size change is not acceptable. You need to add this to a `group:` in css_properties.json5. Either to `"*"` or if there is an existing named group for text properties where this naturally fits in.
Related details
- Ian Kilpatrick
- Minseong Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Morten Stenshorne added 1 comment![]( "Open in Gerrit")
valid_for_page_context: true,Doesn't sound correct?
This seems fine. There's a list of properties that must be supported in `@page`, and this one is not on the list. "Behavior for properties not included in CSS 2.1 is undefined.". Still, I think this one should be allowed. There may be text in `@page` margins.
Related details
- Ian Kilpatrick
- Minseong Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
valid_for_page_context: true,Morten StenshorneDoesn't sound correct?
This seems fine. There's a list of properties that must be supported in `@page`, and this one is not on the list. "Behavior for properties not included in CSS 2.1 is undefined.". Still, I think this one should be allowed. There may be text in `@page` margins.
- Ian Kilpatrick
- Minseong Kim
- Morten Stenshorne
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minseong Kim (Gerrit)
Minseong Kim added 9 comments![]( "Open in Gerrit")
for each one of thos keywords.Please fix this WARNING reported by Spellchecker: "thos" is a possible misspelling of "those" or "this".
To bypass Spellchecker, ...
"thos" is a possible misspelling of "those" or "this".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Opps, I missed this report. Thanks.
valid_for_first_letter: true,Minseong KimDoes this make sense for ::first-letter? Does the spec list it?
Hmm, I think this doesn't make sense for ::first-letter and couldn't find this at the spec list. I removed this.
valid_for_first_line: true,Minseong KimLikewise
The [::first-line spec](https://www.w3.org/TR/css-pseudo-4/#first-line-styling) says:
any typesetting properties that apply to inline elements (see [CSS-TEXT-4])
But, I'm not sure the `word-space-transform` can be used in `inline` elements. I removed this and try to open a CSSWG issue to ensure it.
valid_for_marker: true,Minseong KimI think the spec for ::marker has an explicit list for which properties apply. Is this included?
The [spec(CSS Lists and Counters Module Level 3)](https://www.w3.org/TR/css-lists-3/#marker-properties) says:
However, inheritable properties that apply to text can be set on the ::marker pseudo-element: these will inherit to and take effect on its text contents.
white-space, text-transform, letter-spacing (see [CSS-TEXT-3])
But, there is no `word-space-transform` in the spec. So, I removed this and open a CSSWG issue same as the `valid_for_first_line`.
valid_for_permission_element: true,Minseong KimThis is definitely suspicious.
Ok, I removed this. Thanks.
If you add the syntax as a comment, the code is easier to read:
`// none | [ space | ideographic-space ] && auto-phrase?`
Done
list->Append(*auto_phrase);Since you're adding the values as you go, the serialization won't be canonicalized when serializing declaration values, which I think the spec demands in this case:
"If certain component values can appear in any order without changing the meaning of the value (a pattern typically represented by a double bar || in the value syntax), reorder the component values to use the canonical order of component values as given in the property definition table."
I suggest creating the list value after the loop, always with the auto-phrase at the end.
Yes, you're right. I was confused because [some test](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/css-text/parsing/word-space-transform-valid.html;l=19-20) contained the following invalid expectations.
```
test_valid_value("word-space-transform", "auto-phrase space");
test_valid_value("word-space-transform", "auto-phrase ideographic-space");
```
So, I updated the test by adding `serializedValue` parameters. Thanks!
} else if (word_space_transform & kWordSpaceTransformIdeographicSpace) {This could be a pure `else` because of the DCHECK above. Also, if the `else` failed here, you would get a crash in the dereference below.
How about just:
```
CSSValueID space_keyword = (word_space_transform & kWordSpaceTransformSpace) ? CSSValueID::kSpace : CSSValueID::kWordSpaceTransformIdeographicSpace;
CSSValue* space_value = CSSIdentifiedValue::Create(space_keyword);
```
Thanks. The code looks much simpler and readable.
Member<void*> pointers[12];This size change is not acceptable. You need to add this to a `group:` in css_properties.json5. Either to `"*"` or if there is an existing named group for text properties where this naturally fits in.
Done. I also removed the `independent: true`. Thanks for giving the information!
Related details
- Ian Kilpatrick
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
fabian athayde (Gerrit)
fabian athayde added 1 comment![]( "Open in Gerrit")
Message
Pushing emails again there must be a string they are using for that
On Mon, Aug 18, 2025, 10:36 PM Minseong Kim (Gerrit) <noreply-gerritcodereview+4NaADPwo-z39kpmWJ06gtQ==@> wrote: chromium.org
1 comment
Pushing emails again there must be a string they are using for that
On Mon, Aug 18, 2025, 10:36 PM Minseong Kim (Gerrit) <noreply-gerritcodereview+4NaADPwo-z39kpmWJ06gtQ==@> wrote: chromium.org
fabian athayde (Gerrit)
fabian athayde added 1 comment![]( "Open in Gerrit")
Message
And phone mirrored
On Mon, Aug 18, 2025, 10:36 PM Minseong Kim (Gerrit) <noreply-gerritcodereview+4NaADPwo-z39kpmWJ06gtQ==@> wrote: chromium.org
1 comment
And phone mirrored
Rune Lillesveen (Gerrit)
Rune Lillesveen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rune Lillesveen
| Code-Review | +1 |
1 comment
- Ian Kilpatrick
- Minseong Kim
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ian Kilpatrick
| Code-Review | +1 |
1 comment
field_size: 4,Isn't the value space 3 bits?
Related details
- Minseong Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minseong Kim (Gerrit)
Minseong Kim added 1 comment![]( "Open in Gerrit")
Isn't the value space 3 bits?
Oh. Yes, it seems the `3 bits` is right. But, the [win-libfuzzer-asan-rel](https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket/8705972567820416225/+/u/compile/raw_io.output_text_failure_summary_) is failed after changing. How can I fix this failure? I couldn't find the way.
Related details
- Ian Kilpatrick
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Koji Ishii (Gerrit)
Koji Ishii added 1 comment![]( "Open in Gerrit")
Sorry I should have noticed earlier, but I don't think we can implement this property, because ICU doesn't expose the "virtual expandable separators".
Related details
- Ian Kilpatrick
- Minseong Kim
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Koji Ishii (Gerrit)
Koji Ishii added 1 comment![]( "Open in Gerrit")
Sorry I should have noticed earlier, but I don't think we can implement this property, because ICU doesn't expose the "virtual expandable separators".
If you have ideas how to compute it, can you write a one-pager?
Related details
- Ian Kilpatrick
- Minseong Kim
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minseong Kim (Gerrit)
Minseong Kim added 1 comment![]( "Open in Gerrit")
Koji IshiiSorry I should have noticed earlier, but I don't think we can implement this property, because ICU doesn't expose the "virtual expandable separators".
If you have ideas how to compute it, can you write a one-pager?
Does your comment mean that ICU currently lacks the capability to detect phrase boundaries?
Would it be infeasible to leverage technologies like budouX, as mentioned in [this document that you wrote](https://docs.google.com/document/d/1QyPza8XS4aaYD-yA1MHYx56Hy7DZuEm9cAH-A6lTu8c/edit?usp=sharing)?
I would like to clarify what the exact technical constraints are.
If there are hard limitations, would it be acceptable to implement the property without the `auto-phrase` part for now?
Thank you for your attention.
Related details
- Ian Kilpatrick
- Koji Ishii
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Koji Ishii (Gerrit)
Koji Ishii added 1 comment![]( "Open in Gerrit")
Koji IshiiSorry I should have noticed earlier, but I don't think we can implement this property, because ICU doesn't expose the "virtual expandable separators".
Minseong KimIf you have ideas how to compute it, can you write a one-pager?
Does your comment mean that ICU currently lacks the capability to detect phrase boundaries?
Would it be infeasible to leverage technologies like budouX, as mentioned in [this document that you wrote](https://docs.google.com/document/d/1QyPza8XS4aaYD-yA1MHYx56Hy7DZuEm9cAH-A6lTu8c/edit?usp=sharing)?
I would like to clarify what the exact technical constraints are.
If there are hard limitations, would it be acceptable to implement the property without the `auto-phrase` part for now?
Thank you for your attention.
ICU computes all break opportunities, not only the "virtual expandable separators", so you will need to distinguish whether a break opportunity is a normal one or the virtual one. The only way I can think is to run the break iterator twice, once for `auto-phrase` and another without, and subtract the difference, but it's complicated and slow, and I'm not sure if that'd work reliably.
Also, if we succeeded to compute the virtual one, I'm curious how you're planning to insert the space. I guess it'll need to change pre-layout, line breaker, shaping, editing, and maybe more, but there may be other simpler approaches. I'm curious how you're planning to do it.
It'd help us if you can write up your thoughts on a doc.
Related details
- Ian Kilpatrick
- Minseong Kim
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minseong Kim (Gerrit)
Minseong Kim added 1 comment![]( "Open in Gerrit")
Koji IshiiSorry I should have noticed earlier, but I don't think we can implement this property, because ICU doesn't expose the "virtual expandable separators".
Minseong KimIf you have ideas how to compute it, can you write a one-pager?
Koji IshiiDoes your comment mean that ICU currently lacks the capability to detect phrase boundaries?
Would it be infeasible to leverage technologies like budouX, as mentioned in [this document that you wrote](https://docs.google.com/document/d/1QyPza8XS4aaYD-yA1MHYx56Hy7DZuEm9cAH-A6lTu8c/edit?usp=sharing)?
I would like to clarify what the exact technical constraints are.
If there are hard limitations, would it be acceptable to implement the property without the `auto-phrase` part for now?
Thank you for your attention.
ICU computes all break opportunities, not only the "virtual expandable separators", so you will need to distinguish whether a break opportunity is a normal one or the virtual one. The only way I can think is to run the break iterator twice, once for `auto-phrase` and another without, and subtract the difference, but it's complicated and slow, and I'm not sure if that'd work reliably.
Also, if we succeeded to compute the virtual one, I'm curious how you're planning to insert the space. I guess it'll need to change pre-layout, line breaker, shaping, editing, and maybe more, but there may be other simpler approaches. I'm curious how you're planning to do it.
It'd help us if you can write up your thoughts on a doc.
Thank you very much for the detailed explanation.
After reviewing both your comments and the related code, I now understand why distinguishing the “virtual expandable separators” is quite difficult in practice. I agree that the approach would be complicated and may not work reliably.
I really appreciate your insights. For now, I will keep this patch as WIP, since the implementation does not seem feasible with the current constraints.
Thanks again for your guidance.
Related details
- Ian Kilpatrick
- Koji Ishii
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |