Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a
lang
IDL Attribute to CanvasTextDrawingStyles, and clarify "direction" on same #10873base: main
Are you sure you want to change the base?
Add a
lang
IDL Attribute to CanvasTextDrawingStyles, and clarify "direction" on same #10873Changes from 11 commits
eba506c
caa22ca
488b95b
01e6185
b6df5ee
1b5a1df
152e3a4
60a5971
720ff90
c131fd6
119a4c5
25efd43
369688c
7e5c880
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The /dev/ version of the specs won't have the "below" section, only this note; not sure it's great referencing it that way.
Though I must admit I'm not sure why things like the text-preparation-algo are in the /dev/ version, certainly some cleaning is required around there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost every attribute in this block uses "The possible values are and their meanings are given below." I don't think there's anything to be done without a big refactoring but I will use the same phrasing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an issue with almost every attribute in this section. They use "The possible values and their meanings are given below." Fixing that would be another PR but I'll try to use the same text for lang.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, turns out that these actually are part of the /dev/ version: https://html.spec.whatwg.org/dev/canvas.html#dom-context-2d-direction-inherit.
But there is also some stuff in there that I doubt should have been exposed, like the whole text-preparation-algo, so this indeed might require a new separate PR to clean this up. I think you can ignore for now. The best would probably have been a single sentence description of the behavior, but given its complexity I'm not sure it's doable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though, the "
inherit
" link here is wrong, it links todirection
's "inherit
" value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, all of lang's "inherit" link to the direction one. I'll fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, this doesn't work like
direction
for"inherit"
.If we have something like,
The
direction
used to render the text will bertl
, as determined in the text preparation algorithm, but thefont language
will be"en"
and not"ar"
, because it's directly set during this setter step.So, as currently written, for DOM canvases,
lang
actually has an inherit-only-at-setting behavior whiledirection
has a true inherit behavior.(I think the behavior for OffscreenCanvas is now stable though).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Chrome, at least, the font is re-created before use when any properties that affect the font are changed (letter spacing, font-stretch, font-kerning etc.). The same will happen for language so in your example the behavior will match direction. I'm leveraging the fact that the font section says you must consider the language whenever you set the font, whenever you do update the font.
But yes, the spec does not say anything about this explicitly. The only reference I can find to updating the font is "Font family names must be interpreted in the context of the font style source when the font is to be used". We would need to add something broadening this notion of "when the font is used" and then mention it with all of the font-affecting properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chrome, and I suspect all browsers, currently update the font when any font-affecting properties change (such as letter-spacing, font-kerning, etc). It's not in the spec in any way and I don't want to try to add it now, though I could if folks insist. A change in language should update the font.
There are test like this, for instance, where the font should reflect the value set after the font, and all browsers pass.
ctx.font = '100px serif';
ctx.fontKerning = "none";
Though there do not seem to be explicit dynamic update tests for these properties. I am in the process of adding them for direction and will add for lang too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's in the text-preparation-algorithm (step 5 for these). Every time new text is drawn to the context, or
measureText()
is called, this will be computed again (for the specs, impl. can obviously optimize this).So, given this one and the related review comments, I think I see where is the misunderstanding.
You don't want to handle anything in the
lang
getter and setter, instead you want it to act just likedirection
:And then, you create a new determine the font language1 algo, which would take the
CanvasTextDrawingStyles
as parameter (and maybe itslang
attribute value separately, editors will tell better than me). This algorithm will return either the explicit language, either thefont style source object
's internalinherited language
, or itsfont language
in case it's a<canvas>
.Then, in the text-preparation-algorithm you create a new local variable and set it to the result of running the determine the font language algorithm with
target
and use it where it's needed. At a first glance I'd say the easiest path would be to set theinline-box
'sfont-language-override
CSS property in the step 5, just like it's done for the other attributes, but maybe this property doesn't match entirely with what we want in a way I can't foresee. In this case we'd need a new concept to pass the content language to the CSSinline box
.Footnotes
I let you bike shed the name. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I get your point and matching what I have for direction would make sense. Part of what confused me is that there's a mismatch between the spec and how Chromium at least implements it, because many of the CSS properties in Step 5 of the text preparation algorithm are actually part of step 3, getting the current font.
The problem we run into is that there is no CSS property for language (the override takes the wrong kind of value), but I suppose we can refer to and link to the discussion of the Content Language in CSS.
I'll get another version ready ASAP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see,
font
in step 3 is not an actual font as in an open-type thing that could be used. It's a string that is used later as the value for the CSSfont
shorthand, in the step 5, second row of the table. The actual font selection is done in CSS land.Right... that's unfortunate. Handwavely setting content-language in prose might work though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the latest version I say that the canvas
language
is used as the CSScontent language
with a I linked to the CSS Fonts descriptive block on font localization in which thecontent language
is used, but your link is better.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"above" where? Here we're supposed to already have a value for this
font language
internal value.Should the
lang
setter steps be ran again? With what value? To what effect?Also, is this occurrence of "font language" supposed to link to another concept, maybe in CSS, like for the following line-height and font-size components of the
font
shorthand? Currently it's quite unclear how thisfont language
is being used.CSS has a
font-language-override
property should it use this? Or maybe it's enough to reference the "content language" even though they don't export this term yet?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed it to be explicit as "with the font language as determined by the lang setter steps".
The font selection code in each browser has the notion of the "locale" or "language". It goes through to harfbuzz in Chrome and Core Text in Safari. That's what this is intended to refer to. I'm not sure how to be more specific without referring to an explicit software package. I'll see if there's any way to make it clearer, such as using Core Text's phrasing "Language specifier string to select a font for a particular localization."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #10873 (comment)