Update letterSpacing and wordSpacing to accept String

[yiyix]

• At least two implementers are interested (and none opposed):

  • Chrome
  • Firefox

• Tests are written and can be reviewed and commented upon at:

  • Add tests for letterspacing and wordspacing with different units web-platform-tests/wpt#31735
  • Fix letterSpacing and wordSpacing values web-platform-tests/wpt#32302

• Implementation bugs are filed:

  • Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1263632
  • Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1708327
  • Safari: https://bugs.webkit.org/show_bug.cgi?id=225181

(See WHATWG Working Mode: Changes for more details.)

---

/canvas.html ( diff )

---

/canvas.html ( diff )

[annevk]

Thanks for creating this PR @yiyix.

As far as I know @Kaiido does not speak for Mozilla or Firefox. @jdashg or I do (at times).

Also, this change isn't sufficient. You need to invoke the CSS parser as we do for other canvas members. And we also need to serialize the value in some manner for the getters.

[annevk]

cc @whatwg/canvas

[Kaiido]

Yes, I always speak (maybe too much) only for myself as a web-author, I don't have any affiliation with any vendor.

Regarding the changes I still find the discrepancy between font being converted to px and these values kept as relative <length> a bit surprising ("2vw" may represent two different values entirely just by resizing the window), but overall I don't have strong opinions.

[yiyix]

Thanks for creating this PR @yiyix.

As far as I know @Kaiido does not speak for Mozilla or Firefox. @jdashg or I do (at times).

Thank you for the clarification. What do you think about this change, Define letterSpacing and wordSpacing in terms of String? Or do you think it's better to keep as it is or accept doubles and strings?

Also, this change isn't sufficient. You need to invoke the CSS parser as we do for other canvas members. And we also need to serialize the value in some manner for the getters.

I will re-write this part. Thank you

[Kaiido]

Sorry for this quite sketchy review, I am not sure about all my proposed changes and a review of the review is required.

[domenic]

@Kaiido, it'd be best if you left review to the editors :). I worry that you might end up giving advice which conflicts with our preferences, or applies to a section the editors request be done in a different way entirely, etc. This makes it harder for contributors to understand what is required for merging, and can generate unnecessary work.

[domenic]

Overall it looks like you have the right idea to me, and have found the most relevant algorithms for parsing and serializing. We just need to tighten up the language.

Note that this parse/serialize architecture means that

ctx.letterSpacing = "5PX";
console.assert(ctx.letterSpacing === "5px"); // lowercased!!

will hold. That seems consistent with what we've specified elsewhere, but is worth writing web platform tests for.

[yiyix]

Overall it looks like you have the right idea to me, and have found the most relevant algorithms for parsing and serializing. We just need to tighten up the language.

Note that this parse/serialize architecture means that

ctx.letterSpacing = "5PX";
console.assert(ctx.letterSpacing === "5px"); // lowercased!!

will hold. That seems consistent with what we've specified elsewhere, but is worth writing web platform tests for.

Good point! Tests and Changes are now made.

[domenic]

This looks great. A few style issues and one more substantial issue about the initial value, but after that this is about ready to merge. (I can do a final formatting pass.)

[domenic]

OK, this all looks good! But I realized we forgot to update a couple other parts of the spec which currently refer to letterSpacing/wordSpacing to instead refer to "letter spacing"/"word spacing" (i.e., to refer to the underlying <span data-x="concept-CanvasTextDrawingStyles-letter-spacing">letter spacing</span> private field).

I think the following two places need updating:

• https://html.spec.whatwg.org/#the-canvas-state:dom-context-2d-letterspacing
• https://html.spec.whatwg.org/#text-styles:dom-context-2d-letterspacing-3

After that I think we should be good!

[yiyix]

OK, this all looks good! But I realized we forgot to update a couple other parts of the spec which currently refer to letterSpacing/wordSpacing to instead refer to "letter spacing"/"word spacing" (i.e., to refer to the underlying <span data-x="concept-CanvasTextDrawingStyles-letter-spacing">letter spacing</span> private field).

I think the following two places need updating:

• https://html.spec.whatwg.org/#the-canvas-state:dom-context-2d-letterspacing
  Updated
• https://html.spec.whatwg.org/#text-styles:dom-context-2d-letterspacing-3
  Updated

After that I think we should be good!

This version definitely looks much better than the previous ones. Thank you so much! This should conclude our spec changes for Canvas new API

[Kaiido]

I think the following two places need updating:

• https://html.spec.whatwg.org/#the-canvas-state:dom-context-2d-letterspacing

Is it so? This section gets the "attributes" values, linking to the IDL attributes' getters sounds correct, no? Otherwise I guess this should be moved in a later item like it's done for "The current dash list".
And if this is indeed the correct action, then should we also update strokeStyle, fillStyle, globalCompositeOperation, and filter (maybe in an other PR)?

[domenic]

Yes @Kaiido, this is part of a general program to update the spec to stop relying on JavaScript-modifiable getters and instead get their backing values.