danfickle / danfickle/openhtmltopdf
Font resolution: serif implicitly added even if built-in fallback already matched
- Dominant language
- Java
- Stars
- 2.2k
- Forks
- 423
- PR merge metrics
- No merged PRs in 30d
Description
I noticed that the font resolution algorithm stubbornly adds the `serif` built-in font, _no matter if the selected font families have already found an appropriate built-in match_ (see `com.openhtmltopdf.pdfboxout.PdfBoxFontResolver.resolveFont(..)`):
```java
public class PdfBoxFontResolver implements FontResolver {
. . .
private FSFont resolveFont(SharedContext ctx, String[] families, float size, IdentValue weight, IdentValue style, IdentValue variant) {
. . .
// Built-in fonts.
if (families != null) {
resolveFamilyFont(ctx, families, size, weight, style, variant, fonts, _builtinFonts);
FontDescription serif = _builtinFonts.resolveFont(ctx, "Serif", size, weight, style, variant);
if (serif != null) {
fonts.add(serif);
}
}
. . .
}
. . .
}
```
_That behavior adversely affects the font metrics calculation_ (see `com.openhtmltopdf.pdfboxout.PdfBoxTextRenderer.getFSFontMetrics(..)`) _and, in turn, the actual text placement_ (see `com.openhtmltopdf.layout.InlineBoxing.calculateInlineMeasurements(..)`), as parameters like `ascent` and `descent` are calculated from incoherent typefaces. For example, if `monospace` is selected via CSS, the result is an ugly extra space above the ascender line which unbalances the ascent/descent ratio, making text feel like it's sitting on the line bottom instead of flowing in the middle -- here it is a comparison (on the left, the current wrong behavior, on the right the correct one generated with the code fix here below):
Here it is the way Gecko (Firefox) renders the same input (note the balanced ascent/descent ratio):

Source HTML: [serifFallback.html.txt](https://github.com/danfickle/openhtmltopdf/files/6448142/serifFallback.html.txt)
The nuisance can be easily fixed with a little code change:
```java
public class PdfBoxFontResolver implements FontResolver {
. . .
private FSFont resolveFont(SharedContext ctx, String[] families, float size, IdentValue weight, IdentValue style, IdentValue variant) {
. . .
// Built-in fonts.
if (families != null) {
if (!resolveFamilyFont(ctx, families, size, weight, style, variant, fonts, _builtinFonts)) {
fonts.add(_builtinFonts.resolveFont(ctx, "Serif", size, weight, style, variant));
}
}
. . .
}
. . .
private boolean resolveFamilyFont(
SharedContext ctx,
String[] families,
float size,
IdentValue weight,
IdentValue style,
IdentValue variant,
List fonts,
AbstractFontStore store) {
boolean resolved = false;
for (int i = 0; i < families.length; i++) {
FontDescription font = store.resolveFont(ctx, families[i], size, weight, style, variant);
if (font != null) {
fonts.add(font);
resolved = true;
}
}
return resolved;
}
. . .
}
```
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.