wp-rocket icon indicating copy to clipboard operation
wp-rocket copied to clipboard

Closes #7378: Include import tags in preload fonts

Open Miraeld opened this issue 6 months ago • 5 comments

Description

Fixes #7378 Process @import tags to preload fonts.

Type of change

  • [x] Enhancement (non-breaking change which improves an existing functionality).

Detailed scenario

What was tested

Same as original issue

How to test

Refer to original issue

Technical description

Documentation

This pull request includes updates to improve font processing in the getFontFaceRules method and a dependency update in package.json. The changes enhance code maintainability and functionality, particularly for handling nested stylesheets and font variations.

Font processing improvements:

  • Refactored the getFontFaceRules method in assets/js/wpr-beacon.js to use a recursive approach for processing nested stylesheets, ensuring all font-face rules are captured, even in imported stylesheets.
  • Simplified and clarified regex patterns for extracting URLs and font-family properties, improving accuracy and readability.
  • Streamlined the creation of stylesheetFonts objects by consolidating object initialization and formatting code.

Dependency update:

  • Updated wp-rocket-scripts in package.json to a specific GitHub branch (enhancement/7378-include-import-tags-in-preload-fonts) to include enhancements related to font preloading.

New dependencies

Not new, but updated dependencies for rocket-scripts

Risks

None

Mandatory Checklist

Code validation

  • [x] I validated all the Acceptance Criteria. If possible, provide screenshots or videos.
  • [x] I triggered all changed lines of code at least once without new errors/warnings/notices.
  • [x] I implemented built-in tests to cover the new/changed code.

Code style

  • [x] I wrote a self-explanatory code about what it does.
  • [x] I protected entry points against unexpected inputs.
  • [x] I did not introduce unnecessary complexity.
  • [x] Output messages (errors, notices, logs) are explicit enough for users to understand the issue and are actionnable.

