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

fix(web): fix handling of data: url fonts, fonts with single quotes in filename #13032

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jahorton
Copy link
Contributor

@jahorton jahorton commented Jan 24, 2025

Fixes: #13018
Fixes: #13022

Our fontface-defining stylesheet format has been using single-quotes to enclose the filename. Sadly, encodeURI doesn't mangle single-quotes - it mangles double-quotes instead. So, we should adjust the quote type used there.

Fixed stylesheet:

@font-face {
font-family:soninke_n_ti;
font-style:normal;
font-weight:normal;
src:url("./N'tiFont.otf") format('truetype');
}

When using the KMP loader to diagnose the problem, I found that data URLs were handled... to a point, and then that process was unnecessarily aborted. That was simple enough to fix with the first commit of this PR.

User Testing

This is how the default layer of the keyboard looks in desktop-mode Keyman Engine for Web:

Screenshot 2025-01-24 at 12 56 44 PM
  • TEST_ANDROID: Using the Android test build from this PR and the soninke_n_ti.kmp package from https://jahorton.github.io/soninke_n_ti.kmp, verify that the keyboard loads properly and has proper characters for its font.

  • TEST_IOS: Using the Android test build from this PR and the soninke_n_ti.kmp package from https://jahorton.github.io/soninke_n_ti.kmp, verify that the keyboard loads properly and has proper characters for its font.

@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Jan 24, 2025
@github-actions github-actions bot added web/ and removed user-test-required User tests have not been completed labels Jan 24, 2025
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Jan 24, 2025

@github-actions github-actions bot added the fix label Jan 24, 2025
@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S20 milestone Jan 24, 2025
@mcdurdin
Copy link
Member

Fixes: 13017

I don't think this is true -- that's a separate Keyman Developer bug

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-required User tests have not been completed label Jan 24, 2025
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

LGTM

web/src/engine/dom-utils/src/stylesheets.ts Outdated Show resolved Hide resolved
@jahorton
Copy link
Contributor Author

Fixes: 13017

I don't think this is true -- that's a separate Keyman Developer bug

You think there's something else on that side of things, past the Web issues that'd probably also affect the behavior?

@mcdurdin
Copy link
Member

Fixes: 13017

I don't think this is true -- that's a separate Keyman Developer bug

You think there's something else on that side of things, past the Web issues that'd probably also affect the behavior?

Yes. Take a look at PR #13020 associated with #13017 which already fixed the issue 😁 Font filenames are rewritten by Keyman Developer Server (for reasons relating to how fonts are made available in the dev environment...), so the quote issue cannot arise there in the uri.

@jahorton jahorton force-pushed the fix/web/font-link-robustness branch from 1c934cb to ee7ca4c Compare January 24, 2025 06:27
@jahorton
Copy link
Contributor Author

Force-pushed to remove the link to an issue not actually fixed by this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix has-user-test user-test-required User tests have not been completed web/
Projects
Status: No status
2 participants