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

Serve fonts locally #848

Closed
wants to merge 1 commit into from
Closed

Conversation

aral
Copy link
Contributor

@aral aral commented Dec 11, 2018

This serves the fonts that were previously being loaded from Google locally instead.

This means that people who use mdbook to generate documentation will not unknowingly be making calls to a surveillance capitalist’s servers.

The exact same fonts are being loaded in locally so the visual result is exactly the same.

Fixes #847

@Oreolek
Copy link

Oreolek commented Dec 12, 2018

Note that this PR has only Latin character support.

@aral
Copy link
Contributor Author

aral commented Dec 12, 2018

Unless I’m mistaken, the original third-party implementation was only pulling in Latin also.

@Oreolek
Copy link

Oreolek commented Dec 12, 2018

You are mistaken, look at the CSS.

There are Unicode blocks for Vietnamese, Greek, Cyrillic Extended, Latin Extended.

@aral
Copy link
Contributor Author

aral commented Dec 12, 2018

I can’t see the subsets you mention in the linked to CSS.

Again, unless I’m missing something, only Latin is currently being loaded in. See the HTML:

<link href="https://fonts.googleapis.com/css?family=Open+Sans:300italic,400italic,600italic,700italic,800italic,400,300,600,700,800" rel="stylesheet" type="text/css">

If additional blocks were requested, it would look something like this:

<link href="https://fonts.googleapis.com/css?family=Open+Sans:300,300i,400,400i,600,600i,700,700i,800,800i&amp;subset=cyrillic,cyrillic-ext,greek,greek-ext,latin-ext,vietnamese" rel="stylesheet">

@ccoenen
Copy link

ccoenen commented Dec 15, 2018

How can we get this PR merged? Will adding other blocks suffice?

@aral
Copy link
Contributor Author

aral commented Dec 15, 2018

@ccoenen As far as I can see, my pull request is a refactor. I do not see where the additional blocks @Oreolek mentions are currently being requested in the current codebase (master branch).

As such, afaics, adding additional blocks would constitute an enhancement request.

That said, I’d like to see this request merged also. What’s the process for that? Who’s in charge of making a decision on this here?

(And, again, please let me know if I’m missing something. I’m not against adding additional blocks – even though the theming logic has multiple redundancies and really does require a major refactor – but I intend this pull request to be limited to a refactor and not change/add any behaviour.)

@ehuss ehuss mentioned this pull request Apr 28, 2019
@mrec
Copy link

mrec commented May 7, 2019

@aral

If additional blocks were requested, it would look something like this

This isn't a topic I'm familiar with, but from a bit of poking around in Google Fonts it looks as if your explicit subset=cyrillic,cyrillic-ext,greek,greek-ext,latin-ext,vietnamese is the default behaviour. If you look at the stylesheet returned by the current URL you gave:

https://fonts.googleapis.com/css?family=Open+Sans:300italic,400italic,600italic,700italic,800italic,400,300,600,700,800

it contains @font-face declarations for all these subsets.

@decentral1se
Copy link

Bump 🦈

@kriskeillor
Copy link

If you look at the stylesheet returned by the current URL you gave:

https://fonts.googleapis.com/css?family=Open+Sans:300italic,400italic,600italic,700italic,800italic,400,300,600,700,800

it contains @font-face declarations for all these subsets.

True. The stylesheet has references to all six languages, in italic & normal styles, and five weights. But I can find plenty of free and open source fonts online that do as well. Since google can identify a font requester's address, browser, etc, as well as read the site they're requesting the font from (if it's unencrypted), it does not seem in line with mdBook's goals to serve them up that traffic.

Also, it's strange that their ?family= arguments do not affect the actual fonts returned at all. If we are only requesting latin we should not be bogged down with unwanted files. Which is further argument in favor of refactoring mdbook's fonts away from google to an opensource font.

I see @aral's pull request includes the actual woff & woff2 file fonts for an Open Sans font, which are referenced in the handlebars and rust build code, adding over 230 lines. That seems very complicated. I like the simplicity we currently have, with a single reference to Google's site. I also like that their stylesheet looks first for local font files before loading them remotely.

Can we emulate something like that, with a stylesheet (that anyone familiar with basic web dev could edit, rather than two entwined build processes across js & rust) that first looks for local files, and then looks for fonts sourced from a more trustworth source, say fontlibrary?

@kpp
Copy link
Contributor

kpp commented Jul 6, 2019

How about make a fast temporary patch to switch from google to fontlibrary and then slowly and wisely make a patch to serve fonts locally?

@kriskeillor
Copy link

Unfortunately, fontlibrary does not allow searching for multiple languages at once, or have tags for the extended sets of Cyrillic, Greek, or Latin. Thus far in searching manually I haven't found an open license font that includes cyrillic,cyrillic-ext,greek,greek-ext,latin-ext,vietnamese.

Are those the unicode blocks we are determined to support, though? Seems kind of weird we want latin-ext but don't care about latin? Fonts are a little outside my area of expertise, but what unicode blocks do the main users/team want to support?

@ehuss
Copy link
Contributor

ehuss commented May 20, 2020

Fixed via #1188.

@ehuss ehuss closed this May 20, 2020
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.

Remove Google surveillance
9 participants