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: Fix CSS tag seletor case sensitivity for SVG elements #3189

Merged

Conversation

Psychpsyo
Copy link
Contributor

@Psychpsyo Psychpsyo commented Jan 8, 2025

For things like tag names, the CSS spec defers whether or not they are case-sensitive to the specifications these names come from.
In HTML, tag names are case-insensitive, but in SVG they are case-sensitive.
I'm not sure which of the two MathML falls under, but I also don't think we currently support that.

The test uses foreignObject, cause selecting those by tag name was impossible before this fix.

@Psychpsyo Psychpsyo requested a review from AtkinsSJ as a code owner January 8, 2025 22:20
@Psychpsyo Psychpsyo force-pushed the css-selector-svg-case-sensitivity branch from 5541768 to 0c3ddae Compare January 8, 2025 22:34
@AtkinsSJ
Copy link
Member

AtkinsSJ commented Jan 9, 2025

From what I can find, anything based on XML is case sensitive. So SVG, MathML and XHTML all are. I don't know if we currently have a way to distinguish XHTML.

@Psychpsyo Psychpsyo force-pushed the css-selector-svg-case-sensitivity branch from 0c3ddae to 95819b1 Compare January 9, 2025 11:18
@Psychpsyo
Copy link
Contributor Author

From what I can find, anything based on XML is case sensitive. So SVG, MathML and XHTML all are. I don't know if we currently have a way to distinguish XHTML.

We have, it is element.document().document_type() which returns either DOM::Document::Type::HTML or DOM::Document::Type::XML.
Of course, this only works in the whole-document case, but I don't think it is possible to nest XHTML documents in HTML ones, like with SVG, so that should be fine.

@Psychpsyo
Copy link
Contributor Author

Psychpsyo commented Jan 9, 2025

By which I mean "Yes, good point. Fixed the PR to take that into account."

EDIT: actually do not merge this yet, I found the relevant bit of HTML spec that clarifies this.

Ok, this should now be ready to merge, and with more tests.

They can appear in HTML documents but must not be matched
according to the HTML spec.
@Psychpsyo Psychpsyo force-pushed the css-selector-svg-case-sensitivity branch from 95819b1 to d09c1b5 Compare January 9, 2025 19:45
@AtkinsSJ AtkinsSJ merged commit bb5d1ac into LadybirdBrowser:master Jan 13, 2025
8 checks passed
@AtkinsSJ
Copy link
Member

EDIT: actually do not merge this yet, I found the relevant bit of HTML spec that clarifies this.

For reference, you can turn a PR into a draft, or back again, which makes it really clear that it shouldn't be merged. (Because it disables the merge button!)

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.

2 participants