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

[canvas] Define OffscreenCanvasRenderingContext2D's font setter in detail #7847

Open
emilio opened this issue Apr 22, 2022 · 16 comments
Open

Comments

@emilio
Copy link
Contributor

emilio commented Apr 22, 2022

https://html.spec.whatwg.org/#dom-context-2d-font right now is very vague for this case. In the main thread, with an actual <canvas> element, we have all the context we need for to resolve all CSS units (needed for font-size): There's a viewport, a font we're inheriting from, a content language, etc.

However in workers lots of these things CSS assumes do not hold / are not present. For example, what should viewport units do? What about writing-mode dependent units? What about font-relative units?

Blink seems to implement something a bit inconsistent, see https://crbug.com/1318927 which @jfkthame just filed. Some relative units work, some don't. Viewport units (if my reading of the code is correct) resolve against zero. It's a bit of a mess.

We have some pre-existing, fairly interoperable code to convert lengths without any context used by DOMMatrix. I propose we align with that and disallow non-absolute sizes in the OffscreenCanvas.font setter, both for simplicity and consistency.

The alternative would be resolving against some sort of initial size (the regular CSS initial size? The canvas 10px initial size?), and then define what happens with all other units (font-metric-dependent units, viewport units, container units...). Given the best we can do is mostly resolving them against some fixed value, and that the failure mode for resolving these units against zero is terrible (no text at all) rather than the "font setter has no effect" that we already define when CSS-wide-keywords are used, I think not supporting non-absolute lengths is better.

I don't have a strong opinion on keyword font-sizes, other than if we do the above, then smaller/bigger should be disallowed, obviously.

Thoughts?

cc @jfkthame, @annevk, @lilles, @domenic

@emilio emilio added topic: canvas agenda+ To be discussed at a triage meeting labels Apr 22, 2022
@emilio
Copy link
Contributor Author

emilio commented Apr 22, 2022

cc @aosmond

@annevk
Copy link
Member

annevk commented Apr 25, 2022

This is all the specification currently says about this:

If the computed values are undefined for a particular case (e.g. because the font style source object is not an element or is not being rendered), then the relative keywords must be interpreted relative to the normal-weight 10px sans-serif default.

That did not account for viewport units existing.

DOMMatrix seems to reject everything non-[absolute]. It seems that might break existing code? Perhaps rejecting anything that's not absolute or not font-relative is reasonable? If we can still share with DOMMatrix I agree that might be nicer.

Also, is the proposal here to do this for workers only or always?

@emilio
Copy link
Contributor Author

emilio commented Apr 25, 2022

DOMMatrix seems to reject everything non-relative.

You mean everything non-absolute, right? But yes.

It seems that might break existing code?

If we were to change this on the main thread perhaps, yeah. In workers, however, there's only one implementation, and its behavior isn't particularly good, so I think it's highly unlikely that it'd break code. I'd argue that rejecting stuff like e.g. viewport units will improve the behavior over accepting it and resolving against zero (which is the current Blink behavior, and would hide the text altogether).

Also, is the proposal here to do this for workers only or always?

The idea would be to change this behavior only for workers. Browsers seem interoperable on the main thread and the current definition seems sensible there, afaict.

@annevk
Copy link
Member

annevk commented Apr 25, 2022

Is it well-defined for the main thread? How do you get the viewport of an arbitrary element? E.g., consider

x = new Document()
c = x.createElementNS("http://www.w3.org/1999/xhtml", "canvas")
console.log(c.getContext("2d"))

And the current definition for font-relative lengths seems sensible either way, especially since you can end up without styling information in the main thread too. Unless that would require special-casing in workers I think I'd rather preserve it to give developers a more uniform experience when it comes to canvas.

@emilio
Copy link
Contributor Author

emilio commented Apr 25, 2022

