Problem/Motivation
How about including the fonts in the Umami demo theme, and not rely on Google?
Pros:
- Not relying on the Google CDN.
- Avoid potentially slow load times.
- More privacy-friendly.
Cons:
- A little extra work coding it.
- Increased size of the packaged Drupal core archive. Approximately 100 kB? The fonts are binaries which won't benefit.much from compression.
I originally raised the question in #2943657: Embedding only needed weights for Open Sans and Scope One (#19). There were a few comments about font licenses, but in the end @alexpott commented:
... it looks like there has been a change to what can be packages so maybe these fonts can be packaged - see #2919135: Update /git-repository-usage-policy to allow "GPL friendly" licensed non-code assets
Proposed resolution
Adds Open Sans and Scope One fonts (latin1 encoding only), but all web font formats, to Drupal core.
Remaining tasks
Needs frontend framework manager review.
User interface changes
Comment | File | Size | Author |
---|---|---|---|
#77 | 2960583_77.patch | 3.16 KB | andy-blum |
|
Issue fork drupal-2960583
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 2960583-include-fonts-in changes, plain diff MR !2784
Comments
Comment #3
Eli-TScope One is licensed under SIL Open Font Licence 1.1 https://scripts.sil.org/cms/scripts/page.php?site_id=nrsi&id=OFL_web. This is the same license as used for Font Awesome as listed at https://www.drupal.org/packaging-whitelist?title=font, therefore it looks like we can include it in the theme.
Open Sans uses Apache License 2.0 .This is the same license as used for which Facebook PHP SDK, Google Code Prettify, which are also on our packaging whitelist https://www.drupal.org/packaging-whitelist?page=1 so looks like we can also use it in the theme.
So this looks good to proceed from a licensing PoV. Also would be good for people installing without network connectivity etc. So let's do it!
Comment #4
kjay CreditAttribution: kjay commentedWorking with @eli-t and @mradcliffe at DrupalEurope. We can confirm that we have all just checked the licensing as described in #3. Should be good for including the font in the theme subject to relevant product manager review/approval.
Comment #5
parijke CreditAttribution: parijke as a volunteer commentedI gave it a shot, downloading the fonts from google and embedded them in the base.css
Comment #6
Eli-TComment #7
ressa CreditAttribution: ressa at Ardea commentedThanks @parijke, great work! It looks fine in Firefox and Chromium on Ubuntu. I tested IE11 on Windows 8.1 and Safari 11.1 on macOS High Sierra via Browserstack, and it also looks good.
PS. I really like how patch-testing can be done super fast, with the recently included Quick Start Command:
Comment #8
kjay CreditAttribution: kjay commentedComment #9
siliconmeadow CreditAttribution: siliconmeadow as a volunteer and at CTI Digital commentedGood work @parijke. The patch applied cleanly and all appears to work properly out-of-the-box.
Comment #10
parijke CreditAttribution: parijke as a volunteer commentedThank you for the compliment... it's my first ever contribution to core
Comment #11
Not Real CreditAttribution: Not Real commentedInstead of downloading the fonts from google I recommend using https://google-webfonts-helper.herokuapp.com/fonts/
Also be aware that the line
+ src: local('Open Sans Regular'), local('OpenSans-Regular'),
Will mean that the font is loaded from the users own computer (local) before the font is loaded from the web site
Normally this won't be a problem BUT if the user has a different font with the same name OR they have only installed one weight of the font locally this will cause issues
Comment #12
parijke CreditAttribution: parijke as a volunteer commentedIt is the preferred way recommended by Google. I think because it makes sense to load from locsl if it exists there to optimize performance.
Can't foresee if this would cause any problems.
Comment #13
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedIt adds an extra ~400kb to the packaged Drupal core archive, so I've added a conargument to the issue summary. Currently the TGZ is 16.24mb.
Setting back to needs review, and tagging, for performance. @alexpott's comment in the related issue sounds like a tacit approval, but I'd like to hear a performance opinion.
IMO the privacy and reliability pro arguments have a lot of weight. I think we can live with the extra size.
Comment #14
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedI made some mess in the summary. Cleaning it up.
Comment #15
parijke CreditAttribution: parijke as a volunteer commentedWe could reduce somewhat if we decide to support only Modern Browsers. That would skip the need of packaging the .eot .svg and .ttf files
Comment #16
longwaveWe surely don't need the .eot file at least, as we don't support IE8 and below anyway? IE9 and higher will use the .woff if I understand correctly.
Comment #17
parijke CreditAttribution: parijke as a volunteer commentedI believe so too.... Anyone who can approve this before I make a new patch? @kjay?
Comment #18
Eli-TLooks like [#1475972], specifically https://www.drupal.org/node/1475972#gplv2-compatible-licenses contradicts my reasoning in #3, as pointed out by @laurii on today's initiative call ☹️
Comment #19
parijke CreditAttribution: parijke as a volunteer commentedCan this https://www.drupal.org/project/drupal/issues/2936841 be done with the fonts as well? That would address the con mentioned in #13
Comment #20
parijke CreditAttribution: parijke as a volunteer commented@Eliot is there a difference in licensing using the fonts online or in the distribution? If not, the licensing discussion shouldn't affect this issue but goes for using the fonts in general, right?
Comment #21
kjay CreditAttribution: kjay commentedDiscussed in this week's OOTB call and @smaz noted that [#1475972] states:
Despite Open Sans being Apache 2.0 and is not white-listed as GPLv2 compatible, being a font, it seems reasonable to conclude that we are able to bundle it so long as Apache 2.0 permits us to do so. Checking the Apache 2.0 license is therefore probably the next task.
Comment #22
volkswagenchicktagging for badcamp 2018
Comment #23
drummA well-maintained Unicode font will be updated frequently across various character sets. New emoji are added regularly and there are improvements for various languages.
This might not be a problem since the demo content is relatively static. Regular updates to the packaged fonts, and some discouragement from using the font in production, should be planned on.
Comment #24
volkswagenchickTagging for upcoming contribution days.
Comment #25
Eli-TComment #26
volkswagenchickComment #31
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedHi All,
Just checking in to see if someone could summarize what we need to have this marked as RTBC. It looks like we have a working patch and general agreement about including the fonts.
The main issue that seems to be outstanding is @drumm's thought about fonts being updated, which is mitigated by our content being static and not relying on emojis and things like that.
With that being the case, I _think_ we could move this to RTBC. Anyone against that?
Adding some tags for visibility.
Comment #32
mradcliffeI am removing the Novice tag from this issue because there isn't a task for a Novice to do. This might change. I queued the patch in #5 for 9.2.x. It may make sense to mark this RTBC or try to re-upload the patch for 9.2.x.
Comment #33
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedSince no one has disagreed with #31, I'm going to mark this as RTBC so we can get a frontend framework manager to review it.
Comment #36
ressa CreditAttribution: ressa at Ardea commentedHere is an updated patch with the latest versions of the Open Sans and Scope One fonts, and some other necessary adjustments. I am not including an interdiff, since it would mostly be weird
LF݈CԄ'J'{\w
font characters.Should .eot, and maybe other formats be removed?
Comment #37
ressa CreditAttribution: ressa at Ardea commentedThe .svg fonts contain a lot of words which are caught by
CSpell
. I checked if any other core theme embed fonts, and Olivero does, but only use .woff and .woff2 formats. I had a look at https://caniuse.com/woff (98.48% supported) and https://caniuse.com/woff2 (96.61% supported) and most browsers are supported ... So I removed the older legacy fonts, which also reduces it to ~100 kB.Comment #38
ressa CreditAttribution: ressa at Ardea commentedThings are moving fast now, it would be great to get this issue moving:
Comment #39
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedBumping this to major, we need to give this serious attention very quickly given the court rulings in the EU about Google Fonts.
Comment #40
shaalBy including the font in Drupal core (#37), we eliminate all the potential legal issues?
Comment #41
ressa CreditAttribution: ressa at Ardea commentedExcerpt from German Court Rules Websites Embedding Google Fonts Violates GDPR (January 31, 2022):
Including the font in Umami would prevent this from happening, since Google is not being contacted.
Also, the patch in #37 needs review, so feel free to check if it works or not.
Comment #42
shaalI tested #37 using DrupalPod.
Fonts were loaded locally, without Google's domain.
Webpagetest.org report: https://www.webpagetest.org/result/220308_BiDc0H_ASF/
Screenshot from that report:
Comment #43
ressa CreditAttribution: ressa at Ardea commentedAdding tag.
Comment #44
lauriiiScope One is a compatible license meaning those assets could be packaged in Drupal. 👍
Unfortunately, Open Sans is licensed under Apache 2.0 which is not compatible with our licensing, meaning we can't include it in the package. 😓 We will likely have to try to find an alternative font which would be licensed under a compatible license.
Comment #45
ressa CreditAttribution: ressa at Ardea commentedThanks for confirming that Scope One has a compatible license @lauriii.
Here's a re-rolled patch with the Public Sans font, which is licensed under the SIL Open Font License 1.1, just as Scope One. With 4.1K Github stars, it looks well supported.
The SIL Open Font License 1.1 is listed as allowed under Drupal's Licenses compatible with GPL-2.0-or-later and GPL-friendly licenses so perhaps Public Sans could work as a replacement for Open Sans?
I found it via this search: https://github.com/search?q=sans+font+sil
Comment #46
Eli-TI have installed with and without the patch in #49 on Drupal 9.3.9 on gitpod so we can compare Umami with Open Sans and Public Sans.
Here is the home page with Open Sans (left, unpatched) and Public Sans (right patched). Note the privacy badger warning for calling Google fonts does not show up when patched.
Full homepage with Open Sans
Full homepage with Public Sans
Article page with Open Sans
Article page with Public Sans
Recipe page with Open Sans
Recipe page with Public Sans
Individual recipe with Open Sans
Individual recipe with Public Sans
Tag page with Open Sans
Tag page with Public Sans
Individual article with Open Sans
Individual article with Public Sans
Comment #47
markconroy CreditAttribution: markconroy at Annertech commentedThose screenshots with Public Sans look great to me. I'm not a designer so will defer to @kjay to verify, but this looks like a really good solution.
Comment #48
ressa CreditAttribution: ressa at Ardea commentedThanks for the thorough work @Eli-T, and great to see both Privacy Badger and uBlock Origin in a relaxed state :)
Comment #49
Eli-T@lauriii has suggested a couple of other alternative fonts with licences compatible with Drupal:
Considering these two alongside Public Sans, it looks like Inter is actually the closest font to what we're already proposing so I'll provide another patch to trial that instead and generate some more screenshots.
Comment #50
Eli-TNow I'm not too sure if Inter is the closest suggestion so here's the home page in the original font, the Public Sans font used in the @ressa patch and the two suggested by @lauriii for design feedback/discussion.
Open Sans (original)
Public Sans (as suggested in #45)
Inter
Source Sans Pro
On closer consideration, I think as far as the character shapes are concerned, Source Sans Pro is the closest match and best works with the design, but some small amount of CSS work would be needed to adjust the size and spacing to more closely match the current font.
Comment #51
ckrinaAgreed with @Eli-T about the shape: Source Sans Pro is closer to Open Sans, so the style would match better.
But be aware Source Sans Pro font x-size is smaller, meaning using this one would probably need a follow-up to generally increase font sizes. The design tendency is to use bigger fonts than the ones resulting with this changes (even the same font size is set).
Maybe the second closer font on a style perspective would be Inter.
Comment #52
ressa CreditAttribution: ressa at Ardea commentedThanks for the comprehensive comparisons @Eli-T and feedback @ckrina. It sounds like Source Sans Pro could work, if we adjust font size and spacing.
As a first step, here is a patch with Source Sans Pro. I realized that originally, Open Sans included files with weights 400, 400i, 700, and 700i, so I have included them as well.
Comment #53
ressa CreditAttribution: ressa at Ardea commentedFixing
Expected double quotes string-quotes
message in test.Comment #55
ressa CreditAttribution: ressa at Ardea commentedComment #56
markconroy CreditAttribution: markconroy at Annertech commentedI'm not sure why this issue says #53 failed, since it's green.
Are we good to put this to RTBC?
Comment #57
ressa CreditAttribution: ressa at Ardea commentedIt was a
Quickedit: Element ... is not clickable
false positive (see #2829040: [meta] Known intermittent, random, and environment-specific test failures). I re-ran the test and it completed, which I should have added a comment about for clarity, sorry about that.The feedback from @Eli-T and @ckrina is that Source Sans Pro is the closest match, which is the font in the new patch. But the font size and spacing might need adjusting, so changing Status.
Comment #59
ressa CreditAttribution: ressa at Ardea commentedTo keep this moving, maybe someone could point out places where the font size and spacing needs updating? I just ran the patch with the Source Sans Pro font against 9.5.x and it passed (#53), so that bit is still working.
Comment #60
lauriiiWe still need to address #51.
Let's create a Drupal 10 version of the patch without the "woff" fonts because all browsers supported by Drupal 10 support woff2.
Comment #61
andy-blumComment #63
Anchal_gupta CreditAttribution: Anchal_gupta at Srijan | A Material+ Company for Drupal India Association commentedI have rerolled the patch against 10.1x.
also addressed #60 and applied the changes.
Comment #64
lauriiiThank you for the patch! Changing status to Needs review to trigger a CI run.
Comment #65
andy-blumThanks @Anchal_gupta for the patch! There are a couple issues:
Comment #66
Anchal_gupta CreditAttribution: Anchal_gupta at Srijan | A Material+ Company for Drupal India Association commentedplease ignore this patch. I have again reuploaded the patch in the same thread
Comment #67
Anchal_gupta CreditAttribution: Anchal_gupta at Srijan | A Material+ Company for Drupal India Association commentedThanks for your suggestions @andy-blum
I have addressed point 1. and 2. on comment #65
Here, I have Attached a patch again with fixed the error
Comment #68
ressa CreditAttribution: ressa at Ardea commentedThanks, changing status to trigger test.
Comment #69
andy-blumPatch in #67 still contains .woff files.
Additionally, to aid in the review of your fixes please do the following:
<issue number>-<comment number>
so that the patch number and comment numbers line up instead of incrementing the numbers of the patch.If you're at drupalcon right now and would like some assistance in any of this, you can reach out to one of the mentors in mentored contribution or find me in the orange shirt at the front-end table in general contribution.
Comment #70
Anchal_gupta CreditAttribution: Anchal_gupta at Srijan | A Material+ Company for Drupal India Association commentedThanks, @andy-blum for consistently reviewing my fixes and noticing my mistake, and assisting me. Really I grateful to you and next time I will remember that and follow this step
.Woff Files have been removed
I have attach the patch and interdiff also.
Comment #71
ressa CreditAttribution: ressa at Ardea commentedThanks @Anchal_gupta, please also change status to Needs review, to trigger the test.
Comment #72
Anchal_gupta CreditAttribution: Anchal_gupta at Srijan | A Material+ Company for Drupal India Association commentedOkay, Thanks @ressa, I changed the status to trigger test.
Comment #73
andy-blumPatch #70 works - the only change I'd request is to remove the comments around the font-face declarations. They don't add anything particularly valuable to core, so let's remove those and then we should be in good shape.
Comment #74
andy-blumComment #75
andy-blumNew patch & interdiff
Comment #77
andy-blumNew patch to satisfy the linter
Comment #78
ckrinaEven though this isn't changing the font-size, I think the overall result looks OK. So moving this to RTBC.
Comment #79
lauriiiThis was reviewed by @ckrina and I have reviewed this as well. Also discussed this with @justafish and @xjm and we agreed to backport this down until 9.5.x because Umami has no BC policy.
Comment #80
alexpottCommitted and pushed 1f1b26680c to 10.1.x and 141f888d82 to 10.0.x and ff1f52eb41 to 9.5.x. Thanks!
As per @lauriii and @xjm backported to 9.5.x
Comment #84
alexpottCommitted 9925f1d and pushed to 10.1x. Thanks!
Committed 141f888 and pushed to 10.0.x. Thanks!
Committed ff1f52e and pushed to 9.5.x. Thanks!
Comment #85
ressa CreditAttribution: ressa at Ardea commentedIt's so great to see this issue completed, and no longer rely on a third party. Thanks to everybody here who made it happen!