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

Fix site title block block style missing #26193

Merged
merged 4 commits into from
Sep 20, 2022

Conversation

fullofcaffeine
Copy link
Contributor

@fullofcaffeine fullofcaffeine commented Sep 13, 2022

Fixes the issue reported here: p9F6qB-afB-p2
Related Issue: #26224

Discussion: p1662734279580389-slack-C01DF571R5Y

Changes proposed in this Pull Request:

Set the Automattic\Jetpack\Fonts\Introspectors\Global_Styles::enqueue_global_styles_fonts callback priority in the init hook to 22 to prevent it from causing style issues with sites that have Gutenberg > 13.5 activated.

The default priority meant that this callback ran before this one in Gutenberg, and for some reason (probably CSS specificity?) it was affecting the styles/fonts set by the Gutenberg callback. See the details in the linked P2 page above.

This seems to happen only in org sites connected to JP and with the JP plugin installed. This includes WoA. Simple sites are not affected.

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?

Jetpack product discussion

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

Testing instructions:

AT
  1. Install the TT2 theme, make the site public and view the main page of the site in the frontend;
  2. Install Gutenberg 14.0.3
  3. Verify that the issue in p9F6qB-afB-p2 happens;
  4. Install the Jetpack beta tester plugin and install the plugin from this branch;
  5. Verify that the issue in p9F6qB-afB-p2 doesn't happen and that the title looks as expected;
vanilla JP (jurassic.ninja)

This bug doesn't manifest in plain JP+org sites, but we should still sanity-check the changes:

  1. Install the TT2 theme, make the site public and view the main page of the site in the frontend;
  2. Install Gutenberg 14.0.3
  3. Install the Jetpack beta tester plugin and install the plugin from this branch;
  4. Verify that the issue in p9F6qB-afB-p2 doesn't happen and that the title looks as expected;
Simple sites

This bug doesn't manifest in simple sites, but we should still sanity-check the changes:

  1. Install the TT2 theme, make the site public and view the main page of the site in the frontend;
  2. In you sandbox, run bin/jetpack-downloader test jetpack fix/site-title-block-block-style-missing to install the JP plugin from this branch;
  3. Verify that the issue in p9F6qB-afB-p2 doesn't happen and that the title looks as expected;

@github-actions
Copy link
Contributor

github-actions bot commented Sep 13, 2022

Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run bin/jetpack-downloader test jetpack fix/site-title-block-block-style-missing to get started. More details: p9dueE-5Nn-p2

@github-actions github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Sep 13, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 13, 2022

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: October 4, 2022.
  • Scheduled code freeze: September 27, 2022.

@github-actions github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Sep 13, 2022
@fullofcaffeine
Copy link
Contributor Author

fullofcaffeine commented Sep 13, 2022

I couldn't reproduce the issue in an org+JP site either (using jurassic.ninja). I think it has to do with the fact enqueue_global_styles_fonts didn't specify a priority, maybe? I'm not sure; and I don't think it's worth spending more time as the fix seems to work fine on AT and doesn't seem to affect simple and JP+org sites.

@fullofcaffeine fullofcaffeine self-assigned this Sep 13, 2022
@fullofcaffeine fullofcaffeine added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Sep 13, 2022
@jeyip
Copy link
Contributor

jeyip commented Sep 15, 2022

Testing

Requirements

Atomic

  • Site title font styles are not stripped in frontend
  • Frontend renders as expected
  • Custom google fonts load as expected in editor + frontend

Self-hosted

  • Frontend renders as expected
  • Custom google fonts load as expected in editor + frontend

Simple

  • Frontend renders as expected
  • Custom google fonts load as expected in editor + frontend

Browsers

  • Chrome

Note for other testers -- when testing google fonts on simple sites with this PR's changes, it takes a few moments for the cached fonts to invalidate. After a bit, the proper font should be displayed

@jeyip
Copy link
Contributor

jeyip commented Sep 15, 2022

I don't see any problems with updating the priority. From what I can tell, the frontend for all environments continues to load google fonts properly. 🚢

Thanks for drafting this 🙏

jeyip
jeyip previously approved these changes Sep 15, 2022
@anomiex
Copy link
Contributor

anomiex commented Sep 15, 2022

Fixes the issue reported here: p9F6qB-afB-p2

This works around the problem for sites where Jetpack is the trigger for the bug, but the underlying cause seems to be something in Gutenberg 13.5.0+. Well, maybe. It may be that the resolution there should be to document that things calling the things we're calling should run at priority 21 or later.

probably CSS specificity?

No, not specificity. Somehow Gutenberg 13.5.0+ fails to output the relevant styles at all.

This seems to happen only in org sites connected to JP and with the JP plugin installed. This includes WoA.

It can happen in sites without Jetpack installed at all, if something else calls gutenberg_get_global_styles() at the wrong time during init. See #26224 (comment).

Copy link
Contributor

@anomiex anomiex left a comment

Choose a reason for hiding this comment

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

Investigated to find the real cause of the issue, and suggested an update to the comment accordingly.

projects/plugins/jetpack/modules/google-fonts.php Outdated Show resolved Hide resolved
@anomiex anomiex added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Sep 15, 2022
@samiff samiff added this to the jetpack/11.4 milestone Sep 19, 2022
Copy link
Contributor

@samiff samiff left a comment

Choose a reason for hiding this comment

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

This seemed to work ok on my WoA site with TT2 & Gutenberg v14.0.2, I was able to see the .wp-block-site-title style correctly applied after patching with this branch.

@samiff samiff added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Sep 19, 2022
@kraftbj kraftbj merged commit fc2947c into trunk Sep 20, 2022
@kraftbj kraftbj deleted the fix/site-title-block-block-style-missing branch September 20, 2022 02:52
@github-actions github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Sep 20, 2022
@kraftbj kraftbj modified the milestones: jetpack/11.4, jetpack/11.3.2 Sep 22, 2022
kraftbj added a commit that referenced this pull request Sep 22, 2022
* Set the callback priority to 22 to prevent it from causing style issues with sites that have Gutenberg > 13.5 activated

* Changelog

* Update projects/plugins/jetpack/modules/google-fonts.php

Co-authored-by: Brandon Kraft <[email protected]>
Co-authored-by: Brad Jorsch <[email protected]>
samiff pushed a commit that referenced this pull request Sep 22, 2022
* [not verified] Fix site title block block style missing (#26193)

* Set the callback priority to 22 to prevent it from causing style issues with sites that have Gutenberg > 13.5 activated

* Changelog

* Update projects/plugins/jetpack/modules/google-fonts.php

Co-authored-by: Brandon Kraft <[email protected]>
Co-authored-by: Brad Jorsch <[email protected]>

* Prepare for the release

Co-authored-by: Marcelo Serpa <[email protected]>
Co-authored-by: Brad Jorsch <[email protected]>
dkmyta pushed a commit that referenced this pull request Oct 1, 2022
* Set the callback priority to 22 to prevent it from causing style issues with sites that have Gutenberg > 13.5 activated

* Changelog

* Update projects/plugins/jetpack/modules/google-fonts.php

Co-authored-by: Brandon Kraft <[email protected]>
Co-authored-by: Brad Jorsch <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants