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

feat(object-alt,accessible-text): object-alt rule and accessible text to work with serial virtual nodes with children #2391

Merged
merged 7 commits into from
Jul 17, 2020

Conversation

straker
Copy link
Contributor

@straker straker commented Jul 16, 2020

There is a lot going on here but it was all necessary to get a serial virtual node with children to run through the accessbile-text functions. This allows object-alt and other rules that look at the subtree text to pass/fail with a full descendant tree (before it would just incomplete).

var node = new axe.SerialVirtualNode({
  nodeName: 'object'
});
var child = new axe.SerialVirtualNode({
  nodeName: '#text',
  nodeType: 3,
  textContent: 'foobar'
});
node.children.push(child);

var results = axe.runVirtualRule('object-alt', node);  // passes

We decided that if a virtual node does not have an actualNode property, we will always consider it visible so we don't have to enter the isVisible function which checks visibility with CSS.

We also needed serial virtual nodes to have text children, so they needed to accept nodeType of 3.

Note: I wanted to use the html-elm spec to know if the element was as phrasing element rather than keep that hardcoded list. Unfortunately our hardcoded list is not a complete list and so something like and input, which is a phrasing element, would no longer add space between the label text and the input value such as this acceptance test:

<label for="test">
  foo
  <input type="text" value="David"/>
</label>

Which results in fooDavid rather than foo David. So I had to leave it in, but had to lowercase the names, and also decided to sort the list while I was there (much easier to compare against the list in the spec).

This also lead to discovering some of the html-elm spec was incorrect, like kbd being misspelled and p and summary being phrasing elements when they should not have been. Also made namingMethods an array because it made more sense than being a string or array.

Part of issue #2183

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Code is reviewed for security

… and accessible text to work with serial virtual nodes with children. deprecate native-element-type
@straker straker requested a review from a team as a code owner July 16, 2020 22:13
lib/commons/text/accessible-text-virtual.js Outdated Show resolved Hide resolved
lib/commons/text/accessible-text-virtual.js Outdated Show resolved Hide resolved
lib/core/base/virtual-node/virtual-node.js Outdated Show resolved Hide resolved
lib/core/base/virtual-node/virtual-node.js Outdated Show resolved Hide resolved
}
}
},
ins: {
contentTypes: ['phrasing', 'flow'],
allowedRoles: true
},
kdb: {
kbd: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof! Good catch.

@@ -811,10 +811,9 @@ const htmlElms = {
allowedRoles: true
},
summary: {
contentTypes: ['phrasing', 'flow'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

@straker straker Jul 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like kdb misspelling, it's a correct to the file as 'summary' is not phrasing or flow content.

},
p: {
contentTypes: ['phrasing', 'flow'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like kdb misspelling, it's a correct to the file as 'p' is not phrasing content.

lib/commons/text/native-element-type.js Outdated Show resolved Hide resolved
lib/commons/text/native-text-alternative.js Show resolved Hide resolved

// Use concat because namingMethods can be a string or an array of strings
const methods = nativeType ? [].concat(nativeType.namingMethods) : [];
const elmSpec = getElementSpec(virtualNode);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this utility. This is so useful!

@straker straker changed the title feat(object-alt,accessible-text,native-element-type): object-alt rule and accessible text to work with serial virtual nodes with children. deprecate native-element-type feat(object-alt,accessible-text): object-alt rule and accessible text to work with serial virtual nodes with children Jul 17, 2020
@straker straker merged commit e8e17e4 into develop Jul 17, 2020
@straker straker deleted the object-alt branch July 17, 2020 14:31
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