blink/css: Implement text-transform: full-size-kana [chromium/src : main]
Felipe Erias (Gerrit)
Felipe Erias added 1 comment![]( "Open in Gerrit")
Hello,
This CL adds support for `text-transform: full-size-kana` defined in
https://www.w3.org/TR/css-text-3/#valdef-text-transform-full-size-kana
Chrome Status:
https://chromestatus.com/feature/5173990610042880
Thank you very much for your review.
Best,
Felipe
Related details
- Kent Tamura
- Koji Ishii
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kent Tamura (Gerrit)
Kent Tamura added 1 comment![]( "Open in Gerrit")
offset_map->Append(text.length(), transformed_string.length());The `offset_map` won't have correct content with this code, and I think we will have weird behavior on selecting the transformed text.
Entries in TextOffsetMap are necessary only if text offsets in two strings are different. We need to call `TextOffsetMap::Append` only when `*it > 0xffff` and `FullSizeKanaVariant(*it) > 0xffff` are different.
I found `ApplyFullwidthTransform()` below had the same issue.
Related details
- Felipe Erias
- Koji Ishii
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Erias (Gerrit)
Felipe Erias added 1 comment![]( "Open in Gerrit")
offset_map->Append(text.length(), transformed_string.length());The `offset_map` won't have correct content with this code, and I think we will have weird behavior on selecting the transformed text.
Entries in TextOffsetMap are necessary only if text offsets in two strings are different. We need to call `TextOffsetMap::Append` only when `*it > 0xffff` and `FullSizeKanaVariant(*it) > 0xffff` are different.
I found `ApplyFullwidthTransform()` below had the same issue.
Yes, although I think that `ApplyFullwidthTransform()` is not problematic because all the transformations are from BMP to BMP characters so the UTF-16 length remains the same.
Nevertheless, I have replaced the incorrect implementation there with a clarifying comment.
---
In the case of `ApplyFullSizeKanaTransform()`, some of the characters involved are encoded as a surrogate pair so the total length can change.
For example:
```
case 0x1B132: // Hiragana small ko
return 0x3053;
```
In this case, I have updated the code as suggested.
Should we also add a test specifically for this?
Perhaps something similar to `web_tests/editing/selection/modify_move/move-by-character-text-transform.html`?
Related details
- Kent Tamura
- Koji Ishii
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kent Tamura (Gerrit)
Kent Tamura added 1 comment![]( "Open in Gerrit")
offset_map->Append(text.length(), transformed_string.length());Felipe EriasThe `offset_map` won't have correct content with this code, and I think we will have weird behavior on selecting the transformed text.
Entries in TextOffsetMap are necessary only if text offsets in two strings are different. We need to call `TextOffsetMap::Append` only when `*it > 0xffff` and `FullSizeKanaVariant(*it) > 0xffff` are different.
I found `ApplyFullwidthTransform()` below had the same issue.
Yes, although I think that `ApplyFullwidthTransform()` is not problematic because all the transformations are from BMP to BMP characters so the UTF-16 length remains the same.
Nevertheless, I have replaced the incorrect implementation there with a clarifying comment.
---
In the case of `ApplyFullSizeKanaTransform()`, some of the characters involved are encoded as a surrogate pair so the total length can change.
For example:
```
case 0x1B132: // Hiragana small ko
return 0x3053;
```In this case, I have updated the code as suggested.
Should we also add a test specifically for this?
Perhaps something similar to `web_tests/editing/selection/modify_move/move-by-character-text-transform.html`?
You're right. `ApplyFullwidthTransform()` has no problems.
Adding new test cases to `move-by-character-text-transform.html` would be great.
Also, we may add a WPT like `css/css-text/text-transform/math/text-transform-math-auto-003.html`.
Related details
- Felipe Erias
- Koji Ishii
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kent Tamura (Gerrit)
Kent Tamura added 1 comment![]( "Open in Gerrit")
CodePointIterator begin(text);
CodePointIterator end = CodePointIterator::End(text);
for (auto it = begin; it != end; ++it) {
UChar32 code_point = *it;
UChar32 transformed = FullSizeKanaVariant(code_point);nit: We can simplify this to:
```c++
for (UChar32 code_point : text) {
```
Koji Ishii (Gerrit)
Koji Ishii added 1 comment![]( "Open in Gerrit")
UChar32 FullSizeKanaVariant(UChar32 code_point) {nit: How about moving this to the `Character` class?
Related details
- Felipe Erias
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Erias (Gerrit)
Felipe Erias added 3 comments![]( "Open in Gerrit")
UChar32 FullSizeKanaVariant(UChar32 code_point) {nit: How about moving this to the `Character` class?
Yes, that makes sense. I have updated this CL so that `FullSizeKanaVariant` is already in the `Character` class.
I will do the same for the `full-width` mapping in a separate CL.
CodePointIterator begin(text);
CodePointIterator end = CodePointIterator::End(text);
for (auto it = begin; it != end; ++it) {
UChar32 code_point = *it;
UChar32 transformed = FullSizeKanaVariant(code_point);nit: We can simplify this to:
```c++
for (UChar32 code_point : text) {
```
Done
offset_map->Append(text.length(), transformed_string.length());Felipe EriasThe `offset_map` won't have correct content with this code, and I think we will have weird behavior on selecting the transformed text.
Entries in TextOffsetMap are necessary only if text offsets in two strings are different. We need to call `TextOffsetMap::Append` only when `*it > 0xffff` and `FullSizeKanaVariant(*it) > 0xffff` are different.
I found `ApplyFullwidthTransform()` below had the same issue.
Kent TamuraYes, although I think that `ApplyFullwidthTransform()` is not problematic because all the transformations are from BMP to BMP characters so the UTF-16 length remains the same.
Nevertheless, I have replaced the incorrect implementation there with a clarifying comment.
---
In the case of `ApplyFullSizeKanaTransform()`, some of the characters involved are encoded as a surrogate pair so the total length can change.
For example:
```
case 0x1B132: // Hiragana small ko
return 0x3053;
```In this case, I have updated the code as suggested.
Should we also add a test specifically for this?
Perhaps something similar to `web_tests/editing/selection/modify_move/move-by-character-text-transform.html`?
You're right. `ApplyFullwidthTransform()` has no problems.
Adding new test cases to `move-by-character-text-transform.html` would be great.
Also, we may add a WPT like `css/css-text/text-transform/math/text-transform-math-auto-003.html`.
Yes, this CL now includes:
- new cases in `move-by-character-text-transform.html`
- `text-transform-full-size-kana-009.html` testing the behaviour of `Selection.toString()`
Related details
- Kent Tamura
- Koji Ishii
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kent Tamura (Gerrit)
Kent Tamura voted Code-Review+1![]( "Open in Gerrit")
- Felipe Erias
- Koji Ishii
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Koji Ishii (Gerrit)
Koji Ishii voted and added 1 comment![]( "Open in Gerrit")
Votes added by Koji Ishii
| Code-Review | +1 |
Related details
- Felipe Erias
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Erias (Gerrit)
Felipe Erias added 2 comments![]( "Open in Gerrit")
UChar32 FullSizeKanaVariant(UChar32 code_point) {Felipe Eriasnit: How about moving this to the `Character` class?
Yes, that makes sense. I have updated this CL so that `FullSizeKanaVariant` is already in the `Character` class.
I will do the same for the `full-width` mapping in a separate CL.
Done
offset_map->Append(text.length(), transformed_string.length());Felipe EriasThe `offset_map` won't have correct content with this code, and I think we will have weird behavior on selecting the transformed text.
Entries in TextOffsetMap are necessary only if text offsets in two strings are different. We need to call `TextOffsetMap::Append` only when `*it > 0xffff` and `FullSizeKanaVariant(*it) > 0xffff` are different.
I found `ApplyFullwidthTransform()` below had the same issue.
Kent TamuraYes, although I think that `ApplyFullwidthTransform()` is not problematic because all the transformations are from BMP to BMP characters so the UTF-16 length remains the same.
Nevertheless, I have replaced the incorrect implementation there with a clarifying comment.
---
In the case of `ApplyFullSizeKanaTransform()`, some of the characters involved are encoded as a surrogate pair so the total length can change.
For example:
```
case 0x1B132: // Hiragana small ko
return 0x3053;
```In this case, I have updated the code as suggested.
Should we also add a test specifically for this?
Perhaps something similar to `web_tests/editing/selection/modify_move/move-by-character-text-transform.html`?
Felipe EriasYou're right. `ApplyFullwidthTransform()` has no problems.
Adding new test cases to `move-by-character-text-transform.html` would be great.
Also, we may add a WPT like `css/css-text/text-transform/math/text-transform-math-auto-003.html`.
Yes, this CL now includes:
- new cases in `move-by-character-text-transform.html`
- `text-transform-full-size-kana-009.html` testing the behaviour of `Selection.toString()`
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Erias (Gerrit)
Felipe Erias voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/57671.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
blink/css: Implement text-transform: full-size-kana
Supports CSS text-transform: full-size-kana from CSS Text Level 3.
https://www.w3.org/TR/css-text-3/#valdef-text-transform-full-size-kana
This value transforms small kana characters to their full-size
equivalents. It is typically used for ruby annotation text, where small
kana may be rendered at font sizes too small for legibility.
The character mappings follow CSS Text Level 3, Appendix G:
https://www.w3.org/TR/css-text-3/#small-kana
Tests:
external/wpt/css/css-text/text-transform-full-size-kana-001.html
external/wpt/css/css-text/text-transform-full-size-kana-002.html
external/wpt/css/css-text/text-transform-full-size-kana-003.html
external/wpt/css/css-text/text-transform-full-size-kana-004.html
external/wpt/css/css-text/text-transform-full-size-kana-005.html
external/wpt/css/css-text/text-transform-full-size-kana-006.html
external/wpt/css/css-text/text-transform-full-size-kana-007.html
external/wpt/css/css-text/text-transform-full-size-kana-008.html
external/wpt/css/css-text/text-transform-full-size-kana-009.html
editing/selection/modify_move/move-by-character-text-transform.html
- M third_party/blink/renderer/core/css/css_properties.json5
- M third_party/blink/renderer/core/css/css_value_keywords.json5
- M third_party/blink/renderer/core/css/parser/css_parser_fast_paths.cc
- M third_party/blink/renderer/core/style/computed_style.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M third_party/blink/renderer/platform/text/character.cc
- M third_party/blink/renderer/platform/text/character.h
- M third_party/blink/web_tests/NeverFixTests
- M third_party/blink/web_tests/TestExpectations
- M third_party/blink/web_tests/editing/selection/modify_move/move-by-character-text-transform.html
- M third_party/blink/web_tests/external/wpt/css/css-text/parsing/text-transform-computed-expected.txt
- M third_party/blink/web_tests/external/wpt/css/css-text/parsing/text-transform-valid-expected.txt
- A third_party/blink/web_tests/external/wpt/css/css-text/text-transform/text-transform-full-size-kana-009.html
Code-Review: +1 by Koji Ishii, +1 by Kent Tamura
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/57671
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |