Skip to content
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

CSS Text Modifiers for Canvas2D #5617

Closed
Tracked by #5613
fserb opened this issue Jun 8, 2020 · 29 comments
Closed
Tracked by #5613

CSS Text Modifiers for Canvas2D #5617

fserb opened this issue Jun 8, 2020 · 29 comments
Labels
addition/proposal New features or enhancements topic: canvas

Comments

@fserb
Copy link
Contributor

fserb commented Jun 8, 2020

Currently, some CSS text rendering properties can't be used for canvas text. Some browsers do support setting them a Canvas style object, for an attached canvas. This spec tries to bring those properties inside canvas.

Working proposal: https://github.com/fserb/canvas2D/blob/master/spec/text-modifiers.md

(cc @whatwg/canvas )

@fserb fserb mentioned this issue Jun 8, 2020
9 tasks
@litherum
Copy link

litherum commented Jun 9, 2020

The WebKit team feels this is a good direction.

@annevk
Copy link
Member

annevk commented May 12, 2021

In principle this was closed by #6560, but it seems there's some late feedback in w3ctag/design-reviews#627 about the naming.

@whatwg/canvas do we want to revisit this?

Personally I'm not convinced that changing the shape of the API is the way to go. CSS properties are themselves exposed in camel case as well through ele.style after all. It does seem reasonable however to keep the names consistent with the names in that API, if possible.

@domenic
Copy link
Member

domenic commented Jun 23, 2021

This was fixed in #6560.

@domenic domenic closed this as completed Jun 23, 2021
@domenic
Copy link
Member

domenic commented Jun 23, 2021

Oh, oops, I see there was discussion about revisiting the naming, hmm...

@domenic domenic reopened this Jun 23, 2021
@domenic
Copy link
Member

domenic commented Jul 22, 2021

@yiyix @mysteryDate is the intention still to change textLetterSpacing and textWordSpacing to letterSpacing and wordSpacing? If so it'd be good to do so before people start shipping the old names.

@yiyix
Copy link
Contributor

yiyix commented Jul 27, 2021

Thank you for the reminder! It is still the plan! I will submit a renaming change and code change.

@yiyix
Copy link
Contributor

yiyix commented Aug 3, 2021

@jdashg , @litherum: Could you comment what do you think about changing textLetterSpacing and textWordSpacing to letterSpacing and wordSpacing? This is proposed here.

@yiyix
Copy link
Contributor

yiyix commented Aug 16, 2021

@annevk, Could you share your opinion on the attribute name changes, i.e. textLetterSpacing -> letterSpacing and textWordSpacing -> wordSpacing? This change is proposed here

@annevk
Copy link
Member

annevk commented Aug 31, 2021

@yiyix I'm on the fence. I can see some value in renaming these, but they do not directly map to CSS properties (e.g., these are doubles and don't accept all the various values they would in CSS). So either way seems okay. If this is already shipping in more than one implementation I don't think we should rename.

@monfera
Copy link

monfera commented Sep 2, 2021

Linking the preexisting discussion OpenType Features and Variable Fonts for Canvas

@yiyix
Copy link
Contributor

yiyix commented Sep 3, 2021

@annevk, based on the status of the two bug I created to add these attributes on firefox and safari, I believe we are still the only browser who has implemented it.

ref bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1708327
https://bugs.webkit.org/show_bug.cgi?id=225181

@annevk
Copy link
Member

annevk commented Sep 6, 2021

@LeaVerou how strongly do you feel about this? Also in light of the type mismatches noted above? It seems like there's room to change this based on @yiyix's comments.

@r12a r12a added i18n-tracker Group bringing to attention of Internationalization, or tracked by i18n but not needing response. and removed i18n-tracker Group bringing to attention of Internationalization, or tracked by i18n but not needing response. labels Sep 6, 2021
@LeaVerou
Copy link

LeaVerou commented Sep 6, 2021

@annevk Please note that the feedback we provided was not just my opinion, but the consensus from the relevant discussion with the rest of the TAG. I can bring it back to the group in light of this new info to discuss again and update the design review thread.

