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

Force HTML5 script theme support when printing JSON script #952

Merged
merged 7 commits into from
Feb 6, 2024
Merged

Force HTML5 script theme support when printing JSON script #952

merged 7 commits into from
Feb 6, 2024

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Jan 22, 2024 •
edited
Loading

Summary

When a theme doesn't declare theme support for HTML5 scripts, the JSON script is currently output with some XHTML compatibility wrappers:

<script type="speculationrules">
/* <![CDATA[ */
{"prerender":[{"source":"document","where":{"and":[{"href_matches":"\/*"},{"not":{"href_matches":["\/wp-login.php","\/wp-admin\/*"]}},{"not":{"selector_matches":".no-prerender"}}]},"eagerness":"moderate"}]}
/* ]]> */
</script>

This breaks the parsing of the JSON. The /* <![CDATA[ */ and /* ]]> */ need to be omitted from JSON output.

Relevant technical choices

This needs to be fixed in core (ticket and PR pending), but a quick workaround is to temporarily override the $_wp_theme_features global with the theme support added.

Update: See core ticket: https://core.trac.wordpress.org/ticket/60320

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@westonruter westonruter added [Type] Bug An existing feature is broken no milestone PRs that do not have a defined milestone for release [Plugin] Speculative Loading Issues for the Speculative Loading plugin (formerly Speculation Rules) labels Jan 22, 2024
@felixarntz
Copy link
Member

@westonruter Can you open a Trac ticket for this? I think we should prioritize this, and it would be great to link to that in a code comment here.

If the fix in core is straightforward (which I would expect it to be), then we can maybe ship this in 6.5 already and thus limit or remove this workaround sooner than later.

@westonruter
Copy link
Member Author

westonruter commented Jan 22, 2024 •
edited
Loading

Can you open a Trac ticket for this? I think we should prioritize this, and it would be great to link to that in a code comment here.

Yes, I started doing that right after opening this PR (ref: "(ticket and PR pending)").

Here it is: https://core.trac.wordpress.org/ticket/56313

Oops. I mean here: https://core.trac.wordpress.org/ticket/60320

@westonruter westonruter marked this pull request as ready for review January 22, 2024 19:02
joemcgill
joemcgill approved these changes Feb 6, 2024
View reviewed changes
Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

Looks good to me. One question about the tests, but pre-approving for merge.

tests/modules/js-and-css/speculation-rules/speculation-rules-test.php
if ( $html5_support ) {
add_theme_support( 'html5', array( 'script' ) );
} else {
remove_theme_support( 'html5' );
Copy link
Member

Choose a reason for hiding this comment

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

I don't remember if theme support get reset between tests. Any chance of this causing side effects for other tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. Good point. Fixed in a1037f8

@westonruter westonruter merged commit 075e307 into feature/speculationrules Feb 6, 2024
@westonruter westonruter deleted the fix/non-html5-script-printing branch February 6, 2024 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joemcgill joemcgill joemcgill approved these changes

@swissspidy swissspidy swissspidy approved these changes

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

@adamsilverstein adamsilverstein Awaiting requested review from adamsilverstein

Assignees
No one assigned
Labels
no milestone PRs that do not have a defined milestone for release [Plugin] Speculative Loading Issues for the Speculative Loading plugin (formerly Speculation Rules) [Type] Bug An existing feature is broken
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@westonruter @felixarntz @joemcgill @swissspidy