Directory

Font Library: fix duplicate variants with different file types by jffng · Pull Request #54490 · WordPress/gutenberg · GitHub
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.

Sign up for GitHub

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

Jump to bottom

Font Library: fix duplicate variants with different file types #54490

Merged
merged 10 commits into from
Sep 19, 2023
Merged

Font Library: fix duplicate variants with different file types #54490

merged 10 commits into from
Sep 19, 2023

Conversation

jffng
Copy link
Contributor

@jffng jffng commented Sep 15, 2023

What?

This PR adds checks to ensure that multiple file types of the same font variant are not added, so that only one variation per font style and weight is stored.

Why?

Fixes #54253

How?

For fonts where the font family does not yet exist, checks the list of new font faces and compares their font weight and style, and if there is already an existing font face, skips adding the incoming one.

For font faces where the font family exists, adds a new function get_intersecting_font_faces which returns an array of font faces to be deleted and replaced by the incoming font face.

Testing Instructions

  1. Simultaneously (batch) upload two or more font faces of the same family, style, and weight, but different file types.
  2. Ensure only the first variant is added / used.
  3. Upload a font face to an existing font family with the same style and weight, but different file type.
  4. Ensure the uploaded version is used / replaces the existing one, and an additional variant is not created.

Testing Instructions for Keyboard

Screenshots or screencast

@jffng jffng added [Type] Bug An existing feature does not function as intended Needs PHP backport Needs PHP backport to Core [Feature] Typography Font and typography-related issues and PRs labels Sep 15, 2023
@jffng jffng mentioned this pull request Sep 15, 2023
Closed
@github-actions
Copy link

github-actions bot commented Sep 15, 2023 •
edited

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/experimental/fonts/font-library/class-wp-font-family.php
❔ phpunit/tests/fonts/font-library/wpFontFamily/install.php

@jffng jffng marked this pull request as ready for review September 15, 2023 07:32
@github-actions
Copy link

github-actions bot commented Sep 15, 2023 •
edited

Flaky tests detected in b953b2f.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6226554866
📝 Reported issues:

pbking
pbking approved these changes Sep 15, 2023
View reviewed changes
Copy link
Contributor

@pbking pbking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working as expected for me. Fonts are limited to a single format. When adding a new it seems to always replace the existing, whatever format it is.

When uploading multiple at the same time the format chosen seems to be... alphabetical?

Either way, I can confirm that it fixes the issue at hand.

@matiasbenedetto
Copy link
Contributor

The tests I made manually are working great.
Currently, we are testing the case when two font faces have the same characteristics but different font files in the same request.
Could we add one more unit test case to cover the addition of font faces with the same characteristics in different requests? That's covered in the implementation but not in the unit tests.

@jffng
Copy link
Contributor Author

jffng commented Sep 18, 2023

@matiasbenedetto I moved these tests to the Tests_Fonts_WpFontFamily_Install class, added a case to cover installing multiple font faces that contain duplicates, and one test to cover the addition of font faces with the same characteristics in different requests.

jeffikus
jeffikus approved these changes Sep 19, 2023
View reviewed changes
Copy link

@jeffikus jeffikus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - I tested with with Cardo font.

  • Regular
  • Italic
  • Bold

Used ttf, woff, woff2 file types for each.

This created 3 variants within Cardo, tried uploading 9 files total.

@jffng jffng merged commit 40436a4 into trunk Sep 19, 2023
49 checks passed
@jffng jffng deleted the fix/fonts-library-duplicate-variants branch September 19, 2023 18:57
@github-actions github-actions bot added this to the Gutenberg 16.7 milestone Sep 19, 2023
@ramonjd ramonjd mentioned this pull request Sep 20, 2023
Merged
@SiobhyB SiobhyB mentioned this pull request Sep 25, 2023
Closed
86 tasks
@youknowriad youknowriad added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Needs PHP backport Needs PHP backport to Core labels Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matiasbenedetto matiasbenedetto matiasbenedetto left review comments

@pbking pbking pbking approved these changes

@jeffikus jeffikus jeffikus approved these changes

@spacedmonkey spacedmonkey Awaiting requested review from spacedmonkey spacedmonkey is a code owner

Assignees
No one assigned
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Feature] Typography Font and typography-related issues and PRs [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
Gutenberg 16.7
Development

Successfully merging this pull request may close these issues.

Font Library: Same font faces with different file extensions are added as a separated variant.
5 participants
@jffng @matiasbenedetto @pbking @jeffikus @youknowriad