feat: DOM ergonomics parity + Intl polyfill + async onConfirm - #48
Conversation
Boolean attribute reflection (disabled/required/readOnly/multiple/ autofocus/selected), form.reset(), select.options/.selectedIndex/ .selectedOptions, CSS.escape()/CSS.supports(), and document.visibilityState. Gap list compiled by diffing against jsdom's own supported interfaces, scoped to what CMS embedded widget scripts actually reach for. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
onConfirm now returns Promise<boolean> through the Nitro spec (same double-Promise pattern as setFetchCallback), so a real UI interaction (e.g. an Alert.alert button press) can be awaited instead of forcing a synchronous return. Regenerated via nitrogen. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same attribute-reflection convention as disabled/required/etc from the previous round. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds .href (resolved, settable) and read-only .protocol/.hostname/ .pathname/.search/.hash/.host/.origin/etc, resolved against document.baseURI via the existing URL class. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Tracks the executing <script> element during the initial script pass, so a widget script can find its own container via document.currentScript.parentElement. LexborDocument::getScriptContents() now pairs each script with its element instead of returning bare strings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
QuickJS ships no Intl at all. Pure-JS implementation covering en/pt locale data (currency/percent/decimal formatting, date part ordering and month/weekday names), verified against real V8 Intl output. Also rewires Number/Date toLocaleString family to use it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds DOM attribute, form, CSS, URL, visibility, and Intl APIs to the QuickJS sandbox, exposes ChangesRuntime and dialog behavior
DOM platform APIs
Internationalization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LexborDocument
participant HybridHtmlSandbox
participant QuickJSRuntime
participant DocumentBindings
LexborDocument->>HybridHtmlSandbox: provide script element and contents
HybridHtmlSandbox->>QuickJSRuntime: set current_script
HybridHtmlSandbox->>QuickJSRuntime: evaluate script
DocumentBindings->>QuickJSRuntime: read document.currentScript
QuickJSRuntime-->>DocumentBindings: return current script element
HybridHtmlSandbox->>QuickJSRuntime: clear current_script
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Linker failed with undefined symbol IntlBindings::install — Android's CMakeLists.txt lists source files explicitly (unlike the iOS podspec's glob), and the new file was never added to it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
cpp/quickjs/bindings/FormBindings.cpp (1)
128-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
reset()fires the event but never restores field values.Unlike real
HTMLFormElement.reset(), this only dispatches"reset"— it doesn't restore inputs/selects to their default value/checked/selected state. That's a real gap versus spec, though implementing it fully would need default-value tracking this sandbox doesn't have (per the "no dirty-value flag" model documented at ElementBindings.cpp lines 116-121). At minimum, mirrorsubmit()'s clarifying comment so the limitation is documented rather than silent.📝 Suggested clarifying comment
+ // Per spec, reset() should restore every field to its default value/checked/ + // selected state. This sandbox has no separate "default value" tracking + // (see the value/checked reflection note above), so reset() only dispatches + // the cancelable "reset" event; field values are left untouched. Element.prototype.reset = function() { if (!isFormElement(this)) return; this.dispatchEvent(new Event('reset', { bubbles: true, cancelable: true })); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/quickjs/bindings/FormBindings.cpp` around lines 128 - 131, Update Element.prototype.reset to add a clarifying comment, matching the explanation used by submit(), that this implementation dispatches the reset event but does not restore control values because default-value tracking is unavailable in the sandbox. Leave the existing isFormElement guard and event dispatch behavior unchanged.cpp/quickjs/bindings/UrlBindings.cpp (1)
305-314: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winURL-part properties are read-only; real anchors support writing them.
protocol/hostname/port/pathname/search/hash/host/etc. are getter-only here, but realHTMLHyperlinkElementUtils(implemented by<a>/<area>) allows setting each part individually, which re-serializes back into thehrefattribute. Worth a setter pass for closer DOM parity, reusing the existingURLclass's part setters (construct aURLfrom the current href, set the part, write back viasetAttribute('href', u.href)).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/quickjs/bindings/UrlBindings.cpp` around lines 305 - 314, Update the URL-part property definitions in the binding loop to include setters for anchor elements. For each writable part, construct a URL from the current href, assign the incoming value through the existing URL class setter, and persist the serialized result with setAttribute('href', u.href); preserve the current getter behavior and read-only handling for non-anchor elements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/quickjs/bindings/FormBindings.cpp`:
- Around line 170-195: Align the selectedOptions getter with selectedIndex’s
default-selection behavior: when no option is explicitly selected, include the
first option for single-select elements, while preserving empty results for
empty or multiple-select elements as appropriate. Update the selectedOptions
definition and reuse the existing isSelectElement/selectOptionsArray logic.
In `@cpp/quickjs/bindings/IntlBindings.cpp`:
- Around line 93-99: Update the currency initialization in the Intl.NumberFormat
options setup to require an explicit currency when style is "currency" instead
of defaulting to USD, and derive default minimum and maximum fraction digits
from that currency’s minor-unit precision. Preserve the existing option
overrides and non-currency defaults, ensuring currencies such as JPY use zero
default decimals.
- Around line 209-217: Update the Date.prototype.toLocaleString,
toLocaleDateString, and toLocaleTimeString implementations to detect empty
options or options without date/time fields instead of relying on the truthiness
check. Merge each method’s appropriate date/time defaults into those options,
ensuring toLocaleString and toLocaleTimeString include their required time
fields while preserving explicitly provided option values.
- Around line 130-133: Update the dateStyle/timeStyle handling near the options
mapping in cpp/quickjs/bindings/IntlBindings.cpp:130-133 so short, medium, long,
and full each produce their distinct documented date and time component sets
rather than sharing one pattern. Update the corresponding support claim in
docs/overview.md:561-575 to match the implemented style matrix, or narrow it if
full mappings are not implemented.
---
Nitpick comments:
In `@cpp/quickjs/bindings/FormBindings.cpp`:
- Around line 128-131: Update Element.prototype.reset to add a clarifying
comment, matching the explanation used by submit(), that this implementation
dispatches the reset event but does not restore control values because
default-value tracking is unavailable in the sandbox. Leave the existing
isFormElement guard and event dispatch behavior unchanged.
In `@cpp/quickjs/bindings/UrlBindings.cpp`:
- Around line 305-314: Update the URL-part property definitions in the binding
loop to include setters for anchor elements. For each writable part, construct a
URL from the current href, assign the incoming value through the existing URL
class setter, and persist the serialized result with setAttribute('href',
u.href); preserve the current getter behavior and read-only handling for
non-anchor elements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aac89209-b60b-4966-9c70-6554726e7bb9
⛔ Files ignored due to path filters (2)
example/ios/Podfile.lockis excluded by!**/*.locknitrogen/generated/shared/c++/HybridHtmlSandboxSpec.hppis excluded by!**/generated/**
📒 Files selected for processing (23)
cpp/HybridHtmlSandbox.cppcpp/HybridHtmlSandbox.hppcpp/lexbor/LexborDocument.cppcpp/lexbor/LexborDocument.hppcpp/quickjs/DOMBindings.cppcpp/quickjs/QuickJSRuntime.hppcpp/quickjs/bindings/CSSOMBindings.cppcpp/quickjs/bindings/DocumentBindings.cppcpp/quickjs/bindings/ElementBindings.cppcpp/quickjs/bindings/FormBindings.cppcpp/quickjs/bindings/IntlBindings.cppcpp/quickjs/bindings/IntlBindings.hppcpp/quickjs/bindings/UrlBindings.cppdocs/overview.mdexample/src/__harness__/JSDOM.attributes.harness.tsexample/src/__harness__/JSDOM.cssom.harness.tsexample/src/__harness__/JSDOM.forms.harness.tsexample/src/__harness__/JSDOM.intl.harness.tsexample/src/__harness__/JSDOM.lifecycle.harness.tsexample/src/__harness__/JSDOM.url.harness.tssrc/classes/JSDOM/JSDOM.class.tssrc/classes/JSDOM/types/IJSDOMOptions.tssrc/specs/HtmlSandbox.nitro.ts
| Date.prototype.toLocaleString = function(locales, options) { | ||
| return new DateTimeFormat(locales, options || { year: 'numeric', month: 'numeric', day: 'numeric', hour: 'numeric', minute: 'numeric' }).format(this); | ||
| }; | ||
| Date.prototype.toLocaleDateString = function(locales, options) { | ||
| return new DateTimeFormat(locales, options || { year: 'numeric', month: 'numeric', day: 'numeric' }).format(this); | ||
| }; | ||
| Date.prototype.toLocaleTimeString = function(locales, options) { | ||
| return new DateTimeFormat(locales, options || { hour: 'numeric', minute: 'numeric', second: 'numeric' }).format(this); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Under ECMA-402, what are the default date/time components for Date.prototype.toLocaleString and Date.prototype.toLocaleTimeString when options is an empty object?
💡 Result:
Under the ECMA-402 specification, when the options parameter provided to Date.prototype.toLocaleString or Date.prototype.toLocaleTimeString is an empty object, the default date/time components are determined by the internal mechanics of the Intl.DateTimeFormat constructor [1][2][3]. For Date.prototype.toLocaleString, if the properties weekday, year, month, day, dayPeriod, hour, minute, second, and fractionalSecondDigits are all undefined (which is the case when an empty options object is passed), the implementation sets the following components to "numeric": year, month, day, hour, minute, and second [1][3]. For Date.prototype.toLocaleTimeString, if the properties dayPeriod, hour, minute, second, and fractionalSecondDigits are all undefined (which is the case when an empty options object is passed), the implementation sets hour, minute, and second to "numeric" [2]. In both cases, providing an empty object results in the inclusion of these specific time components with a "numeric" representation [1][2][3].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleString
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleTimeString
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global%5FObjects/Date/toLocaleString
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='cpp/quickjs/bindings/IntlBindings.cpp'
wc -l "$file"
sed -n '180,240p' "$file" | cat -nRepository: Salve-Software/react-native-nitro-jsdom
Length of output: 2638
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='cpp/quickjs/bindings/IntlBindings.cpp'
# Map the relevant symbols first
ast-grep outline "$file" --view expanded
# Then inspect the DateTimeFormat constructor and formatter logic with line numbers
python3 - <<'PY'
from pathlib import Path
p = Path('cpp/quickjs/bindings/IntlBindings.cpp')
for i, line in enumerate(p.read_text().splitlines(), 1):
if 1 <= i <= 190:
print(f"{i:4d}: {line}")
PYRepository: Salve-Software/react-native-nitro-jsdom
Length of output: 8583
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('cpp/quickjs/bindings/IntlBindings.cpp')
for i, line in enumerate(p.read_text().splitlines(), 1):
if 1 <= i <= 190:
print(f"{i:4d}: {line}")
PYRepository: Salve-Software/react-native-nitro-jsdom
Length of output: 8495
Apply method-specific defaults for empty options.
{} is truthy, so the fallback never runs. That leaves toLocaleString('en-US', {}) and toLocaleTimeString('en-US', {}) on the generic empty-object path, which only adds date defaults. Merge each method’s defaults when no date/time fields are present.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/quickjs/bindings/IntlBindings.cpp` around lines 209 - 217, Update the
Date.prototype.toLocaleString, toLocaleDateString, and toLocaleTimeString
implementations to detect empty options or options without date/time fields
instead of relying on the truthiness check. Merge each method’s appropriate
date/time defaults into those options, ensuring toLocaleString and
toLocaleTimeString include their required time fields while preserving
explicitly provided option values.
The new anchor .host getter (added for <a>/<area> URL decomposition)
made 'host' in element true for every Element, not just ShadowRoot.
CustomElementsBindings' isConnected() and ElementBindings'
getRootNode({composed}) both used that idiom to detect a shadow
boundary, so every connectedCallback/disconnectedCallback silently
stopped firing. Both now check `instanceof ShadowRoot` instead.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
selectedIndex defaulted to 0 with nothing selected even for <select multiple>, while selectedOptions stayed []. Only default to the first option for single-select, matching spec. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- style:'currency' now requires an explicit currency (throws
TypeError, matching spec) instead of silently defaulting to USD
- default fraction digits come from a small per-currency table
(JPY/KRW/etc. -> 0, BHD/KWD/etc. -> 3) instead of always 2
- dateStyle/timeStyle short/medium/long/full now map to distinct
component sets instead of collapsing to one pattern
- toLocaleString/toLocaleDateString/toLocaleTimeString apply their
own defaults for an empty {} options object, not just undefined
All verified against real V8 Intl output.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
example/src/__harness__/JSDOM.intl.harness.ts (1)
26-28: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMatch the pt-BR currency spacing to V8's NBSP. The expected
brlstring should useR$\u00A01.234,50instead of an ASCII space.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@example/src/__harness__/JSDOM.intl.harness.ts` around lines 26 - 28, Update the brl expectation in the JSDOM.intl harness to use a non-breaking space (U+00A0) between “R$” and the amount, matching V8’s pt-BR currency formatting; leave the other expected values unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@example/src/__harness__/JSDOM.intl.harness.ts`:
- Around line 74-79: Update the Intl.DateTimeFormat calls in the harness to set
timeZone to UTC, and change the long and full timeStyles expectations to include
“9:05:03 AM UTC” and “9:05:03 AM Coordinated Universal Time” while preserving
the other assertions.
---
Outside diff comments:
In `@example/src/__harness__/JSDOM.intl.harness.ts`:
- Around line 26-28: Update the brl expectation in the JSDOM.intl harness to use
a non-breaking space (U+00A0) between “R$” and the amount, matching V8’s pt-BR
currency formatting; leave the other expected values unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d670a63-a5be-4e00-9bcb-31ee89d89269
📒 Files selected for processing (7)
cpp/quickjs/bindings/CustomElementsBindings.cppcpp/quickjs/bindings/ElementBindings.cppcpp/quickjs/bindings/FormBindings.cppcpp/quickjs/bindings/IntlBindings.cppdocs/overview.mdexample/src/__harness__/JSDOM.forms.harness.tsexample/src/__harness__/JSDOM.intl.harness.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- cpp/quickjs/bindings/FormBindings.cpp
- example/src/harness/JSDOM.forms.harness.ts
- cpp/quickjs/bindings/IntlBindings.cpp
- docs/overview.md
- cpp/quickjs/bindings/ElementBindings.cpp
| timeStyles: ['short', 'medium', 'long', 'full'].map((s) => new Intl.DateTimeFormat('en-US', { timeStyle: s }).format(d)), | ||
| }); | ||
| `); | ||
| expect(JSON.parse(result)).toEqual({ | ||
| dateStyles: ['7/24/26', 'Jul 24, 2026', 'July 24, 2026', 'Friday, July 24, 2026'], | ||
| timeStyles: ['9:05 AM', '9:05:03 AM', '9:05:03 AM', '9:05:03 AM'], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
node <<'NODE'
const d = new Date(2026, 6, 24, 9, 5, 3);
for (const style of ['short', 'medium', 'long', 'full']) {
console.log(style, JSON.stringify(
new Intl.DateTimeFormat('en-US', { timeStyle: style, timeZone: 'UTC' }).format(d)
));
}
NODERepository: Salve-Software/react-native-nitro-jsdom
Length of output: 280
🏁 Script executed:
sed -n '1,140p' example/src/__harness__/JSDOM.intl.harness.tsRepository: Salve-Software/react-native-nitro-jsdom
Length of output: 6309
🏁 Script executed:
rg -n "timeStyle|timeZone|DateTimeFormat" example/src/__harness__ -SRepository: Salve-Software/react-native-nitro-jsdom
Length of output: 2708
Pin the timezone and update the long/full assertions.
timeStyle: 'long' and 'full' can include timezone names, and without timeZone the result depends on the simulator’s default timezone. With timeZone: 'UTC', these should be 9:05:03 AM UTC and 9:05:03 AM Coordinated Universal Time, not the medium output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@example/src/__harness__/JSDOM.intl.harness.ts` around lines 74 - 79, Update
the Intl.DateTimeFormat calls in the harness to set timeZone to UTC, and change
the long and full timeStyles expectations to include “9:05:03 AM UTC” and
“9:05:03 AM Coordinated Universal Time” while preserving the other assertions.
|
🎉 This PR is included in version 2.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Summary
Element(disabled/required/readOnly/multiple/autofocus/selected/hidden), plus.title/.lang/.dirform.reset(),select.options/.selectedIndex/.selectedOptionsCSS.escape()/CSS.supports(),document.visibilityState<a>/<area>.href(resolved, settable) and read-only.protocol/.hostname/.pathname/.search/.hash/.host/.origin/etc., resolved againstdocument.baseURIdocument.currentScript, tracked during the initial<script>execution passIntl.NumberFormat/Intl.DateTimeFormatpure-JS polyfill (QuickJS ships no ICU), coveringen/ptlocale data; rewiresNumber.prototype.toLocaleString/Date.prototype.toLocaleString/toLocaleDateString/toLocaleTimeStringto use itonConfirmis now awaitable end-to-end (Nitro spec → generated code →HybridHtmlSandbox→ TS), so a real UI interaction (e.g. anAlert.alertbutton press) can be awaited instead of forcing a synchronous returnGap list compiled by diffing this project against jsdom's own supported interfaces (
lib/jsdom/living/interfaces.js), scoped to what a real-world CMS embedded script (countdown timer, personalized greeting, discount badge) actually reaches for. Seedocs/overview.mdv0.17/v0.18 for full detail and documented limitations.Test plan
.cppfiles touched compile clean withclang++ -fsyntax-onlyagainst real Nitro/JSI/QuickJS/Lexbor headersIntlpolyfill output verified against real V8Intlin Node before portingtsc --noEmitclean at repo root and inexample/docs/testing.mdrequirement)yarn --cwd example pod+yarn --cwd example test:harness:ios/:androidon a real simulator (not run in this environment)🤖 Generated with Claude Code
Summary by CodeRabbit
CSS.escape()andCSS.supports().form.reset();select.options,selectedIndex, andselectedOptions.hidden,title,lang,dir, etc.), plus<a>/<area>URL component accessors viahref.Intl(Intl.NumberFormat/Intl.DateTimeFormat) with locale methods, plusdocument.currentScriptanddocument.visibilityState.onConfirmhandlers.Node.prototype.getRootNode({ composed: true })forShadowRoot.