Skip to content

Conversation

@westonruter
Copy link
Member

@westonruter westonruter commented Jan 22, 2024

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

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
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.

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

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

Development

Successfully merging this pull request may close these issues.

5 participants