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

Sharing: switch to only a base64-encoded icon font #29441

Merged
merged 5 commits into from
Mar 14, 2023
Merged

Sharing: switch to only a base64-encoded icon font #29441

merged 5 commits into from
Mar 14, 2023

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Mar 13, 2023

Proposed changes:

Follow-up to #28849

In #28849, we moved away from using a base64-encoded font and an eot file for multiple reasons:

Instead, we switched to loading 3 files from local paths:

Given how widespread woff2 support looks like today, we may be able to use just that. We will, however, need to revert to a base64-encoded font to avoid CORS issues when the files are served from a CDN, like Photon CDN:

Access to font at 'https://c0.wp.com/p/jetpack/11.9/_inc/social-logos/social-logos.woff2' from origin 'https://yoursite.com' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource.

Both approaches seem to be problematic:

  • base64-encoded fonts are broken for some people.
  • fonts loaded via a path have CORS issues for everyone using a CDN. => This seems like the biggest problem for us right now, given that Jetpack includes such a CDN.

This commit is consequently not a perfect solution. Switching to SVGs, as discussed in #28861, seems to be the path forward.

Note
This PR edits a file that was previously auto-generated in https://github.com/Automattic/social-logos . I wouldn't normally want to manually edit auto-generated files, but the social-logos build tools are not working anymore (see https://github.com/Automattic/social-logos/issues/116). As a result, I think it would be fine to apply this temporary fix here until we get rid of the icon font entirely.

Other information:

  • Have you written new tests for your changes, if applicable?
  • Have you checked the E2E test CI results, and verified that your changes do not break them?
  • Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

  • p1678548282092199-slack-CBG1CP4EN

Does this pull request change what data or activity we track or use?

  • No

Testing instructions:

Test with different browsers

  • Start with a site connected to WordPress.com, and running the 11.9 stable release of Jetpack.
  • Go to Jetpack > Settings > Performance, and enable the site accelerator, specifically the static file CDN (second option).
  • Go to Jetpack > Settings > Sharing and enable Sharing buttons
  • Configure sharing buttons: add a Mastodon button to the list.
  • View a post on the frontend; the Mastodon icon should be broken.
  • You should also see the CORS error mentioned above in your browser console.
  • Open your browser dev tools, and in the sources tab, find the social-logos.min.css file under c0.wp.com > p > jetpack > 11.9 > _inc > social-logos
  • Edit that file, paste in the modified version from this PR.
  • The Mastodon icon should now look good.

@jeherve
Sharing: switch to only a base64-encoded icon font
fce1575 
Follow-up to #28849

In #28849, we moved away from using a base64-encoded font and an eot file for multiple reasons:

- #6076
- #7148
- #28694 (comment)

Instead, we switched to loading 3 files from local paths:

- woff2, supported by all modern browsers: https://caniuse.com/woff2
- woff and ttf, around for compatibility with old browsers: https://css-tricks.com/snippets/css/using-font-face-in-css/

Given how widespread woff2 support looks like today, we may be able to use just that. We will, however, need to revert to a base64-encoded font to avoid CORS issues when the files are served from a CDN, like Photon CDN:

Access to font at 'https://c0.wp.com/p/jetpack/11.9/_inc/social-logos/social-logos.woff2' from origin 'https://yoursite.com' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource.

Both approaches seem to be problematic:

- base64-encoded fonts are broken for some people.
- fonts loaded via a path have CORS issues for everyone using a CDN. => This seems like the biggest problem for us right now, given that Jetpack includes such a CDN.

This commit is consequently not a perfect solution. Switching to SVGs, as discussed in #28861, seems to be the path forward.
@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Sharing Post sharing, sharing buttons [Status] In Progress [Pri] High labels Mar 13, 2023
@jeherve jeherve self-assigned this Mar 13, 2023
@github-actions github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Mar 13, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 13, 2023 •
edited
Loading

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

  • Next scheduled release: April 4, 2023.
  • Scheduled code freeze: March 28, 2023.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 13, 2023 •
edited
Loading

Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run

bin/jetpack-downloader test jetpack fix/woff2-cdns

to get started. More details: p9dueE-5Nn-p2

@jeherve jeherve added the [Status] Needs Testing We need to add this change to the testing call for this month's release label Mar 13, 2023
@jeherve
Copy link
Member Author

jeherve commented Mar 13, 2023

@kraftbj Since you were running into those font decoding issues on your site before (see #28694 (comment)), I was wondering if you could give this a try, see how things look like on your end?

Thank you!

@jeherve jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Mar 14, 2023
@sdixon194 sdixon194 added this to the jetpack/11.9.1 milestone Mar 14, 2023
sdixon194
sdixon194 approved these changes Mar 14, 2023
View reviewed changes
Copy link
Contributor

@sdixon194 sdixon194 left a comment

Choose a reason for hiding this comment

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

Does the job for me

@sdixon194 sdixon194 merged commit dccd806 into trunk Mar 14, 2023
@sdixon194 sdixon194 deleted the fix/woff2-cdns branch March 14, 2023 14:35
sdixon194 pushed a commit that referenced this pull request Mar 14, 2023
@jeherve @sdixon194
[not verified] Sharing: switch to only a base64-encoded icon font (#2…
2cb11e2 
…9441)
@sdixon194
Copy link
Contributor

Cherry picked to release branch in 2cb11e2

@github-actions github-actions bot removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Mar 14, 2023
@jeherve
Copy link
Member Author

jeherve commented Mar 14, 2023

Thank you!

@jeherve jeherve mentioned this pull request Mar 14, 2023
Merged
@anomiex anomiex removed the [Status] Needs Testing We need to add this change to the testing call for this month's release label May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdixon194 sdixon194 sdixon194 approved these changes

Assignees

@jeherve jeherve

Labels
[Feature] Sharing Post sharing, sharing buttons [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Pri] High [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
jetpack/11.9.1
Development

Successfully merging this pull request may close these issues.
3 participants
@jeherve @sdixon194 @anomiex