Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Font Library: Font Family post type retained when unable to install any font faces #59486

Open
peterwilsoncc opened this issue Mar 1, 2024 · 16 comments
Labels
[Feature] Font Library [Type] Bug An existing feature does not function as intended

Comments

@peterwilsoncc
Copy link
Contributor

Description

When attempting to upload or install fonts via the Font Manager, the font family post type is retained even if there are no successful font faces uploaded.

This results in a Font Family listing with no variants. However, once can open the font family and by default the normal font weight will be displayed allowing the user to activate it and select it in the side bar.

If no fonts are successfully uploaded, the font-family post ought to be removed.

Step-by-step reproduction instructions

  1. Modify WP_REST_Font_Faces_Controller::handle_font_file_upload() to simply return a WP_Error object
    protected function handle_font_file_upload( $file ) {
    return new WP_Error( 'rest_font_error', 'No uploads' );
    }
  2. Open the site editor > Styles > Typography > Typography Manager > Install
  3. Chose a font to install, click the install button
  4. A fetch error will be displayed
  5. Return to the Installed fonts tab
  6. The font family is included showing no variants installed
  7. Clicking on the font family invites the user to activate the normal weight font
  8. Returning to the sidebar the user can enable the font on various elements

Screenshots, screen recording, code snippet

no-fonts

Environment info

  • WordPress 6.4
  • Gutenberg trunk 9957b85

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@peterwilsoncc peterwilsoncc added [Type] Bug An existing feature does not function as intended [Feature] Font Library labels Mar 1, 2024
@annezazu annezazu changed the title Font Family post type retained when unable to isntall any font faces. Shows normal weight as available. Font Library: Font Family post type retained when unable to install any font faces Mar 4, 2024
@mikachan
Copy link
Member

mikachan commented Mar 4, 2024

I can't replicate this on Gutenberg trunk, although I believe it should have been fixed by #59451, as that PR checks if a new font family has been added and will uninstall/remove it if no fonts are successfully uploaded for the font family.

Can you see this issue with Gutenberg trunk?

@getdave
Copy link
Contributor

getdave commented Mar 4, 2024

although I believe it should have been fixed by #59451,

...worth noting that this will be synced to WP Core as part of WP 6.5 RC 1 which is scheduled for tomorrow.

@matiasbenedetto
Copy link
Contributor

I can't replicate this on Gutenberg trunk, although I believe it should have been fixed by #59451

I think this was fixed by #59436

@mikachan
Copy link
Member

mikachan commented Mar 4, 2024

I think this was fixed by #59436

It looks like that's also going to be included with WP 6.5 RC1.

@annezazu
Copy link
Contributor

annezazu commented Mar 4, 2024

Awesome. Removing this from 6.5 :)

@peterwilsoncc
Copy link
Contributor Author

Verified: if all fonts fail to upload the font family is not available.


However, I'm still seeing the Normal font as available if there are childless font family posts in the database

  1. Install Open Sans via google
  2. Run the wp cli command to delete font faces: wp post delete $(wp post list --post_type='wp_font_face' --format=ids) --force
  3. Refresh site editor, go to typeography manager
  4. Able to enable Open Sans Normal and use it in the site editor

Screen shot, note that the system font is underlined in Firefox dev tools indicating the active font.

Screen Shot 2024-03-05 at 10 57 26 am

Should I log that as a separate issue or do you want to work on a fix off this one?

@matiasbenedetto
Copy link
Contributor

However, I'm still seeing the Normal font as available if there are childless font family posts in the database
Install Open Sans via google
Run the wp cli command to delete font faces: wp post delete $(wp post list --post_type='wp_font_face' --format=ids) --force
Refresh site editor, go to typeography manager
Able to enable Open Sans Normal and use it in the site editor

With #59436 we are ensuring that users using the font library UI don't end up with font families with no font faces when that was not the intention. Using a low-level method, as the one in your example, we can not assume the user's intention, and I don't think we should remove the font family parent of the deleted font faces. If the user is using those low-level commands to manipulate the fonts installed, it's better to let them remove the font family manually using a similar command.

@peterwilsoncc
Copy link
Contributor Author

With #59436 we are ensuring that users using the font library UI don't end up with font families with no font faces when that was not the intention.

This is what I am saying, the font family still appears in the UI if there are no child font faces. This is why the dev tools are showing the default sans-serif font is in use in the screen shot

The use of a WP CLI command in the reproduction steps is a developer convenience for the purpose of reproduction. As the font-face REST API endpoint doesn't remove childless font-families, the same can occur removing font-faces using the recommended technique.

@pbking
Copy link
Contributor

pbking commented Mar 6, 2024

A font family without a font face is still a legit font family, it would just be a system font. The one in question isn't helpful, sure, but not having font faces associated with it wouldn't be a reason to exclude it from API responses or to delete it.

It's tricky and the resolution is unclear.

@matiasbenedetto
Copy link
Contributor

matiasbenedetto commented Mar 6, 2024

The video from the issue's description is no longer accurate regarding the behavior of the font library after #59436 was merged. However, if you installed a font family and all the font faces failed to install, it will still be listed in the font library if it was installed in before #59436 was merged. This is because a font family without font faces installed (system fonts) are valid font families, so they should be listed.

