Static font name for loading fonts

[woutk88]

Description (*)

The variable @font-family-name__base shouldn't be used when loading the Open Sans font because it's not really something that should change, since the source will still point to the Open Sans font. Using the variable here (can) cause issues when you change the base font (e.g. in a child theme).

Example 1:

If your custom theme extends from the Blank Theme and you change the base font with:

@font-family-name__base: 'Trenda';

This will be the compiled CSS:

Which is already wrong, but doesn't directly lead to problems since 'Trenda' is a custom font and you'll probably do something like this in your theme:

.lib-font-face(
    @family-name: 'Trenda',
    @font-path: '@{baseDir}fonts/trenda/Trenda-Regular',
    @font-weight: @font-weight__regular,
    @font-style: normal,
    @font-display: swap
);

In which case this @font-face block will be included in the compiled CSS later, and your browser will only 'use' this one. However, if you only load the font in regular font-weight here, and you want to show text in bold, the browser will show that text in Open Sans (bold).

Example 2:

If your custom theme extends from the Blank Theme and you want to switch to a 'websafe' font, for example Times New Roman:

@font-family-name__base: 'Times New Roman';

Text won't be displayed in the real Times New Roman but in Open Sans because that is loaded under the name 'Times New Roman'.

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

1. Change @font-family-name__base to 'Times New Roman' (either temporary in Blank Theme or in a theme that extends from Blank)
2. Text should be shown in 'Times New Roman' (serif), not in Open Sans (sans-serif).
3. Check the compiled CSS and see if the open sans is properly loaded (name and src of @font-face block should still be of Open Sans)

Questions or comments

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds are green)

Resolved issues:

1. resolves [Issue] Static font name for loading fonts #29515: Static font name for loading fonts

[m2-assistant]

Hi @woutk88. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE,
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

[woutk88]

@magento run Functional Tests CE

[mrtuvn]

this is not actually a bug in my opinion as frontender
If you change font family name you also need to change font path

In css/source/_variables.less change font here is correct but not enough
You need to define font path too

In css/source/_typography.less
you also need redefine font path for new @font-family-name__base redefined in _variables.less

Example in typography

@font-path: '@{baseDir}fonts/trenda/Trenda-Regular',

[mrtuvn]

https://devdocs.magento.com/guides/v2.4/frontend-dev-guide/css-topics/using-fonts.html

[woutk88]

Ok, sure it makes sense to override web/css/source/_typography.less when you would want to switch to a different custom font and not use Open Sans at all. But still, simply setting @font-family-name__base to a different valid value shouldn't produce incorrect code, right?

And what if you want to switch to a web safe font, which doesn't require any loading of fonts with @font-face? I would simply want to do @font-family-name__base: 'Helvetica';, but that is currently not enough because @font-family-name__base is used for loading Open Sans, which I think is not really in line with the documentation.

Or, if you change the base font but still want to use Open Sans for headings? You should be able to do this by only setting @heading__font-family__base: 'Open Sans';, but that won't work because the value of @font-family-name__base changed.

Therefore I think, regardless of the way or the place you load and set/change your fonts, @font-family-name__base shouldn't be used for loading the Open Sans font.

[magento-engcom-team]

Hi @ihor-sviziev, thank you for the review.
ENGCOM-7901 has been created to process this Pull Request

[ihor-sviziev]

@mrtuvn I think it's an issue, as the font path is already defined as open sans, and not getting from some variable

[ihor-sviziev]

@magento run all tests

[ihor-sviziev]

We found some time ago following code in styles-m.css file on the frontend: 🤯

@font-face {
    font-family: 'MyCustomFont';
    src: url('../fonts/opensans/light/opensans-300.woff2') format('woff2'), url('../fonts/opensans/light/opensans-300.woff') format('woff');
    font-weight: 300;
    font-style: normal;
    font-display: swap
}

@font-face {
    font-family: 'MyCustomFont';
    src: url('../fonts/opensans/regular/opensans-400.woff2') format('woff2'), url('../fonts/opensans/regular/opensans-400.woff') format('woff');
    font-weight: 400;
    font-style: normal;
    font-display: swap
}

@font-face {
    font-family: 'MyCustomFont';
    src: url('../fonts/opensans/semibold/opensans-600.woff2') format('woff2'), url('../fonts/opensans/semibold/opensans-600.woff') format('woff');
    font-weight: 600;
    font-style: normal;
    font-display: swap
}

@font-face {
    font-family: 'MyCustomFont';
    src: url('../fonts/opensans/bold/opensans-700.woff2') format('woff2'), url('../fonts/opensans/bold/opensans-700.woff') format('woff');
    font-weight: 700;
    font-style: normal;
    font-display: swap
}

As result when you're trying to use some custom font by changing variable @font-family-name__base: 'MyCustomFont'; - you're getting this issue. In our case we're using Google Fonts to load custom font - so it just overrides it, but in some case it might cause issues - your browser uses on the website custom font, but visually it's the same as Open Sans.

I thing we need to change severity to S2

[ihor-sviziev]

@magento create issue

[engcom-Alfa]

✔️ QA Passed

Precondition:

A custom theme that extends from Blank

Manual testing scenario:

1. Change @font-family-name__base to 'Times New Roman'
2. The text should be shown in 'Times New Roman' (serif), not in Open Sans (sans-serif).
3. Check the compiled CSS and see if the open sans is properly loaded (name and src of @font-face block should still be of Open Sans)

Before: ✖️ The text is shown in Open Sans font

compiled CSS. ✖️ Name changed to 'Times New Roman'

After: ✔️ The text is shown in Times New Roman font

compiled CSS. ✔️ Name and src of the @ font-face block remain Open Sans

[engcom-Kilo]

@magento run all tests

[engcom-Kilo]

@woutk88
Please update your branch with mainline. Seems code it outdated.
Or also can grant permission to a branch.
Thanks

[engcom-Kilo]

@magento run all tests

[m2-assistant]

Hi @woutk88, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.