@annevk
Copy link
Member

annevk commented Sep 6, 2021

Thanks @LeaVerou, appreciate it!

@litherum
Copy link

litherum commented Oct 6, 2021

I don't think we have opinions about naming.

@yiyix
Copy link
Contributor

yiyix commented Oct 13, 2021

@annevk, I believe @LeaVerou hold another review meeting and still believe that the naming should be the same as CSS, i.e. textLetterSpacing -> letterSpacing and textWordSpacing -> wordSpacing. Reference

Could you reconsider the attribute name choices?

@annevk
Copy link
Member

annevk commented Oct 14, 2021

@yiyix if Chromium wants to rename these and take the hit, I don't see a problem.

@yiyix
Copy link
Contributor

yiyix commented Nov 3, 2021

There is another issue for letterSpacing and wordSpacing attributes. Now these attributes are defined as double with the default unit px. We believe it might be a good idea to allow web developers to define the spacing in their own unit, ex: 0.2em, so they can define spacing with font related values instead of using absolute spacing.

@Kaiido
Copy link
Member

Kaiido commented Nov 3, 2021

There is another issue for letterSpacing and wordSpacing attributes. Now these attributes are defined as double with the default unit px. We believe it might be a good idea to allow web developers to define the spacing in their own unit, ex: 0.2em, so they can define spacing with font related values instead of using absolute spacing.

I'm personally not really convinced by this idea.
The canvas API is mainly geared toward low level JS arithmetic, having to compose a string every time we want to update the font-size property in an animation is already a pain point. Having more such properties doesn't seem like a very good idea.
Moreover, relative units are a big vector of hard to debug issues. Determining to what a certain unit will be relative to for a canvas is complicated, for instance the 0.2em would represent different values if one were to draw on a disconnected (buffer) canvas, or on an "in-DOM" canvas. (https://jsfiddle.net/dz5ky9La/)
The general advice I see (and gave) is to avoid relative units as much as possible and to stick to px, also as a way to avoid weird interop issues.

I guess it would be interesting to check the usage of such units in the font property today as a mean to check if there is really a demand for it to grow over new properties, is there such statistics available already?

@Kaiido Kaiido reopened this Nov 3, 2021
@fserb
Copy link
Contributor Author

fserb commented Nov 3, 2021

I agree that usually string properties can be annoying. The reason this one may be different, is that letterSpacing/wordSpacing usually make more sense when defined relative to the font size (like with em units).

This case is different from setting the font size, because the value is always in relation to the canvas' font size (so it won't matter if the canvas is disconnected or not).

It's true that this is not a major problem, as anyone can just compute ctx.letterSpacing = 0.2 * myFontSize instead of ctx.letterSpacing = '0.2em'. But there's the case where you could only set letterSpacing once and change font size as it goes.

Do you think it would be better if we allowed both? Numbers as px and string as CSS values?

@Kaiido
Copy link
Member

Kaiido commented Nov 4, 2021

Oh right, em would be relative to the canvas font-size here, I overlooked that.

there's the case where you could only set letterSpacing once and change font size as it goes.

Would that work? Would the internal letterSpacing be parsed again when font changes? What about other relative units?

Do you think it would be better if we allowed both? Numbers as px and string as CSS values?

I thought of this too, but I fear this would make for a very confusing API, no?

Given your arguments, I am less strongly opposed to this idea, I still kind-of dislike having to compose strings, but font-size is probably a bigger issue here, as you said, letterSpacing and wordSpacing should probably keep their relation with font-size in most common cases.
An alternative idea would be to keep a double but make it a ratio relative to the canvas font-size (i.e em) and the ones willing px would have to do the calculations. Though that sounds like just moving the problem somewhere else.

@annevk
Copy link
Member

annevk commented Nov 4, 2021

I don't know how far along CSSOM types are, but they could be another alternative here.

@yiyix
Copy link
Contributor

yiyix commented Nov 5, 2021

