Skip to content
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

LibWeb: Change IDL attribute type to USVString where applicable #1059

Merged
merged 2 commits into from
Aug 17, 2024

Conversation

tcl3
Copy link
Member

@tcl3 tcl3 commented Aug 13, 2024

This PR changes some IDL attribute return types to USVString, where the specification says that's what should be used and updates the bindings generator to ensure that reflected USVString attributes behave correctly.

USVString attributes that represent a URL are now explicitly marked with the URL extended attribute, as the specification says that they are supposed to behave differently to attributes that don't represent a URL.

Fixes 5 subtests on: http://wpt.live/html/dom/usvstring-reflection.https.html

@awesomekling
Copy link
Member

Reflected USVString attributes now have any surrogates with U+FFFD and any USVString attributes that are URLs are now percent encoded.

I'm struggling to parse this. :thonk:

@tcl3 tcl3 force-pushed the libweb_usvstring_attributes branch from b8eba4b to e539cb4 Compare August 13, 2024 13:09
@tcl3
Copy link
Member Author

tcl3 commented Aug 13, 2024

I'm struggling to parse this. :thonk:

I've got no idea how I mangled that so badly! Hopefully, it should now make sense.

I also found a few more places where USVString should be used, so I've updated those places too. I had to update a few tests due to these changes. The test expectations now match what is generated by other browsers in these cases.

@tcl3 tcl3 force-pushed the libweb_usvstring_attributes branch 3 times, most recently from 377b661 to df1c985 Compare August 13, 2024 14:46
@jamierocks
Copy link
Contributor

Fixes 5 subtests on: http://wpt.live/html/dom/usvstring-reflection.https.html

This probably fixes more tests - I've noticed quite a few issues related to attributes that are URLs :)

USVString attributes Now replace any surrogates with the replacement
character U+FFFD and resolve any relative URLs to an absolute URL. This
brings our implementation in line with the specification.
@tcl3 tcl3 force-pushed the libweb_usvstring_attributes branch from df1c985 to 3183f9b Compare August 14, 2024 21:46
@tcl3
Copy link
Member Author

tcl3 commented Aug 14, 2024

I've done some more testing on this and the set of USVString attributes that are marked as containing a URL seems to vary across browsers. I've updated the PR so that we only mark an attribute as containing a URL if that's what's done by all the major engines.

Also mark USVString attributes as containing a URL, where applicable.
@tcl3 tcl3 force-pushed the libweb_usvstring_attributes branch from 3183f9b to 256f9b4 Compare August 14, 2024 22:17
@awesomekling awesomekling merged commit 1369fc5 into LadybirdBrowser:master Aug 17, 2024
6 checks passed
@tcl3 tcl3 deleted the libweb_usvstring_attributes branch August 17, 2024 09:44
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.

4 participants