Add inline styles for custom fonts#3345
Merged
swissspidy merged 4 commits intodevelopfrom Sep 26, 2019
Merged
Conversation
Fixes issue for fonts not originating from Google Fonts
westonruter
reviewed
Sep 25, 2019
Contributor
|
Seems to work as expected. It's marked WIP right now -- what's still todo -- was it this? |
Collaborator
Author
|
According to Weston's comment above the Also, this method doesn't seem to have any tests at all, which is a bit worrisome. |
Member
|
This is working for me. Given this story page, with three system fonts being used (including 3 Text blocks with the same font):
Before
<!--
The style[amp-custom] element is populated with:
0 B: link#wp-block-library-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/gutenberg/build/block-library/style.css?ver=1569279408][media=all]
160 B (19%): link#amp-stories-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/amp/assets/css/amp-stories.css?ver=1.3.1-alpha][media=all]
289 B (22%): link#amp-stories-frontend-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/amp/assets/css/amp-stories-frontend.css?ver=1.3.1-alpha][media=]
84 B: amp-story-page#11fc4530-aa8d-4ad2-a037-65e0269c4bdf.wp-block-amp-amp-story-page amp-wp-74885c3
123 B: div.amp-story-block-wrapper amp-wp-7b64e67
123 B: div.amp-story-block-wrapper amp-wp-5a4df0a
128 B: div.amp-story-block-wrapper amp-wp-cd08bbf
128 B: div.amp-story-block-wrapper amp-wp-5d86f2f
123 B: div.amp-story-block-wrapper amp-wp-97fdaf8
75 B: h1.wp-block-amp-amp-story-text amp-wp-f7dbba7[data-font-family=Arial Narrow]
Total included size: 1,233 bytes (8% of 15,402 total after tree shaking)
-->After
<!--
The style[amp-custom] element is populated with:
0 B: link#wp-block-library-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/gutenberg/build/block-library/style.css?ver=1569279408][media=all]
160 B (19%): link#amp-stories-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/amp/assets/css/amp-stories.css?ver=1.3.1-alpha][media=all]
289 B (22%): link#amp-stories-frontend-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/amp/assets/css/amp-stories-frontend.css?ver=1.3.1-alpha][media=]
89 B: style[data-font-family=Arial]
108 B: style[data-font-family=Arial Black]
82 B: style[data-font-family=Arial Narrow]
84 B: amp-story-page#11fc4530-aa8d-4ad2-a037-65e0269c4bdf.wp-block-amp-amp-story-page amp-wp-74885c3
123 B: div.amp-story-block-wrapper amp-wp-7b64e67
123 B: div.amp-story-block-wrapper amp-wp-5a4df0a
128 B: div.amp-story-block-wrapper amp-wp-cd08bbf
128 B: div.amp-story-block-wrapper amp-wp-5d86f2f
123 B: div.amp-story-block-wrapper amp-wp-97fdaf8
75 B: h1.wp-block-amp-amp-story-text amp-wp-f7dbba7[data-font-family=Arial Narrow]
Total included size: 1,512 bytes (9% of 15,681 total after tree shaking)
-->Note that even though there are 5 Text blocks being used, with 3 of them using the same font (Arial Black), the manifest shows that only one copy is actually included after the style sanitizer processes the stylesheets. This is confirmed by looking at the CSS, where there are only 3 rules with /* ... */
[data-font-family="Arial"] {
font-family: "Arial", "Helvetica Neue", "Helvetica", "sans-serif"
}
[data-font-family="Arial Black"] {
font-family: "Arial Black", "Arial Black", "Arial Bold", "Gadget", "sans-serif"
}
[data-font-family="Arial Narrow"] {
font-family: "Arial Narrow", "Arial", "sans-serif"
}
/* ... */ |
Member
|
I realized this could be simplified a bit more and harmonize style rule creation for system vs Google Fonts, and as a bonus it ensures that if by chance someone enqueues a Google Font early, it won't fail to get the inline style. In d29bfe0, a page with 4 fonts being used (3 system and 1 Google Font):
<!--
The style[amp-custom] element is populated with:
0 B: link#wp-block-library-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/gutenberg/build/block-library/style.css?ver=1569279408][media=all]
160 B (19%): link#amp-stories-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/amp/assets/css/amp-stories.css?ver=1.3.1-alpha][media=all]
289 B (22%): link#amp-stories-frontend-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/amp/assets/css/amp-stories-frontend.css?ver=1.3.1-alpha][media=]
89 B: style[data-font-family=Arial]
108 B: style[data-font-family=Arial Black]
82 B: style[data-font-family=Arial Narrow]
65 B: style[data-font-family=Aref Ruqaa]
84 B: amp-story-page#11fc4530-aa8d-4ad2-a037-65e0269c4bdf.wp-block-amp-amp-story-page amp-wp-74885c3
123 B: div.amp-story-block-wrapper amp-wp-7b64e67
123 B: div.amp-story-block-wrapper amp-wp-5a4df0a
128 B: div.amp-story-block-wrapper amp-wp-cd08bbf
128 B: div.amp-story-block-wrapper amp-wp-5d86f2f
123 B: div.amp-story-block-wrapper amp-wp-97fdaf8
126 B: div.amp-story-block-wrapper amp-wp-1a8febe
75 B: h1.wp-block-amp-amp-story-text amp-wp-f7dbba7[data-font-family=Aref Ruqaa]
Total included size: 1,703 bytes (10% of 15,872 total after tree shaking)
--><style amp-custom="">
/*...*/
[data-font-family="Arial"]{font-family:"Arial","Helvetica Neue","Helvetica","sans-serif"}
[data-font-family="Arial Black"]{font-family:"Arial Black","Arial Black","Arial Bold","Gadget","sans-serif"}
[data-font-family="Arial Narrow"]{font-family:"Arial Narrow","Arial","sans-serif"}
[data-font-family="Aref Ruqaa"]{font-family:"Aref Ruqaa","serif"}
/*...*/
</style>
<!-- ... -->
<link crossorigin="anonymous" rel="stylesheet" id="aref-ruqaa-font-css" href="https://fonts.googleapis.com/css?family=Aref%20Ruqaa%3A400%2C700&subset=latin%2Clatin-ext" media="all"> |
Member
Added some tests in 2d089c4. |
westonruter
approved these changes
Sep 25, 2019
Collaborator
Author
|
Thanks a ton @westonruter! Works like a charm now 🎉 |
swissspidy
added a commit
that referenced
this pull request
Sep 26, 2019
* Fixes issue for fonts not originating from Google Fonts * Simplifies font style rule generation * Adds test for `AMP_Story_Post_Type::render_block_with_google_fonts`
westonruter
added a commit
that referenced
this pull request
Oct 1, 2019
* tag '1.3.0': (318 commits) Bump 1.3.0 Add inline styles for custom fonts (#3345) Limit deeply-nesting test to 200 to fix Xdebug error (#3341) Bump 1.3-RC2 (#3335) Sanitize invalid children of amp-story and amp-story-page elements to prevent white story of death (#3336) Remove unused Travis deploy stage (#3340) Implement automated accessibility testing using Axe (#3294) Only add all Google Font style rules in editor context Prevent adding AMP query var to Story URLs in Compatibility Tool Prevent attempting to redirect Stories with rejected validation errors Ensure all AMP scripts (including v0.js) get moved to the head Make sure that media picker is background types are filter correctly. Normalize style[type] attribute quote style after r46164 in WP core Fix phpunit covers tags Bump version to 1.3-RC1 Strip 100% width/height from layout=fill elements Fix issue with cut (#3246) Remove unused Google Fonts SVGs (#3289) Fix resize for non-fit text box (#3259) Use template_dir consistently as signal for transitional mode ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes issue for fonts not originating from Google Fonts.
Fixes #3344.