[text-emphasis] Place emphasis marks outside ruby annotations [chromium/src : main]
Minseong Kim (Gerrit)
Minseong Kim added 2 comments![]( "Open in Gerrit")
unsigned annotation_height_;Can I ask there is better idea to get the height of the ruby annotation when painting text-emphasis? Currently I added `annotation_height_` in `FragmentItem`, but I'm not sure this is proper way.
Related details
- Kent Tamura
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
- Koji Ishii
- 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. |
Kent Tamura (Gerrit)
Kent Tamura added 1 comment![]( "Open in Gerrit")
unsigned annotation_height_;Can I ask there is better idea to get the height of the ruby annotation when painting text-emphasis? Currently I added `annotation_height_` in `FragmentItem`, but I'm not sure this is proper way.
We'd like to avoid to increase the `FragmentItem` size if possible.
- Can we re-compute it on the paint time? (I guess it's difficult)
- Can we move it to `SvgFragmentData`?
Minseong Kim (Gerrit)
Message from Minseong Kim![]( "Open in Gerrit")
- Kent Tamura
- Koji Ishii
- 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")
unsigned annotation_height_;Kent TamuraCan I ask there is better idea to get the height of the ruby annotation when painting text-emphasis? Currently I added `annotation_height_` in `FragmentItem`, but I'm not sure this is proper way.
We'd like to avoid to increase the `FragmentItem` size if possible.
- Can we re-compute it on the paint time? (I guess it's difficult)
- Can we move it to `SvgFragmentData`?
I moved it to `SvgFragmentData`. Thanks for your suggestions!
Related details
- Kent Tamura
- Koji Ishii
Code-Coverage
Code-Owners
Code-Review
| 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")
Related details
- Kent Tamura
- Koji Ishii
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kent Tamura (Gerrit)
Kent Tamura added 3 comments![]( "Open in Gerrit")
kAnnotationHeightThe fit-text feature and ruby+emphasis should work together on a single FragmentItem.
So, we should have
- A flag whether SVG or not
- A flag whether FitTextInline or not
and we should assume `annotation_height` is available if it's not for an SVG.
Would you split `TextScaleType` into two flags in a separated CL first please?
column->annotation_height = height.LineHeight().Ceil();We usually compute anything in `LayoutUnit` in the layout stage, and convert it to integers in the paint stage. Do we have any specific reason of `Ceil()` here?
Would you add test for nested rubies and text-emphasis on a rt please?
e.g.
```html
<ruby><ruby>base<rt style="text-emphasis: circle;">inner rt</rt></ruby><rt>outer rt <ruby>base<rt>rt</ruby></rt></ruby>
```
Related details
- Koji Ishii
- 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 4 comments![]( "Open in Gerrit")
I updated this CL. Would you please take a look again?
The fit-text feature and ruby+emphasis should work together on a single FragmentItem.
So, we should have
- A flag whether SVG or not
- A flag whether FitTextInline or notand we should assume `annotation_height` is available if it's not for an SVG.
Would you split `TextScaleType` into two flags in a separated CL first please?
The fit-text feature and ruby+emphasis should work together on a single FragmentItem.
I didn't know those should work together. Thanks for your help!
column->annotation_height = height.LineHeight().Ceil();We usually compute anything in `LayoutUnit` in the layout stage, and convert it to integers in the paint stage. Do we have any specific reason of `Ceil()` here?
I don't have any specific reason. I updated all to `LayoutUnit` in the layout stage. Thanks for giving information!
Would you add test for nested rubies and text-emphasis on a rt please?
e.g.
```html
<ruby><ruby>base<rt style="text-emphasis: circle;">inner rt</rt></ruby><rt>outer rt <ruby>base<rt>rt</ruby></rt></ruby>
```
I added the test and the rendering seems incorrect, which means the emphasis marks and ruby annotations are overlapped. But it aligns with Firefox. Is this behavior acceptable to land?
Related details
- Kent Tamura
- Koji Ishii
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kent Tamura (Gerrit)
Kent Tamura added 4 comments![]( "Open in Gerrit")
void SetSvgFragmentData(const FitTextScale* scale,This operation is unrelated to SVG, and we should rename `SVgFragmentData` in the future. So, would you rename this to `SetTextRareData()` please?
LayoutUnit AnnotationHeight() const;This needs a comment.
- What the origin of the height? block-start, block-end, or baseline?
- What happens for now if this FragmentItem is associated to multiple ruby-texts?
- Is this the height of the `over` or `under` annotation?
<div lang="ja">Would you add a larger `font-size` to this element please? The current image results are hard to read.
Minseong KimWould you add test for nested rubies and text-emphasis on a rt please?
e.g.
```html
<ruby><ruby>base<rt style="text-emphasis: circle;">inner rt</rt></ruby><rt>outer rt <ruby>base<rt>rt</ruby></rt></ruby>
```
I added the test and the rendering seems incorrect, which means the emphasis marks and ruby annotations are overlapped. But it aligns with Firefox. Is this behavior acceptable to land?
Yes, it's acceptable. We don't need to care about the behavior of emphasis marks for ruby annotations for now. I believe it's an unusual usage, and rendering emphasis marks for ruby bases is more important.
Related details
- Koji Ishii
- 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 5 comments![]( "Open in Gerrit")
I updated this CL. Would you please review this again?
This operation is unrelated to SVG, and we should rename `SVgFragmentData` in the future. So, would you rename this to `SetTextRareData()` please?
Done
This needs a comment.
- What the origin of the height? block-start, block-end, or baseline?
- What happens for now if this FragmentItem is associated to multiple ruby-texts?
- Is this the height of the `over` or `under` annotation?
Done. Thanks for the guide!
Would you add a larger `font-size` to this element please? The current image results are hard to read.
Done
Minseong KimWould you add test for nested rubies and text-emphasis on a rt please?
e.g.
```html
<ruby><ruby>base<rt style="text-emphasis: circle;">inner rt</rt></ruby><rt>outer rt <ruby>base<rt>rt</ruby></rt></ruby>
```
Kent TamuraI added the test and the rendering seems incorrect, which means the emphasis marks and ruby annotations are overlapped. But it aligns with Firefox. Is this behavior acceptable to land?
Yes, it's acceptable. We don't need to care about the behavior of emphasis marks for ruby annotations for now. I believe it's an unusual usage, and rendering emphasis marks for ruby bases is more important.
- Kent Tamura
- Koji Ishii
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kent Tamura (Gerrit)
Kent Tamura added 2 comments![]( "Open in Gerrit")
// - If both 'over' and 'under' annotations are present, returns their
// united height, which means it cannot distinguish between them.
// - If only one annotation ('over' or 'under') is present, returns itsWhat does it mean by "united height"?
We should have a test case like `<em><ruby><ruby style="ruby-position:under">圏点<rt>けんてん</ruby><rt style="font-size:24px">ケンテン</ruby></em>` and `<em><ruby><ruby style="ruby-position:under">圏点<rt style="font-size:24px">けんてん</ruby><rt>ケンテン</ruby></em>`
// ruby), it calculates the height from the associated ruby column only andDo you mean it calculates the height of the inner-most ruby column? We should have a test case like `<em><ruby><ruby>圏点<rt>けんてん</ruby><rt style="font-size:24px">ケンテン</ruby></em>` and `<em><ruby><ruby>圏点<rt style="font-size:24px">けんてん</ruby><rt>ケンテン</ruby></em>`.
Related details
- Koji Ishii
- 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. |