Improve parsing of the "font" field #1486
Conversation
| return font; | ||
| } | ||
|
|
||
| Font::operator std::string() const |
There was a problem hiding this comment.
Is this re-encoding the original string that was passed into Parse?
There was a problem hiding this comment.
Yeah, my thinking was just to store the font object itself and then re-encode to a string when the getter is called
There was a problem hiding this comment.
Would it make more sense to just store the original string (the one passed to Parse) and return it directly, so that we don't have to maintain code going both ways (rather only the parse direction)?
There was a problem hiding this comment.
Happy to do that. This is closer to how the spec describes the getter for the font field though
| @@ -90,7 +91,7 @@ namespace Babylon::Polyfills::Internal | |||
| NativeCanvas* m_canvas; | |||
| std::shared_ptr<NVGcontext*> m_nvg; | |||
|
|
|||
| std::string m_font{}; | |||
| Font m_font; | |||
There was a problem hiding this comment.
Should this be std::optional so we can easily tell when it is invalid?
There was a problem hiding this comment.
Iirc in the spec if the font is invalid it keeps the old value
There was a problem hiding this comment.
spec: https://html.spec.whatwg.org/multipage/canvas.html#dom-context-2d-font
If the new value is syntactically incorrect [...], then it must be ignored, without assigning a new font value.
Also for above comment:
On getting, the font attribute must return the serialized form of the current font of the context (with no 'line-height' component).
There was a problem hiding this comment.
Is this initialized upon construction of the Canvas, or does it need to be Font m_font{};?
There was a problem hiding this comment.
Ye its initialised to its defaults. I can add the {} to be more explicit if you want though
This improves the handling of the "font" field on the context to properly parse out weight/style/size. Currently weight and style are unused since the font map only takes the family name into consideration.