Improvements webfont loading for theme #29044

mrtuvn · 2020-07-08T14:56:08Z

Description (*)

This PR aims to bring improvement while theme load icons font. Apply font-display while magento import font icons
Ensure text remains visible during webfont load
The best option finally is block or optional for optimize web icons font load

Related Pull Requests

Fixed Issues (if relevant)

Fixes magento/magento2#<issue_number>

Manual testing scenarios (*)

...

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:

resolves [Issue] Improvements webfont loading for theme #29526: Improvements webfont loading for theme

m2-assistant · 2020-07-08T14:56:10Z

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

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
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.

mrtuvn · 2020-07-08T14:56:59Z

@magento run all tests

mrtuvn · 2020-07-08T14:58:01Z

@krzksz @ptylek can you guys give me a review ?

krzksz

I always like it when we remove some redundancy, good job!

krzksz · 2020-07-09T06:24:39Z

No possible to test with automatic tests. We need to manually make sure that icons are still working on Luma theme.

magento-engcom-team · 2020-07-09T06:24:40Z

Hi @krzksz, thank you for the review.
ENGCOM-7803 has been created to process this Pull Request

woutk88 · 2020-07-22T07:44:38Z

font-display: swap isn't suitable for an icon font since it has no fallback font.

See: https://developers.google.com/web/updates/2016/02/font-display#swap

Similar to block, this value should only be used when rendering text in a particular font is important for the page, but rendering in any font will still get a correct message across

Rendering 'any font' won't do for icons.

app/design/frontend/Magento/blank/web/css/source/_icons.less

app/design/frontend/Magento/luma/Magento_Theme/layout/default_head_blocks.xml

mrtuvn · 2020-07-24T18:29:49Z

@magento run all tests

engcom-Delta

@woutk88 @krzksz Could you review last commit?

woutk88 · 2020-07-29T07:31:51Z

@mrtuvn Earlier I suggested using either block or optional (since swap isn't an option for an icon font), and because block has a longer bock period, the latter might be a good choice. But now I'm having second thoughts about it (sorry). Read this description about using optional:

Similar to fallback, this is a good choice for when the downloading font is more of a “nice to have” but not critical to the experience. The optional value leaves it up to the browser to decide whether to initiate the font download, which it may choose not to do or it may do it as a low priority depending on what it thinks would be best for the user. This can be beneficial in situations where the user is on a weak connection and pulling down a font may not be the best use of resources.

However some icons are quite critical to the experience. For example the icons in the header, especially on mobile:

(Imagine not having the icons here, because the browser chose not to download the font because of a poor connection)

Therefore block does seems to be the best options here. And since we're preloading the font, the block-period probably isn't an issue.

Do you agree?

mrtuvn · 2020-07-29T08:00:23Z

@woutk88 I have tried optional display but sometimes font not display properly (browser not priority download font)
seem block is good option now

mrtuvn · 2020-07-29T08:02:17Z

@magento run all tests

sidolov · 2020-08-13T19:58:32Z

@magento create issue

mrtuvn · 2020-08-27T03:53:00Z

can we have QA testing on this

engcom-Delta · 2020-08-31T08:09:24Z

✔️ QA passed
Was checked icons are loaded on catalog page, product details page, shopping cart page, checkout page, wish list page and compare page on Luma/Blank themes and mobile/tablet/desktop views
Results on Luma:
Mobile:
Catalog page:

Product details page

Shopping cart page

Wish list page

Compare page

Tablet:
Catalog page:

Product details page

Shopping cart page

Wish list page

Compare page

Desktop:
Catalog page:

Product details page

Shopping cart page

Wish list page

Compare page

engcom-Delta · 2020-09-01T05:54:05Z

Note: Functional tests are failing

mrtuvn · 2020-09-01T11:20:59Z

@magento run all tests

engcom-Charlie · 2020-09-01T19:01:09Z

@magento run all tests

m2-assistant · 2020-09-07T16:05:21Z

Hi @mrtuvn, 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.

ghost added the Progress: pending review label Jul 8, 2020

magento-engcom-team added Area: Design/Frontend Release Line: 2.4 labels Jul 8, 2020

krzksz approved these changes Jul 9, 2020

View reviewed changes

ghost assigned krzksz Jul 9, 2020

ghost removed the Progress: pending review label Jul 9, 2020

krzksz added Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Progress: pending review labels Jul 9, 2020

ghost added the Progress: ready for testing label Jul 9, 2020

ihor-sviziev added Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. and removed Progress: pending review labels Jul 9, 2020

woutk88 suggested changes Jul 22, 2020

View reviewed changes

app/design/frontend/Magento/blank/web/css/source/_icons.less Outdated Show resolved Hide resolved

app/design/frontend/Magento/luma/Magento_Theme/layout/default_head_blocks.xml Outdated Show resolved Hide resolved

mrtuvn force-pushed the theme-improvements-webfont-preload branch 2 times, most recently from 0e68656 to 0293456 Compare July 24, 2020 18:28

mrtuvn requested a review from woutk88 July 26, 2020 06:28

engcom-Delta requested changes Jul 27, 2020

View reviewed changes

ghost assigned engcom-Delta Jul 27, 2020

ghost added Progress: needs update and removed Progress: ready for testing labels Jul 27, 2020

woutk88 approved these changes Jul 29, 2020

View reviewed changes

m2-assistant bot mentioned this pull request Aug 13, 2020

[Issue] Improvements webfont loading for theme #29526

Closed

ghost added the Priority: P3 May be fixed according to the position in the backlog. label Aug 13, 2020

sidolov added Priority: P3 May be fixed according to the position in the backlog. and removed Priority: P3 May be fixed according to the position in the backlog. labels Aug 13, 2020

ghost added Progress: testing in progress and removed Progress: ready for testing labels Aug 31, 2020

ghost added Progress: extended testing and removed Progress: testing in progress labels Aug 31, 2020

engcom-Charlie self-assigned this Aug 31, 2020

ghost added Progress: testing in progress and removed Progress: extended testing labels Aug 31, 2020

ghost added Progress: extended testing and removed Progress: testing in progress labels Sep 1, 2020

Improvements webfont loading for icons

be3c1ec

mrtuvn force-pushed the theme-improvements-webfont-preload branch from 3b397fa to be3c1ec Compare September 1, 2020 11:20

engcom-Charlie added the QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) label Sep 3, 2020

