Skip to content

LibWeb: Return base Document for non-HTML parses #5557

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 25, 2025

Conversation

mikiubo
Copy link
Contributor

@mikiubo mikiubo commented Jul 22, 2025

The HTML specification does not explicitly require a specific return type for parseFromString(),
but according to Web Platform TestsDOMParser-parseFromString.html, the expected return value for
XML MIME types is a Document—not an XMLDocument.

@ADKaster
Copy link
Member

ADKaster commented Jul 22, 2025

This change causes at least Tests/LibWeb/Text/input/DOM/DOMParser-xml-document.html to fail. Could you run your branch against all of the dom/ wpt tests to see what's going on here? It seems like either our local test is wrong, or there's some inconsistency we're papering over somewhere else.

./Meta/WPT.sh run dom should do the trick (modulo extra args)

@mikiubo
Copy link
Contributor Author

mikiubo commented Jul 22, 2025

This change causes at least Tests/LibWeb/Text/input/DOM/DOMParser-xml-document.html to fail. Could you run your branch against all of the dom/ wpt tests to see what's going on here? It seems like either our local test is wrong, or there's some inconsistency we're papering over somewhere else.

./Meta/WPT.sh run dom should do the trick (modulo extra args)

this is the run result:
WithDocument

i also run without the commit:
WithXmlDocument

I add one observation:
all major browser fail this wpt test https://wpt.fyi/results/domparsing/DOMParser-parseFromString-xml.html
but for example flow 7.0 pass it. So i think there is something strange about the test.
Also there is a discussion on chrome about it:
https://issues.chromium.org/issues/41305457

let me know
thanks

@tcl3
Copy link
Member

tcl3 commented Jul 22, 2025

I had a quick look and there's no regressions in either dom or domparsing.
Looking at the WPT results for the imported test, it looks like all major browsers return an XMLDocument rather than Document from parseFromString in this case, which disagrees with the test expectations.

I'm not sure whether the test is wrong or not, but if the imported test is correct then the failing test simply needs to be rebaselined, as it's testing exactly what you've changed in this PR.

The HTML specification does not explicitly require
a specific return type for parseFromString(),
but according to Web Platform TestsDOMParser-parseFromString.html,
the expected return value for
XML MIME types is a Document—not an XMLDocument.
@mikiubo
Copy link
Contributor Author

mikiubo commented Jul 22, 2025

Ok i change the local test.
Let me know if you need anything else.
Thanks

PS: when i run ./Meta/WPT.sh run dom against the main branch i got the result above.
It is a problem of my local config or is normal?

@tcl3
Copy link
Member

tcl3 commented Jul 22, 2025

PS: when i run ./Meta/WPT.sh run dom against the main branch i got the result above.
It is a problem of my local config or is normal?

The results you got are what I would expect. To compare WPT results this is what I did:

> git checkout master
> ./Meta/WPT.sh run --log results.log dom domparsing
> git checkout <this-pr-branch>
> ./Meta/WPT.sh compare results.log dom domparsing

@mikiubo
Copy link
Contributor Author

mikiubo commented Jul 23, 2025

The results you got are what I would expect. To compare WPT results this is what I did:

> git checkout master
> ./Meta/WPT.sh run --log results.log dom domparsing
> git checkout <this-pr-branch>
> ./Meta/WPT.sh compare results.log dom domparsing

Thank you.

@ADKaster ADKaster merged commit acf1fe7 into LadybirdBrowser:master Jul 25, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants