Fix `getSelector` for namespaced elements + introduce XHTML fixtures #582

rdeltour · 2017-10-24T23:57:51Z

Replaces #566
Closes #563

rdeltour · 2017-10-25T00:09:37Z

Not quite sure what I should do about the codacy checks, I tried to follow the code style from the other files. Please LMK if you want me to update the PR!

marcysutton · 2017-10-25T00:11:28Z

Thanks @rdeltour! The codacy thing was just added but it conflicts with our jshint rules, so you can probably disregard for now.

WilcoFiers · 2017-10-25T11:10:35Z

Apologies. I meant to delete the Codacy hook but I hadn't done so yet. Shouldn't turn up on new commits anymore.

marcysutton · 2017-10-25T23:48:58Z

test/core/utils/is-xhtml.js

+	});
+
+	it('should return true on any document that is XHTML', function () {
+		var doc = document.implementation.createDocument('http://www.w3.org/1999/xhtml', 'html', null);


I had no idea this interface existed.

marcysutton · 2017-10-25T23:50:32Z

test/xhtml/core/utils/is-xhtml.js

@@ -0,0 +1,7 @@
+describe('axe.utils.isXHTML', function () {


I don't think you need this file, it's redundant with test/core/utils/is-xhtml.js.

see explanation in the general comment below

specifically, this will test that the document from the current fixture is XHTML. I copied the pattern from aXe's pre-existing is-html5.js tests

marcysutton · 2017-10-25T23:52:34Z

test/xhtml/core/utils/get-selector.js

@@ -0,0 +1,24 @@
+
+describe('axe.utils.getSelector', function () {


It's a little strange to have a test for get-selector duplicated in another directory. Can you make it work in the existing core unit tests by creating a document like in test/core/utils/is-xhtml.js?

see explanation in the general comment below

This makes sense, although I'd suggest we move it to tests/integrations instead.

rdeltour · 2017-10-26T07:07:21Z

Thank you @marcysutton for the review!

re:

It's a little strange to have a test for get-selector duplicated in another directory. Can you make it work in the existing core unit tests by creating a document like in test/core/utils/is-xhtml.js?

and

I don't think you need this file, it's redundant with test/core/utils/is-xhtml.js.

let me try to explain why I did this:

As far as I understand, all aXe’s tests are run in an HTML fixture, injected in the runner.tmpl template. It means that in those tests, the DOM is always an HTML document.

But for some tests, I needed to be in an XHTML context. So, I created another fixture. All the tests in the xhtml directory are intended to be run in an XHTML fixture. That's why I also created a new template in test/xhtml/runner.tmpl, and added it to the test config.

The idea is that the xhtml directory only contains the tests specific to XHTML particularities, and the idea was intentionally to duplicate the structure of other tests.

For instance, if test/core/utils/is-xhtml.js contains the usual tests, run in an HTML fixture, and test/xhtml/core/utils/is-xhtml.js contains only the tests for is-xhtml that depend on an XHTML environment.

If the XHTML-specific tests were in the same directories as their HTML counterparts, they'd need an option to be ignored in the regular HTML-fixture-based tests, and enabled in the XHTML-fixture-based tests. I thought for now it would be clearer to just have to separate directory structures.

I hope it's clear 😅

Alternatively, another option would actually make aXe run all the existing tests both in HTML and XHTML environments. That would provide better coverage but would basically duplicate the testing time, so I thought that would be a bit aggressive.
This option would likely require to update build/task/fixture.js to generate both an HTML and XHTML fixtures from templates, and build/test/config.js to run them. (As far as I can see, I didn't dig very deep).

WilcoFiers · 2017-10-28T15:27:29Z

lib/core/utils/get-selector.js

@@ -130,7 +129,9 @@ const createSelector = {
 * recognize the element by (IDs, aria roles, custom element names, etc.)
 */
 function getElmFeatures (elm, featureCount) {
-  const nodeName = elm.nodeName.toLowerCase();
+  const nodeName = escapeSelector(axe.utils.isXHTML(document)?


We could probably just check this once at the top of the file and keep the boolean around instead of having to call this every time this method runs.

We could probably just check this once at the top of the file and keep the boolean around

where would be the best place to memoïze the boolean value? Using a const at the top of the file won't work.
Calling the function shouldn't be too costly in any case, so maybe it's too early an optimization?

This could potentially create many thousands new elements, so I do think this is relevant. Why wouldn't a const up top work? Alternative the result can be stored in utils.isXHTML itself.

Why wouldn't a const up top work?

I tried and got an error when running tests :/ But I haven't tried hard nor dug deep into the issue.

@rdeltour Can you update this according what we discussed at TPAC?

WilcoFiers · 2017-10-28T15:33:50Z

lib/core/utils/is-xhtml.js

+	if (!doc.createElement) {
+		return false;
+	}
+	return doc.createElement('A').localName === 'A';


What does this do on JSDOM? We should make sure we don't accidentally treat JSDOM as XHTML.

Yes, confirmed to work on JSDOM.

WilcoFiers · 2017-10-28T15:36:59Z

test/core/utils/is-xhtml.js

@@ -0,0 +1,21 @@
+describe('axe.utils.isXHTML', function () {


@rdeltour Have you tested this with Edge and IE? This seems like one of those things that might not work across the board :)

Confirmed to work on Edge and IE too.
The logic relies on the fact that HTML parsers lower-case the tag names when parsing. It's been suggested as a possible reliable option by Anne van Kesteren on Twitter.

WilcoFiers · 2017-10-28T15:39:13Z

test/xhtml/core/utils/get-selector.js

@@ -0,0 +1,24 @@
+
+describe('axe.utils.getSelector', function () {


This makes sense, although I'd suggest we move it to tests/integrations instead.

WilcoFiers · 2017-10-28T15:42:21Z

Gruntfile.js

@@ -281,6 +281,21 @@ module.exports = function (grunt) {
 					}
 				}
 			},
+			xhtml: {


I think it would be easier to move this xhtml test into tests/integrations. That way we don't have to do all this scaffolding work. WDYT?

these are not strictly speaking integration tests, but if you feel that will fit better there (and disturb less the code organization), sure! let me know.

Yep, I think so.

ok, I'll try to update the PR accordingly (I'm currently on vacation)

WilcoFiers · 2017-12-15T11:03:58Z

@rdeltour Any updates on this PR?

rdeltour · 2017-12-15T11:20:41Z

@rdeltour Any updates on this PR?

I was just working on it, in fact 😀. If time allows I will update it this afternoon.

WilcoFiers · 2017-12-15T11:37:13Z

@rdeltour That's good to know. We are planning to release 2.6 today and I'd like to have this in if we could. Otherwise it'll have to wait for 3.0.

- by default, ensure the nodename is escaped - for XHTML documents, only use the local name Replaces dequelabs#566 Closes dequelabs#563

…aced XHTML element

rdeltour · 2017-12-15T13:02:11Z

@WilcoFiers ready to be reviewed!

) * 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

…s/shadow-dom (dequelabs#582) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

marcysutton reviewed Oct 25, 2017

View reviewed changes

WilcoFiers reviewed Oct 28, 2017

View reviewed changes

WilcoFiers mentioned this pull request Nov 14, 2017

fix(getSelector): Ensure nodename is eescaped #566

Closed

WilcoFiers added the waiting on feedback/tests label Nov 15, 2017

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

701f72f

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

d6dab39

rdeltour force-pushed the fix/selector-namespace branch from 936184f to 083b701 Compare December 15, 2017 12:43

WilcoFiers and others added 2 commits December 15, 2017 13:50

fix(getSelector): improve selectors for namespaced elements

0215713

- by default, ensure the nodename is escaped - for XHTML documents, only use the local name Replaces dequelabs#566 Closes dequelabs#563

test(getSelector): add a test for axe.utils.getSelector on a namesp…

69a3950

…aced XHTML element

rdeltour force-pushed the fix/selector-namespace branch from 083b701 to 69a3950 Compare December 15, 2017 12:51

WilcoFiers approved these changes Dec 15, 2017

View reviewed changes

WilcoFiers merged commit 9e3ca45 into dequelabs:develop Dec 15, 2017

dependencies bot mentioned this pull request Dec 19, 2017

Update 10 dependencies from npm storybookjs/storybook#2520

Merged

mrtnvh pushed a commit to mrtnvh/axe-core that referenced this pull request Nov 24, 2023

chore(deps): bump minimist and http-server in /packages/react/example…

f34873f

…s/shadow-dom (dequelabs#582) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `getSelector` for namespaced elements + introduce XHTML fixtures #582

Fix `getSelector` for namespaced elements + introduce XHTML fixtures #582

rdeltour commented Oct 24, 2017

rdeltour commented Oct 25, 2017 •

edited

Loading

marcysutton commented Oct 25, 2017

WilcoFiers commented Oct 25, 2017

marcysutton Oct 25, 2017

marcysutton Oct 25, 2017

rdeltour Oct 26, 2017 •

edited

Loading

rdeltour Oct 26, 2017

marcysutton Oct 25, 2017

rdeltour Oct 26, 2017 •

edited

Loading

WilcoFiers Oct 28, 2017

rdeltour commented Oct 26, 2017 •

edited

Loading

WilcoFiers Oct 28, 2017

rdeltour Oct 28, 2017 •

edited

Loading

WilcoFiers Oct 29, 2017

rdeltour Oct 31, 2017

WilcoFiers Nov 14, 2017

WilcoFiers Oct 28, 2017

rdeltour Oct 28, 2017

WilcoFiers Oct 28, 2017

rdeltour Oct 28, 2017

WilcoFiers Oct 28, 2017

WilcoFiers Oct 28, 2017

rdeltour Oct 28, 2017

WilcoFiers Oct 29, 2017

rdeltour Oct 31, 2017

WilcoFiers commented Dec 15, 2017

rdeltour commented Dec 15, 2017

WilcoFiers commented Dec 15, 2017

rdeltour commented Dec 15, 2017

		@@ -0,0 +1,24 @@

		describe('axe.utils.getSelector', function () {

Fix getSelector for namespaced elements + introduce XHTML fixtures #582

Fix getSelector for namespaced elements + introduce XHTML fixtures #582

Conversation

rdeltour commented Oct 24, 2017

rdeltour commented Oct 25, 2017 • edited Loading

marcysutton commented Oct 25, 2017

WilcoFiers commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdeltour Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdeltour Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdeltour commented Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

rdeltour Oct 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilcoFiers commented Dec 15, 2017

rdeltour commented Dec 15, 2017

WilcoFiers commented Dec 15, 2017

rdeltour commented Dec 15, 2017

Fix `getSelector` for namespaced elements + introduce XHTML fixtures #582

Fix `getSelector` for namespaced elements + introduce XHTML fixtures #582

rdeltour commented Oct 25, 2017 •

edited

Loading

rdeltour Oct 26, 2017 •

edited

Loading

rdeltour Oct 26, 2017 •

edited

Loading

rdeltour commented Oct 26, 2017 •

edited

Loading

rdeltour Oct 28, 2017 •

edited

Loading