Bug 194005 – AX: Audit tab should have built-in accessibility tests.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Aaron Chu]

Audit tab should have built-in accessibility tests.

[Radar WebKit Bug Importer]

<rdar://problem/47657503>

[Aaron Chu]

Created attachment 360553 [details]
Patch

[Aaron Chu]

Created attachment 360601 [details]
Patch

[Devin Rousso]

Comment on attachment 360601 [details]
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"]};
    }

[Aaron Chu]

Created attachment 360681 [details]
Patch

[Devin Rousso]

Comment on attachment 360681 [details]
Patch

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

I'd like to see one more iteration with these fixes.  I think that the logic is fine, but could use some style/wording polish.

I highly recommend you install <https://eslint.org> (as well as their Sublime Text/TextMate/Atom plugin), as it will very clearly point out to you some quick fixes, as we have a  configured ruleset for Web Inspector <https://trac.webkit.org/browser/webkit/trunk/Source/WebInspectorUI/.eslintrc>.  It will warn you about "`WebInspectorAudit` is not defined" or "strings must use doublequote", but you can ignore those as they are somewhat expected.

> Source/WebInspectorUI/ChangeLog:13
> +        (WI.AuditManager.prototype.addDefaultTestsIfNeeded.hasChildWithRole):

You don't need to list these in the ChangeLog since they're variables.  I understand if this was generated by `prepare-Changelog`, but I think it could be cleaned up.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:386
> +localizedStrings["Ensure \u0022%s\u0022 and \u0022%s\u0022 to have required children."] = "Ensure \u0022%s\u0022 and \u0022%s\u0022 to have required children.";

Grammar: "to have" is not valid English.  I think you should remove the "to", and maybe even explain what you mean by "required children".

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:388
> +localizedStrings["Ensure \u0022%s\u0022 to have required children."] = "Ensure \u0022%s\u0022 to have required children.";

Ditto (>386).

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:390
> +localizedStrings["Ensure elements of accessibility role \u0022%s\u0022 to have required children."] = "Ensure elements of accessibility role \u0022%s\u0022 to have required children.";

Ditto (>386).

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:391
> +localizedStrings["Ensure elements of role \u0022%s\u0022 to have labels."] = "Ensure elements of role \u0022%s\u0022 to have labels.";

Ditto (>386).

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:945
> +localizedStrings["Test for buttons with labels."] = "Test for buttons with labels.";

I think this could have more details.  What is it exactly that we're testing for?  Just that buttons have labels, or something more specific?

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:947
> +localizedStrings["Test for links to have labels."] = "Test for links to have labels.";

Ditto (>386).

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:948
> +localizedStrings["Test for multiple banners on the page."] = "Test for multiple banners on the page.";

Ditto (>945).

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:949
> +localizedStrings["Test for multiple live regions."] = "Test for multiple live regions.";

Ditto (>945).

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:950
> +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, or lowercase?  You have one string with each.  We should be consistent.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1083
> +localizedStrings["\u0022%s\u0022 and \u0022%s\u0022 must contain at least one \u0022%s\u0022, \u0022%s\u0022 or \u0022%s\u0022."] = "\u0022%s\u0022 and \u0022%s\u0022 must contain at least one \u0022%s\u0022, \u0022%s\u0022 or \u0022%s\u0022.";

Grammar: we should use an oxford comma.

    "\u0022%s\u0022 and \u0022%s\u0022 must contain at least one of \u0022%s\u0022, \u0022%s\u0022, or \u0022%s\u0022."

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1087
> +localizedStrings["\u0022%s\u0022 must contain at least one \u0022%s\u0022."] = "\u0022%s\u0022 must contain at least one \u0022%s\u0022.";

Where are these localized strings actually used?  Did you add them yourself, or run `extract-localizable-strings`?

    Tools/Scripts/extract-localizable-js-strings --utf8 Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js Source/WebInspectorUI/UserInterface

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:309
> +          let domNodes = [];

Style: it looks like you're using a different indentation level than our style.  We use four spaces rather than two.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:310
> +          const Relationships = {

Style: we don't capitalize variables unless they are classes.  Declaring the variable as `const` is enough.

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

Style: we don't normally put quotes around the keys for objects, as they are optional.

    menu: ["menuitem", "menuitemcheckbox", "menuitemradio"],

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:313
> +          }

Style: missing `;`.

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

Is there a reason you're aliasing this instead of just calling the functions on `WebInspectorAudit.Accessibility` directly?

    WebInspectorAudit.Accessibility.getEelementsByComputedRole
    WebInspectorAudit.Accessibility.getComputedProperties

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:330
> +            while(childNode) {

Style: there should be a space between `while` and `(`.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:333
> +                if (properties.role !== "" && expectedRoles.includes(properties.role))

NIT: checking that `properties.role !== ""` is a bit unnecessary, as `expectedRoles.includes("")` would never return true anyways.  It does avoid the work of iterating over the array and checking each value, but since there are only 2-3 items in the array to begin with, its not saving much effort.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:343
> +          return { level: domNodes.length ? "fail" : "pass", domNodes, domAttributes: ["role"] }

Style: missing `;`.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:344
> +        }

Style: missing `;` after the variable declaration, and we should also have a newline before the next variable.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:383
> +          let domNodes = banners.slice(1);

Rather than `slice`, why not just check that `domNodes.length > 1`?

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:388
> +          let dialogs = Array.from(audit.getElementsByComputedRole("dialog"));

You shouldn't need to call `Array.from` on this, as the value returned by `WebInspectorAudit.Accessibility.getElementsByComputedRole` is a plain array.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:389
> +          let domNodes = dialogs.filter(dialog => {

Style: we wrap the parameters list in parenthesis for arrow functions.

    let domNodes = dialogs.filter((dialog) => {

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:390
> +            return !dialog.getAttribute("aria-modal") === "true";

For simple predicate arrow functions, you can have it be on one line instead of adding the `{` and `return`.

    let domNodes = dialogs.filter((dialog) => dialog.getAttribute("aria-modal") !== "true");

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:824
> +                new WI.AuditTestCase(`testMenuRoleForRequiredChidren`, testMenuRoleForRequiredChidren.toString(), {description: WI.UIString("Ensure \u0022%s\u0022 and \u0022%s\u0022 to have required children.").format("menu","menubar")}),

NIT: we try to use `WI.unlocalizedString` for values that are going to be shown in the UI, but shouldn't be localized.

    WI.UIString("Ensure \u0022%s\u0022 and \u0022%s\u0022 to have required children.").format(WI.unlocalizedString("menu"), WI.unlocalizedString("menubar"))

[James Craig]

Comment on attachment 360681 [details]
Patch

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

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:948
>> +localizedStrings["Test for multiple banners on the page."] = "Test for multiple banners on the page.";
> 
> Ditto (>945).

I don’t recall multiple banners being a critical issue. Okay to leave if this is in the spec somewhere and I just forgot.

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:950
>> +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, or lowercase?  You have one string with each.  We should be consistent.

Lowercase everything other than proper nouns.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:951
> +localizedStrings["Test that all dialogs have labels."] = "Test that all dialogs have labels.";

Not: At some point you switched from “Ensure that…” to “Test that…” but theses should probably be the same.

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

Remove “ToHave” for the same grammatical reasons listed above in the strings.

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

Camelcase mismatch. By should be capitalized.

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

Camelcase mismatch. List.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:826
> +                new WI.AuditTestCase(`testForMultipleBanners`, testForMultipleBanners.toString(), {description: WI.UIString("Test for multiple banners on the page.")}),

If multiple banners is a problem, the description should be more clear as to why.

Multiple mains are more obvious to people.

[Aaron Chu]

Comment on attachment 360681 [details]
Patch

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

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1087
>> +localizedStrings["\u0022%s\u0022 must contain at least one \u0022%s\u0022."] = "\u0022%s\u0022 must contain at least one \u0022%s\u0022.";
> 
> Where are these localized strings actually used?  Did you add them yourself, or run `extract-localizable-strings`?
> 
>     Tools/Scripts/extract-localizable-js-strings --utf8 Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js Source/WebInspectorUI/UserInterface

nifty, did not know about `extract-localizable-strings`. Thanks.

[Aaron Chu]

Created attachment 360703 [details]
Patch

[James Craig]

Comment on attachment 360703 [details]
Patch

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

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:386
> +localizedStrings["Ensure that \u0022%s\u0022 is spelled correctly."] = "Ensure that \u0022%s\u0022 is spelled correctly.";

I think this should be more direct. “aria-labelledby is misspelled” or “Correct the spelling of aria-labelledby.”

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:389
> +localizedStrings["Ensure that dialogs have aria-modal set to true."] = "Ensure that dialogs have aria-modal set to true.";

I think this one needs to be removed or modified. Right now it is disallowing non-modal dialogs, which seems wrong.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:390
> +localizedStrings["Ensure that element of role \u0022%s\u0022 and \u0022%s\u0022 have required owned elements in accordance with WAI-ARIA 1.1."] = "Ensure that element of role \u0022%s\u0022 and \u0022%s\u0022 have required owned elements in accordance with WAI-ARIA 1.1.";

Don’t put version numbers in these. 1.2 will probably be out before this feature is in a major release. 

“WAI-ARIA” is fine here without the version.

Any spec citations should be linked once markdown is allowed in here. Add a todo or placeholder bug?

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:395
> +localizedStrings["Ensure that only one banner is used on the page."] = "Ensure that only one banner is used on the page.";

Seems like you may have missed my comments about these on the last patch. Re-review?

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:396
> +localizedStrings["Ensure that only one live region is used on the page."] = "Ensure that only one live region is used on the page.";

Most self-voicing web apps use at least two. If there is justification for this, include it. Otherwise, this one may need to come out too.

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

I think this should be changed to multiple main. I remember an ARIA requirement for one main. I do not remember multiple banners being disallowed (on mobile now or I’d cross ref)

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

1. There is another “ToHave” here.

2. I think this test may be invalid. It’s okay to have non-modal aria dialogs.

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

Nit: LinkLabels

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

Nit: ImageLabels

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:599
> +            let domNodes = images.filter((image) => !WebInspectorAudit.Accessibility.getComputedProperties(image).label);

The explanation string for this one should also have a friendly message that includes something like, if this is a purely decorative image with no content, try using alt=“” or role=“none”

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

Typo: Multiple

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

I mentioned on the last patch this camelcase mismatch here “TabList” and on LabelledBy elsewhere.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:833
> +                new WI.AuditTestCase(`testForAriaHiddenFalse`, testForAriaHiddenFalse.toString(), {description: WI.UIString("Ensure aria-hidden=\u0022%s\u0022 is not used").format(WI.unlocalizedString("false"))}),

Missing trailing period and sufficient explanation. I’d add, “…because it’s known to work inconsistently across browsers.

[James Craig]

Comment on attachment 360703 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:768
> +          let domNodes = dialogs.filter((dialog) => !WebInspectorAudit.Accessibility.getComputedProperties(dialog).label);

User friendly message for this one should include something like, “Try using aria-labelledby to reference the first heading or label text of the dialog.”

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

Camelcase mismatch: ListBox

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:824
> +                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 1.1.").format(WI.unlocalizedString("menu"), WI.unlocalizedString("menubar"))}),

Ditto earlier comment about not including version numbers.

[Aaron Chu]

Created attachment 360713 [details]
Patch

[Aaron Chu]

Comment on attachment 360703 [details]
Patch

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

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:386
>> +localizedStrings["Ensure that \u0022%s\u0022 is spelled correctly."] = "Ensure that \u0022%s\u0022 is spelled correctly.";
> 
> I think this should be more direct. “aria-labelledby is misspelled” or “Correct the spelling of aria-labelledby.”

This string is meant to be a description of the test. I think what you are proposing here is more fitting in the explanation field (when it lands).

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:389
>> +localizedStrings["Ensure that dialogs have aria-modal set to true."] = "Ensure that dialogs have aria-modal set to true.";
> 
> I think this one needs to be removed or modified. Right now it is disallowing non-modal dialogs, which seems wrong.

removed.

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:395
>> +localizedStrings["Ensure that only one banner is used on the page."] = "Ensure that only one banner is used on the page.";
> 
> Seems like you may have missed my comments about these on the last patch. Re-review?

ARIA 1.1 states: "Within any document or application, the author SHOULD mark no more than one element with the banner role."

This string is just the description of the test, I'll write up the explanatory info for the explanation text.

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:396
>> +localizedStrings["Ensure that only one live region is used on the page."] = "Ensure that only one live region is used on the page.";
> 
> Most self-voicing web apps use at least two. If there is justification for this, include it. Otherwise, this one may need to come out too.

if multiple live regions are allowed, then I think this should be a warning rather than a failure. This is definitely one area that needs warning. Some sites implement multiple live regions to a point where the entire site is unusable.

As for the string, this is a description for the test, any explanatory info can be added to the explanation text.

>> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:381
>> +        let testForMultipleBanners = function() {
> 
> I think this should be changed to multiple main. I remember an ARIA requirement for one main. I do not remember multiple banners being disallowed (on mobile now or I’d cross ref)

I added a test case for multiple main.

>> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:390
>> +        let testDialogToHaveAriaModal = function() {
> 
> 1. There is another “ToHave” here.
> 
> 2. I think this test may be invalid. It’s okay to have non-modal aria dialogs.

removed.

>> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:833
>> +                new WI.AuditTestCase(`testForAriaHiddenFalse`, testForAriaHiddenFalse.toString(), {description: WI.UIString("Ensure aria-hidden=\u0022%s\u0022 is not used").format(WI.unlocalizedString("false"))}),
> 
> Missing trailing period and sufficient explanation. I’d add, “…because it’s known to work inconsistently across browsers.

Will add the period back, but will save the additional text for explanation since this is simply a description of the test.

[Aaron Chu]

Created attachment 360714 [details]
Patch

[Devin Rousso]

Comment on attachment 360714 [details]
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.

[Aaron Chu]

Created attachment 360750 [details]
Patch

[Devin Rousso]

Comment on attachment 360750 [details]
Patch

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

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

Style: missing space between `)` and `{`

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:317
> +                if (childNode.parentNode)

After testing around with this, we should also check that `node` actually exists, and `return false` if not.

    function hasChildWithRole(node, expectedRoles) {
        let childNode = node;
        if (!childNode)
            return false;

        if (childNode.parentNode)
            visitedParents.add(childNode.parentNode);

        ...

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:336
> +                      continue;

Style: this should have four spaces

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:339
> +                      domNodes.push(parentNode);

Ditto (>336).

[Devin Rousso]

Comment on attachment 360750 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:391
> +            return {level: domNodes.length ? "fail" : "pass", domNodes};

We might also want to highlight any `role` attributes for any nodes we return, so that it's obvious if there are two elements with `role="banner"`.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:397
> +            return {level: domNodes.length ? "fail" : "pass", domNodes};

Ditto (>391), but for `aria-label` and/or `title` (e.g. if they add the attribute, but it is empty, like <div title>).

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:481
> +            return {level: domNodes.length ? "warn" : "pass", domNodes};

Ditto (>391), but for `aria-live`.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:714
> +            return {level: domNodes.length ? "fail" : "pass", domNodes};

Ditto (>397).

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:795
> +            return {level: domNodes.length ? "fail" : "pass", domNodes};

Ditto (>391).

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:801
> +          return {level: domNodes.length ? "fail" : "pass", domNodes};

Ditto (>397).

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:806
> +            return {level: domNodes.length ? "fail" : "pass", domNodes};

Ditto (>391), but for `aria-hidden`.

[Devin Rousso]

Comment on attachment 360750 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:523
> +            return {level: domNodes.length ? "fail" : "pass", domNodes};

Ditto (>397).

[Devin Rousso]

Comment on attachment 360750 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:478
> +            liveRegions = liveRegions.concat(Array.from(document.querySelectorAll("[aria-live=polite], [aria-live=assertive]")));

Just in case, we might want to wrap the value of any attribute in quotes.

    liveRegions = liveRegions.concat(Array.from(document.querySelectorAll("[aria-live=\"polite\"], [aria-live=\"assertive\"]")));

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:527
> +            let domNodes = Array.from(document.querySelectorAll("[aria-hidden=false]"));

Ditto (>478).

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:805
> +            let domNodes = Array.from(document.querySelectorAll("[aria-hidden]:not([aria-hidden=true]):not([aria-hidden=false])"));

Ditto (>478).

This can also be simplified as `[aria-hidden]:not([aria-hidden="true"], [aria-hidden="false"])`.

[Aaron Chu]

Created attachment 361376 [details]
Patch

[chris fleizach]

Created attachment 361381 [details]
Patch

[chris fleizach]

Comment on attachment 361381 [details]
Patch

argh wrong bug!

[James Craig]

Comment on attachment 361376 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:479
> +        const testForAriaLabelledbySpelling = function() {

Camelcase mismatch. By should be capitalized.

[James Craig]

These can be addressed as separate bugs after the main patch lands but:


All from the NYT main page.
- The image label test is flagging all SVG nodes: <rect>, <path>, etc. and throwing spurious errors.
- The multiple banners test is flagging <header> elements (which resolve to banner) and throwing spurious errors. These are valid HTML with no explicit role, so this should not throw an error.
- The link label test is throwing spurious errors on valid links with rendered text. I'm guessing this might be a label vs contents issue. (e.g. <a class="css-1wjnrbv" href="https://www.nytimes.com/pages/world/index.html">World</a>)

[James Craig]

Comment on attachment 361376 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:702
> +        const testComboboxRoleForRequiredChidren = function() {

Camelcase mismatch: Box should be capitalized.

[James Craig]

Filed bug 1943887, bug 1943888, bug 1943889 mentioned above.

[Aaron Chu]

Created attachment 361426 [details]
Patch

[EWS Watchlist]

Comment on attachment 361426 [details]
Patch

Attachment 361426 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11070120

New failing tests:
fast/viewport/ios/device-width-viewport-after-changing-view-scale.html

[EWS Watchlist]

Created attachment 361442 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Aaron Chu]

Created attachment 362155 [details]
Patch

[Devin Rousso]

Comment on attachment 362155 [details]
Patch

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

r=me, thanks for fixing my NITs

Is there a page that you've been using to test this?  Just for future reference, we may want to link/attach some page that demonstrates the fact that these tests work.  Alternatively, you could write a LayoutTest/ManualTest (as a followup) if you wanted :)

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

Style: unnecessary brace.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:835
> +          let dialogs = WebInspectorAudit.Accessibility.getElementsByComputedRole("dialog");

Style: indent should be four spaces.

[WebKit Commit Bot]

Comment on attachment 362155 [details]
Patch

Clearing flags on attachment: 362155

Committed r241639: <https://trac.webkit.org/changeset/241639>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.