After #59436 was merged is all the font faces failed to be installed, the font family will be deleted right after it was created, and the font faces creation failed.

Here's a screencast following the steps detailed in this issue's description, where you can see that:

2024-03-06.10-49-58.mp4

The example from this comment #59486 (comment) doesn't seem to be valid because the low-level data manipulation where the extender's intention should not be assumed, so the font family should not be removed and it should be listed because font families without font faces associated are valid fonts (system font stacks).

In my opinion, this issue should be closed as already fixed.

@peterwilsoncc
Copy link
Contributor Author

As I said, the low level manipulation was a developer convenience for reproducing. It allows me to avoid instructions requiring an Application password, etc.

When deleting font-faces, it's possible to determine whether the font-familiy is a web-font or a local font from two sources of data:

  • The _wp_font_face_file post meta
  • The src attribute of the post content

If these items on the font-face are present then keeping around the font-family is to show fonts in the typography sidebar that are unavailable to the user. Instead of getting the expected font they get the system default font. If the library is showing a preview of the correctly formated font via the SVG then it's more confusing for the user.

@matiasbenedetto
Copy link
Contributor

@peterwilsoncc I'm not sure what the issue is currently.The issue's original video is outdated. Could you please add a new screencast featuring what you consider the issue to be?

@getdave
Copy link
Contributor

getdave commented Mar 7, 2024

I've read back through this discussion to try and get a handle on what the point of concern is. My understanding is as follows:

  • There was previously an issue whereby Font Families would still be listed in the library even if Font Faces were not installed (e.g. error occurred during install).
  • This issue was resolved in Avoid creating font families without font faces. #59436 by ensuring that if the installation of font faces fails, then the font family is removed from the database.
  • However, if a Font Family post exists in the database it will still be shown in the Font Library even if there are no Font Faces associated with it.
  • There is disagreement as to whether this should be classed as a problem.

Argument against this being a bug

  • it is no longer possible to end up with "empty" Font Families using the Font Library UI within the editor.
  • removing Font Face posts using CLI, REST (.etc) without using the UI means that we cannot guarantee user intent.
  • removing posts manually in this way is not typically not something a user of WordPress would do.
  • therefore if someone chooses to manipulate posts in this fashion we cannot guarantee what they are intending to do.
  • as a result we shouldn't make presumptions about how the UI of the Font Library should respond.
  • there is also an argument that a font family without a font face is still a legitimate font family, it would just be a system font.

Argument in support of this being a valid bug

  • agree that it is no longer possible to end up with "empty" Font Famioly posts using the Font Library UI
  • argument is that methods still exist (REST API, CLI...etc) within the WP software which allow you to end up with Font Family posts which have no Font Face Posts associated with them.
  • if it's possible to use the software to end up with empty Font Families then the UI must account for this but hiding these.
  • having Font Family posts listed which don't have Font Faces could be very confusing for users.

I'm going to continue investigating this. In the meantime I would welcome input from Editor release leads as to whether they would consider this to be a bug. cc @fabiankaegy @annezazu @youknowriad.

@peterwilsoncc
Copy link
Contributor Author

Coming back to this, I noticed the font families are listed in the typography sidebar even when the font-family post type is removed:

  1. Upload a couple of different font types
  2. Don't apply the fonts to any blocks, styles or whatever. The fonts exist as post types bit are not used by the global styles
  3. Run wp post delete $(wp post list --post_type='wp_font_face,wp_font_family' --format=ids) --force to delete all font face and font family post types
  4. Reload the site editor
  5. Open styles > typography

Screen Shot 2024-03-15 at 10 48 51 am

@peterwilsoncc
Copy link
Contributor Author

peterwilsoncc commented Mar 14, 2024

Long form UI steps

  1. Activate 2024
  2. Upload a couple of fonts (Open Sans, Nato in screen shot) via modal
  3. Save theme
  4. Switch to 2023
  5. Delete the fonts completely via modal
  6. Save theme
  7. Switch back to 2024
  8. Go to Styles > Typography
  9. Unavailable fonts are listed in sidebar

This should apply to the earlier notes too.

I think what will probably need to happen is this:

  • Ensure that font families listed in global styles have related font-family and font face post types
  • Same before displaying fonts in the typography sidebar, block styles and elsewhere
  • If a site owner applies a non-theme provided font to a block/style feature the font-family should be declared as: font-family: "uploaded font", "theme default font", system-font; -- I think this can done in the CSS variable

@peterwilsoncc
Copy link
Contributor Author

I've opened #59974 for above comments as it seems to be an interaction between global styles and the font library.

I still think there's room for improvement on this ticket though and safe to conclude the font-family post type can be removed once all the child font-face post types are removed.

I tested using System fonts with the font library using matiasbenedetto/modern-fonts-stacks-for-wp-font-library and system fonts only create a font-family post type, not any font-face post types.

If the font face exists and the _wp_font_face_file post meta exists, then I think it's safe & best advised to remove the font family.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Font Library [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

6 participants