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

CanvasRenderingContext2D.fontStretch - make live example work #20794

Merged
merged 5 commits into from
Sep 23, 2022

Conversation

hamishwillee
Copy link
Collaborator

@hamishwillee hamishwillee commented Sep 16, 2022

Fixes #20208

@Elchi3 and @bsmth - got font stretch working for rendering text.

The trick was knowing that I was working with a font that contained variable font information "for sure". Once I had that, I was able to realize the FontFace name for the font was wrong.

I have split up the example so that there is more explanation of the relevant code.

@hamishwillee hamishwillee requested a review from a team as a code owner September 16, 2022 07:18
@hamishwillee hamishwillee requested review from sideshowbarker and removed request for a team September 16, 2022 07:18
@github-actions github-actions bot added the Content:WebAPI Web API docs label Sep 16, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 16, 2022

Preview URLs

External URLs

URL: /en-US/docs/Web/API/CanvasRenderingContext2D/fontStretch
Title: CanvasRenderingContext2D.fontStretch

(this comment was updated 2022-09-20 07:06:42.239339)

});
We then assign the font face we downloaded to the context, and use the context to draw text to the canvas at each of the keyword stretch levels.
Note that again the size of the desired font is specified.
This does not have to match the loaded font size; but may not render as well if there is a mismatch.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that you can load a font in one size and draw in another.

My understanding is that this is because the font engine can scale the size font it has downloaded. So you will get best result if they loaded and drawn fonts are the same, but it will still work. However I am not 100% certain on this, so if you know differently ....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested with

document.fonts.load("12px Inconsolata").then(
  () => {
    ctx.font = "150px 'Inconsolata'";
    // ...

versus

document.fonts.load("150px Inconsolata").then(
  () => {
    ctx.font = "12px 'Inconsolata'";
    // ...

It doesn't have any visible side-effects of upscaling / downscaling, at least.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I am deleting this and adding the comment above: https://github.com/mdn/content/pull/20794/files#r974955540

Essentially saying it doesn't have to be the same font size, but not explaining why.
That allows us to merge this as "not wrong" and address reasons later.

Suggested change
This does not have to match the loaded font size; but may not render as well if there is a mismatch.

@Elchi3 Do you know someone who could explain this/confirm?

It might well be that it works for this particular type of font, but it might not work in another case. That's why I used the weasel words "but may not render as well if there is a mismatch."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying my luck and pinging @fserb here. Fernando, do loaded font size and ctx specified font sizes have to match? What if they don't?

document.fonts.load("30px Inconsolata").then(
  () => {
    ctx.font = "30px 'Inconsolata'";

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fserb ^^^ Any advice you can give would be much appreciated. I'll merge this next week if we don't get a response.

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Good to see this working. Some comments

});
We then assign the font face we downloaded to the context, and use the context to draw text to the canvas at each of the keyword stretch levels.
Note that again the size of the desired font is specified.
This does not have to match the loaded font size; but may not render as well if there is a mismatch.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right.

document.fonts.load("30px Inconsolata").then(
() => {
ctx.font = "30px 'Inconsolata'";
// Default (normal)
Copy link
Collaborator Author

@hamishwillee hamishwillee Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Elchi3 Leaving this comment - it is not obvious otherwise this is "normal"

Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, Hamish, thanks 👍🏻

@sideshowbarker sideshowbarker removed their request for review September 21, 2022 03:10
Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as well. Let's merge this

@Elchi3 Elchi3 merged commit 8c2fe7b into mdn:main Sep 23, 2022
himanshugarg pushed a commit to himanshugarg/content that referenced this pull request Sep 27, 2022
)

* CanvasRenderingContext2D.fontStretch - fix live example

* Split up example with more cross links showing what is going on

* Apply suggestions from code review

Co-authored-by: Florian Scholz <fs@florianscholz.com>

Co-authored-by: Estelle Weyl <estelle@weyl.org>
Co-authored-by: Florian Scholz <fs@florianscholz.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CanvasRenderingContext2D needs improved fontStretch property example
4 participants