Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
This issue is a follow-up from Github.
Now, the Umami theme is embedding h fonts Scope One and Open Sans.
'https://fonts.googleapis.com/css?family=Open+Sans:400,400i,700,700i|Scope+One'
But only the needed weights should be used for each one:
- Scope One has only one weight, so changing wrong
700
weights to 400. - Open Sans has several weights and we're loading all the font. Only the needed weights should be used: 400 and 700.
Proposed resolution
Load only the needed weights for each font.
Comment | File | Size | Author |
---|---|---|---|
#44 | interdiff-43-44.txt | 325 bytes | markconroy |
#44 | 2943657-44.patch | 3.93 KB | markconroy |
#43 | interdiff_39-43.txt | 433 bytes | pawandubey |
#43 | embedding_needed_weights_open_sans-2943657-43.patch | 3.93 KB | pawandubey |
#41 | Screenshot 2019-03-29 at 08.35.23.png | 127.6 KB | markconroy |
Comments
Comment #2
ckrinaAdding quick patch and a few more info for the library.
Comment #4
smazComment #5
Shiva Srikanth T CreditAttribution: Shiva Srikanth T as a volunteer commented"Open Sans" will not load all the font weights until we specify the font weights, it will load only regular font with font weight 400.
Loading extra weights to the font will effect the Loading time. So there is need of adding extra font weights to the theme. i.e., 400,400i,700,700i so code remains same as
'https://fonts.googleapis.com/css?family=Open+Sans|Scope+One'
Screenshots form https://fonts.google.com
Comment #6
Shiva Srikanth T CreditAttribution: Shiva Srikanth T as a volunteer commentedComment #7
ckrinaThanks for working on this @Shiva Srikanth T! Good to learn that if you don't specify the weight it only loads the 400.
Anyway, we need the italic font(400i) and the bold one (700 and its italic 700i) because otherwise the browser will simulate them. It means it'll use faux weights and styles instead the real ones and designed specifically for this. You can read a bit more about this in this article.
Comment #8
finnsky CreditAttribution: finnsky at Skilld commentedHi! Added most complicated fix:)
Also seems here we need to check:
Somewhere we have properties duplicated:
and somewhere: only 400 or only normal
so maybe we need to choose one policy everywhere.
Comment #9
finnsky CreditAttribution: finnsky at Skilld commentedComment #10
Shiva Srikanth T CreditAttribution: Shiva Srikanth T as a volunteer commentedThanks @ckrina for the information I have read the article.
Yes, @finnsky we can use
font-weight: 400;
and removefont-weight: normal;
from the code as a choose one policy.Comment #11
Shiva Srikanth T CreditAttribution: Shiva Srikanth T as a volunteer commentedComment #12
finnsky CreditAttribution: finnsky at Skilld commentedIt was not only about base.css but in other components as well. But i'm sure it is not the question of this task.
Comment #13
ckrinaAs @finnisky says it looks like there are several components using both syntax:
font-weigh: normal
andfont-weigh: 400
, so I'd create another issue to discuss which one should we implement and address the related changes for all components there.So I'd leave for this issue only the font declaration in libraries and fix weight for Scope One like patch from #8 does.
Comment #14
Shiva Srikanth T CreditAttribution: Shiva Srikanth T as a volunteer commentedOkay got it.
Comment #15
ckrinaJust tested it and it's loading the needed fonts, so RTBC! Thanks @finnsky and @Shiva Srikanth T for working on this!
I've created a follow-up issue for the duplicity in the syntax: #2950158: Choose policy for defining font-weight on Umami theme
Comment #16
ckrinaI forgot to upload the screenshot:
Comment #18
ckrinaComment #19
ressa CreditAttribution: ressa at Ardea commentedIs it worth considering including the fonts in the theme, and not rely on Google? The Google webfonts helper can help download the required web font and suggests css code for the implementation: https://google-webfonts-helper.herokuapp.com/fonts/open-sans?subsets=latin
I am not sure if including the fonts in the theme, in stead of calling them remotely is a problem? See fx #2916161: License considerations for the Out of the box initiative.
Comment #20
ckrina@ressa thank you for the suggestion! Although it's an interesting discussion to have, it's out of the scope of this issue since this one is more a clean up of things already there.
I'll RTBC this one again and let's face this in another issue. I'd be great if you could open a new one related to this one so we can discuss it further. Also, there's this related issue to see the approach it's being taken for images: #2936841: Remove images from demo_umami profile and download upon installation instead.
Comment #21
alexpottWe can't download Open sans because it uses the Apache license which is not GPL 2 or later compatible - and Scope One is SIL which is also not GPL compatible - see https://www.gnu.org/licenses/license-list.html#Fonts so I don't think we're going to be able to progress on #19.
Comment #22
lauriiiLooks good other than the fact that Scope One font is licensed under Open Font License.
Comment #23
Utkarsh_Mishra CreditAttribution: Utkarsh_Mishra at OpenSense Labs for DrupalFit commentedFixed the license name and link as suggested.
Comment #24
alexpottThis is a bit more complex than this :( Scope+One is SIL and Open Sans is Apache - not sure how to deal with this.
Comment #25
ressa CreditAttribution: ressa at Ardea commentedI know I am bit late to this, but is it worth considering to only use fonts with the most copyright "freedom", to avoid getting bogged down over license issues?
Comment #26
alexpott@ressa the issue here is not copyright freedom - to quote Google
https://developers.google.com/fonts/faq
The issue is one of open-source license compatibility which is much much thornier.
Comment #27
alexpottI think we have to take the hit of the extra web request here because we need the licenses to be correct. Also 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.
Comment #28
ressa CreditAttribution: ressa at Ardea commentedSure @ckrina, I have created the issue #2960583: Include fonts in the Umami demo theme. And thank you @alexpott, for sharing the info about packaging fonts perhaps being an option, I have included it in the issue.
Comment #29
Eli-TComment #30
borisson_I think we can start by doing #27 and see if we can do the packaging in a followup later?
Comment #31
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedIf Scope One is only using the 700 weight, should we not be only adding that weight to the theme? Looks like we are now correctly adding the weights we want for Open Sans, but the full font for Scope One?
+ 'https://fonts.googleapis.com/css?family=Scope+One': { type: external, minified: true }
If that's the case, this needs to go back to 'Needs (a tiny bit of) work'. If it's not the case, I think it's ready for RTBC.
Comment #32
Shiva Srikanth T CreditAttribution: Shiva Srikanth T as a volunteer commentedScope one font having only 400 weight please refer to the image. No need to specify weight while loading the font by default it will load both regular and 400 weight. I think it's ready for RTBC.
Comment #35
smazThis needs a re-roll.
Comment #36
vadim.hirbu CreditAttribution: vadim.hirbu at FFW commentedRerolled #27 and added interdiff.
Comment #37
pawandubey CreditAttribution: pawandubey as a volunteer and at TATA Consultancy Services commentedPatch#36 looks good and working for me.
I have re-rolled the patch where I have removed the existing font-weight declaration from the media query for tag
<h5>
and<h6>
Please review this patch and let me know if any changes are require.
Comment #38
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedThanks very much for working on this. Looks like it's almost ready to RTBC.
Two small items here:
I think we can remove this `font-weight: 400;` if Scope One only has the 400 weight
I think we can remove this `font-weight: 400;` if Scope One only has the 400 weight
The patch seems to remove the font-weight (which is good), but also seems to introduce it again (which is a little less than good). Can we have it removed in each place?
Sidenote (just to throw a spanner in the works):
I wouldn't be against leaving in the 400 in each place right across the theme and specifiying it in our @import call. On the off chance that a 500 or 700 weight is ever released, we don't want to be downloading those and have to come back to this patch. I won't hold up RTBC on this point, but something maybe to consider for later.
Comment #39
pawandubey CreditAttribution: pawandubey as a volunteer and at TATA Consultancy Services commented@markconroy
I have re-rolled the patch as per your suggestion. I have removed all the
font-weight: 400
style from the code.Please review the same and let me know if any changes are require.
Comment #40
pawandubey CreditAttribution: pawandubey as a volunteer and at TATA Consultancy Services commentedComment #41
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedHi @pawandubey
I'm very sorry, but it looks like removing the
font-weight: 400
actually causes a regression, so I think we are going to need to put something like this in the code:I think it's being caused by browsers automatically assigning
font-weight: bold
to headings.See screenshot - left = with patch; right = current, without patch
Comment #42
pawandubey CreditAttribution: pawandubey as a volunteer and at TATA Consultancy Services commented@markconroy
That's what my understanding that removing this style will make the element to be appear as bold which are default in nature.
As there is no bold font release yet for Scope One, so for the end users the heading will not appear as bold which must be default in nature. I will suggest to have this behavior to be considered for the end users by which heading can be visible in bold be default via patch#39.
Meanwhile, I will re-roll the patch again if don't agree with this. :)
Let me know your thought.
Comment #43
pawandubey CreditAttribution: pawandubey as a volunteer and at TATA Consultancy Services commented@markconroy
I have re-rolled the patch again as per your suggestion. Please review it and let me know if any changes are require.
Comment #44
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedThanks @pawandubey
I'll leave it up to @kjay as the lead designer to decide if we want bold or not for those headings. For now, I'm happy that this patch does what it sets out to do without causing regressions.
I'm marking this as RTBC but uploading a new patch and interdiff that just removes two spaces at the end of line 48 so the commit doesn't fail because of our coding standards.
Comment #45
kjay CreditAttribution: kjay commentedThanks everyone. It is as @pawandubey points out. Scope One does not offer a bold weight and so font weight 400 is correct only. +1 for the RTBC.
Comment #48
Gábor HojtsyThanks all, committed.