[webkit-reviews] review denied: [Bug 194005] AX: Audit tab should have built-in accessibility tests. : [Attachment 360601] Patch

[bugzilla-daemon at webkit.org]

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"]};
    }