feat(html): flesh out the browser environment (navigator, screen, performance, crypto, iframe) - #274
feat(html): flesh out the browser environment (navigator, screen, performance, crypto, iframe)#274Sonic-Y3k wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 9 minutes and 44 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
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. Comment |
…formance, crypto, iframe, ...) Adds commonly-read parts of the browser environment that scripts feature-detect or branch on: - a populated navigator (userAgent, platform, languages, hardwareConcurrency, webdriver=false, userAgentData, ...) - screen, performance (now/timeOrigin) and crypto (getRandomValues/randomUUID), implemented natively - window.top/frames/length for a top-level context (previously threw) - document.compatMode/characterSet/cookie/title/visibilityState/hidden/hasFocus, and a real document.readyState that is "loading" during parsing and "complete" afterwards - the legacy Document.createEvent factory plus initEvent/initMouseEvent/... - HTMLIFrameElement.contentWindow / contentDocument
965d963 to
2e74f2a
Compare
|
Hi. There's a lot going on here, and as I mentioned in another PR, something in scope - but some things are out of scope. As I mentioned in another PR comment, I want to keep this repo as lean as possible. However, there is a path to support the functionality that is out of scope. I have only briefly skimmed the code, and description; so this may not be a full review, but I hope it can spawn a constructive discussion :) iframe supportiframe support is definitely in scope; but this has enough complexity to a warrant dedicated PR by itself. I haven't used iframes for 15-ish years, so my knowledge how a document and iframe documents can interact is very dated. I assume run scripts in a separate JavaScript realm, and there some message passing capabilities between the two - like workers. As I have skeleton worker support with message passing capabilities and a shared clock, i.e., all workers run in with the same simulated clock. I haven't merged yet, at I'm not happy with the implementation; but maybe I should, so there's a foundation for message passing between different realms. Having said that, I will be happy releasing preliminary iframe support without full script isolation and message passing. But I want to have a clear knowledge about what the level of compliance is, what works as it should, and what is lacking. User-agent properties on the Navigator objectFirst of all, which problem is this solving? Is it solving a real problem; or was it mostly just a low-hanging fruit for making it more specs-compliant? The intended use case is to provide a useful tool for testing modern web applications. Does you client code actually depend on what the navigator reports? If yes, I'd go for a solution where client code can provide these options; e.g., as options to But it's something I'd like to avoid in the core browser module.
|
|
Thanks for the detailed feedback - agreed on all of it. Rather than reshape this
|
Adds commonly-read parts of the browser environment that scripts feature-detect or branch on:
navigator(userAgent,platform,languages,hardwareConcurrency,webdriver === false,userAgentData, …)screen,performance(now/timeOrigin) andcrypto(getRandomValues/randomUUID), implemented nativelywindow.top/frames/lengthfor a top-level context (previously threw)document.compatMode/characterSet/cookie/title/visibilityState/hidden/hasFocus, and a realdocument.readyStatethat is"loading"during parsing and"complete"afterwardsDocument.createEventfactory plusinitEvent/initMouseEvent/…HTMLIFrameElement.contentWindow/contentDocumentNote:
contentWindowresolves to the owning window's realm rather than a separate same-origin realm — the bundledgost-dom/v8gobuild doesn't expose security-token control, so a child context's globals aren't cross-realm accessible (V8 raises "no access"). Native built-ins oncontentWindoware accessible and genuine; only object identity differs. Happy to revisit if a separate realm is preferred.Testing: added
TestEnvFidelity; the existing htmx integration suite and full test suite stay green.document.readyStateis backed by the real load state specifically so script-timing-sensitive consumers (e.g. htmx) keep working.AI disclosure: This change was developed with the help of an AI coding assistant. I've reviewed and tested it myself; it follows the existing conventions and the full test suite (main module,
v8engine,sobekengine) passes locally.