Would that work? Would the internal letterSpacing be parsed again when font changes? What about other relative units?

In our current design, after each font change, the letterSpacing and wordSpacing will be re-applied. So all the relative units will be automatically related to the new font.

@yiyix
Copy link
Contributor

yiyix commented Nov 8, 2021

An alternative idea would be to keep a double but make it a ratio relative to the canvas font-size (i.e em) and the ones willing px would have to do the calculations. Though that sounds like just moving the problem somewhere else.

I am fine with this suggestion - taking doubles as em. My only concern is that other canvas attributes take doubles as pixels (although taking letterSpacing as strings are also different from other setting).

cc more people to get more inputs, @domenic, @litherum

@domenic
Copy link
Member

domenic commented Nov 8, 2021

My only concern is that other canvas attributes take doubles as pixels

Yes, this seems like a serious concern to me. I think using doubles as ems for these properties would be bad.

@domenic
Copy link
Member

domenic commented Nov 8, 2021

I don't know how far along CSSOM types are, but they could be another alternative here.

I tried looking through https://drafts.css-houdini.org/css-typed-om . I think this might work?

ctx.letterSpacing = CSS.em(0.2).to("px");

but this is a bit confusing because it seems like it should need a context in which to perform the em -> px conversion... and indeed, Chrome Canary throws when I try CSS.em(0.2).to("px") in the console.

@tabatkins, any hope for something like CSS.em(0.2).to("px", ctx.canvas)?

@Kaiido
Copy link
Member

Kaiido commented Nov 9, 2021

Unless I'm missing something I believe CSS Typed OM types would not be of great help here.
The main goal @yiyix's proposal is trying to reach is to be able to do

ctx.font = "20px sans-serif";
ctx.letterSpacing = "0.2em"; // resolves to 4px
ctx.fillText(txt, x, y);
// ...
// then some times later
ctx.font = "40px sans-serif";
// here letterSpacing is still "0.2em", but it now resolves to 8px
ctx.fillText(txt, x, y);

The conversion em to px is actually quite simple: const px = fontSize * em, what is less simple is to allow the engine to do this conversion automagically without the author having to update all the letterSpacing + wordSpacing every time they change the font-size. Hence my suggestion to define these as ratios of the font-size directly, if this is indeed the most common use for these properties.

But I agree that having only these two properties take in ratios rather than px is disturbing.

In my mind, the side effect of changing ctx.font to also update these properties will also be disturbing when these values are units that are relative to something else than the font-size, e.g vw. (Currently, such units on .font are apparently converted to px internally, meaning they get recalculated only at setting of this property, which is less surprising.)

Now one advantage of accepting a DOMString like .font does, would actually be to allow users to pass in CSSValues directly (their .toString() would get called on that). This might be a good thing, given things like #6609.

@yiyix
Copy link
Contributor

yiyix commented Nov 11, 2021

@Kaiido explained better than I did. Based on our discussion so far, we believe 1. making letterSpacing/wordSpacing takes String and double is confusing to the devs. 2. Having only letterSpacing/wordSpacing taking ration, i.e. em instead of px is prone to mistakes.

Maybe we can make letterSpacing and wordSpacing accept dimension-token in String form? a number with its length unit?

@domenic
Copy link
Member

domenic commented Nov 11, 2021

Switching entirely to string properties seems fine to me. Probably following the model that the font property does.

dandclark pushed a commit to dandclark/html that referenced this issue Dec 4, 2021
textLetterSpacing → letterSpacing, and textWordSpacing → wordSpacing.

Closes whatwg#5617.
asankah pushed a commit to asankah/html that referenced this issue Feb 7, 2022
mfreed7 pushed a commit to mfreed7/html that referenced this issue Jun 3, 2022
textLetterSpacing → letterSpacing, and textWordSpacing → wordSpacing.

Closes whatwg#5617.
mfreed7 pushed a commit to mfreed7/html that referenced this issue Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements topic: canvas
Development

No branches or pull requests

9 participants