Handle documents without a root element without panicking - #655
Open
nicoburns wants to merge 1 commit into
Open
Conversation
Iframe sub-documents parsed by the default DummyHtmlParserProvider (and other childless documents such as about:blank) have no root element. Touching such a document's viewport panicked in scroll_viewport_by via BaseDocument::root_element()'s unwrap, and painting or dispatching events to one panicked the same way. Treat a document without a root element as having zero scrollable content, nothing to paint, and no event target.
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
BaseDocument::root_element()unwrapsfirst_element_child(), which panics for documents with no root element. Such documents arise whenever an<iframe>sub-document is parsed by the defaultDummyHtmlParserProvider(whoseparse_documentreturns an emptyPlainDocument— this is what happens in the WPT runner, which doesn't pass anhtml_parser_provider), and can also arise for genuinely childless documents (e.g. an empty XML document).The panic fired in
resolve(): the parent document sets the sub-document's viewport viaviewport_mut(), andViewportMut::dropcallsscroll_viewport_by(0.0, 0.0)to clamp the scroll offset, which calledroot_element()unconditionally.Three call sites are made to explicitly handle the no-root-element case, where the correct behaviour is "there is nothing to do":
BaseDocument::scroll_viewport_by_has_changed: a document with no root element has zero scrollable content, so the scroll offset clamps to(0, 0):BlitzDomPainter::paint_scene: nothing to paint — return early viatry_root_element()EventDriver::handle_ui_event: no fallback event target — return early instead oftarget.unwrap_or_else(|| ... root_element().id)Fixes 113 of the 244 CRASHing tests in the WPT css suite (244 → 131 CRASH; PASS 11021 → 11065; no test or subtest that previously passed regresses).
Note: a follow-up improvement would be for the WPT runner (and/or
HtmlDocument::from_html) to defaulthtml_parser_providerto blitz-html'sHtmlProvider, so iframe sub-documents actually parse their HTML instead of being empty.Link to Devin session: https://dioxus.staging.devinenterprise.com/sessions/5cb0957e4dae408aaf1d7c1860935027
Requested by: @nicoburns
WPT results
44 newly passing, 0 newly failing (net +44), 69 other status changes.
Full diff (113 changed tests)
Generated by the WPT workflow.