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

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted 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 360714: Patch

https://bugs.webkit.org/attachment.cgi?id=360714&action=review




--- Comment #15 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 360714
  --> https://bugs.webkit.org/attachment.cgi?id=360714
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360714&action=review

r=me, awesome work!  Please address/respond to any comments below before
landing.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:308
> +	   let testMenuRoleForRequiredChidren = function() {

NIT: thinking about it again, this variable could be `const` because it's not
run-specific (e.g. it's always the same for every inspection target).

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:310
> +	       let domNodes = [];
> +	       const relationships = {

NIT: I prefer to put constants at the very beginning of any scope, so I'd
switch the order of these variables.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:312
> +		   menubar: ["menuitem", "menuitemcheckbox", "menuitemradio"]

NIT: we like to add trailing commas to the last item of arrays/objects, as it
makes the code more uniform (and any future diffs are cleaner).

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:314
> +	       let audit = WebInspectorAudit.Accessibility;

I still don't understand why you're saving this to a local variable when it can
be inlined.  You inline it in some tests, but not others.  We should be
consistent.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:325
> +	       function hasChildWithRole(node, expectedRoles){

This function should be placed higher than it's first usage.  Please move it
above the `for` loop.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:328
> +		   visitedParents.add(childNode.parentNode);

Style: this should be indented.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:331
> +		       if (childNode.nodeType === 1 && properties) {

NIT: rather than use `1` (which is a non-descriptive constant value), you can
use `Node.ELEMENT_NODE`.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:389
> +	       if (banners.length > 1) {

NIT: single-line `if`/`for`/`while` don't need brackets.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:475
> +	       let liveRegionRoles = ["alert", "log", "status", "marquee",
"timer"];

NIT: this could be `const`.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:827
> +		   new WI.AuditTestCase(`testMenuRoleForRequiredChidren`,
testMenuRoleForRequiredChidren.toString(), {description: WI.UIString("Ensure
that element of role \u0022%s\u0022 and \u0022%s\u0022 have required owned
elements in accordance with WAI-ARIA.").format(WI.unlocalizedString("menu"),
WI.unlocalizedString("menubar"))}),

Are we set on using camel-cased or kebab-cased for the test name?  The existing
"Demo" audit uses kebab-case, so we should match (or change) that to be
consistent.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:849
> +	       ], {description: WI.UIString("Diagnoses common accessibility
problems affecting screen readers and other assistive technology.")}),

For any of the tests that require any `WebInspectorAudit` functions, we may
want to either add logic that would return `{result: "unsupported, errors:
[...]}` within the test or just add a `supports: 1` value to the model object. 
That way, if these tests get run on an older device, we don't throw any errors.