Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to No confirmed issue prevents merging after normal checks. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description Check | |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the primary parser change: recognizing CDATA directly inside SVG and MathML integration points. |
✨ Finishing Touches
-
-
🧪 Generate unit tests (beta)
-
-
- Create a new PR
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
I’m a rabbit; I hop by the stream,
Where SVG and MathML meet.
CDATA finds its proper name,
Even when it ends incomplete.
I thump my paws and nibble greens,
Then bound away through parsing dreams.
Comment @coderabbitai help to get the list of available commands.
 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/Parser.ts">
<violation number="1" location="src/Parser.ts:357">
P3: For an integration point, this branch returns the context entry before it (`foreignContext[length - 2]`), i.e. the nearest svg/math ancestor rather than the integration point's actual parent element. Because `emitOpenTag` pushes onto `foreignContext` only for `svg`, `math`, and integration-point elements, any HTML element opened inside foreign content (e.g. `<div>` inside `<svg>`) leaves no entry, so the lookup skips over it. Concretely, for `<svg><div><mi><![CDATA[x]]></mi></div></svg>` the stack is `[svg, div, mi]` with `foreignContext = [None, Svg, None]`; the check reads `foreignContext[1] === Svg` and treats the CDATA as foreign text. Per the HTML spec, `div` inside SVG is an HTML element, so the nested `mi` content is HTML and `<![CDATA[` should be a bogus comment. (The same HTML-in-foreign approximation already existed for `<svg><div><![CDATA[` before this PR; this method extends it to nested integration points.) Worth documenting as an approximation or handling the immediate parent's namespace, since it affects whether CDATA text vs comment events are emitted.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| htmlIntegrationElements.has(this.stack[this.stack.length - 1]) | ||
| ) { | ||
| return ( | ||
| this.foreignContext[this.foreignContext.length - 2] !== |
There was a problem hiding this comment.
P3: For an integration point, this branch returns the context entry before it (foreignContext[length - 2]), i.e. the nearest svg/math ancestor rather than the integration point's actual parent element. Because emitOpenTag pushes onto foreignContext only for svg, math, and integration-point elements, any HTML element opened inside foreign content (e.g. <div> inside <svg>) leaves no entry, so the lookup skips over it. Concretely, for <svg><div><mi><![CDATA[x]]></mi></div></svg> the stack is [svg, div, mi] with foreignContext = [None, Svg, None]; the check reads foreignContext[1] === Svg and treats the CDATA as foreign text. Per the HTML spec, div inside SVG is an HTML element, so the nested mi content is HTML and <![CDATA[ should be a bogus comment. (The same HTML-in-foreign approximation already existed for <svg><div><![CDATA[ before this PR; this method extends it to nested integration points.) Worth documenting as an approximation or handling the immediate parent's namespace, since it affects whether CDATA text vs comment events are emitted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/Parser.ts, line 357:
<comment>For an integration point, this branch returns the context entry before it (`foreignContext[length - 2]`), i.e. the nearest svg/math ancestor rather than the integration point's actual parent element. Because `emitOpenTag` pushes onto `foreignContext` only for `svg`, `math`, and integration-point elements, any HTML element opened inside foreign content (e.g. `<div>` inside `<svg>`) leaves no entry, so the lookup skips over it. Concretely, for `<svg><div><mi><![CDATA[x]]></mi></div></svg>` the stack is `[svg, div, mi]` with `foreignContext = [None, Svg, None]`; the check reads `foreignContext[1] === Svg` and treats the CDATA as foreign text. Per the HTML spec, `div` inside SVG is an HTML element, so the nested `mi` content is HTML and `<![CDATA[` should be a bogus comment. (The same HTML-in-foreign approximation already existed for `<svg><div><![CDATA[` before this PR; this method extends it to nested integration points.) Worth documenting as an approximation or handling the immediate parent's namespace, since it affects whether CDATA text vs comment events are emitted.</comment>
<file context>
@@ -341,6 +341,26 @@ export class Parser implements Callbacks {
+ htmlIntegrationElements.has(this.stack[this.stack.length - 1])
+ ) {
+ return (
+ this.foreignContext[this.foreignContext.length - 2] !==
+ ForeignContext.None
+ );
</file context>
There was a problem hiding this comment.
Checked in Chromium, Firefox and WebKit: <div> closes the <svg>, so <svg><div><mi><![CDATA[x]]></mi></div></svg> parses as an empty svg followed by div > mi with a [CDATA[x]] comment. htmlparser2 keeps HTML elements such as div, p and b inside the svg, so <svg><div><![CDATA[x]]> already gives text instead of that comment on master, and with this PR the nested mi follows the same model. Matching the browsers there would mean closing foreign content on those start tags, which I've left out of this PR.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91388d58e2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
This branch has not been deployed
1 participant
Follow-up to #2458. Greptile flagged this case on that PR before it was merged (comment), and it is right.
That PR uses
isInForeignContext()to decide whether<![CDATA[starts a CDATA section. Directly inside an HTML integration point such asforeignObject,titleormi, that is false, because opening one pushesForeignContext.None. So<svg><foreignObject><![CDATA[x><b>]]>now gives a[CDATA[xcomment followed by a live<b>. Before #2458 it gave a single[CDATA[x><b>]]comment.The markup declaration open state switches to the CDATA section state when the adjusted current node is not an element in the HTML namespace.
foreignObjectis an SVG element, so this is a CDATA section and its content is text. whatwg/html#4016 was closed on this in July ("Gecko, WebKit, and tests match the standard"), and web-platform-tests/wpt#61253 added the tests. Chromium and parse5 still produce a comment here, so parse5 is not a usable oracle for these inputs.This adds
Parser#isCurrentNodeForeign(), which treats an integration point as being in the namespace it was opened in, and uses it for the CDATA decision in the tokenizer and inoncdata. A tokenizer whose callbacks only implementisInForeignContextgets the same events as before, except for unclosed CDATA at the end of input (see below). The open PR #2492 adds a similar check for the element being opened, for the self-closing flag; this one is about the current node, and the two branches merge without conflicts.A second commit handles the end of input: unclosed CDATA now goes to
oncdatawhen the current node is SVG or MathML, so<svg><![CDATA[x><b>and<svg><foreignObject><![CDATA[x><b>end with the textx><b>instead of a[CDATA[comment, as in Firefox and WebKit. The first of those already gave a comment before #2458; Codex flagged the second one below. For a tokenizer with onlyisInForeignContext, unclosed CDATA now goes tooncdatawhen that returns true, as closed CDATA already did.<svg><foreignObject><![CDATA[x><b>]]></foreignObject></svg>x><b><b><b>x><b><svg><title><![CDATA[x><b>]]></title></svg>x><b><b><b>x><b><math><mi><![CDATA[x><b>]]></mi></math>x><b><b><b>x><b><math><annotation-xml><![CDATA[x><b>]]></annotation-xml></math>x><b>x><b><b>x><b><svg><foreignObject><div><![CDATA[x><b>]]></div></foreignObject></svg><b><b><b><b>The six cases from wpt#61253, run through
parseDocumentand compared with the trees in that file: master matches 3, this PR matches 6.Tests: five event tests next to the existing foreign CDATA ones. Four fail on master:
foreignObject,mtextand the two unclosed-at-EOF cases. The fifth puts a<p>insideforeignObjectand passes on both, as a check that HTML content there still gets a bogus comment.npm run test:vi: 205 passed, andnpm run lintpasses, after merging the current master.Summary by CodeRabbit