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.

CommentFileSizeAuthor
#44 interdiff-43-44.txt325 bytesmarkconroy
#44 2943657-44.patch3.93 KBmarkconroy
#43 interdiff_39-43.txt433 bytespawandubey
#43 embedding_needed_weights_open_sans-2943657-43.patch3.93 KBpawandubey
#41 Screenshot 2019-03-29 at 08.35.23.png127.6 KBmarkconroy
#39 interdiff_37-39.txt1.93 KBpawandubey
#39 embedding_needed_weights_open_sans-2943657-39.patch3.77 KBpawandubey
#37 interdiff_36-37.txt1.01 KBpawandubey
#37 embedding_needed_weights_open_sans-2943657-37.patch2.58 KBpawandubey
#36 interdiff_27-36.txt3.33 KBvadim.hirbu
#36 embedding_needed_weights_open_sans-2943657-36.patch2.33 KBvadim.hirbu
#32 scopeone.PNG17.58 KBShiva Srikanth T
#27 2943657-27.patch2.54 KBalexpott
#27 23-27-interdiff.txt1.44 KBalexpott
#23 font_weight-2943657-23.patch1.68 KBUtkarsh_Mishra
#16 fonts-umami.png86.92 KBckrina
#10 font_weight-2943657-10.patch1.67 KBShiva Srikanth T
#10 interdiff-2943657-8-10.txt460 bytesShiva Srikanth T
#8 2943657-8.patch1.49 KBfinnsky
#8 interdiff-2943657-2-8.txt499 bytesfinnsky
#5 font_weight-2943657-5.patch750 bytesShiva Srikanth T
#2 font_weight-2943657-2.patch1.49 KBckrina
#5 regular.png17.84 KBShiva Srikanth T
#5 regular-with-extra-property.png20.4 KBShiva Srikanth T
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ckrina created an issue. See original summary.

ckrina’s picture

Status: Active » Needs review
FileSize
1.49 KB

Adding quick patch and a few more info for the library.

Status: Needs review » Needs work

The last submitted patch, 2: font_weight-2943657-2.patch, failed testing. View results

smaz’s picture

Issue tags: +dclondon, +Novice
Shiva Srikanth T’s picture

"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
regular fontRegular font with extra property

Shiva Srikanth T’s picture

Status: Needs work » Needs review
ckrina’s picture

Status: Needs review » Needs work

Thanks 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.

finnsky’s picture

Hi! Added most complicated fix:)
Also seems here we need to check:

Somewhere we have properties duplicated:

font-weight: 400;
font-weight: normal;

and somewhere: only 400 or only normal
so maybe we need to choose one policy everywhere.

finnsky’s picture

Status: Needs work » Needs review
Shiva Srikanth T’s picture

Thanks @ckrina for the information I have read the article.

Yes, @finnsky we can use font-weight: 400; and remove font-weight: normal; from the code as a choose one policy.

Shiva Srikanth T’s picture

finnsky’s picture

It was not only about base.css but in other components as well. But i'm sure it is not the question of this task.

ckrina’s picture

As @finnisky says it looks like there are several components using both syntax: font-weigh: normal and font-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.

Shiva Srikanth T’s picture

Okay got it.

ckrina’s picture

Status: Needs review » Reviewed & tested by the community

Just 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

ckrina’s picture

Issue summary: View changes
FileSize
86.92 KB

I forgot to upload the screenshot:

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: font_weight-2943657-10.patch, failed testing. View results

ckrina’s picture

Issue tags: +dcruhr18
ressa’s picture

Is 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.

ckrina’s picture

Status: Needs work » Reviewed & tested by the community

@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.

alexpott’s picture

We 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.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/profiles/demo_umami/themes/umami/umami.libraries.yml
@@ -60,9 +60,13 @@ two-columns:
+    name: Apache License, Version 2.0
...
+      'https://fonts.googleapis.com/css?family=Open+Sans:400,400i,700,700i|Scope+One': { type: external, minified: true }

Looks good other than the fact that Scope One font is licensed under Open Font License.

Utkarsh_Mishra’s picture

Status: Needs work » Needs review
FileSize
1.68 KB

Fixed the license name and link as suggested.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/profiles/demo_umami/themes/umami/umami.libraries.yml
@@ -60,9 +60,13 @@ two-columns:
+  remote: https://fonts.google.com
+  license:
+    name: SIL Open Font License, Version 1.1
+    url: http://scripts.sil.org/OFL_web

This is a bit more complex than this :( Scope+One is SIL and Open Sans is Apache - not sure how to deal with this.

ressa’s picture

I 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?

alexpott’s picture

@ressa the issue here is not copyright freedom - to quote Google

Yes. The open source fonts in the Google Fonts catalog are published under licenses that allow you to use them on any website, whether it’s commercial or personal.

https://developers.google.com/fonts/faq

The issue is one of open-source license compatibility which is much much thornier.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.44 KB
2.54 KB

I 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.

ressa’s picture

@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.

Sure @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.

Eli-T’s picture

Issue tags: -dclondon, -dcruhr18 +Nwdug_may18
borisson_’s picture

I think we can start by doing #27 and see if we can do the packaging in a followup later?

markconroy’s picture

If 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.

Shiva Srikanth T’s picture

FileSize
17.58 KB

Scope 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.

Scope One

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

smaz’s picture

Status: Needs review » Needs work

This needs a re-roll.

vadim.hirbu’s picture

Status: Needs work » Needs review
FileSize
2.33 KB
3.33 KB

Rerolled #27 and added interdiff.

pawandubey’s picture

Patch#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.

markconroy’s picture

Status: Needs review » Needs work

Thanks very much for working on this. Looks like it's almost ready to RTBC.

Two small items here:

  1. +++ b/core/profiles/demo_umami/themes/umami/css/base.css
    @@ -182,7 +182,7 @@ h4 {
    +  font-weight: 400;
    

    I think we can remove this `font-weight: 400;` if Scope One only has the 400 weight

  2. +++ b/core/profiles/demo_umami/themes/umami/css/base.css
    @@ -190,14 +190,13 @@ h5 {
    +  font-weight: 400;
    

    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.

pawandubey’s picture

@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.

pawandubey’s picture

Status: Needs work » Needs review
markconroy’s picture

Status: Needs review » Needs work
Issue tags: -Nwdug_may18 +CSS, +CSS novice
FileSize
127.6 KB

Hi @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:

h1, h2, h3, h4, h5, h6 {
  font-weight: 400;
}

I think it's being caused by browsers automatically assigning font-weight: bold to headings.

See screenshot - left = with patch; right = current, without patch

umami heading font weight

pawandubey’s picture

@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.

pawandubey’s picture

Status: Needs work » Needs review
FileSize
3.93 KB
433 bytes

@markconroy

I have re-rolled the patch again as per your suggestion. Please review it and let me know if any changes are require.

markconroy’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.93 KB
325 bytes

Thanks @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.

kjay’s picture

Thanks 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.

  • Gábor Hojtsy committed 73e510e on 8.8.x
    Issue #2943657 by pawandubey, Shiva Srikanth T, markconroy, ckrina,...

  • Gábor Hojtsy committed 9df6800 on 8.7.x
    Issue #2943657 by pawandubey, Shiva Srikanth T, markconroy, ckrina,...
Gábor Hojtsy’s picture

Version: 8.8.x-dev » 8.7.x-dev
Status: Reviewed & tested by the community » Fixed

Thanks all, committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.