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

Require Pango over "toy" Cairo font methods. Redo the custom font API #715

Merged
merged 14 commits into from
Oct 23, 2016

Conversation

chearon
Copy link
Collaborator

@chearon chearon commented Feb 11, 2016

Custom font problems

I have spent a lot of time on the custom fonts issue plaguing this project. They work for the regular build, but it does not work for the Pango build because of 3 issues:

  • Pango uses the system's font resolver, but there is only code to add custom fonts to FontConfig. There are no win32 calls or calls to CoreText so it could not possibly ever work on OS X or Windows. The following 2 points assume you're on Linux/FontConfig
  • CanvasRenderingContext2d#addFont breaks use of custom fonts unless you put something other than family name as the first argument. How wrong!
  • The family name given to ctx.font needs to be the real family name embedded in the font, not what you pass to new Canvas.Font

TL;DR: the Pango and non-Pango builds work completely differently and have incompatible APIs

Pango

I propose that we require Pango. I think there is a strong case for it:

  • The Cairo font methods currently in use are not supposed to be used seriously.
  • Wherever Cairo is available, Pango is going to be available
  • It's more powerful, so going forward we'll be able to match the HTML5 canvas spec more closely

TL;DR: Pango is better for fonts than Cairo

New custom font API

There will have to be a new API which is Canvas.registerFont(ttf) (equivalent to what people are currently doing, new Canvas.Font('doesnt do anything', 'font.ttf');). It's implemented in Node C++, and will delegate to either FontConfig, CoreText, or WinGDI. Note that the current font API isn't even documented so this isn't the end of the world

Issues this PR addresses

I believe these can all be closed because they become obsolete or because they are fixed: #624, #570, #547, #578, #628, #543, #525, #733, #694, #783, #266

@jakeg
Copy link
Contributor

jakeg commented Feb 11, 2016

Yes, yes. 100x yes. Massive thank you for working on the code for this :D

This is the right decision. It's either this or remove Pango support, and this is clearly the correct option.

I'll try to look over the code in a bit myself.

