Concatenate contiguous font-family tokens

[jayaddison]

Summary
As noted in #1881, rendering of some named fonts using html2canvas can be troublesome.

Many of the reported examples relate to free distribution of the FontAwesome version 5 fonts. Web applications using this font frequently set the CSS-based font-family property to the string Font Awesome 5 Free, as included in some of the default stylesheets (ref).

The root cause of the issues encountered when using Font Awesome 5 Free and html2canvas in combination is that the latter's font-family parser emits one token for each of the whitespace-or-comma-tokenized input values.

In the case of the string Font Awesome 5 Free, this results in three tokens: ['Font', 'Awesome', 'Free'] being passed to the rendering engine (the 5 token does not pass the isStringToken filter check). These are not valid font names, and as a result html2canvas renders the styled text incorrectly.

This pull request modifies the parser by ensuring that it concatenates previously-encountered font-family tokens when the parser reaches a termination point (either a comma token, or the end-of-stream).

Test plan (required)
These changes have been tested locally under both Firefox and Chrome, and the CI test coverage introduced by #2202 should be useful to provide additional confirmation across other browser environments.

Unit tests to illustrate expected parsing of font names containing numbers and multiple fonts within a property value are included.

Closing issues
Fixes #1881

[jayaddison]

ReftestResults rendering output for text-fontawesome!Chrome_Stable-Safari-7.x.png:

Before @ 4dd4a69 (master)

After @ a3d1565 (font-family-tokenization)

[jayaddison]

@niklasvh Sorry to nag a bit; do you think this is a workable approach?

I'm confident that it solves the problem, but part of me wonders if there might - strictly speaking - be a more correct (if riskier) fix within the tokenzer.ts code. I can look into that if you can offer a sense of whether it'd be useful to investigate.

[harshsapra92]

@jayaddison @niklasvh any idea when this code will be merged?
Appreciate your efforts in fixing this

[jayaddison]

@harshsapra92 It's difficult to say; given that code (and bugs) have a tendency to exist for quite a while once released, I'd recommend we wait to make sure that this genuinely is the correct place to fix the issue.

If this is in fact a tokenizer-level issue then fixing it there instead could potentially resolve an entire class of other issues.

[jayaddison]

@petermanders89 Apologies for snooping; I saw that you were looking another approach to resolve this issue in # 1948 (not tagging that issue again here, to avoid spam).

If you have the time and interest, would you be able to provide any opinions and/or verify this alternative approach?

[shane-arthur]

Tried this approach but still being met with blank squares.

[jayaddison]

@shane-arthur Thanks - is there a minimal test/repro case that you can provide? I'd love to track down the remaining issues here.

[IAmRC1]

@shane-arthur Thanks - is there a minimal test/repro case that you can provide? I'd love to track down the remaining issues here.

I am going to try this fix now, will revert back.

Edit- Same as Shane, no luck here.. Icons rendering as blank squares!

[shane-arthur]

@shane-arthur Thanks - is there a minimal test/repro case that you can provide? I'd love to track down the remaining issues here.

https://github.com/shane-arthur/resume-builder, download and install all node modules, open on port 4200, and i am re-rendering the canvas below the original content to see the deltas.

Of course you will have to build the html2canvas and drop its updated distrubution folder into the existing html2canvas dependency.

[jayaddison]

@shane-arthur Thanks - I'll take another look at this shortly.

[jayaddison]

@shane-arthur Nice project. There's some bad news and good news:

• Bad news: the fix in this pull request does not affect SVG icon rendering; only CSS-based 'webfont icon' rendering - and you're using SVG icons currently

• Good news: you already have fontawesome's CSS assets in your project. If it's an acceptable fix for you, all you need to do is add src/assets/fontawesome/css/all.min.css to your list of Angular stylesheets, and remove FontAwesome's script-based SVG implementation and you should find that the icons render correctly via html2canvas.

cc @IAmRC1 - I'm not sure whether this is also true of your application, but if the icons you're attempting to render are also SVG, you may be encountering the same problem

[shane-arthur]

@jayaddison

Thanks so much! Got it yeah that was my misunderstanding, just took this html and tossed it into angular to get modules and a test env.

Confirmed your patch is now working with css font icons.

Thanks again for taking the time and responding so promptly!

[jayaddison]

Although I initially thought they might be redundant, it seems that surrounding font names with single-quotes does improve the reliability of font-family name rendering across browsers (with the exception of Internet Explorer, which seems to handle strings containing spaces just fine).

I'll add some comments to the code and perhaps some additional test coverage to make sure that escaping of single quotes within font family names works as expected.

[jayaddison]

ReftestResults rendering output for text-fontawesome!Chrome_Stable-Safari-7.x.png:

Current @ 148d626 (font-family-tokenization)

[jayaddison]

@niklasvh This code isn't the prettiest and isn't entirely consistent with the rest of the parsing code, although that seems fairly unavoidable to me.

I did look into moving the code lower down into the tokenizer/parser code, but my sense now is that the behaviour here needs to match the font-family property syntax specifically, and that it's tricky to generalize to other CSS properties.

Test coverage is now provided and I feel more confident in the results; this uncovered a bug regarding handling of numeric tokens which I believe neither this nor the alternative icon-rendering fix PR had handled.

During user testing to validate the fix, it appears that icon rendering problems fall into two broad categories - people using font-names-containing-spaces, and issues with foreignObjectRendering, typically related to SVG icons. This pull request strictly only solves the former.

Glad to apply any cleanups and further changes here.

[KyleAsaff]

When is this going to be merged? I need this for my project

[niklasvh]

@jayaddison thanks for the PR and apologies for the slow response to it. I'll try to either get this merged or check what needs to change before it can get merged in the coming weeks. This particular issue has been popping on and off a number of times over the years, and due to lack of proper testing, changes to the behavior quite often caused regressions for some other browsers/fonts (of which I dont currently recall the details over).