Chinese Noto fonts mixin does not allow using symbolic subsets

[NightTsarina]

Describe the bug

I am using the faces mixin to integrate fonts in my SASS files, and due to some weird defaults (most fonts will only produce CSS for the latin subset!) I need to specify the font subsets to use.

This worked well for most Noto fonts, but for the Chinese fonts it did not generate any CSS.

Steps to Reproduce

@use '@fontsource-variable/noto-sans-sc/scss/mixins' as NotoSansSC
@include NotoSerifSC.faces($directory: $fonts_dir, $subsets: all)

Expected behavior

Multiple @font-facedefinitions for the NotoSansSC font. But nothing is generated

Version

No response

OS

No response

Browser

No response

Additional context

No response

[RBilly]

I have the same issue trying to generate faces for Noto CJK, it seems related to the following open issues:
#730
From my observation, there is an issue with the metadata google-font-v2 json file (https://github.com/fontsource/font-files/blob/main/metadata/google-fonts-v2.json).
All defSubset are set to "latin" which is used to generate the metada.scss mixin file.
Since the metada.scss mixin file has $defaults: (subset: latin) for all CJK instead of chinese-simplified, japanese and korean, the mixin generator is failing when passing the expected subset. This is because the generator is looking for a unicode subset that matches the subset or for all number unicode subset if the subset matches the metada default subset

@if (
        ($subset == $unicodeSubset) or
          (
            // Expecting to use numeric subset but since default subset is latin, it will not produce any faces
            ($subset == map.get($metadata, defaults, subset)) and not
              list.index(map.get($metadata, subsets), $unicodeSubset)
          )
      )

[ayuhito]

I'm actually not too familiar with Sass so I would really appreciate if someone could make a PR that addresses this and #738. It's a pretty simple template to edit if you know what you are doing.

[RBilly]

I think the current template is perfectly fine, the issue for me is the value set as default subset in the metada mixin template. I'm not exactly sure what is the source of this metadata.defSubset exactly.
I found 2 places:

1. In this custom/create.ts, the defSubset is hard set to 'latin' .
2. In this google/build.ts, the defSubset comes from font object that seems to be from the metadata google-font-v2 json file that I mentionned in my previous message (I may be wrong about that).

As the Noto Sans font is a google font I guess that the metada comes from this json file, but I have no idea how this json file could be fixed. Is this json file generated by some script or is it retrieved from some google api ?

If the source can't be fixed, there is always a possibility to hack something in the sass mixin itself but that would be a last resort

[ayuhito]

But I have no idea how this json file could be fixed. Is this json file generated by some script or is it retrieved from some google api ?

It's retrieved by a Google API so we can't really change it because there are thousands of fonts and they can have differing defSubset values. e.g. Almarai has a default subset of Arabic. Each font is split into subsets, such as latin, latin-ext, cyrillic..., but CJK fonts are so huge that Google splits them into numbered subsets such as [0], [1],..., [165] and so on.

Technically, the template default for subset is inconsistent with our existing CSS files. Default implementation should just output ALL subsets and let the browser optimise the requests using the unicode-range selector. We shouldn't rely on defSubset in this case except as a fallback if everything else fails.

[RBilly]

I get what you're saying and I agree. I dug a bit more in the generation of metadata from the google-font-api-v2 and found that the json returned by the api does not provide a defSubset, it is added by the metadata generator
I don't know much about fonts management but doing const defSubset = font.subsets[0]; seems more correct for me, but maybe it's a bit unpredictable and I don't know what defSubset is used for, so maybe the impact would be too great to change that.
My other solution would be to modify the sass template to ignore the defSubset, so the bit of sass code that I shared in my first message would be modified to

@if (
        ($subset == $unicodeSubset) or
          (
              /* 
                 Instead of trying to match the subset with the defSubset
                 we just check that the subset is in the list of subsets but not in the unicode subsets
                 which means we are looking to get the numbered unicode subsets 
              */
              map.has-key($metadata, subsets, $subset) and not
              list.index(map.get($metadata, subsets), $unicodeSubset)
          )
      )

This solution is quite easy and would solve the issue without breaking anything.

What do you think ?

[ayuhito]

json returned by the api does not provide a defSubset, it is added by the metadata generator

Apologies. I completely forgot that it was added by us.

I don't know much about fonts management but doing const defSubset = font.subsets[0]; seems more correct for me, but maybe it's a bit unpredictable and I don't know what defSubset is used for, so maybe the impact would be too great to change that.

font.subsets is ordered alphabetically. So a lot of fonts would be marked with cyrillic instead of latin which isn't correct. Default subset is just a hint that the website uses as the first language to display. It somewhat broke when Google introduced numbered unicode subsets though.

This solution is quite easy and would solve the issue without breaking anything.

Give it a go! We have tests for compiled CSS (not too much though) so you can verify if something is going wrong. I still think we should update the default to use all instead since that is the most efficient approach for end users to save on bandwidth. Would be an acceptable breaking change paired with #798.

[RBilly]

font.subsets is ordered alphabetically. So a lot of fonts would be marked with cyrillic instead of latin which isn't correct. Default subset is just a hint that the website uses as the first language to display. It somewhat broke when Google introduced numbered unicode subsets though.

Ah yes, if it is ordered alphabetically then I understand the problematic and it does even make more sense to use all as default then and any other specific value would be kind of random anyway.

Give it a go! We have tests for compiled CSS (not too much though) so you can verify if something is going wrong. I still think we should update the default to use all instead since that is the most efficient approach for end users to save on bandwidth. Would be an acceptable breaking change paired with #798.

Ok, thank you for your guidance, I'll give it a go for both issues.

[RBilly]

@ayuhito It seems like I did not understood the contribution guideline correctly as I'm not able to open a PR
I did it on my fork you can check it there: https://github.com/RBilly/fontsource/pull/1/files?diff=unified&w=1
Would you be able to help me get it done correctly ?

[ayuhito]

Perhaps this guide would help you point your PR to this repository?

TLDR; when clicking on "New pull request", you should see many options on where you can point your current branch to, including upstream repositories.

[RBilly]

@ayuhito thanks, it seems like I just missed the small link "compare accross fork" link when create a new PR.