Miraeld avatar Jun 12 '25 01:06 Miraeld

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
Report missing for e7d52c2623b64431d1c55923c73fc37f6f6248f8[^1] :white_check_mark: (target: 50.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (e7d52c2623b64431d1c55923c73fc37f6f6248f8) Report Missing Report Missing Report Missing
Head commit (ef3aaade1a133fdef2bced6ede4073891ecc98e6) 39689 17509 44.12%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#7458) 0 0 ∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

[^1]: Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.

codacy-production[bot] avatar Jun 12 '25 01:06 codacy-production[bot]

Thank you, @Miraeld, for this PR.

I tested it using multiple templates where Google Fonts were included via @import, but it looks like the preload logic still isn’t picking them up.

  • Test template with Google Fonts added directly in a
  • Test template with Google Fonts included via an external CSS file that uses @import: https://newer.rocketlabsqa.ovh/preloadfonts_gf_import_css/

Would appreciate a second look!

hanna-meda avatar Jun 16 '25 22:06 hanna-meda

Hello @hanna-meda these happens because of CORS blocking it... Not sure how we can handle that, I can't find a reliable solution. @jeawhanlee do you have any idea passing through your mind ?

Miraeld avatar Jun 17 '25 01:06 Miraeld

The Problem

The original beacon code had an issue with fonts loaded via @import statements:

  1. ✅ It could detect the @import URLs and fetch the external CSS
  2. ✅ It could parse the @font-face rules from the imported CSS
  3. ✅ It could match font families used by elements on the page
  4. But it failed to include these fonts in the final results because they weren't found in performance.getEntriesByType("resource")

The Solution

I modified the summarizeMatches function to:

  1. Prioritize above-the-fold usage: If a font is used by elements above the fold, include it regardless of whether it appears in performance API results
  2. Fallback URL logic: Use stylesheet URLs when network performance entries don't contain the font URLs
  3. Better handling of @import fonts: Recognize that fonts loaded via @import may not appear in performance entries due to timing or CORS restrictions

Key Changes Made

  1. Enhanced getFontFaceRules(): Made it async and added support for processing @import rules by fetching external CSS content
  2. Improved summarizeMatches(): Added fallback logic to include above-the-fold fonts even when they're not in performance entries

The beacon should now correctly detect and preload Google Fonts (and other external fonts) loaded via @import statements, both in inline <style> blocks and external CSS files.

@hanna-meda could you confirm ? It's working on my side.

Miraeld avatar Jun 18 '25 03:06 Miraeld

Hi @Miraeld,

I can confirm that the beacon now correctly picks up fonts added via @import, both in inline

  1. Unexpected preloading of theme assets The beacon sometimes picks up and preloads theme assets like this font file: https://e2e.rocketlabsqa.ovh/wp-content/themes/twentytwenty/assets/fonts/inter/Inter-upright-var.woff2 even though the font doesn’t seem to be present in NW (of ?nowprocket). Pages where this occurs (on e2e, using the Twenty Twenty theme and a 1600x700px viewport):
  • https://e2e.rocketlabsqa.ovh/preloadfonts_inlinestyle/
  • https://e2e.rocketlabsqa.ovh/preloadfonts_cssfile/
  • https://e2e.rocketlabsqa.ovh/preloadfonts_incssmega2/
  • https://e2e.rocketlabsqa.ovh/preloadfonts_displaywithscript2/
  1. Duplicate font pickup with corrupted URL On this page: https://e2e.rocketlabsqa.ovh/preloadfonts_internal_inline_external/ the Playfair font appears to be picked up twice, but the second URL seems truncated or malformed:
"https:\/\/fonts.gstatic.com\/s\/playfairdisplay\/v39\/nuFvD-vYSZviVYUb_rj3ij__anPXJzDwcbmjWBN2PKdFvXDTbtPK-F2qC0usEw.woff2",
"https:\/\/fonts.gstatic.com\/s\/playfairdisplay\/v39\/nuFvD-vYSZviVYUb_rj3ij__anPXJzDwcbmjWBN2PKdFvXDXbtPK-F2qC0s.woff2"
preloadfonts_internal_inline_external

hanna-meda avatar Jun 19 '25 10:06 hanna-meda

After discussing this with @DahmaniAdame he mentioned that we can go with the current PR if the only issue is that import inside import and open another GH issue to track this.

cc @Miraeld let's make this next week after finishing the sprint priority issues.

wordpressfan avatar Jul 04 '25 11:07 wordpressfan

@hanna-meda from what I've tested following my latest commit, I don't get these regressions anymore. Could you confirm ?

Miraeld avatar Jul 08 '25 03:07 Miraeld

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
Report missing for e7d52c2623b64431d1c55923c73fc37f6f6248f8[^1] :white_check_mark: (target: 50.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (e7d52c2623b64431d1c55923c73fc37f6f6248f8) Report Missing Report Missing Report Missing
Head commit (c00f8508af1d7cf14c66897bd319656f54af4c47) 39811 17574 44.14%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#7458) 0 0 ∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

[^1]: Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.

codacy-production[bot] avatar Jul 08 '25 03:07 codacy-production[bot]

@Miraeld, Regarding the 2 points above:

  1. Unexpected preloading of theme assets - This is no longer happening
  2. Duplicate font pickup with corrupted URL - Turns out both URLs were valid: the longer one included Latin + Cyrillic, and the shorter one is Latin-only. We’re preloading the Latin-only version (the shorter one) now, same as before, and this remains unchanged in this PR

Exploratory work: This is mostly complete. I’ve reviewed all templates that use the @import tag.

  • On rocketlabsqa.ovh, everything is working as expected.
  • On e2e.rocketlabs, I’m seeing different results — we’ll need to investigate further.

The only template that fails on both environments is this one: https://rocketlabsqa.ovh/preloadfonts_10atf_10btf_gfonly_externalcsswithimport/ Template: preloadfonts_10atf_btf_gfonly_externalcssWithimport.php

  • This template uses a CSS file loaded via < link >, which in turn loads Google Fonts via @import.

I don’t believe this issue is related to the changes in this PR, though, as other templates with a similar structure (CSS file loading Google Fonts via @import) are working as expected. For example https://rocketlabsqa.ovh/preloadfonts_import2-2 Template: preloadfonts_import2.php @DahmaniAdame any idea what might be off with this specific page template? Would appreciate your thoughts.

hanna-meda avatar Jul 08 '25 22:07 hanna-meda