-
Notifications
You must be signed in to change notification settings - Fork 668
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
Fonts #1163
Fonts #1163
Conversation
Thanks Ron -- these changes are great. Also very breaking. Adding a comment to the 4.0 issue about prototyping the API changes with VexTab. |
tests/formatter_tests.ts
Outdated
} | ||
function glyphWidth(vexGlyph: string): number { | ||
const glyph: FontGlyph = Flow.DEFAULT_FONT_STACK[0].getGlyphs()[vexGlyph]; | ||
const glyph: FontGlyph = Flow.MUSIC_FONT_STACK[0].getGlyphs()[vexGlyph]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would need updates to the Wiki too, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will make sure to update the wiki.
Are you okay with this change though?
My reasoning is that many classes have properties that include the word "font". But it is not clear whether the font is for music engraving purposes, or for text display purposes.
My initial idea was to have Element have two properties: musicFont
and textFont
.
/** The font used to render musical glyphs (e.g., treble clef).*/
protected musicFont!: Font;
/** Some elements include text. You can customize the font family, size, weight, and style. */
protected textFont?: Required<FontInfo>;
However, there is a descendant of Element that has its own .textFont
property! :-( That class is ChordSymbol.
Perhaps the TextFont class should be renamed (to TextFontMetrics, or TextFontRegistry)? @AaronDavidNewman
Currently in this PR, the Element class has a .musicFont and a .font property, where .font is assumed to be for text purposes.
This is not bad, because CanvasContext and SVGContext also have a .font accessor which is for text rendering purposes, to adhere to the CanvasRenderingContext2D API. But perhaps it would be most clear to rename it to textFont
, and then have ChordSymbol rename its member to textFontMetrics
...
To further improve the naming, I went from DEFAULT_FONT_STACK to DEFAULT_MUSIC_FONT_STACK!
I then realized there are only three properties starting with DEFAULT_
, even though there are all sorts of defaults all over VexFlow. So I figured we could drop the DEFAULT_ prefix for those three (all related to music fonts).
The breaking changes will be added to the CHANGELOG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AaronDavidNewman to take a look too please
I'm glad you're taking this on. I have been too busy at work to react, but I feel like TextFont was getting lost in this refactoring. I will set aside some time to look at this over the weekend.
Yeah, that's a tough one. For lazy-loaded fonts there is no way to know exactly when the font is loaded. I think for rendering on canvas, it may require a different approach. I'll check that also. The other issue is with font metrics. The TextFont class is really about metrics, and using them to format your text. It is independent of the actual loading of fonts. We should be able to provide metrics for built-in fonts and use TextFont in place of the draw-and-erase method currently used in rendering context (if that is still used). It is more performant, and more reliable because web fonts may not have loaded when the measurement is made. For large blocks of text, it may give different results due to kerning. Also, most fonts have pretty similar width/height for the same glyphs at the same size/weight, so even if we can't find metrics for the exact font, we should be able to do pretty well with our fallback font metrics. |
Thanks for chiming in, Aaron. First, I want to clarify that there are two topics at hand: When I first started this PR, I was mostly concerned about topic A (fonts for text). Many elements had their own individual setFont() method and internal .font property. I wanted to unify them all and adhere to the CSS convention as much as possible. I think I mostly succeeded. However, at some point I dug into the TextFont class. At first I had no idea what it was for. :-) Mostly, I was just annoyed that ChordSymbol had both a .font property and also a .textFont property. I had wanted to clearly distinguish "fonts for text" vs. "fonts for music engraving" and my plan was to name the properties .textFont and .musicFont. That plan was going great until I ran into ChordSymbol, where I would have two .textFont properties, haha. So anyways, as the PR developed I slowly moved some of my font handling methods into the TextFont class. Perhaps I was attempting to unify topic A (fonts for text) and topic B (TextFont class). Hopefully they can be unified somewhat, because I noticed multiple classes (including SVGContext and TextFont) had different ways to determine whether a font-weight was 'bold' or a font-style was 'italic', or duplicated ways to convert point units to pixel units. It'd be nice if the TextFont class was ultimately the one stop shop for all things related to measuring and laying out fonts. I'm happy to incorporate your feedback into this PR.
From stackoverflow, I found a suggested approach for loading web fonts: const myFont = new FontFace('My Font', 'url(https://myfont.woff2)');
myFont.load().then((font) => {
document.fonts.add(font);
console.log('Font loaded');
}); I can try this to see if it fixes the CanvasContext test case! If it does, perhaps we need to include this into VexFlow as the official way to load a web font before rendering the stave. Right now, on the flow.html test page, Roboto Slab is included in the head, while PetalumaScript is included from the releases/ folder. It looks like Roboto Slab isn't picked up by the Canvas tests due to the async font loading. @0xfe Perhaps we can upload PetalumaScript to the unpkg cdn? I assume the font license allows us to host the woff file on a server of our choice? Perhaps we need to make a npm package called "vexflow-fonts" so that we aren't linking into a subfolder of the vexflow npm package (e.g., https://unpkg.com/[email protected]/releases/fonts/petaluma-script.woff) Summary
Addendum / Further Discussion on TextFontI am still not completely sure I understand TextFont. I know that it is used by ChordSymbol and briefly in Annotation. My question (for both @AaronDavidNewman and @0xfe): If we can ensure via the FontFace API that the web font is loaded before we start rendering the VexFlow score.... can we just rely on the browser's font rendering engine to lay out the text symbols properly? It seems quite heavy weight to have to generate a whole textmetrics file for any web font that a developer would like to include. I suppose ChordSymbol has a bunch of things going on. There are the letters like A E V 1, glyphs like ° that are pulled from Bravura/Petaluma, and then there's a custom line that we draw for the slash. |
With the FontFace API, we are now able to load custom web fonts (woff2 / woff) and have them available for Here's the approach that I use in // Load web fonts before running the tests.
const host = 'https://unpkg.com/[email protected]/';
const srcURLs = (path) => `url(${host}${path}2) format('woff2'), url(${host}${path}) format('woff')`;
const robotoFont = new FontFace('Roboto Slab', srcURLs('robotoslab/RobotoSlab-Medium_2.001.woff'));
const petalumaFont = new FontFace('PetalumaScript', srcURLs('petaluma/PetalumaScript_1.10.woff'));
loadVexFlow()
.then(() => robotoFont.load())
.then(() => petalumaFont.load())
.then(() => {
document.fonts.add(robotoFont);
document.fonts.add(petalumaFont);
})
// Load qunit.js as late as possible to avoid a race condition
// where the QUnit module drop down box doesn't appear if
// Vex.Flow.Test.run() happens too late,
.then(() => loadScript('support/qunit.js'))
.then(() => {
// Show only failed tests.
QUnit.config.hidepassed = true;
QUnit.config.noglobals = true;
Vex.Flow.Test.run();
}); The buggy test case is fixed: To make this work, I uploaded the open source fonts onto unpkg (via npm publish vexflow-fonts): https://unpkg.com/browse/[email protected]/ Our web fonts can now be accessed directly from CDN, e.g.: https://unpkg.com/[email protected]/petaluma/PetalumaScript_1.10.woff2 The Post 4.0 release, we could bake this font loading method into VexFlow. It can load Roboto Slab and PetalumaScript by default. The developer could also provide a list of font names and *.woff URLs to load, and VexFlow will guarantee they are loaded before we render to the CanvasContext. |
This PR has a lot of prose in it :) Rather than add to it, I feel like we should create an issue for discussion. And this can be one of several font-reform PRs. There are a lot of ideas in here, some good and others I think need some more discussion. I will add that loading vs. formatting fonts are mostly unrelated. I have only been concerned with the formatting, and avoided the loading issues for the most part (and I think you are starting to understand why). But maybe it's time to take that on. What do you think? |
Sorry for the essays. My goal is to explain the design decisions to you and Mohit so you don't think I'm just changing code randomly. I'm happy to assist with your code review, if you have any questions. If you can identify smaller PRs that can be cleanly broken off, let me know and I'll accommodate. I understand smaller bites are easier to review and get accepted. |
If you'd like to discuss the larger topic of fonts, feel free to start a topic on the GitHub discussions tab. I'll be happy to join in. |
Are we dropping support for web fonts? Maybe that's OK if we only want to support statically-loaded fonts, but that is why the canvas tests were 'buggy'. They were intended for display fonts, and so I only ran them on svg. Someone else added the canvas tests later. If you still want to support the webfonts, we could have one test svg only, and one test for canvas + svg that uses the statically loaded font. I have a couple of questions about the web loading API:
|
One more question and one comment/suggestion:
But textFont is an actual instance of TextFont class. Couldn't you still make textFont a property of Element? I don't see the conflict.
How about TextFontFormatter? There is already a TextFontMetrics interface, and a TextFontRegistry. TextFontMetrics seems pretty consistent to how metrics are used elsewhere. TextFontRegistry could become 'TextFontMetricsRegistry'. Since it is really the metrics you are registering. I don't think you should worry about TextFont... name changes being breaking. Other than me, I doubt if may people use it or even know about it. DEFAULT_FONT_STACK is a much more breaking change (not saying it shouldn't be done). |
?? I think there's a miscommunication here, and we probably need to get synced. NO we are not dropping support for anything. The JS FontFace API and the css @font-face API should be equivalent. https://developer.mozilla.org/en-US/docs/Web/API/FontFace The only difference is in browser support. FontFace seems to be a bit newer. (Note: developer == someone who uses VexFlow in his/her own project vs. maintainer == Mohit, you, me, others.) Developers can choose to use whichever path suits their project. The problem is if they use a CanvasContext, VexFlow can draw the text before the font has loaded. This was the bug in our ChordSymbol canvas test case. Before, in flow.html, we included the Google CSS for Roboto Slab, and we implemented our own CSS @font-face for a local copy of PetalumaScript. I changed it so that we use the FontFace API to load them from the unpkg CDN. This way allows us to use Promises / async to wait for the fonts to be available before starting the CanvasContext tests. In a different PR, we can also add two examples to |
:-) I think we need to sync up. From what I've seen, VexFlow has three things: A is well understood by us maintainers. However, maybe B is used to set the CSS font in the rendered svg or canvas element. Before this PR, each element that needed to set the font (e.g., to 'bold 10pt Arial') implemented its own setFont() method and internal .font property. I moved them all up to the Element class so the API could be consistent, and match the CanvasRenderingContext2D.font API as closely as possible. So while working on this PR, I wanted to rename the Element.font to Element.textFont, so that we could distinguish it from Element.musicFont. Then I encountered ChordSymbol.textFont, and the TextFont class :-). The purpose of C (the TextFont class) overlaps a bit with B (native CSS fonts). To me it seems like the purpose of TextFont is for registering web fonts so that we can use them in elements. Is this correct? Is there any way we can merge B and C in a clean way? Certainly, web fonts that are fully loaded can be assigned to the CanvasContext / SVGContext as 'italic bold 16pt PetalumaScript'. Looking at all the uses of Can we rename the member to Then we could rename The bigger thing I see is that it seems like a big hurdle for a developer to add a new web font so that it can be used in ChordSymbol. Is there any way we can avoid having to generate a Canvas and SVG know how to draw their own font glyphs, as long as the web font has been loaded properly. Can we get away with just having hand tuned kerning hints (a la bravura_metrics.ts) that a developer can provide for his/her preferred web font? Anyways, this topic is for a different PR. :-) My main goal for this PR is to 1) consolidate how we handle the .font property (CSS fonts) and 2) clearly distinguish between music fonts and text fonts. If you are OK with me renaming the ChordSymbol.textFont field (and maybe refactoring the TextFont class a bit more), I think I could achieve this. |
.gitattributes
Outdated
*.otf binary | ||
*.woff binary | ||
*.woff2 binary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prevent git from corrupting our font files.
tools/woff/package.json
Outdated
{ | ||
"name": "vexflow-fonts", | ||
"version": "1.0.1", | ||
"description": "Web fonts compatible with VexFlow.", | ||
"author": "", | ||
"license": "SEE LICENSE.txt in each font folder." | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AaronDavidNewman @0xfe
Since we are redistributing open source fonts on the unpkg CDN, I figured it'd be good practice to include the LICENSE files from each respective project.
https://unpkg.com/[email protected]/
These can be the web font files that VexFlow supports by default. A developer can choose his/her own (e.g., from Google Fonts) and then host on a different CDN.
I'm keeping these font files in the vexflow/tools/woff/ folder, but publishing them as a separate npm project. This allows us to update font files (if necessary) without having to push a new vexflow release.
OK, I get it. It sounds like the least-breaky thing is to rename ChordSymbol.textFont to ChordSymbol.textFormatter, or textMetrics if you like that better. Then it wont eclipse textFont in Element. I have no strong opinion about the name of TextFont class. The thing about git corrupting the font files is a good find- I always wondered how that happened, now we know. |
Converting to draft for now, while I incorporate some of Aaron's feedback. I think the best way is to call music engraving fonts MusicFont / MUSIC_FONT_STACK / this.musicFont / etc. Then system built-in fonts & web fonts for displaying text will be called Font / FONT / this.font / this.fontFormatter / etc. This aligns with the browser context2d API, which allows you to set the .font to 'bold 10pt Arial'. I'll make sure I get everyone's buy-in on the changes. |
2dc08ae
to
316dce5
Compare
@AaronDavidNewman Do you know where we got the Petaluma Script woff file that is located at I got a copy from here: Both woff files specify version 1.10, but they are very different! I used https://fontdrop.info/ to inspect, and my version has 549 glyphs (the current vexflow version has 185 glyphs). However, the current vexflow version seems to have better kerning! I assume that we should go forward with the one that is released on their GitHub repo, but I'm just confused. :-) The GitHub release is designed at 1000 upem, but the current vexflow woff is 2048 upem, which indicates it is related to a TrueType font.... The GitHub version is named "PetalumaScript" in the font metadata, and the vexflow copy is called "Petaluma Script Regular". Any ideas? |
I got it from the same place. I only extracted metrics for the symbols 33-127 (ASCII), so it's possible I used the font tools to extract only those glyphs. But I have no memory of doing this. EDIT: I think it's more likely that I used font squirrel to do this |
OK, I was about to say it feels like we extracted a subset of the font. Steinberg has only ever released one version of PetalumaScript, but the first version you ever checked in is only 45KB and has a subset of the glyphs. In my PR, I'm going to opt for using the official Steinberg woff file. Is that OK? As long as we preserve the way for developers to select / register their own woff files (via your registerFont method). :-) |
I have verified that FontSquirrel produces the same petaluma woff file: https://www.fontsquirrel.com/tools/webfont-generator I uploaded the OTF file and selected FontSquirrel defaults (which includes the 2048 upem). Since Steinberg released an official woff file, we should use theirs (but allow developers to load their own smaller woff files if desired). |
Is this going to increase the size of the release or is this just an intermediary build file? Because Vexflow is already big, and it would be nice to keep its size in check. |
cbebfb0
to
1532f44
Compare
declarations. Fix eslint warnings.
to support older TS compilers that complain about object spread with undefined.
@ronyeh just a few conflicts, IMO ready to merge afterwards. |
Includes relative path imports for test files from: 0xfe#1209
Please merge these sub PRs next:
We are getting close. I'll fix the conflicts and rebase after the above two are reviewed & merged, and then I think this branch might be reviewable on its own! I will do more testing today, and will put up a *.tgz for folks to npm install into their own projects. Once the major integration issues are ironed out, we can have Mohit ship a version to npm with the |
Update demos. Fix capitalization. Fix README. eslint autofix markdown and JS.
Fix an integration issue where common.d.ts isn't exported but it is referenced by TS files. Solution: remove common.d.ts and move types to where they are actually used. Fix issues caused by renaming output files.
I'm migrating VexTab right now. Will update folks with a list of what API changes happened between 3.0.8 and 4.0.0 (from the perspective of VexTab). |
I believe that this one can be now closed. |
Even though the 4.0.0 PR subsumes this one, I'll keep it open until all the major parts of the Font work have been merged. I haven't yet submitted sub PRs for the core font work, which includes:
These PRs will come over the next week, to aid in code review. |
Do not forget to move the linked issues to 4.0.0 PR! |
Closing this PR for now. The 4.0.0 branch / PR includes the font improvements from this PR. I will continue to examine how to submit smaller PRs to the main repo so we can get our branches merged and a beta released. |
Current Info
Please merge these sub PRs next:
Already Merged:
Original Post
This is the final improvement that I'd like to get merged into the 4.0 release.
The high level goal is to consolidate the way we handle text fonts (e.g., Arial, Times, sans-serif), and follow CSS conventions wherever possible.
Before
font
property.setFont()
was declared in 11 classes with 3 different method signatures. 1 class allowed direct access to thefont
property.After
Before
protected font: FontInfo;
was declared 16 times. 10 of those classes did not provide a setFont() accessor.After
Element
class. Now it is declared once:protected font?: Required<FontInfo>;
static TEXT_FONT
property, along the lines of:this.setFont(this.getDefaultFont());
.Before
weight
parameter handled both bold and italic. In CSS, italic is specified byfont-style
instead offont-weight
.After
context.font
to a CSS font string (e.g., 'italic bold 1.5em Arial, sans-serif'). Make use of a hidden<span>
element to parse arbitrary CSS font strings.Before
getFontStack()
method, but those fonts are not for rendering text.After
Element
now hassetMusicFontStack() / getMusicFontStack()
vssetFont() / getFont()
.MUSIC_FONT_STACK
. No need for the DEFAULT_ prefix.Coming soon
Review visual changes, which mostly come from changes to the default sans-serif font. I added Helvetica Neue to the sans-serif text font stack, which should only affect macOS / iOS. Designers seem to prefer this font for macOS over Arial.Edit: I am abandoning Helvetica Neue for now. It looks too thin in the PNG visual tests. Will revisit for 4.1.## Future Work- There is aTextFont
class that is for using custom fonts loaded with @font-face. We need to make it clearer how to integrate web fonts into VexFlow. Additionally, the test cases that use TextFont on a CanvasContext are buggy (e.g., ChordSymbol). This is because the web font needs to be fully loaded before we draw to the canvas.EDIT: After a discussion below with Aaron, I ended up revamping the TextFont class by 1) moving generic Font methods into the Font class and 2) moving the formatting code into a new
TextFormatter
class.