Font Library: Load/Unload the font face in browser when toggling the variants #59066
Conversation
This file contains 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
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
arthur791004
requested review from
okmttdhr,
matiasbenedetto and
creativecoder
arthur791004
added
[Type] Bug
|
Size Change: +128 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d12325c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7914302243
|
okmttdhr
approved these changes
Feb 16, 2024
There was a problem hiding this comment.
✅ Confirmed;
- the font-weight gets reflected instantly after installing and applying it
- the font-weight gets reflected after deleting and refreshing the page (not instantly)
Am I correct in understanding that addressing the latter issue would necessitate a fundamental change to the font installation process in the Font Library?
I made a change, 872c35f, to unload the deleted font faces as well so it should get reflected instantly 🙂 |
There was a problem hiding this comment.
Apart from a few inline minor comments, I'm wondering about these questions: Is this PR still relevant? Could you please define more clearly the problem it is fixing? I tested how it behaves in trunk vs this branch and couldn't find differences. It could be useful to post screencasts featuring the behavior of this fix and the behavior of trunk.
Comment on lines
+128
to
+129
| * Note that Font faces that were added to the set using the CSS @font-face rule | ||
| * remain connected to the corresponding CSS, and cannot be deleted. |
There was a problem hiding this comment.
I'm not sure what is the meaning of this comment
There was a problem hiding this comment.
It's from MDN. The fonts can be added to the FontFace set as follows, and the first method cannot be removed from the FontFace set.
- The CSS rule
- The
document.fonts.add
| * Note that Font faces that were added to the set using the CSS @font-face rule | ||
| * remain connected to the corresponding CSS, and cannot be deleted. | ||
| */ | ||
| export function unloadFontFaceInBrowser( fontFace, addTo = 'all' ) { |
There was a problem hiding this comment.
if we are removing something using the parameter name addTo seems contradictory with the functionality. Should it be removeFrom instead?
As per my understanding, this PR addresses the second Observed Issue outlined in #58764, where changes in font-weight aren't immediately visible on the canvas after installing or removing Google Fonts. Upon testing with the latest version of trunk, I observed that while the issue seems to be resolved when a font is installed (probably in #58765), the problem persists upon removing the font. trunk Screen.Recording.2024-02-27.at.10.40.51.movthis PR Screen.Recording.2024-02-27.at.11.00.08.mov |
|
@okmttdhr. Thanks for the additional details. Is the video posted in #59066 (comment) from trunk or this branch? Could you record the same example using the one to compare? |
I updated the comment to include this PR's change :) |
arthur791004
force-pushed
the
fix/font-library-load-font-face-on-demand
branch
from
4274256
to
581d9ee
Compare
matiasbenedetto
approved these changes
Mar 4, 2024
There was a problem hiding this comment.
This seems ready to go now.
I just made 3 small changes:
- I replace a a function that was implemented again in this PR by a call to the existing function: a8b8938
- Removed an inaccurate comment: 3e4f064
- Added a call to formatFontFaceName to make these changes work in Firefox too: ec32719
It looks good to me.
Thanks for working on this.
matiasbenedetto
added
the
Backport to WP 6.7 Beta/RC
|
This PR is slated for inclusion in WP 6.5 during the RC phase as it fixes a important bug in the Fonts Library feature. Specifically this fixes the editor so that when a Font variant is activated/deactivated the editor will automatically reflect that change without requiring a reload. This is critical to the feature as without it users could inadvertently have the wrong variants active/inactive and not notice the change until they reload the editor. @matiasbenedetto or @arthur791004 May also be able to provide additional context. |
getdave
pushed a commit
that referenced
this pull request
Mar 11, 2024
…variants (#59066) * Font Library: Load/Unload the font face in browser when toggling the variants * Font Library: Unload the deleted font faces * Address feedback * replace repeated implementation by the use of a existing fuction * remove inaccurate comment * added a formatFontFace call to be able to match fonts in firefox too. --------- Co-authored-by: Matias Benedetto <[email protected]> --- Co-authored-by: arthur791004 <[email protected]> Co-authored-by: matiasbenedetto <[email protected]> Co-authored-by: okmttdhr <[email protected]>
|
I just cherry-picked this PR to the pick/wp-65-rc-2 branch to get it included in the next release: 59e7d64 |
getdave
added
Backported to WP Core
|
Thank you 👍 |
getdave
pushed a commit
that referenced
this pull request
Mar 12, 2024
…variants (#59066) * Font Library: Load/Unload the font face in browser when toggling the variants * Font Library: Unload the deleted font faces * Address feedback * replace repeated implementation by the use of a existing fuction * remove inaccurate comment * added a formatFontFace call to be able to match fonts in firefox too. --------- Co-authored-by: Matias Benedetto <[email protected]> --- Co-authored-by: arthur791004 <[email protected]> Co-authored-by: matiasbenedetto <[email protected]> Co-authored-by: okmttdhr <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
Load/Unload the font face in browser when toggling the variants.
Why?
Referring to “Changes in font-weight are not immediately reflected on the canvas, requiring a page reload for updates to be visible.”, we have to load/unload the specified font face in the browser when the user is managing the fonts.
How?
Call the
loadFontFaceInBrowserwhen the user is enabling a font-variant. When the user is disabling a font-variant, call theunloadFontFaceInBrowserinstead.Note that Font faces that were added to the set using the CSS font-face rule remain connected to the corresponding CSS, and cannot be deleted.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
load-font-variants-on-demand.mov