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

Resolve fonts using CSS Level 3 algorithm when using html() #3040

Merged
merged 16 commits into from
Jan 14, 2021

Conversation

allansson
Copy link
Contributor

Issue

The context2d.font property uses a different algorithm from the browser when choosing a font. This causes jsPDF to choose a different font, meaning that the rendered PDF will not look the same as the rendered page. Additionally the current algorithm does not fully support all the possible values for font-weights, font-stretch and font-styles. This PR is related to #2920 and its PR #3036.

Feature

This PR adds an option to specify font-faces to the html() method, which will make the context2d.font use the same algorithm as the browser (with some limitations) when deciding which font to use.

The supported properties of the font style matching algorithm of the CSS Level 3 specification are:

  • font-family
  • font-stretch
  • font-style
  • font-weight

In order to use this feature, the user has to specify font-faces matching the ones in given by the user's CSS:

doc.html(el, {
    fontFaces: [{
        family: 'Roboto',
        weight: 700,
        style: italic,
        src: [{
            format: "truetype",
            url: "https://mysite.test/roboto.ttf"
        }]
    }]
})

The interface of each font-face object is modelled to, as close as possible, match the interface of the CSS Font Loading API specified in CSS Font Loading Module Level 3 in the hopes that, as browser support increases, users can use that API to get the available font-faces from the browser instead of defining them a second time. The one exception is the src-property, which is not included in the specification but is required in order to fetch the fonts.

All fonts configured will be automatically downloaded and added to the document.

The change should be backwards compatible since the algorithm only takes effect when the fontFaces option is set.

Limitations

  • While implementation of the font style matching algorithm takes the font-stretch into account, the context2d.font property currently does not support extracting font-stretch values from the given value. This is due to difficulties in modifying the regex used to parse the font value.
  • The font style matching algorithm rules out fonts that don't support a given unicode character. This has not been implemented.
  • The implementation does not take font-size into consideration when selecting a font. It will use matched fonts no matter the size.
  • It only supports TrueType fonts.

There are probably a lot more limitations, but I don't have the time figure them all out.

@HackbrettXXX
Copy link
Collaborator

Wow, thanks for this great PR! I will have a closer look at it as soon as I find the time ;)

@allansson
Copy link
Contributor Author

allansson commented Dec 28, 2020

There are four build errors (three in IE11 and 1 in Chrome).

* addImage: base64 validation in use with addImage-Call : 2 ok, 1 failed
* addImage: validateStringAsBase64 : 2 ok, 1 failed
* addImage: extractImageFromDataUrl : 2 ok, 1 failed
 ---
* renders font-faces : failed

I cannot figure out what's going wrong with the first three, since my changes haven't touched anything related to addImage.

The last one works on my computer. The only reason I can think of that would cause such a huge difference is that the fonts cannot be downloaded, making the algorithm choose the default fonts instead.

Copy link
Collaborator

@HackbrettXXX HackbrettXXX left a comment

Choose a reason for hiding this comment

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

The last one works on my computer. The only reason I can think of that would cause such a huge difference is that the fonts cannot be downloaded, making the algorithm choose the default fonts instead.

Yeah, that's odd. I get different results on my machine, as well. Seems like the fonts are correct, but the line height is computed differently. I assume html2canvas doesn't correctly pick up the fonts or the browser hasn't loaded the font yet when measuring the line height. I committed the file my PC creates, maybe that helps.

The errors in IE are probably due to non-es5 code in the src folder. Currently, we can't use es6, because the code is not transpiled. We should probably change this some time ;)

@@ -0,0 +1,292 @@
function toLookup(arr) {
return arr.reduce((lookup, name, index) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't use arrow functions. All code must be completely es5 compliant, since it is not transpiled (except for import/export statements).

if (this.fontFaces) {
var fontFaceMap = getFontFaceMap(this.pdf, this.fontFaces);

var rules = parts.map(ff => ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

no arrow functions ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aarrgh... muscle memory you know. 😅

@@ -436,6 +525,23 @@ import { console } from "../libs/console.js";

this.pdf.setFontSize(fontSize);

var parts = fontFamily.replace(/"|'/g, "").split(/\s*,\s*/);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will fail if a font name contains a ",": e.g. `foo, "foo,bar", bar".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have pushed a (mostly) proper parser for this, but I am a bit wary of it being too strict. 🤔

The string-split above matches the previous behaviour, so maybe it's best to keep that?

if (fontFaces) {
for (var i = 0; i < fontFaces.length; ++i) {
var font = fontFaces[i];
var src = font.src.find(src => src.format === "truetype");
Copy link
Collaborator

Choose a reason for hiding this comment

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

arrow function ;)

{
pattern: "test/**/*.+(svg|png|jpg|jpeg|ttf|txt)",
included: false,
served: true
},
{
pattern: "test/reference/**/*.pdf",
pattern: "test/reference/**/*.*",
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't think that's necessary. The *.ttf files should be served correctly by the previous rule.

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 was more of a test to see if there was something weird going on with serving the fonts on saucelabs. I just copied the pattern from unit/karma.conf.js since it was working locally.

I'll revert it, but would you prefer that I do a git revert or git reset HEAD^ --hard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it was just a test, I'll reset the commit. No point in having two unnecessary commits.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll squash the PR commits anyway when merging, so it won't make a difference in the end.

it("should match exact weight when 500", () => {
const fontFaces = buildFontFaceMap([w300, w400, w500]);

const result = resolveFontFace(fontFaces, [w400]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be w500, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch!

expect(result).toEqual(normalizeFontFace(w900));
});

it("should pick larger font-weight when no smaller is available", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the description doesn't fit to the test case here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it should be the reverse: smaller font-weight when no larger is available. There's another test-case with the same name on line 227, and this is basically the inverse of that.

Copy link
Collaborator

@HackbrettXXX HackbrettXXX left a comment

Choose a reason for hiding this comment

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

Thanks for implementing a better font family algorithm.

return [input.substring(0, index), input.substring(index + 1)];

// Mismatching quote
case ",":
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I know "," is allowed within a font family name and not an error. At least the browser accepts it, e.g.

font-family: "comma,comma", sans-serif;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! Thanks for pointing out my brain fart!

Comment on lines 395 to 397
return result.map(function(f) {
return f.toLowerCase();
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should not call toLowerCase here, although technically this would be correct. The old font matching algorithm won't work anymore with lowercase font names, since jsPDF stores the fonts case sensitive. The test case Context2D: standard tests > context2d: custom fonts currently fails because of that. I think we should move the toLowerCase call to the context2d font property:

if (this.fontFaces) {
  parts = parts.map(function(f) {
    return f.toLowerCase();
  }
  // ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this as well. Thanks for being so thorough!

@HackbrettXXX
Copy link
Collaborator

I found out what made the AMD test fail: due to the asynchronous loading of the html2canvas module with AMD require, the browser has time to load the fonts leading to different scroll heights of the div. In all the other tests, html2canvas is already loaded in a global variable, so the browser has no time to load the fonts.

I set the div to fixed dimensions as a workaround.

@HackbrettXXX
Copy link
Collaborator

Alright, I think it's fine now. I'll merge it as soon as CI is done. Thanks again for this awesome pull request!

@HackbrettXXX HackbrettXXX merged commit 6579099 into parallax:master Jan 14, 2021
@allansson
Copy link
Contributor Author

Alright, I think it's fine now. I'll merge it as soon as CI is done. Thanks again for this awesome pull request!

And thank you for a great code review!

This was referenced Mar 12, 2021
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