engcom-Delta added QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope and removed QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) labels Sep 4, 2020

ghost added Progress: accept and removed Progress: extended testing labels Sep 4, 2020

magento-engcom-team pushed a commit that referenced this pull request Sep 7, 2020

ENGCOM-7803: Improvements webfont loading for theme #29044

735a512

magento-engcom-team merged commit 23acb42 into magento:2.4-develop Sep 7, 2020

mrtuvn deleted the theme-improvements-webfont-preload branch September 12, 2020 06:44

Improvements webfont loading for theme #29044

Improvements webfont loading for theme #29044

Uh oh!

Conversation

mrtuvn commented Jul 8, 2020 • edited by m2-assistant bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

Resolved issues:

Uh oh!

m2-assistant bot commented Jul 8, 2020

Uh oh!

mrtuvn commented Jul 8, 2020

Uh oh!

mrtuvn commented Jul 8, 2020

Uh oh!

krzksz left a comment

Choose a reason for hiding this comment

Uh oh!

krzksz commented Jul 9, 2020

Uh oh!

magento-engcom-team commented Jul 9, 2020

Uh oh!

woutk88 commented Jul 22, 2020

Uh oh!

Uh oh!

Uh oh!

mrtuvn commented Jul 24, 2020

Uh oh!

engcom-Delta left a comment

Choose a reason for hiding this comment

Uh oh!

woutk88 commented Jul 29, 2020

Uh oh!

mrtuvn commented Jul 29, 2020

Uh oh!

mrtuvn commented Jul 29, 2020

Uh oh!

sidolov commented Aug 13, 2020

Uh oh!

mrtuvn commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

engcom-Delta commented Aug 31, 2020

Uh oh!

engcom-Delta commented Sep 1, 2020

Uh oh!

mrtuvn commented Sep 1, 2020

Uh oh!

engcom-Charlie commented Sep 1, 2020

Uh oh!

m2-assistant bot commented Sep 7, 2020

Uh oh!

Uh oh!

mrtuvn commented Jul 8, 2020 •

edited by m2-assistant bot

Loading

mrtuvn commented Aug 27, 2020 •

edited

Loading