Repository navigation
fix: avoid a duplicate H1 when a hidden title's page provides one elsewhere - #4658
HardeepAsrani wants to merge 16 commits into
Conversation
…ewhere The screen-reader H1 fallback only scanned post_content, so an H1 from a synced pattern, a page builder or a Neve Pro custom layout produced a second H1. The main area is now buffered and the fallback is kept only when no other H1 rendered. Fixes #4652 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes requested
The fix scans only the main area, so a custom layout H1 in the header or footer still gives two H1s. The H1 check, three tests and one type also need fixes.
Validation details
- Files reviewed: 5/5 changed files. Unchecked history rows: Footer Menu duplicates
footer-menuID across responsive footer layouts, Page title disappears on post category pages, Accessibility changes to match EU accessibility requirements (EN 301 549) that become mandatory in June 2025, Submenu toggle does not open through screen-reader activation, Page Title visible, even when it's disabled from Neve options, Issue with archive title, Featured post section enabled makes page titles disappear, Pass TA guidelines. - The PR's PHPUnit class passes on HEAD with PHP 8.3 and WordPress 7.1.3:
mkdir -p /workspace/checks/R/head && cd /workspace/checks/R/head && rm -rf neve && cp -a /repo neve && cd neve && set -o…printedOK (8 tests, 16 assertions)(run R2). - The same class on the base tree:
cd /workspace/checks/R && rm -rf basetree && mkdir basetree && cp -a /repo basetree/neve && cd basetree/neve && for f in…printedTests: 8, Assertions: 11, Errors: 2, Failures: 2.(run R4) Only the two hooked-H1 tests fail there. - An H1 on a header or footer hook still duplicates: `mkdir -p /workspace/checks/K; cat > /workspace/checks/K/test-zz-review.php <<'EOF'
Untested areas
- The e2e spec did not run. The sandbox has no built assets or served site. - Neve Pro was not available. Its custom layout rendering was not exercised. - These history rows were not re-verified by a dedicated check: Footer Menu duplicates `footer-menu` ID across responsive footer layouts, Page title disappears on post category pages, Accessibility changes to match EU accessibility requirements (EN 301 549) that become mandatory in June 2025, Submenu toggle does not open through screen-reader activation. These rows were checked only with ad-hoc unit runs: Page Title visible, even when it's disabled from Neve options, Issue with archive title, Featured post section enabled makes page titles disappear, Pass TA guidelines.Start at wp_body_open and resolve at wp_footer so header and footer custom layouts count, ignore H1s inside HTML comments, and strip scripts before comments so a lone "<!--" in a script cannot swallow real markup. Tests now cover header/footer hooks, patterns, comments, and each fails on the base tree. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes requested
The earlier findings are fixed, but an H1 on the body start or body end hook still gets a second H1. Fix that, one stale comment and two test issues.
Validation details
- Files reviewed: 5/5 changed files. Unchecked rows: Optional setting off: Producer, Setting unset, Override, Stale data.
- The PR's PHPUnit class passes on HEAD with PHP 8.3 and WordPress 7.1.3:
grep -E "^[0-9]+\) |Error: |Tests:" /tmp/sandbox-phpunit-ab.zVTgcX/base.log; mkdir -p /workspace/checks/R/head && cd /wo…printedOK (10 tests, 26 assertions)(run R1). - On the base tree, eight tests fail on their assertions:
cd /workspace/checks/R && rm -rf basetree && mkdir basetree && cp -a /repo basetree/neve && cd basetree/neve && for f in…printedTests: 10, Assertions: 9, Errors: 2, Failures: 8.(run R2) - Real template renders give one H1 for header and footer hooks:
cd /workspace/checks/R/head/neve && phpunit --do-not-cache-result --filter 'TestZzReview::test_real_page with data set "…printed[after_header_hook] h1=1 sr=0(run R20),cd /workspace/checks/R/head/neve && phpunit --do-not-cache-result --filter 'TestZzReview::test_real_page with data set "…printed[before_footer_hook] h1=1 sr=0(run R21). - The body start hook still duplicates:
cd /workspace/checks/R/head/neve && phpunit --do-not-cache-result --filter 'TestZzReview::test_real_page with data set "…printed[body_start_after] h1=2 sr=1(run R15).
Untested areas
- The e2e spec did not run. The theme needs built assets before a served site loads it.
- Neve Pro was not available. Its custom layout hook list was not read.
test_unbuffered_path_keeps_content_scanerrors on the base tree on a missing method. Its base result is inconclusive.- These setting rows were not settled: Optional setting off: Producer, Setting unset, Override, Stale data.
🤖 Automated review · run code-review-agent_6ad1ce22a74bb3.68194372.
🤖 Review agent — review posted ✅ on 937dfe56 · changes requested · 4 findings · 46 min
Run code-review-agent_6ad1ce22a74bb3.68194372 · trail
Open on neve_body_start_after (wp_body_open as fallback) and resolve on neve_body_end_before so custom layouts on the body start and end hooks count. Tests: one data-provider case per body hook, a stray open buffer, the wp_body_open fallback, and a placement test that fails on the base tree. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes requested
The earlier findings are fixed. Three test issues remain: one order-dependent test, one weak assertion and one duplicated helper body.
Validation details
- Files reviewed: 5/5 changed files. Unchecked rows: Optional setting off: Producer, Setting unset, Override, Empty data.
- The PHPUnit class ran on HEAD with PHP 8.3 and WordPress test library
mkdir -p /workspace/checks/R/head /workspace/checks/R/html /workspace/checks/T && cd /workspace/checks/R/head && rm -rf …printedTests: 14, Assertions: 36, Failures: 1.(run R2) - With
kses_remove_filters()added, it passescd /workspace/checks/R/head/neve && cp /workspace/checks/R/orig-test.php tests/test-neve-hidden-title-h1.php && sed -i '…printedOK (14 tests, 38 assertions)(run R6) - On the base tree, twelve tests fail on assertions
cd /workspace/checks/R && rm -rf base && mkdir base && cp -a head/neve base/neve && cp orig-test.php base/neve/tests/tes…printedTests: 14, Assertions: 14, Errors: 2, Failures: 12.(run R14) - Real template renders give one H1 for the body start hook
cd /workspace/checks/R/head/neve && for ds in body_start_after body_end_before after_header_hook before_footer_hook nest…printed[body_start_after] h1=1 sr=0(run R11)
Untested areas
- The e2e spec did not run. The theme needs built assets.
- Neve Pro was not available. Inside-content layouts were not checked.
- Two tests error on the base tree on missing symbols. Their base result is inconclusive.
- These setting rows were not settled: Optional setting off: Producer, Setting unset, Override, Empty data.
🤖 Automated review · run code-review-agent_6ad1db014396c8.21985341.
🤖 Review agent — review posted ✅ on 121d8f84 · changes requested · 3 findings · 27 min
Run code-review-agent_6ad1db014396c8.21985341 · trail
The bootstrap assigns user 1 directly, so wp_set_current_user() left kses on and stripped the noscript fixture when the class ran alone. Assert the exact buffer level in the stray-buffer case and reuse render_page() for the wp_body_open-only render. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approved
The PR passes. The three earlier test findings are fixed, and the new commit adds no problem.
Validation details
- Files reviewed: 5/5 changed files. Unchecked rows: Optional setting off: Producer, Optional setting off: Setting unset, Optional setting off: Override.
- The hidden-title test class passes alone on HEAD with PHP 8.3:
mkdir -p /workspace/checks/R1/head; cp -a /repo /workspace/checks/R1/head/neve; python3 /workspace/checks/R1/record.py R…printedOK (14 tests, 38 assertions)(run R1) - The stray-buffer test fails when the buffer-level guard is deleted:
set -o pipefail; phpunit --do-not-cache-result --filter 'TestNeveHiddenTitleH1::test_stray_open_buffer_is_left_alone' 2>…printedFailed asserting that 3 is identical to 4.(run R2) - Both modified tests fail on the base tree:
python3 /workspace/checks/R1/record.py R103 /workspace/checks/R1/head/neve "sandbox-phpunit-ab.sh /workspace/checks/R1/h…printedFailed asserting that 2 is identical to 1.(run R3) - Product-file checks are reused from run code-review-agent_6ad1db014396c8.21985341, not rerun. Those files did not change since then.
Untested areas
- These setting rows were not settled: Optional setting off: Producer, Optional setting off: Setting unset, Optional setting off: Override.
- The e2e spec did not run. The theme needs built assets.
- Neve Pro was not available. Inside-content layouts were not checked.
🤖 Automated review · run code-review-agent_6ad2132037a466.47610119.
🤖 Review agent — review posted ✅ on 0830d445 · approved · 0 findings · 25 min
Run code-review-agent_6ad2132037a466.47610119 · trail
…ktrack limit The lazy regex that stripped script, template, noscript and comment blocks gave up on an inline script larger than pcre.backtrack_limit (1 MB by default). preg_replace returned null, the unstripped body was scanned instead, and a literal "<h1>" inside the script counted as a real heading, so a hidden-title page lost its only H1. Found by QA on issue #4652; the same scan miscounted on the base tree. Replace it with a single left-to-right pass over the markup that skips comments and the raw-text elements by string search, which is linear and has no limit. Tests: the 1.2 MB script fixture plus a data provider for the comment and raw-text edge cases. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes requested
The new scanner calls ctype_space(), which can be missing on some PHP builds. Replace it with strspn().
Validation details
- Files reviewed: 5/5 changed files. Unchecked rows: Optional setting off: Producer, Optional setting off: Setting unset, Optional setting off: Override.
- The hidden-title tests pass on HEAD with PHP 8.3:
command -v phpunit; phpunit --version 2>&1 | head -2; cd /workspace/checks/R-T1/head/neve && set -o pipefail && timeout …printedOK (28 tests, 54 assertions)(run R3) - The new tests fail with the previous regex scanner:
cd /workspace/checks/R-T1 && rm -rf variant-b && mkdir variant-b && cp -a head/neve variant-b/neve && cd variant-b/neve …printedFailures: 3(run R4) - The new and old scanners differ on two inputs only. Both now match browser parsing [R5].
- Hook and render checks are reused from run code-review-agent_6ad2132037a466.47610119, not rerun.
Untested areas
- Optional setting off: Producer, Setting unset, Override. Open
inc/views/page_header.php:97-113. Confirm a filteredpage_for_postsor theme mod gives the gate and the view the same result. - Forms like
<!-->and</script/>hide a real H1 from the scanner. No producer was found. - The e2e spec did not run. Neve Pro was not available.
🤖 Automated review · run code-review-agent_6ad5bb7fbe33a0.27973925.
🤖 Review agent — review posted ✅ on 6e633c29 · changes requested · 1 finding · 30 min
Run code-review-agent_6ad5bb7fbe33a0.27973925 · trail
ctype_space() needs ext-ctype, which WordPress only recommends. Use strspn() over the HTML whitespace set instead. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approved
The PR passes. The ctype_space() finding is fixed with strspn(), and the new commit adds no problem.
Validation details
- Files reviewed: 5/5 changed files. Commit
6e22b17changes onlyinc/views/page_header.php. - An isolated PHP 8.3 run compared the old and new closing-tag scans. Only a vertical tab differs:
python3 /workspace/checks/L1/build.py && { printf 'Run recorded: R1 (L1)\nCommand: timeout 30 php /workspace/checks/L1/c…printedwarnings/exceptions=0(run R3) - The PR's PHPUnit class passes at HEAD:
set -o pipefail; rm -rf /workspace/checks/R1/head && mkdir -p /workspace/checks/R1/head/neve && cd /repo && git archive …printedOK (28 tests, 54 assertions)(run R6) - The earlier checks on unchanged code come from run code-review-agent_6ad5bb7fbe33a0.27973925 and were not rerun.
Untested areas
- The requirements, security and standards passes were not rerun. The new commit changes two scanner lines and no trust boundary.
🤖 Automated review · run code-review-agent_6ad707cff2da17.80540549.
🤖 Review agent — review posted ✅ on 6e22b17a · approved · 0 findings · 27 min
Run code-review-agent_6ad707cff2da17.80540549 · trail
Browsers parse <template> contents as markup, so a nested template, or a comment or script holding "</template>", must not close the outer one. The scan now tracks template depth in its main loop and ignores H1s inside it; find_closing_tag() stays a raw-text search for script and noscript. Found by QA on issue #4652. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes requested
The new template depth counter works, but two new test rows pass without the change. Delete them.
Validation details
- Files reviewed: 2/5 changed files in commit
b01881a. The other three are unchanged since the approved review. Unchecked rows: Optional setting off: Producer, Optional setting off: Setting unset, Optional setting off: Override. - The PR's PHPUnit class passes at HEAD:
OK (35 tests, 61 assertions)(R3). - With the
6e22b17scanner, four new rows fail:Failures: 4(R4). - An isolated PHP 8.3 run checked nested, unterminated and stray template tags:
php /workspace/checks/C1a/driver.php(R5). - Earlier checks on unchanged code come from run code-review-agent_6ad707cff2da17.80540549 and were not rerun.
Untested areas
- Optional setting off: Producer, Optional setting off: Setting unset and Optional setting off: Override were not rechecked. The commit does not change
is_title_hidden()ininc/views/page_header.php. - The requirements and security passes were not rerun. The commit changes scanner logic and test rows only.
🤖 Automated review · run code-review-agent_6ad72d158c78d6.04717316.
🤖 Review agent — review posted ✅ on b01881a8 · changes requested · 1 finding · 16 min
Run code-review-agent_6ad72d158c78d6.04717316 · trail
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approved
The PR passes. Commit 469eeae deletes the two test rows that passed without the change, and it adds no new problem.
Validation details
- Files reviewed: 1/5 changed files in commit
469eeae. The other four are unchanged since the last review. - The commit only deletes the
template name prefixrow and theh1 after template holding a commented closerrow intests/test-neve-hidden-title-h1.php. - The PHPUnit class passed on
b01881awithOK (35 tests, 61 assertions). Source: run code-review-agent_6ad72d158c78d6.04717316, not rerun. - A source check at HEAD rechecked the producers, defaults, overrides and empty values of the hidden-title settings.
Untested areas
- The PHPUnit class was not rerun at
469eeae. The commit only removes two data-provider rows.
🤖 Automated review · run code-review-agent_6ad74175b801c2.96762909.
🤖 Review agent — review posted ✅ on 469eeaea · approved · 0 findings · 18 min
Run code-review-agent_6ad74175b801c2.96762909 · trail
Literal tag text inside a quoted attribute was read as markup, so a "<template>" in a data attribute hid the real H1 and an "<h1>" in one dropped the fallback. The scan now skips to the end of each tag, honouring quoted values, before looking for the next tag. Found by QA on issue #4652. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Dev review: The new H1 scan misreads custom-element tags with tag text in quoted attributes. Confirm that no body output carries such markup.
Validation details
- Files reviewed: 2/5 changed files in commit
ff60927. The other three are unchanged since the approved review. - An isolated PHP 8.3 run passed every scanner row at HEAD:
timeout 30 php /workspace/checks/L1/check.phpprinteda: 24/24 pass(run R1) - The six new rows fail on the previous scanner:
timeout 30 php /workspace/checks/L1/check.phpprintedb: 18/24 pass(run R1) - WordPress 7.1.3
WP_HTML_Tag_Processorsees an H1 that the scan misses:mkdir -p /workspace/checks/L2a/core cp /var/www/html/wp-includes/html-api/class-wp-html-{tag-processor,attribute-token,d…printed2 HEAD=false tags=MY-EL,H1(run R3) - On a rendered page,
wptexturizechanged the attribute quote in saved content:cd /var/www/html && wp eval 'echo wptexturize("<div data-tpl=\"<script>\"></div>"), "\n"; echo wptexturize("<my-el data-…printed<my-el data-tpl="<script>“></my-el>(run R29) - The settings rows were rechecked from source at HEAD.
Untested areas
- Open
inc/views/page_header.php:205. Check whether any plugin prints a hyphenated custom element, such as<my-el data-tpl="<script>">, unescaped into the page body. If none does, the scan is safe. If one does, the page gets a second H1, or none. Then widen the tag name to[a-z][^\s/>]*. - The block editor UI and REST saves were not exercised.
🤖 Automated review · run code-review-agent_6ad9a0b23053c1.68865080.
🤖 Review agent — review posted ✅ on ff60927a · commented · 0 findings
Run code-review-agent_6ad9a0b23053c1.68865080 · trail
The tag-name match only allowed letters and digits, so a hyphenated custom element was not seen as a tag and its attribute text was read as markup. Match the name up to whitespace, slash or ">" as the HTML tokenizer does. Found by QA on issue #4652. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes requested
The custom-element fix works, but the widened tag pattern now counts a trailing <h1 as an H1. Restore the tag-name boundary.
Validation details
- Files reviewed: 2/5 changed files in commit
28d2125. The other three are unchanged since the last review. Unchecked rows: Optional setting off: Producer, Optional setting off: Setting unset, Optional setting off: Override. - An isolated PHP 8.3 scanner run passed every provider row at HEAD:
timeout 30 php /workspace/checks/L3a/check.php; printf '\n--- SOURCE LINES ---\n'; nl -ba /workspace/checks/L3a/page_hea…printedHEAD: 26/26 pass; failing rows: none(run R1) - The two new rows fail on the previous scanner:
timeout 30 php /workspace/checks/L3a/check.php; printf '\n--- SOURCE LINES ---\n'; nl -ba /workspace/checks/L3a/page_hea…printedff60927: 24/26 pass; failing rows: h1 text in custom element attribute; template opener in custom element attribute(run R1) - Question for the author: the PR body says 39 test cases. HEAD has 41.
Untested areas
- Optional setting off: Producer, Setting unset and Override were not rechecked in this run. Their earlier answers rest on unchanged code in
inc/views/page_header.php:98-112. - The block editor and full page renders were not exercised.
🤖 Automated review · run code-review-agent_6ad9e7a97060a0.96894047.
🤖 Review agent — review posted ✅ on 28d21255 · changes requested · 1 finding · 12 min
Run code-review-agent_6ad9e7a97060a0.96894047 · trail
Browsers drop a tag the input ends inside, so count an H1 only once its tag end is found. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approved
The PR passes. Commit 9ee144f fixes the trailing <h1 finding, and it adds no new problem.
Validation details
- Files reviewed: 2/5 changed files in commit
9ee144f. The other three are unchanged since the last review. Unchecked rows: Optional setting off: Producer, Optional setting off: Setting unset, Optional setting off: Override. - An isolated PHP 8.3 scanner run passed every provider row at HEAD:
set -o pipefail; timeout 30 php /workspace/checks/L1a/check.php 2>&1 | tee /workspace/checks/L1a/R1.log; result=${PIPEST…printedSUMMARY 9ee144f: 28/28 pass; failing rows: [](run R1) - The two new rows fail on the previous scanner:
set -o pipefail; timeout 30 php /workspace/checks/L1a/check.php 2>&1 | tee /workspace/checks/L1a/R1.log; result=${PIPEST…printed26/28 pass; failing rows: ["h1 opener at end of input","h1 with unterminated attribute"](run R1) - A source read of the new comment and test rows found no style or test problem.
Untested areas
- Optional setting off: Producer, Setting unset and Override were not rechecked. Their earlier answers rest on unchanged code in
inc/views/page_header.php:98-112. - The PHPUnit suite, e2e tests and full page renders were not run in this review.
🤖 Automated review · run code-review-agent_6ad9eb5f82f5f3.07096078.
🤖 Review agent — review posted ✅ on 9ee144f1 · approved · 0 findings · 6 min
Run code-review-agent_6ad9eb5f82f5f3.07096078 · trail
Browsers never parse tags inside style, textarea, title, xmp, iframe, noembed or noframes either, so H1 text there dropped the fallback. Found by QA on issue #4652. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes requested
The raw-text tag list now misreads a self-closing SVG <title/>, so a later real H1 still gets the fallback H1. Fix the scan.
Validation details
- Files reviewed: 2/5 changed files in commit
55d313c. The other three are unchanged since the last review. Unchecked row: Optional setting off: Producer. - An isolated PHP 8.3 run of the HEAD and
9ee144fscanners shows each new test row fails on the old scanner [R1]. - The same run shows
<svg><title/></svg><h1>Real</h1>returnsfalseat HEAD andtrueat9ee144f[R1]. - Source checks found no new security boundary and no stale comment.
Untested areas
- Optional setting off: Producer. The writers of the hide-title settings were not rechecked.
- The YITH gallery page was not rendered in a browser.
- Gutenberg's opt-in full-page navigation closes the top output buffer on
wp_footer. Its effect on a later H1 was not run. - The
stripos()cost per raw-text element on PHP below 8.2 was not measured.
🤖 Automated review · run code-review-agent_6ada12780d4c95.45340871.
🤖 Review agent — review posted ✅ on 55d313c4 · changes requested · 1 finding · 45 min
Run code-review-agent_6ada12780d4c95.45340871 · trail
Inside <svg> and <math>, title and style are ordinary elements, tags can self-close and an <h1> breaks out as HTML, so a self-closing SVG <title/> must not start a raw-text skip. <foreignObject> restores the HTML rules. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes requested
The self-closing SVG <title/> finding is fixed. The new SVG and MathML scan state has no test rows, so add the four rows named inline.
Validation details
- Files reviewed: 2/5 changed files in commit
26fded7. The other three are unchanged since the last review. Unchecked rows: Optional setting off: Producer, Optional setting off: Setting unset, Optional setting off: Override. - An isolated PHP 8.3 run shows both new rows fail on the
55d313cscanner and pass at HEAD [R2]. - The same run shows four scanner mutants still pass all 33 provider rows
python3 /workspace/checks/L1a/build.py && timeout 30 php /workspace/checks/L1a/run.php | tee /workspace/checks/L1a/provi…printedB M1 ALL_PASS=true pass=33/33 failed=[](run R2). - A parse5 comparison agrees with HEAD on ordinary SVG icon markup [R3].
Untested areas
- Optional setting off: Producer, Setting unset and Override. These rows were not rechecked. This commit does not touch those lines.
- Open
inc/views/page_header.php:229. HEAD stays in SVG mode after a breakout tag such as<p>. It also ignores MathML<mtext>and a</template>inside SVG. Malformed SVG or MathML can then hide or duplicate the fallback H1. A WordPress.org search found no plugin or theme that emits such markup.
🤖 Automated review · run code-review-agent_6adb0a2e204584.03852332.
🤖 Review agent — review posted ✅ on 26fded7a · changes requested · 1 finding · 21 min
Run code-review-agent_6adb0a2e204584.03852332 · trail
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approved
The PR passes. Commit 0504889 adds the four requested SVG and MathML test rows, and it adds no new problem.
Validation details
- Files reviewed: 1/5 changed files in commit
0504889. The other four are unchanged since the last review. Unchecked row: Optional setting off: Override. - An isolated PHP 8.3 run used the HEAD scanner. Each new row passes on HEAD and fails on its target mutant:
git rev-parse HEAD; timeout 30 php /workspace/checks/L2/run.php; timeout 30 php -l /repo/tests/test-neve-hidden-title-h1…printedAll four HEAD rows pass and each kills exactly its target mutant(run R1) - The same run reports
git rev-parse HEAD; timeout 30 php /workspace/checks/L2/run.php; timeout 30 php -l /repo/tests/test-neve-hidden-title-h1…printedNo syntax errors detected in /repo/tests/test-neve-hidden-title-h1.php(run R1) - A source read found no duplicate row, wrong label or stale comment.
- Checks on the unchanged files are reused from run code-review-agent_6adb0a2e204584.03852332, not rerun.
Untested areas
- Optional setting off: Override. Neve Pro or site filters that return different hide-title values per call were not checked.
- The full PHPUnit and e2e suites were not rerun. CI runs them.
🤖 Automated review · run code-review-agent_6adb1367d5ecc6.28539162.
🤖 Review agent — review posted ✅ on 0504889d · approved · 0 findings · 24 min
Run code-review-agent_6adb1367d5ecc6.28539162 · trail
Browsers close script and style on an end tag that carries attributes or a stray slash, so the closer search now accepts those forms. Inside svg and math, title, desc and the MathML text elements restore the HTML rules like foreignObject does; a context stack replaces the two counters so an HTML <title> under an integration point stays raw text. Found by QA on issue #4652. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes requested
The new integration-point stack never pops on </title>, </desc> or </foreignObject>, so a later SVG H1 is missed. Fix the closing check.
Validation details
- Files reviewed: 2/5 changed files in commit
14cd6e7. The other three are unchanged since the approved review. Unchecked rows: Optional setting off: Override, Optional setting off: Producer, Optional setting off: Setting unset. - An isolated PHP 8.3 run compared the scanner at HEAD and
0504889.timeout 30 php /workspace/checks/L1a/run.php | tee /workspace/checks/L1a/output.logprintedProvider passes / 43: HEAD=43 prior=38 fix=43(run R1) - Headless Chrome
DOMParserfinds one H1 in<svg><title>t</title><style><h1>x</h1></style></svg>. - The end-tag change matches browsers for attributes,
/, newlines and end of input.
Untested areas
- Optional setting off rows (Override, Producer, Setting unset): open
inc/views/page_header.php:111-126. Check that Neve Pro hide-title filters give the same answer to the buffer gate and the view. Different answers mean a missing or extra fallback H1. inc/views/page_header.php:44ignores namespaces.<math><title>counts as an integration point. No producer was found.annotation-xml encoding="text/html"is not an integration point. No producer was found.
🤖 Automated review · run code-review-agent_6adb34ee3eaff2.27206306.
🤖 Review agent — review posted ✅ on 14cd6e78 · changes requested · 1 finding · 14 min
Run code-review-agent_6adb34ee3eaff2.27206306 · trail
A closing title, desc, foreignObject or MathML text tag never left the stack, so HTML rules outlived the element and an <h1> that breaks out of the following foreign content was skipped. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approved
The PR passes. Commit 0fe4373 fixes the integration-point stack finding and adds no confirmed problem.
Validation details
- Files reviewed: 2/5 changed files in commit
0fe4373. The other three are unchanged. Unchecked rows: Optional setting off: Producer, Optional setting off: Setting unset, Optional setting off: Override. - An isolated PHP 8.3 scanner run passed at HEAD:
git -C /repo show 0fe43736b91e0a9381385b0745b1a2a889687285:tests/test-neve-hidden-title-h1.php > /workspace/checks/L1/he…printedSUMMARY HEAD: 47/47 PASS; 0 FAIL(run R1) - The four new rows fail on
14cd6e7:git -C /repo show 0fe43736b91e0a9381385b0745b1a2a889687285:tests/test-neve-hidden-title-h1.php > /workspace/checks/L1/he…printedSUMMARY PRIOR: 43/47 PASS; 4 FAIL(run R1)
Untested areas
- At
inc/views/page_header.php:240,</desc>pops the context while an unclosed<div>is open inside it. Chromium ignores that end tag, so the scan counts an H1 the page lacks. No producer of such markup was found. - The three unchecked rows above were not rechecked.
- PHPUnit was not run. CI owns it.
🤖 Automated review · run code-review-agent_6adb3ae3c0a2c9.96967871.
🤖 Review agent — review posted ✅ on 0fe43736 · approved · 0 findings · 10 min
Run code-review-agent_6adb3ae3c0a2c9.96967871 · trail
Summary
When the page title is hidden,
views/page-header.phpprinted a screen-reader H1 unless it found an<h1in the post's rawpost_content. Anything else that renders an H1 was invisible to that check, so the page ended up with two: Neve Pro custom layouts on hooks or inside the content, synced patterns, page builders that store markup in meta, and every non-singular page (the blog page hides its title by default for sites installed after 4.0).The decision is now deferred.
Page_Headeropens an output buffer on the theme's first body hook,neve_body_start_after(withwp_body_openas a fallback for other templates), when the title is hidden. The view prints a placeholder comment where the title would be, and the buffer callback swaps it for the screen-reader H1 only if the buffered body contains no H1. The scan is a single linear pass, so a large inline script cannot hit the PCRE backtrack limit; it ignores HTML comments, the raw-text elements (script, style, noscript, textarea, title, xmp, iframe, noembed, noframes), tag text inside quoted attribute values, and anything inside a<template>, whose contents nest like regular markup. Inside<svg>and<math>it follows the parser's foreign-content rules: no raw text, self-closing tags, and an<h1>that breaks out as HTML, with the HTML rules restored inside integration points such as<foreignObject>, SVG<title>and<desc>and the MathML text elements. Script and style end tags that carry attributes still close the element, as in browsers. The buffer closes on the last body hook,neve_body_end_before, so every custom layout hook from body start to body end counts; if that hook never fires, PHP runs the callback at shutdown. When no buffer is active (a template without the body hooks), the previouspost_contentscan still applies.Reproduced on a local site with Neve Pro: hook layout before the loop, hook layout before content, inside-content layout, synced pattern, and the blog page. All produced an extra H1 before and none after. Beaver Builder and Brizy were not available locally; the hooked-H1 unit tests cover the same mechanism.
Will affect visual aspect of the product
NO. The fallback H1 is visually hidden; only its presence changes.
Test instructions
h1.screen-reader-textrenders.neve_before_content,neve_after_header_hookorneve_before_footer_hook, or an inside-content layout: no screen-reader H1.neve_before_loop: one H1.<h1>renders and no placeholder comment appears in the source.Check before Pull Request is ready:
tests/test-neve-hidden-title-h1.php(62 cases: every body hook, the stray-buffer guard, a 1.2 MB inline script, and a provider of comment, raw-text, nested-template and attribute edge cases; the behavior cases fail on the base tree) and a synced-pattern fixture plus assertion ine2e-tests/specs/a11y-ready/structure.spec.ts.wp_kses_post(); buffered template output is passed through unchanged.Review follow-up
The automated review asked whether the test-name conventions in
CONTRIBUTING.md(lines 246 and 260-263) should be updated, since no existing test follows them. That predates this change and applies to the whole suite, so it is left for a separate docs PR rather than mixed into this fix.Closes #4652.
🤖 Generated with Claude Code