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

Fix cutting when render text through TTF_RenderText_Solid #5269

Merged
merged 1 commit into from
Jun 13, 2017

Conversation

caiiiycuk
Copy link
Contributor

@caiiiycuk caiiiycuk commented Jun 2, 2017

Hi! I open this PR to fix two problems in SDL RenderText feature, that I found during working with cocos2d. Understanding this problem was very hard to me. So I try to explain better.

filename in TTF_OpenFont

If you pass some custom file name into TTF_OpenFont then size parameter is ignored. filename is stored as font name and later used in TTF_RenderText_Solid as font attribute of canvas: var fontString = h + 'px ' + fontData.name;. If this font name is not known for browser then it renders text with default size! Why? I don't know. But you can easily check, just run test browser.test_sdl_alloctext. It should render text "hello world" with height near 40px, but actually it renders text with much less size. Change "myfont.ttf" to "sans-serif" and you see that it renders text with correct height.

I don't have great idea how to detect is font name is known for browser, so I add warn check if font name is not standard (sans, sans-serif, ...)

cutting text

TTF_RenderText_Solid uses 'top' textBaseline, and we all know that it works different in firefox and other browsers (https://bugzilla.mozilla.org/show_bug.cgi?id=737852). As a result of this bug, text renders correct in Firefox and always cutted in other browser.

FF:
firefox_top

Chrome:
chrome_top

To fix that I suggest to use bottom alignment, I hope it works same in all browsers. Btw with this fix FF and Chrome renders text in same manner.

if (filename != 'serif' && filename != 'sans-serif' &&
filename != 'cursive' && filename != 'fantasy' &&
filename != 'monospace') {
Runtime.warnOnce("TTF_OpenFont may not work correctly with custom font, please use one of ['serif', 'sans-serif', 'cursive', 'fantasy', 'monospace']");
Copy link
Member

Choose a reason for hiding this comment

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

I don't know much about this stuff. Is this list something standard, defined in a spec somewhere? Is it the same across browsers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is generic font-family names (https://www.w3.org/TR/CSS2/fonts.html#generic-font-families). Maybe we need add to filename some generic font family name to fallback. Like this:

'"' + filename +'", serif'

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the link, now I see.

I don't understand your question? Where would the comma-separated list go?

surfData.ctx.fillText(text, 0, 0);
// use bottom alligment, because it works
// same in all browsers, more info here:
// https://bugzilla.mozilla.org/show_bug.cgi?id=737852
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. I know little about this too, but it does sound like your change is a good one.

@caiiiycuk
Copy link
Contributor Author

I fixed the PR to show what I mean.

@kripken
Copy link
Member

kripken commented Jun 13, 2017

I see. Yeah, this looks good to me.

Looks like no one else has concerns, merging.

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.

2 participants