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

[PENDING] fix(svg): use textBaseline specified in text style instead of fixed default value. #763

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

plainheart
Copy link
Collaborator

@plainheart plainheart commented May 22, 2021

THIS IS A PENDING CHANGE

I noticed the text node's position rendered by the SVG renderer currently differs from the canvas renderer.

Here are some cases,

Before After this change

I'm not sure about the reason for setting the dominant-baseline attribute of the text node as a fixed middle(previously) or central(currently) in SVG renderer, but if we use textBaseline specified in text style, it will render the text as the same as the canvas renderer. At least, it will look quite obviously better than now.

@pissang
Copy link
Contributor

pissang commented Jun 1, 2021

@plainheart Sorry about the late reply. Text is centered by adjusting y in

const y = adjustTextY(style.y || 0, getLineHeight(font), style.textBaseline);
manually. I can't remember the reason exactly. Seems it's because other baseline configuration may have unexpected result in some font-family like Microsoft Yahei. @100pah Do you still remember the detail?

@100pah
Copy link
Contributor

100pah commented Jun 1, 2021

I can't remember the reason exactly. Seems it's because other baseline configuration may have unexpected result in some font-family like Microsoft Yahei.

Yes, I remember for Canvas renderer, it seems not easy to control make the same effect when font family changed if using different value of textBaseline.

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

Successfully merging this pull request may close these issues.

3 participants