[webkit-reviews] review denied: [Bug 194005] AX: Audit tab should have built-in accessibility tests. : [Attachment 360601] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jan 30 13:12:21 PST 2019
Devin Rousso <drousso at apple.com> has denied Aaron Chu <aaron_chu at apple.com>'s
request for review:
Bug 194005: AX: Audit tab should have built-in accessibility tests.
https://bugs.webkit.org/show_bug.cgi?id=194005
Attachment 360601: Patch
https://bugs.webkit.org/attachment.cgi?id=360601&action=review
--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 360601
--> https://bugs.webkit.org/attachment.cgi?id=360601
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=360601&action=review
r-, as I think this needs to be much "cleaner" to review
> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:109
> +localizedStrings["An accessibility test suite that uses WebCore for
testing."] = "An accessibility test suite that uses WebCore for testing.";
This isn't really a good name, as I'd imagine most web developers don't even
know what WebCore is. The description can be more of "this is what this is
_and_ how it's useful", as there's no "limit" (although space is somewhat
constrained) as to how much information/explanation we provide.
> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:387
> +localizedStrings["Ensure `aria-labelledby` is spelled correctly."] = "Ensure
`aria-labelledby` is spelled correctly.";
We typically use "\u201C" (LEFT DOUBLE QUOTATION MARK) and "\u201D" (RIGHT
DOUBLE QUOTATION MARK) for quotes in localized strings. You could also
alternatively use \u022 (QUOTATION MARK), but that's a little less "stylistic".
> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:955
> +localizedStrings["Test for Buttons with labels."] = "Test for Buttons with
labels.";
Should "Button" be capitalized?
> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:960
> +localizedStrings["Test that all Dialogs have aria-modal set to true."] =
"Test that all Dialogs have aria-modal set to true.";
Should "Dialogs" be capitalized?
> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:324
> + new WI.AuditTestCase(`testMenuRoleForRequiredChidren`,
`async function() {let domNodes = [];const Relationships = {"menu":
["menuitem", "menuitemcheckbox", "menuitemradio"],"menubar": ["menuitem",
"menuitemcheckbox", "menuitemradio"]};let t = (function(){let r = function() {
let audit = WebInspectorAudit.Accessibility; function init(relationships,
domNodes) {let visitedParents = [];if (!relationships) { return
console.error("Missing Relationship Mapping.");};if (!Array.isArray(domNodes))
{ return console.error("\`domNodes\` must be an instance of Array.");};for (let
role in relationships) { let parentNodes =
audit.getElementsByComputedRole(role); if (!parentNodes.length)continue; for
(let parentNode of parentNodes) {if (visitedParents.includes(parentNode)) {
continue;};let res = [];traverseForChildRole(parentNode.firstChild,
relationships[role], visitedParents, res);if (!res.length)
domNodes.push(parentNode); }} } function traverseForChildRole(node,
expectedRoles, visitedParents = [], res = []){let childNode = node;if
(childNode) { visitedParents.push(childNode.parentNode);};while(childNode) { if
(childNode.nodeType === 1 && audit.getComputedProperties(childNode)) {let
targetRole = audit.getComputedProperties(childNode).role;if (targetRole !== ""
&& expectedRoles.includes(targetRole)) { res.push(childNode);};if
(childNode.hasChildNodes()) { traverseForChildRole(childNode.firstChild,
expectedRoles, visitedParents, res)} } childNode = childNode.nextSibling;} }
return {init: init }};return new r;})();t.init(Relationships, domNodes);return
{level: domNodes.length ? "fail" : "pass", domNodes, domAttributes:
["role"]}}`, {description: WI.UIString("Ensure `menu` and `menubar` to have
required children.")}),
I've been thinking a bit about this format, and I think it might actually be
better/easier/nicer to write these functions in actual JavaScript, and then use
`toString()` to get the string version for the tests.
async function() {
let domNodes = []
const Relationships = {
"menu": ["menuitem", "menuitemcheckbox", "menuitemradio"],
"menubar": ["menuitem", "menuitemcheckbox", "menuitemradio"],
};
let t = (function() {
let r = function() {
let audit = WebInspectorAudit.Accessibility;
function init(relationships, domNodes) {
let visitedParents = [];
if (!relationships) {
return console.error("Missing Relationship Mapping.");
};
if (!Array.isArray(domNodes)) {
return console.error("\`domNodes\` must be an instance
of Array.")
};
for (let role in relationships) {
let parentNodes =
audit.getElementsByComputedRole(role);
if (!parentNodes.length)
continue;
for (let parentNode of parentNodes) {
if (visitedParents.includes(parentNode)) {
continue;
};
let res = [];
traverseForChildRole(parentNode.firstChild,
relationships[role], visitedParents, res);
if (!res.length)
domNodes.push(parentNode);
}
}
}
function traverseForChildRole(node, expectedRoles,
visitedParents = [], res = []) {
let childNode = node;
if (childNode) {
visitedParents.push(childNode.parentNode);
};
while(childNode) {
if (childNode.nodeType === 1 &&
audit.getComputedProperties(childNode)) {
let targetRole =
audit.getComputedProperties(childNode).role;
if (targetRole !== "" &&
expectedRoles.includes(targetRole)) {
res.push(childNode);
};
if (childNode.hasChildNodes()) {
traverseForChildRole(childNode.firstChild,
expectedRoles, visitedParents, res)
}
}
childNode = childNode.nextSibling;
}
}
return {init: init }
};
return new r;
})();
t.init(Relationships, domNodes);
return {level: domNodes.length ? "fail" : "pass", domNodes,
domAttributes: ["role"]}
}
...
new WI.AuditTestCase(`testMenuRoleForRequiredChidren`,
testMenuRoleForRequiredChidren.toString(), {description: WI.UIString("Ensure
`menu` and `menubar` to have required children.")}),
There are some style issues in the above code, but I think they can be cleaned
up better once I can see them. Here are a few for now:
- why are you creating/nesting `t` and `r`? You can just call these functions
in the top-level function scope instead of creating all these objects.
- when returning an error, we don't want to use `console.error`. You should
be using the Audit's system for errors, which is `{result: "error", errors:
[...]}`. This way, the error will actually appear in the Audit tab, instead of
the console.
- avoid adding ";" after a curly-brace (e.g. "};") for loops/functions
- single-line `if`/`for`/`while`/etc don't need to have curly-braces
- why is this function `async`?
Here's what I'd like it to look like:
let testMenuRoleForRequiredChidren = function() {
const Relationships = {
"menu": ["menuitem", "menuitemcheckbox", "menuitemradio"],
"menubar": ["menuitem", "menuitemcheckbox", "menuitemradio"],
};
let domNodes = []
let visitedParents = new Set;
function hasChildWithRole(node, expectedRoles) {
let childNode = node;
if (childNode.parentNode)
visitedParents.add(childNode.parentNode);
while (childNode) {
let properties =
WebInspectorAudit.Accessibility.getComputedProperties(childNode);
if (properties) {
if (expectedRoles.includes(properties.role))
return true;
if (childNode.hasChildNodes() &&
hasChildWithRole(childNode.firstChild, expectedRoles))
return true;
}
childNode = childNode.nextSibling;
}
return false;
}
for (let role in relationships) {
for (let parentNode of
WebInspectorAudit.Accessibility.getElementsByComputedRole(role)) {
if (visitedParents.has(parentNode))
continue;
if (!hasChildWithRole(parentNode.firstChild,
relationships[role], res))
domNodes.push(parentNode);
}
}
return {level: domNodes.length ? "fail" : "pass", domNodes,
domAttributes: ["role"]};
}
More information about the webkit-reviews
mailing list