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 3 commits
eba506c
caa22ca
488b95b
01e6185
b6df5ee
1b5a1df
152e3a4
60a5971
720ff90
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.
Should it be mentioned explicitly that if the string used for setting is invalid, then the stored value in the context doesn't change? For most other
CanvasTextDrawingStyles
it isn't needed as they are defined using IDL enums, but for the attributes that need parsing, likeletterSpacing
, the setter steps are outlined and therefore it's clear that an invalid value results in the attribute not being changed.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.
For the HTML lang attribute it is explicitly said that "unknown" values should be passed through unchanged. I believe that's because it's hard to write validation for locale strings. Anyways, I think here we also need to just accept whatever is given.
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: It's a bit confusing to have this algo so close to the setter step. (Actually inside of it since there is no rendered return). It makes it look like it's called at that time only and thus doesn't make
"inherit"
dynamic.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. I'll move it.
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.
Decided against a move because of the way the font setting is described, and because this is input to the font, not the text preparation steps.
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.
It should return https://html.spec.whatwg.org/#language. The
lang
attribute can be specified in the ancestor chain.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 creates race conditions. We could snapshot the language when we create the worker, but I suspect it's not worth the complexity. On the fence as to whether it's worth doing when we create an
OffscreenCanvas
from a<canvas>
.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.
It's easy enough to snapshot the language when the OffscreenCanvas is created, at least in Chromium. I think we should snapshot for all OffscreenCanvas objects to keep things easy to understand. And we should only update the value for a canvas element when the lang attribute is set, plus at creation.
It's moderately expensive to look up the language because it requires walking up the DOM tree evaluated a bunch of conditionals. So snapshotting is also a performance win.
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 the new version for snapshotting language.
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.
Why would that create race conditions? Could you please give an example.
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.
Is "primary language" here the same as CSS's content language?
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 was intending it to mean the "primary language" as used in https://html.spec.whatwg.org/multipage/dom.html#the-lang-and-xml:lang-attributes. The solution is to convert that linked reference to a definition and then refer to the definition in the canvas change.
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 concept has been cleaned up.
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.
Does this work to define the used value here? What if I don't ever set the
font
attribute, but set thelang
one. Will my default sans-serif font be able to get this used value?Would it work better if it was called in text preparation algo instead?
+Nit: might be better as "used value" (without the "-") for easing search in page
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 will take a look at what happens in Chromium for a non-defined font, but regardless I will update this to make it clear what happens when there is no defined font. And yes to moving it to the text preparation algorithm (and the direction resolution) because it does indeed need to be dynamic. I was wondering about the right way to ensure that dynamic changes were handled, and you've answered my question.
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.
Looking at this some more, this section going through getters and setters defines how the font is determined, while the
text preparation algorithm
saysLet font be the current font of target, as given by that object's font attribute.
The
current font
is never really defined. With that in mind, given that thelang
is an input into the font selection process (at least in Chromium), I think the description of how to determine thelang
belongs here, just after the font source determination method.I will add text to clarify that the language applies even for the default font (which it does in Chromium), and also that any inherited value is snapshot when an offscreen canvas is created and for canvas elements is snapshot when the font or lang attributes are set.
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 the new version.
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.
Be aware that https://whatpr.org/html/10873/canvas.html doesn't reflect the last commit's changes. Don't know why.
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 doesn't actually define what the setter does.
I recommend looking at
letterSpacing
andwordSpacing
. We want the same kind of language forlang
anddirection
with the distinction between public API and internal state.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.
Done, I think.
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.
Couple issues here:
letterSpacing
does.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.
Done.
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'm not sure I did "2" as you meant.
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 needs to manipulate an internal concept.
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.
Not sure what you mean here. Has the last set of edits addressed this?
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.
No. If you look at the
letterSpacing
getter it is defined as follows:"letter spacing" there is the internal concept. That is what's missing here. The internal concept can have the same name as the getter, but it is distinct.
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 believe we're talking about the setter here. I feel I'm being slow, but is this sort of what you mean (sans links):
The lang setter steps are to set the value and update the internal font language
I think we agree that the value in
lang
is the string that round-trips, while the internal font language really is internal and not visible to authors.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.
It's about both the getter and the setter. If the internal font language is the concept you could rewrite the getter as:
(Note that you need to use "this" as well. We need to know where to find this internal concept after all.)
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.
The desired behavior for the getter relates to #10884. Do we want to return the "inherit" string or the internal font language? For
direction
Chromium and Webkit does the latter, while Firefox does the former.From a back-compat position it's probably very hard to change
direction
, so maybe we should speclang
anddirection
to do the Chromium/WebKit thing. That's probably what developers most want. But it means no round-tripping.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.
Since there is no interop, is there really a back-compat issue? Has Firefox received bug reports about this?
I am very interested in the results from your counters, but as a web-dev I was very surprised when you metionned Chrome and Safari's behavior for
direction
. I personnaly think it's bad to have the computed value instead of"inherit"
there. We don't have a granular way of resetting the context's attributes, so I wouldn't be surprised someone made their own thing by just reading all the props on the context and setting them back later. Returning the computed value would break that.For
lang
, as currently written,"inherit"
does not update outside of the setter, because the value stored is computed in the setter itself. So in that case it would make sense to return the stored (computed) value, but I think"inherit"
wouldn't be the proper keyword. If"inherit"
really did update even after setting, then returning"inherit"
would make more sense IMO.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.
It will take a couple of months to get counter information, as far as I can tell. I'm not sure what it can tell us about intent or desire, so I'll also try to find some hints from various developer forums (like how many people have asked about this issue, or asked how to get the direction or something like 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.
FWIW, per https://w3ctag.github.io/design-principles/#attributes-like-data, it should roundtrip, if it doesn't it should be, a setter method, and a getter method.
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.
Yeah I tend to agree with @Kaiido as to what we want here. It seems very strange to return the computed value, especially if it's informed by layout. (To be clear, I know how it all works, just speaking from the perspective of HTML standard semantics.)