Canvas Text Writing Mode - prototype [chromium/src : main]
Stephen Chenney (Gerrit)
Stephen Chenney added 2 comments![]( "Open in Gerrit")
This is one more hurdle on the road to launching Extended Text Metrics. Hopefully I've learned some things since adding `lang`.
if ('{{ ctx_writing_mode }}' == 'vertical' && '{{ text_direction }}' == 'rtl')Due to issues with DOM selection rects for RTL upright text, do not try to test it here. I'll try to create some ref tests.
Related details
- Jean-Philippe Gravel
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. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel added 1 comment![]( "Open in Gerrit")
Assigning to Andres, he would be a better reviewer for this change.
Related details
- Andres Ricardo Perez
- Stephen Chenney
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. |
Andres Ricardo Perez (Gerrit)
Andres Ricardo Perez added 7 comments![]( "Open in Gerrit")
This looks really good! I still want to look deeper into parts of the CL but these are my comments so far. I hope to be able to keep looking into it on Monday.
paint_canvas->translate(x, y);
if (use_max_width) {
paint_canvas->scale(1, ClampTo<float>(width / font_width));
}
paint_canvas->rotate(90);
paint_canvas->translate(-x, -y);Can you add a comment explaining why this order of operations is needed for vertical text rendering?
// We draw when fontWidth is 0 so compositing operations (eg, a "copy" op)
// still work. As the width of canvas is scaled, so text can be scaled to
// match the given maxwidth, update text location so it appears on desired
// place.Nit: Not sure if we should move this comment further up now since it still applies for vertical text.
- name: 2d.text.draw.align.start.ltr
desc: textAlign start with ltr is the left edge
canvas: dir="ltr"
test_type: promise
fonts:
- CanvasTest
code: |
{{ load_font }}
ctx.font = '50px CanvasTest';
{% if canvas_type != 'HtmlCanvas' %}
ctx.direction = 'ltr';
{% endif %}
ctx.fillStyle = '#f00';
ctx.fillRect(0, 0, 100, 50);
ctx.fillStyle = '#0f0';
ctx.textAlign = 'start';
ctx.fillText('DD', 0, 37.5);
@assert pixel 5,5 ==~ 0,255,0,255;
@assert pixel 95,5 ==~ 0,255,0,255;
@assert pixel 25,25 ==~ 0,255,0,255;
@assert pixel 75,25 ==~ 0,255,0,255;
@assert pixel 5,45 ==~ 0,255,0,255;
@assert pixel 95,45 ==~ 0,255,0,255;
expected: green
variants:
- *load-font-variant-definitionCould we add versions of these tests to check that the positioning is as expected?
if ('{{ writing_mode }}' == 'horizontal') {These tests can be written using Jinja conditionals instead of having both version in JS. They are used in other places of this yaml file: crsrc.org/c/third_party/blink/web_tests/external/wpt/html/canvas/tools/yaml/text.yaml;l=1478-1481;drc=138bb615274409ac21fffd63f29fe5eba0ee0ad0
That way, the resulting HTML doesn't have an unnecessary arm of the if statement that will never execute.
const text = '横p横p';Do these characters render in the `Ahem` font? From https://github.com/Kozea/Ahem I understand that they would all be the square glyph assigned to `X`.
- name: 2d.text.textOrientation.settings.tentative
desc: Testing value setting of textOrientation in Canvas
code: |
// Initial value is mixed
@assert ctx.textOrientation === "mixed";
// Setting writingMode to valid values
ctx.textOrientation = "upright";
@assert ctx.textOrientation === "upright";
ctx.textOrientation = "sideways";
@assert ctx.textOrientation === "sideways";
ctx.textOrientation = "mixed";
@assert ctx.textOrientation === "mixed";
// Check that likely incorrect values do not apply
ctx.textOrientation = "vertical";
@assert ctx.textOrientation === "mixed";
ctx.textOrientation = "horizontal";
@assert ctx.textOrientation === "mixed";
ctx.textOrientation = "up";
@assert ctx.textOrientation === "mixed";
ctx.textOrientation = "side";
@assert ctx.textOrientation === "mixed";
ctx.textOrientation = "left";
@assert ctx.textOrientation === "mixed";
ctx.textOrientation = "right";
@assert ctx.textOrientation === "mixed";
ctx.textOrientation = "inherit";
@assert ctx.textOrientation === "mixed";
- name: 2d.text.writingMode.settings.tentative
desc: Testing value setting of writingMode in Canvas
code: |
// Initial value is horizontal
@assert ctx.writingMode === "horizontal";
// Setting writingMode to valid values
ctx.writingMode = "vertical";
@assert ctx.writingMode === "vertical";
ctx.writingMode = "horizontal";
@assert ctx.writingMode === "horizontal";
// Check that CSS values do not apply
ctx.writingMode = "vertical-lr";
@assert ctx.writingMode === "horizontal";
ctx.writingMode = "vertical-rl";
@assert ctx.writingMode === "horizontal";
ctx.writingMode = "sideways-lr";
@assert ctx.writingMode === "horizontal";
ctx.writingMode = "sideways-rl";
@assert ctx.writingMode === "horizontal";
// Invalid but likely values
ctx.writingMode = "sideways";
@assert ctx.writingMode === "horizontal";
ctx.writingMode = "inherit";
@assert ctx.writingMode === "horizontal";I think these tests match some tests higher up this file that use a `valid` and `invalid` naming scheme. If they have they same objective, can we have them up with the other ones and use the same naming?
Related details
- Stephen Chenney
Stephen Chenney (Gerrit)
Stephen Chenney added 7 comments![]( "Open in Gerrit")
Thanks for the first pass. It's worth saying there's no big rush because the tests should not land until the Spec PR is landed. I might even be asked to move them out to a separate WPT PR.
I'll work on the things you mentioned in the meantime.
paint_canvas->translate(x, y);
if (use_max_width) {
paint_canvas->scale(1, ClampTo<float>(width / font_width));
}
paint_canvas->rotate(90);
paint_canvas->translate(-x, -y);Can you add a comment explaining why this order of operations is needed for vertical text rendering?
Will do.
// We draw when fontWidth is 0 so compositing operations (eg, a "copy" op)
// still work. As the width of canvas is scaled, so text can be scaled to
// match the given maxwidth, update text location so it appears on desired
// place.Nit: Not sure if we should move this comment further up now since it still applies for vertical text.
Yes, I think it moves up.
Yes, I can do that. It was a bit tricky figuring out all the testing required for his change.
if ('{{ writing_mode }}' == 'horizontal') {These tests can be written using Jinja conditionals instead of having both version in JS. They are used in other places of this yaml file: crsrc.org/c/third_party/blink/web_tests/external/wpt/html/canvas/tools/yaml/text.yaml;l=1478-1481;drc=138bb615274409ac21fffd63f29fe5eba0ee0ad0
That way, the resulting HTML doesn't have an unnecessary arm of the if statement that will never execute.
Thanks. I'll take a look and update.
const text = '横p横p';Do these characters render in the `Ahem` font? From https://github.com/Kozea/Ahem I understand that they would all be the square glyph assigned to `X`.
I did open the Ahem font in a font viewer to check what which glyphs were used, but now I don't recall the result. I'll have to check again.
- Andres Ricardo Perez
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. |
Andres Ricardo Perez (Gerrit)
Andres Ricardo Perez voted and added 5 comments![]( "Open in Gerrit")
Votes added by Andres Ricardo Perez
| Code-Review | +1 |
5 comments
Finally did my second pass. LGTM with some testing thoughts and nits.
// If to the left (or right), clamp to the left (or right) pointI think this comment needs to be rephrased given the possibility of vertical text.
state.GetWritingMode(), 0, text.length());This is originally our fault, but given that you are adding a parameter, could you please add inline comments for `run_start` and `run_end`?
```suggestion
state.GetWritingMode(), /*run_start=*/0, /*run_end=*/text.length());
```
text_cluster->start(), text_cluster->end(), nullptr,Same as above, could you please add the inline comment here too?
```suggestion
text_cluster->start(), text_cluster->end(), /*max_width=*/nullptr,
```
void BaseRenderingContext2D::setTextOrientation(
const V8CanvasTextOrientation orientation) {
UseCounter::Count(GetTopExecutionContext(),
WebFeature::kCanvasRenderingContext2DTextOrientation);
CanvasRenderingContext2DState& state = GetState();
if (!state.HasRealizedFont()) {
setFont(font());
}
if (state.GetTextOrientation() == orientation) {
return;
}
state.SetTextOrientation(orientation.AsEnum(), GetFontSelector());
}
void BaseRenderingContext2D::setWritingMode(
const V8CanvasTextWritingMode mode) {
UseCounter::Count(GetTopExecutionContext(),
WebFeature::kCanvasRenderingContext2DWritingMode);
CanvasRenderingContext2DState& state = GetState();
if (!state.HasRealizedFont()) {
setFont(font());
}
if (state.GetWritingMode() == mode) {
return;
}
state.SetWritingMode(mode.AsEnum(), GetFontSelector());
}Seeing the setters made me think of the relationship of these new properties with `direction`. Does it make sense to have `ctx.direction = 'rtl'`, `ctx.writingMode = 'vertical'`, and `ctx.textOrientation = 'upright`?
Should we have tests for the edge cases with ligatures and other scripts? I found the following W3C guide for vertical upright text in Arabic: https://www.w3.org/TR/alreq/#h_vertical_upright.
I understand the behavior depending from `textOrientation` is handled from the `FontDescription` update, so I'm just wandering if this should be explicitly tested and/or added to the explainer as a potential edge case clarification.
Related details
- Stephen Chenney
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. |