That (in Gecko, and also in Blink, afaict, since they do have this special resolver just for off-the-main-thread canvas) does require special-casing code in workers. All existing main-thread codepaths do have an answer for the document case (you need an answer for matchMedia(..).matches etc.

At least in Gecko, all the data structures used for stuff like style inheritance and unit resolution access stuff like the document and layout data-structures, so we'd need to special-case or at least factor all that out.

@annevk
Copy link
Member

annevk commented Apr 25, 2022

This also affects letterSpacing and wordSpacing.

cc @yiyix

@yiyix
Copy link
Contributor

yiyix commented Apr 25, 2022

cc @fserb

@Kaiido
Copy link
Member

Kaiido commented May 1, 2022

ctx.filter is also affected, both drop-shadow() and blur() can take relative values.

It seems that the two implementations do differ from the specs, which has a similar font-relative special casing than font, and asks for these units to be computed against the font style source object while both implementations (Firefox and Chrome) actually make these relative to the context's font computed value. https://jsfiddle.net/k6sn30pw/

As for viewport units nothing is being said about it, here either, and Chrome indeed apparently uses 0 as a base there too for OffscreenCanvas.

I guess the easiest move here would be to change the specs to reflect what both implementations do for font-size relative units (i.e base it on ctx.font), which has the advantage of also working for OffscreenCanvas. And for viewport units do the same as what will be decided for font.

@domenic domenic changed the title [canvas] Define OffscreenCanvasRenderingContext2D.font setter in detail [canvas] Define OffscreenCanvasRenderingContext2D's font setter in detail Jun 2, 2022
@past past removed the agenda+ To be discussed at a triage meeting label Jun 2, 2022
@mysteryDate
Copy link
Contributor

I'm getting a dead link for https://drafts.fxtf.org/geometry/#dommatrix-parse

If we were to change this on the main thread perhaps, yeah. In workers, however, there's only one implementation, and its behavior isn't particularly good, so I think it's highly unlikely that it'd break code. I'd argue that rejecting stuff like e.g. viewport units will improve the behavior over accepting it and resolving against zero (which is the current Blink behavior, and would hide the text altogether).

Personally I'd vote for rejecting everything non-absolute on workers, like with DOMMatrix. I'm happy to write a draft for that change based on https://drafts.fxtf.org/geometry/#dommatrix-parse, is there anywhere where that's actually available?

On the main thread, are viewport units the only issue? @annevk your example of:

x = new Document()
c = x.createElementNS("http://www.w3.org/1999/xhtml", "canvas")
console.log(c.getContext("2d"))

A lot of strange stuff happens for font rendering (https://jsfiddle.net/rLy6hnf7/). Chrome doesn't let the default font get set and font rendering does not appear to work at all. Firefox throws and error. Safari works for absolute units but produces strange results with relative units. This seems like perhaps a different issue than workers?

@emilio
Copy link
Contributor Author

emilio commented Sep 1, 2022

@mysteryDate ah, it seems you hit the link when the CSSWG spec was down, seems back now.

Yeah, in general I think the most principled approach is using something like what DOMMatrix is doing, if you could write a spec PR that'd be amazing!

On the main thread, your fiddle seems to work for me in Firefox Nightly at least, but yeah what we're doing wrt viewport units at least doesn't seem to make a lot of sense to me. I'd move that to a different issue, though it's still worth figuring out a path to interop there. In particular, defining CSS on out of document elements is not great (getComputedStyle doesn't work there for example, getComputedStyle(document.createElement("div")).fontSize).

@annevk
Copy link
Member

annevk commented Sep 1, 2022

Perhaps specification-wise the conditional can be whether the canvas is rendered or not? And that would imply that for workers and disconnected elements you'd go down the not rendered code path that rejects non-absolute units and such?

@annevk
Copy link
Member

annevk commented Sep 1, 2022

cc @whatwg/canvas

@Kaiido
Copy link
Member

Kaiido commented Sep 1, 2022

Perhaps specification-wise the conditional can be whether the canvas is rendered or not? And that would imply that for workers and disconnected elements you'd go down the not rendered code path that rejects non-absolute units and such?

That would most-likely not be web-compatible. It's very frequent to use detached canvases (for a lack of a layer API), and having a detached canvas in the same context (document) behave differently than a connected one would be both very surprising, and a change in the current behavior (that one is stable across all UAs).

I guess that would require something more tedious like "belongs to an active Document" or alike (I didn't check the exact definition of "active Document" to be sure it's correct either, just throwing off a potential way toward).

@kdashg
Copy link

kdashg commented Sep 1, 2022

Rejecting non-absolute units when they don't make sense seems best to me!

Something like "belongs to an active Document" is where my mind went first too, but I don't know how this is normally determined in the spec language.

@Kaiido
Copy link
Member

Kaiido commented Sep 2, 2022

I'm not sure whether to throw or simply ignore the value.
On one hand it seems that throwing here could match the other cases where we throw, e.g an invalid color for CanvasGradient#addColorStop, negative radii for all ellipse based drawing methods etc. On the other hand, I think there is not a single [Offscreen]CanvasRenderingContext setter that does throw currently, and it's a bit weird that other invalid values for the same setter don't throw.

Also I'm not entirely sure why we should follow DOMMatrix here, wouldn't FontFaceSet, which is also available in Workers be more appropriate since it does actually parse the same font syntax, and will anyway be used in most cases in conjunction with ctx.font? Though I didn't had the chance to throroughly read the specs there, but it seems to actually do nothing with the font-size value? However I think that between two CSSWG outages, I had a glimpse of a potential algorithm for converting relative font-weight values, which would probably help here too.
And anyway, if we don't follow FontFaceSet, FontFaceSet should probably be updated to match what will be done with ctx.font so that self.fonts.load(font); and ctx.font = font; are consistent.

[Edit]: CSSWG is back. So for FontFaceSet#load() it says

Absolutize all relative lengths against the initial values of the corresponding properties. (For example, a relative font weight like bolder is evaluated against the initial value normal.)

Which if I read it correctly doesn't help much for viewport units.

@jfkthame
Copy link

jfkthame commented May 4, 2023

For the letterSpacing and wordSpacing setters, ISTM that supporting font-relative units (resolved at time of use against the canvas's font) would be highly desirable. It'd be sad if authors can't do something like ctx.letterSpacing = "1ch" and have it work in the "obvious" way relative to the font in use for OffscreenCanvas as well as <canvas> elements.

Other things like viewport-relative units don't make much sense, IMO, and we should probably ignore them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

8 participants