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

fix(getSelector): Ensure nodename is eescaped #566

Closed
wants to merge 1 commit into from

Conversation

WilcoFiers
Copy link
Contributor

Closes #563

@rdeltour
Copy link
Contributor

Thanks @WilcoFiers!
Would it be possible however to use the local name instead of the escaped node name in the selector? Or with an "any namespace" type selector? On an XHTML document, the selector m\\:ci wouldn’t match whereas ci or *|ci would.

@WilcoFiers
Copy link
Contributor Author

@rdeltour What's the the use case for that? When would the same system want to change the namespace, even though the element would be the same. I think it's a little clearer what element you're supposed to look at with the namespace specified, no?

@rdeltour
Copy link
Contributor

What's the the use case for that?

The use case is to make selectors match the element in an XHTML context, such as in EPUB.

Let me try to clarify. This has to deal with whether the element has a namespace or not in the DOM, which depends on how the document was parsed (for instance, as XML or as HTML).

Consider this example HTML document:

<!doctype html>
<title>Example</title>
<meta charset="utf-8" />
<h1>Hi</h1>
<math xlink:href="#foo"></math>
<div xlink:href="#foo"></div>
<foo:bar></foo:bar>

It's HTML, and parsed as such by a browser. On this document:

  • document.querySelectorAll('[xlink\\:href]') will return the div element
  • document.querySelectorAll('[*|href]') will return the math element
  • document.querySelectorAll('foo\\:bar') will return the foo:bar element

It's all in the rules defined by the HTML parsing algorithm. For instance, when the parser encounters an xlink:href attribute on an SVG or MathML element, it adjusts it to put it in the XLink Namespace.

Now, consider this XHTML document:

<html xmlns="http://www.w3.org/1999/xhtml" xmlns:m="http://www.w3.org/1998/Math/MathML" xml:lang="en" lang="en">
<head>
  <meta charset="utf-8" />
  <title>Example</title>
</head>
<body>
<h1>Hi</h1>
<m:math></m:math>
</body>
</html>

If parsed as XHTML (for instance, put it in a file with the '.xhtml' extension and open it in a browser, or serve it with the application/xhtml+xml media type, then:

  • document.querySelectorAll('m\\:math) is empty
  • document.querySelectorAll('math) contains the m:math element
  • document.querySelectorAll('*|math) contains the m:math element

Again, the reason lies in the parsing algorithm. The MathML namespace is known and defined, so the local name is just "math" and the namespace is "http://www.w3.org/1998/Math/MathML".

So, what should aXe do (ideally)?

The easy approach would be to create *|name selectors for prefixed element names. That would probably work in most of the cases, since prefixed foreign names are very rarely found in HTML, and it would work in an XHTML context.
This is assuming that you're only creating type selectors (i.e. for elements). If you're also creating attribute selectors, you'd have to make cases depending on foreign attribute adjustments.

A more correct approach would be to define the selector based on the value of document.contentType. If it is text/html, the selector would be prefix\\:localname; if it is application/xhtml+xml, the selector would be *|localname.

Voilà. I hope it makes sense :-)
I'm happy to help with the PR if needed (btw, is there a way to run a single test in axe without having to run everything with grunt test? I couldn't find it at first sight, and it's not in the docs either).

@WilcoFiers
Copy link
Contributor Author

Makes sense, and I learned something new today :D. Thanks Romain. @dylanb What do you think?

Also, yes you can run specific tests by doing --grep="test name".

@dylanb
Copy link
Contributor

dylanb commented Oct 18, 2017

I think we should implement option 2 because it will only change selectors for XHTML documents which is less disruptive to our users

@WilcoFiers
Copy link
Contributor Author

Agreed. Any idea how to detect what parser was used on a page? I want to avoid changing the selectors as much as possible. If we can make it so that localName is used in xml documents, and nodeName in all other cases, that'd be ideal. Do you want to take a stab at updating this PR @rdeltour ?

@rdeltour
Copy link
Contributor

Agreed. Any idea how to detect what parser was used on a page?

I think we can look at document.contentType

Do you want to take a stab at updating this PR @rdeltour ?

Sure, I can try in the coming days (end of next week at the latest).

rdeltour pushed a commit to rdeltour/axe-core that referenced this pull request Oct 24, 2017
- by default, ensure the nodename is escaped
- for XHTML documents, only use the local name

Replaces dequelabs#566
Closes dequelabs#563
@marcysutton
Copy link
Contributor

I saw a real-world case of invalid/namespaced HTML tags rendered from the server today, and they took down axe-core and our extensions. Here's an example:

<search:facet-list></search:facet-list>

I have a question out as to whether this was an XHTML or HTML5 doctype, but I'd be curious to see if namespaced HTML tags in a regular document like this cause any problems for assistive technologies. If they do cause problems, it might be a good thing for axe-core to flag.

@rdeltour
Copy link
Contributor

rdeltour commented Oct 25, 2017

I'd be curious to see if namespaced HTML tags in a regular document like this cause any problems for assistive technologies. If they do cause problems, it might be a good thing for axe-core to flag.

Good question. To be honest I have no idea, but I suspect most ATs would just ignore it (as HTML parsers do).
If they do have a problem to process them, yes it should certainly be flagged! (using a specific rule rather than the current syntax error of course).

@WilcoFiers
Copy link
Contributor Author

PR superseded by #582

@WilcoFiers WilcoFiers closed this Nov 14, 2017
@WilcoFiers WilcoFiers deleted the selector-escape branch November 14, 2017 12:34
rdeltour pushed a commit to rdeltour/axe-core that referenced this pull request Dec 15, 2017
- by default, ensure the nodename is escaped
- for XHTML documents, only use the local name

Replaces dequelabs#566
Closes dequelabs#563
rdeltour pushed a commit to rdeltour/axe-core that referenced this pull request Dec 15, 2017
- by default, ensure the nodename is escaped
- for XHTML documents, only use the local name

Replaces dequelabs#566
Closes dequelabs#563
WilcoFiers pushed a commit that referenced this pull request Dec 15, 2017
)

* feat(utils): add function `isXHTML` to test if a document node is XHTML

* test(utils): add a test for `axe.utils.isXHTML` on an XHTML document

* fix(getSelector): improve selectors for namespaced elements

- by default, ensure the nodename is escaped
- for XHTML documents, only use the local name

Replaces #566
Closes #563

* test(getSelector): add a test for `axe.utils.getSelector` on a namespaced XHTML element
WilcoFiers pushed a commit that referenced this pull request Dec 15, 2017
)

* feat(utils): add function `isXHTML` to test if a document node is XHTML

* test(utils): add a test for `axe.utils.isXHTML` on an XHTML document

* fix(getSelector): improve selectors for namespaced elements

- by default, ensure the nodename is escaped
- for XHTML documents, only use the local name

Replaces #566
Closes #563

* test(getSelector): add a test for `axe.utils.getSelector` on a namespaced XHTML element
mrtnvh pushed a commit to mrtnvh/axe-core that referenced this pull request Nov 24, 2023
Co-authored-by: Steven Lambert <2433219+straker@users.noreply.github.com>
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.

None yet

4 participants