One question: does this (and how does this) cope with multiple variants (faces) of a font? e.g. different weights and italics? Currently addFace(ttf, weight, italic) is used for that (see #628 (comment)), what would the new usage be?

... actually, I just looked over your changes to font.js and see how you intend to do that. Looks great!

Canvas.registerFont(fontFile('PfennigBold.ttf'));
ctx.font = 'bold 50px Pfennig';

I presume the word 'Pfennig' is the name of the font within the font file and the file name itself is completely irrelevant? Are you certain that in new Canvas.Font('doesnt do anything', 'font.ttf') previously the first argument did indeed do nothing? And that in your new usage registerFont(ttf) will pick up the correct variant/face, including if weights are given as numbers (400, 700 etc) instead of as simply 'bold', 'normal' etc? So the following should also work:

Canvas.registerFont(fontFile('BlahBlah:n7.ttf')); // same font file just renamed
ctx.font = '700 50px Pfennig'; // Pfennig' still works as that's the name within the font file. 700 is alias for bold weight

Once again, huge thank you @chearon !!!


var canvas = new Canvas(320, 320)
var ctx = canvas.getContext('2d')
Canvas.registerFont(fontFile('Pfennig.ttf'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest adding a comment here to explain that the file name itself is irrelevant and that the font is found based on the name of the font in the ttf file itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done in d2d6324 👍

@chearon
Copy link
Collaborator Author

chearon commented Feb 11, 2016

does this (and how does this) cope with multiple variants (faces) of a font

We no longer have any control over that. It's up to the font host layer of the operating system to match the style and weights we give to Pango (and Pango gives to the font layer) with the correct font. It sounds like a bad thing at first but actually that matches the behavior of non-custom fonts, so I think it's better!

I presume the word 'Pfennig' is the name of the font within the font file and the file name itself is completely irrelevant?

Exactly.

Are you certain that in new Canvas.Font('doesnt do anything', 'font.ttf') previously the first argument did indeed do nothing?

I'm certain - if you look at the old code, addFont was implemented in JS. It would add the font to an internal list. When the setter for ctx.font was called later, if it had a matching font that was passed to addFont, it would call ctx._setFace (implemented in C++) instead of ctx._setFont (also C++). Only problem was, the former never had a Pango implementation! So you would end up with nothing happening. #543 would have fixed it, I think, but it didn't quite get the bigger problem

And that in your new usage registerFont(ttf) will pick up the correct variant/face, including if weights are given as numbers (400, 700 etc) instead of as simply 'bold', 'normal' etc? So the following should also work

I believe so, if the font is formatted correctly - would you be able to try that for me?

@chearon
Copy link
Collaborator Author

chearon commented Feb 22, 2016

I just got Canvas.addFont working in between new Canvas calls across all platforms! @LinusU is there any chance of getting this accepted? I know it's a pain because it breaks backwards compatibility, but I believe in this enough to maintain my own fork if I must

@LinusU
Copy link
Collaborator

LinusU commented Feb 23, 2016

I absolutely want to get this merged, just give me some time to look thru the code. Hopefully I'll get some time this weekend. Thank you for your hard work!

@chearon
Copy link
Collaborator Author

chearon commented Feb 24, 2016

Awesome, glad to help!

@LinusU
Copy link
Collaborator

LinusU commented Mar 5, 2016

This looks very awesome, but there is one thing that I don't like:

When you set ctx.font, refer to the styles and the family name as it is embedded in the TTF. If you aren't sure, open the font in FontForge and visit Element -> Font Information and copy the Family Name

I don't have FontForge installed, furthermore, it seems very fragile to rely on the name inside the TTF to be used as an identifier inside the canvas api.

I would like us to be as close to the web canvas as possible. So starting at the @font-face specification, what do you think of an api like this?

// 1
Canvas.registerFontFace({
  fontFamily: 'Awesome Font',
  src: 'fonts/Pfennig.ttf'
})

// 2
Canvas.registerFontFace({
  fontFamily: 'Awesome Font',
  src: 'fonts/PfennigBold.ttf',
  fontWeight: 'bold'
})

ctx.font = '50px "Awesome Font"' // 1
ctx.font = 'bold 50px "Awesome Font"' // 2

@@ -10,7 +10,7 @@

#include "Canvas.h"
#include <stdlib.h>
#include "v8.h"
#include <v8.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch

@chearon
Copy link
Collaborator Author

chearon commented Mar 5, 2016

This looks very awesome, but there is one thing that I don't like

See my comment to @jakeg, it could actually be better, however the main reason is that we are now offloading font resolution to the OS, so that's impossible. Unless I were to parse the font file with FT, modify the family name, then send it to the OS as a font buffer instead of a file. Hmm, that sounds fun. I could take a crack at it if I get time tomorrow.

@chearon
Copy link
Collaborator Author

chearon commented Mar 9, 2016

Been reading a lot about fonts again. I think I would have to add a third party library like opentype.js (quite an impressive library) to parse and write the font to match the user's font spec so that the OS picks it up correctly. I think it would also have to write a new OTF/TTF/WOFF to a temporary directory, which makes me very sad. Does that seem OK?

@chearon chearon force-pushed the prefer-pango branch 2 times, most recently from 3c1b2b9 to e3c2759 Compare March 10, 2016 01:23
@terrancesnyder
Copy link

+1

@LinusU
Copy link
Collaborator

LinusU commented Mar 25, 2016

Hmm, I see the problem, it would be very interesting to see how Firefox made it work since, I believe, that they have a backend for Cairo in Moz2D...

Maybe this is good as it is, I would love some opinions from other maintainers.

This will have to be released under a new major bump since we now require pango.

@LinusU LinusU mentioned this pull request Mar 25, 2016
@chearon
Copy link
Collaborator Author

chearon commented Mar 25, 2016

It would be great if we matched web fonts as closely as possible, it's just going to be hard.

I looked at FX but I can't remember what I found. WebKit holds its own native font handles and does its own selection before it defers to the OS for non webfonts. Which is exactly what we should do since CSS3 specifies the font selection algorithm.

It would involve creating our own PangoFontMap though, which isn't currently possible. Luckily I got in contact with the Pango maintainer so we can get some solid feedback, he's on vacation right now tho

@LinusU
Copy link
Collaborator

LinusU commented Mar 25, 2016

Luckily I got in contact with the Pango maintainer so we can get some solid feedback, he's on vacation right now tho

Mad props, awesome 🙌

Thank you for taking your time on this, it's highly appreciated!

@veltman
Copy link

veltman commented Mar 30, 2016

I've been struggling with this as well. I'm currently using a bunch of OS-specific patch code to harmonize font loading between OS X and Ubuntu, so thanks @chearon for all the work on it.

FWIW I think requiring the font name used to match the family name internal to the font file solves this problem pretty well and is consistent with how it would handle pre-existing fonts. I don't think failing to match the web spec perfectly is a killer, especially if doing so would require writing out new temporary font files to alias them.

@chearon
Copy link
Collaborator Author

chearon commented Mar 30, 2016

NP, I don't think the PangoFontMap thing I was going to try will work since all over the code, Pango expects the system to have all of the fonts.

But I was thinking that I could let the user specify the family name, weight etc, another way: just maintain a mapping between the user-set name and style, and what is actually in the file. Then I could send the right attributes to Pango when user-specified family names are sent to ctx.font.

There would be problems if a user's font shared a family name with the OS, and it would add an external dependency to be able to read the font file. Is that all still better than having no ability to specify family name and style?

@LinusU
Copy link
Collaborator

LinusU commented Mar 30, 2016

@chearon I kind of like that approach actually 👍, I don't think that the conflict part would be an actual problem in most cases...

@veltman
Copy link

veltman commented Mar 30, 2016

Hmmm... it seems like someone trying to add a font with a family name that already exists on the OS would be a somewhat common mistake, moreso than someone needing to add a custom font and being unable to use its actual family name, don't you think?

If there is a descriptive error message that explains the conflict, that seems OK, but if it just fails silently I could imagine that confusing a lot of people who don't have a perfect grasp of the font internals (basically everyone).

@chearon
Copy link
Collaborator Author

chearon commented Mar 30, 2016

Well, the example in this repo uses a renamed font, and it's really common for sites like FontSpring to rename the font in @font-face css that they bundle. I think it's mostly about matching CSS?

I dunno about it being a common mistake though, can't imagine why someone would be registering Helvetica or Arial

Also, registered fonts might take precedence over system fonts, hopefully they do on all systems. I'll check tonight

@veltman
Copy link

veltman commented Apr 12, 2016

Sure someone probably wouldn't register Arial, but isn't it easy to imagine a situation where several developers are working on the same codebase, and some, but not all of them, already have a particular font on their local machines, so some of them would experience a conflict if the font was being manually added from an included font file?

@chearon
Copy link
Collaborator Author

chearon commented Apr 12, 2016

Yeah that's fair, I realized that after my comment. I did get around to testing it, though, and neither Linux or OS X throw an error if you register a duplicate font family name. And even better than that: it chooses the one you registered over the OS font. So there's nothing to worry about. I don't know about Windows, though.

I should have the PangoFontDescription thing done very soon

@veltman
Copy link

veltman commented Apr 12, 2016

Hooray! Thank you again for working on this problem.

chearon and others added 11 commits October 21, 2016 09:20
this has the benefit of really simplifying the code, since we can use
pango_font_description_copy and remove the need for a couple helpers.
there is also no more need for setFontFromState since doing that becomes
a one-liner

the behavior of ctx.font should be unchanged, I tested the default font
as well as save and restore, of course
* registerFont is now implemented as a static member function of the
  Canvas class itself. there is a static list of fonts registered
  on the class, which are picked up by setting the font when necessary.
* register_font will fill in its second arg with a PangoFontDescription
  that should resolve to the file specified
* to do that, freetype is now required again
* Canvas.registerFont is now implemented entirely in C++
* second arg is required and must have at least `family`.
  the others default to 'normal' per w3 fonts specification
* when there is a family match for registered fonts, it
  should be chosen even if weight/style are wrong. this is
  also in the spec
* update example and documentation
OSes will register the font with the preferred family name instead of
the family name if the preferred entry is present
particularly, the wiki links to a GTK build that does not have
g_list_free_full for building on Windows
* so you can now specify ctx.font = 'custom1, arial'; etc and the later
  fonts will be used for glyphs that aren't in the first ones
* i'm now calling them sys_desc and user_desc to distinguish the
  description that matches the font on the system vs the description the
  user passes to Canvas.registerFont
* cleaned up some style
this should make it much less likely registering a font will fail:

1. paths to fonts with .., sym links, etc are resolved before passed to
the OS and freetype

2. when you register a font with identical metadata to one you have
already registered, its description will be adjusted accordingly (if you
call registerFont with name = 'a' then name = 'b', you will then need
to set ctx.font = 'b' to select it)

this commit also fixes some minor memory leaks
@LinusU
Copy link
Collaborator

LinusU commented Oct 23, 2016

This is where we want to go so let's merge this and start testing it out. We can then iterate further in another PR if necessary 🚀

@LinusU LinusU merged commit 7704d8d into Automattic:master Oct 23, 2016
@LinusU
Copy link
Collaborator

LinusU commented Oct 23, 2016

Also, a huge thank you @chearon for putting in the work on this, really great 👍

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.

Cannot use custom font
9 participants