search

WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.48k stars 4.18k forks source link

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

Open peterwilsoncc opened 8 months ago

peterwilsoncc commented 8 months ago

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

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

mikachan commented 7 months ago

I can't replicate this on Gutenberg trunk, although I believe it should have been fixed by https://github.com/WordPress/gutenberg/pull/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 commented 7 months ago

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 commented 7 months ago

I can't replicate this on Gutenberg trunk, although I believe it should have been fixed by https://github.com/WordPress/gutenberg/pull/59451

I think this was fixed by https://github.com/WordPress/gutenberg/pull/59436

mikachan commented 7 months ago

I think this was fixed by https://github.com/WordPress/gutenberg/pull/59436

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

annezazu commented 7 months ago

Awesome. Removing this from 6.5 :)

peterwilsoncc commented 7 months ago

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 commented 7 months ago

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 https://github.com/WordPress/gutenberg/pull/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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

The video from the issue's description is no longer accurate regarding the behavior of the font library after https://github.com/WordPress/gutenberg/pull/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 https://github.com/WordPress/gutenberg/pull/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 https://github.com/WordPress/gutenberg/pull/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:

https://github.com/WordPress/gutenberg/assets/1310626/5df1878b-01a9-49a4-bc70-e49dc4d69bcd

The example from this comment https://github.com/WordPress/gutenberg/issues/59486#issuecomment-1977698462 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 commented 7 months ago

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:

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 commented 7 months ago

@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 commented 7 months ago

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:

Argument against this being a bug

Argument in support of this being a valid bug

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 commented 7 months ago

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 commented 7 months ago

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:

peterwilsoncc commented 7 months ago

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.