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

Android-specific patch for generic font families #149

Closed
JayPanoz opened this issue Aug 1, 2024 · 2 comments
Closed

Android-specific patch for generic font families #149

JayPanoz opened this issue Aug 1, 2024 · 2 comments

Comments

@JayPanoz
Copy link
Collaborator

JayPanoz commented Aug 1, 2024

I'm submitting a feature request

Short description of the issue/suggestion:

While platforms usually pick Times (New Roman) for serif, and Arial/Helvetica for sans-serif – or very similar fonts –, Android is an outlier with Droid Serif and Roboto in the sense their metrics are significantly different (x-height, etc.).

That creates issues in practice with Fixed-Layout EPUBs in which these fonts are not embedded. Given text is absolutely positioned, too much variation in metrics has a noticeable impact e.g. overlapping and unreadable text, etc.

Steps to reproduce the issue/enhancement:

  1. Open this demo on an Android device.
  2. Flip through the publication and notice overlapping text
  3. Compare with the same publication on Windows/Mac/iOS

What is the expected behaviour?

It’s best practice to embed fonts in FXL EPUBs to avoid this issue in the first place, but it might be considered a special case as one platform is an obvious outlier. So maybe it could be a good idea to create a patch such as the one we created for EBPAJ font declarations.

What is the current behaviour?

We’re not doing anything for such cases.

Do you have screenshots, GIFs, demos or samples which demonstrate the problem or enhancement?

This is a good example as there is a lot of text crammed on a single page. This is the reference using Times New Roman.

Reference, everything is readable and OK.

On Android, it becomes unreadable:

Every line is overlapping on the next one because the font metrics are way larger than Times New Roman

What is the motivation / use case for changing the behaviour?

Solving an inconsistency across major platforms.

Do you know which CSS modules (stylesheets) are impacted?

None, this would be a new patch outside of the build process, that implementers have to add depending on some logic, as-is already the case for the EBPAJ patch.

Please tell us about your environment:

  • Platform: Android

Other information (e.g. related issues, suggestions how to fix, links for us to have context)

It’s not possible to patch the generic font-family e.g. serif or sans-serif, it’s only possible to patch Times, Arial, etc. using @font-face.

While there is no issue finding libre/open source alternatives such as Nimbus Roman No9 L or TeX Gyre Termes for Times (New Roman), this also means the app has to embed additional fonts (normal, regular, bold, bold italic). Given they would have been embedded in the Fixed-Layout EPUB in the first place, it’s not necessarily a penalty, but it also means logic should be smart in order to request them via the injected CSS file only when strictly necessary.

JayPanoz added a commit that referenced this issue Aug 2, 2024
@estoleruvoxa
Copy link

Waiting for this to be merged @JayPanoz Do you know if there is a way to fix this at runtime with Readium CSS?

@JayPanoz
Copy link
Collaborator Author

@estoleruvoxa I’m afraid I don’t have any other option than what we are doing in the patch we’ll be shipping in v2, that is embedding the missing fonts or an alternative font with the same-ish metrics.

ReadiumCSS isn’t even loaded for Fixed-Layout EPUBs in the Readium toolkits as this type of (what the spec calls) rendition shouldn’t be overridden with user settings, etc. due to its highly-graphical nature. Not that it would help with this issue anyway, as the issue is the lack of installed system fonts in the first place, and setting another font-family wouldn’t even solve it.

v2 is currently in preview so that implementers can take a look at the migration guide, and should be merged immediately after #150 is reviewed and merged into the main branch for labelling as the last v1.

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

No branches or pull requests

2 participants