Boost: keep position-dependent inline scripts (document.write) in place when Defer JS is enabled#49545
Boost: keep position-dependent inline scripts (document.write) in place when Defer JS is enabled#49545kraftbj wants to merge 4 commits into
Conversation
…is enabled The Render Blocking JS (Defer Non-Essential JavaScript) module moves all script tags to the end of the document. Inline scripts that call document.write()/document.writeln() insert markup at the script's location, so moving them renders their output after the footer instead of where the block was placed (e.g. a Custom HTML block in post content). See issue #30012. Inline scripts (no src) whose body contains document.write are now tagged with the existing ignore attribute during output filtering, so they stay in their original position. Scripts already carrying the ignore attribute, external scripts (with src), and plain inline scripts are unaffected. https://claude.ai/code/session_01PgpTrtTCH4hpz6Krh3ssho
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
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 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Boost plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 1 file.
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a Jetpack Boost “Defer Non-Essential JavaScript” edge case where position-dependent inline scripts using document.write()/document.writeln() were being relocated to the footer, causing their output to render in the wrong place. It does so by marking such inline scripts as ignored during output filtering so they execute in their original position.
Changes:
- Tag inline
<script>blocks (nosrc) containingdocument.writewithdata-jetpack-boost="ignore"so they are not moved to the end of the document. - Add unit tests that exercise the real
handle_output_stream()+append_script_tags()pipeline fordocument.write/writelnscenarios. - Add a Boost changelog entry documenting the fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| projects/plugins/boost/app/modules/optimizations/render-blocking-js/class-render-blocking-js.php | Marks inline document.write scripts as ignored so they stay in place under Defer JS. |
| projects/plugins/boost/tests/php/modules/optimizations/render-blocking-js/Render_Blocking_JS_Test.php | Adds pipeline-level unit tests for document.write/writeln behavior and existing ignore handling. |
| projects/plugins/boost/changelog/fix-defer-js-document-write | Records the user-facing fix in the Boost changelog. |
…P 7.x unit runs - The new tests are the first to exercise append_script_tags() in the WP-less unit suite; trunk's str_contains() call there relies on the WordPress runtime polyfill, so add the equivalent guard to the test bootstrap for PHP <= 7.4. - Silence WordPress.WP.EnqueuedResources.NonEnqueuedScript on the two literal <script src> test fixtures. - Review feedback: move the external script before </p> in the relocation test so the position assertion can only pass if the pipeline moved it, and add a stripos() fast path so buffers without document.write skip the inline-script scan entirely. https://claude.ai/code/session_01PgpTrtTCH4hpz6Krh3ssho
…t polyfill Phan flags the polyfill as redefining an internal function; the docblock @Suppress form matches existing monorepo precedent and keeps the function-comment sniff happy. https://claude.ai/code/session_01PgpTrtTCH4hpz6Krh3ssho
Review follow-up to the inline document.write fix:
- Skip pinning scripts that document.write() their own '<script ...>' markup.
add_ignore_attribute() does a global str_replace() on '<script', which would
rewrite the inner literal and break the quoting of a double-quoted payload
(e.g. document.write("<script src=\"...\">")); tagging only the outer tag would
instead let get_script_tags() match and move that inner literal. Neither is
safe, so such scripts keep the default move behavior and are never corrupted.
- Guard the preg_replace_callback() null returns (PCRE failure) so a pathological
buffer falls back to the original instead of blanking the page, mirroring
is_opened_script().
- Extract pin_position_dependent_scripts() and a shared ignore_attribute_lookahead()
helper, and add a POSITION_DEPENDENT_OUTPUT_NEEDLE constant so the fast-path
guard and per-script check stay in lockstep.
- Strengthen tests: each "stays in place" case now includes a sibling movable
script so it exercises the pin path instead of passing vacuously, plus a
regression guard that a document.write-of-a-script payload is left intact.
Fixes #30012
Proposed changes
document.write()/document.writeln()(e.g. added via a Custom HTML block) were relocated to after the footer, so their output rendered at the bottom of the page instead of where the block was placed.<script>tags (nosrc) whose body containsdocument.writeare now tagged with the existingdata-jetpack-boost="ignore"attribute, so they execute in their original position. The check is an intentionally conservative case-insensitive substring match — a false positive merely leaves a script in place, which is the safe outcome.document.write()their own<script ...>markup are deliberately left to the default behavior: there is no safe in-place rewrite for them (tagging would either corrupt the written markup or let the inner literal be moved), so the change never mutates a script's body and never corrupts page markup. Exotic call forms a substring check cannot see (document['write'], uppercase<SCRIPT>, scripts split across an output-buffer chunk) likewise fall back to today's move behavior — a miss is never worse than the current state.srcpresent, even if the inert body mentionsdocument.write), plain inline scripts (still deferred), or scripts already carrying the ignore attribute (markup untouched, no duplicate attribute).handle_output_stream()+append_script_tags()pipeline (each "stays in place" test includes a sibling movable script so it actually exercises the pin path rather than passing vacuously).Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
data-jetpack-boost="ignore".</body>.jp install -r plugins/boost && jp test php plugins/boost -- --filter Render_Blocking_JS_Test(and the full Boostunitsuite should remain green).