Web Inspector: AXI: Web Accessibility Audit Unblocked and adding to the public tracker now that the Tabs UI is available.
<rdar://problem/15449377>
Created attachment 262044 [details] old comp (partially irrelevant)
Related to bug 118317
Created attachment 262045 [details] old design comp (partially irrelevant)
*** Bug 151544 has been marked as a duplicate of this bug. ***
Audit idea I don't want to forget: The icon font anti-pattern (Glyphicons, etc) probably all use a detectable range of unicode characters. We could warn of no pronounceable label if any UI element contained only these characters, or if the UI element's label content was generated exclusively by CSS ::before/::after pseudo-elements.
Engineers at gov.uk created a pretty great "inaccessible page" test case we could use in the validation of this audit tool. https://accessibility.blog.gov.uk/2017/02/24/what-we-found-when-we-tested-tools-on-the-worlds-least-accessible-webpage/
Created attachment 302914 [details] Patch for arch. and design review. All copy and all interface design are FPO.
Created attachment 302915 [details] test case for review patch. This is a test case used to develop the patch.
Comment on attachment 302914 [details] Patch for arch. and design review. View in context: https://bugs.webkit.org/attachment.cgi?id=302914&action=review > Source/JavaScriptCore/inspector/protocol/DOM.json:127 > + "id": "AXAuditResult", I think this could just be "AuditResult." There is nothing in the transport layer than needs to be accessibility-specific. Leave this open to future use by other audits, even if accessibility is all it ships with initially. > Source/JavaScriptCore/inspector/protocol/DOM.json:137 > + { "name": "treeScope", "type": "boolean", "optional": true, "description": "The actual Document Node." }, is treeScope the root node for the current audit, or always the document node? If the latter, should this be called documentNode? > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:106 > +Ref<Inspector::Protocol::DOM::AXAuditResult> AccessibilityAuditObject::checkForMisspelledAriaLabelledBy(AccessibilityAuditObject* instance) > +{ > + bool exists = (instance->m_sample->hasAttribute("aria-labeledby")); > + > + Ref<Inspector::Protocol::DOM::AXAuditResult> result = Inspector::Protocol::DOM::AXAuditResult::create() > + .setExists(exists) > + .release(); > + > + if (exists) { > + result->setTitle("aria-labelledby is misspelled as aria-labeledby."); > + result->setHint("The correct spelling is 'aria-labelledby'"); > + result->setLogLevel(Inspector::Protocol::DOM::AXAuditResult::LogLevel::Errors); > + if (instance->m_sampleNodeObject) > + result->setNode(instance->m_sampleNodeObject); > + } > + return result; > +} > + > +Ref<Inspector::Protocol::DOM::AXAuditResult> AccessibilityAuditObject::checkForImgAltText(AccessibilityAuditObject* instance) > +{ > + bool exists = (instance->m_sample->tagName() == "IMG" && !instance->m_sample->hasAttribute("alt")); > + > + Ref<Inspector::Protocol::DOM::AXAuditResult> result = Inspector::Protocol::DOM::AXAuditResult::create() > + .setExists(exists) > + .release(); > + > + if (exists) { > + result->setTitle("Image elements should have an 'alt' attribute."); > + result->setHint("Consider adding and 'alt' attribute describing the image. If the images does not need description, include the attribute with an empty value."); > + result->setLogLevel(Inspector::Protocol::DOM::AXAuditResult::LogLevel::Warnings); > + if (instance->m_sampleNodeObject) > + result->setNode(instance->m_sampleNodeObject); > + } > + return result; > +} > + > +Ref<Inspector::Protocol::DOM::AXAuditResult> AccessibilityAuditObject::checkForTabIndex(AccessibilityAuditObject* instance) I think a lot of these could be combined into a single method testSimpleSelectorCases()... // pseudo code AuditObject::testSimpleSelectorCases() { for case of simpleSelectorCases { new AuditObject instance // there is already a way to get nodes by selector bool exists = getNodesBySelector(case.selector) if exists { result->setTitle(case.title); result->setLogLevel(logLevelEnumForString(case.level)); ... } } } // Where selectorCases would be a simple array of objects: simpleSelectorCases = [ { "selector": "[aria-labeledby]", "title": "misspelled, yo!", "level": "error" }, { "selector": "img:not([alt])", "title": "img element missing alt attr…", "level": "error" }, { "selector": "[tabindex]:not([tabindex=0]):not([tabindex=-1])", "title": "Avoid using tabindex values other than …", "level": "note" }, // etc, etc. there will be dozens (if not hundreds) of these selector cases. // if will be easier for general web developers to contribute to a selector-based test list. ] // Likewise there would be other test "types" where you could reduce the complexity of the simple test additions. AuditObject::testLabelRequiredForRole(array of AuditObject* instances) { // same principle for labelRequiredForRoleCases } labelRequiredForRoleCases = [ { "role": "button", "level": "error" }, { "role": "table", "level": "note" }, ]; You'd still need one-off methods for some of the complex tests, but any that you can simplify into arrays should be done that way. > Source/WebInspectorUI/UserInterface/Views/AuditReportContentView.js:178 > + Logs: "log-logs" log-note? log-info? > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:39 > - RefPtr<Inspector::Protocol::DOM::AXAuditResult> res = (TestFunction)(m_instancePtr); > + RefPtr<Inspector::Protocol::DOM::AccessibilityAuditResult> res = (TestFunction)(m_instance); Why are these diffs on a brand new file? Can you clean up the patch and reupload? It seems like some of the pre-review suggestions were already taken. Without a clean patch this will be more difficult to review. > Source/WebInspectorUI/UserInterface/Views/AuditReportContentView.css:84 > + alt: "Error"; WebInspectorUI is localized. These should be: alt: attr(data-localized-level); /* populate localized @data-localized-level in the markup */
Comment on attachment 302914 [details] Patch for arch. and design review. View in context: https://bugs.webkit.org/attachment.cgi?id=302914&action=review I'm super confused by this diff. It looks like its two diffs. A lot of my early comments are addressed by portions of the diff below, but I don't understand why the changes show up twice. Can you upload a single combined diff? > Source/JavaScriptCore/inspector/protocol/DOM.json:134 > + { "name": "logLevel", "type": "string", "enum": ["errors", "warnings", "logs"], "optional": true, "description": "The error level of the accessibility issue." }, This is a single result right? So singular would probably make more sense (error, warning, log). > Source/JavaScriptCore/inspector/protocol/DOM.json:137 > + { "name": "treeScope", "type": "boolean", "optional": true, "description": "The actual Document Node." }, Does any code actually set this? Is it useful? (Maybe frameId would be more useful) > Source/JavaScriptCore/inspector/protocol/DOM.json:138 > + { "name": "node", "$ref": "Node", "optional": true, "description": "Test test." } Heh: "Test test" should be a better description. > Source/JavaScriptCore/inspector/protocol/DOM.json:208 > + Style: Remove empty line. We've already discussed moving this to its own domain. Something like: (rough draft) { "domain": "AXAudit", "description": "Perform an Accessibility Audit.", "availability": "web", "types": [ { "id": "Event", "description": "Result for a single test.", "type": "object", "properties": [ { "name": "test", "type": "string", "description": "Test name. Example: `Accessibility.Images.Alt`" }, { "name": "pass", "type": "boolean", "description": "True if no warnings / errors. False otherwise." }, { "name": "results", "$ref": "Result", "optional": true, "description": "Individual results." } ] }, { "id": "Result", "description": "Audit result.", "type": "object", "properties": [ { "name": "title", "type": "string", "optional": true, "description": "..." }, { "name": "hint", "type": "string", "optional": true, "description": "..." }, { "name": "logLevel", "type": "string", "enum": ["error", "warning", "log"], "optional": true, "description": "..." }, { "name": "errorType", "type": "string", "enum": ["label", "attribute"], "optional": true, "description": "..." }, { "name": "documentation", "type": "string", "optional": true, "description": "..." }, { "name": "nodeId", "$ref": "DOM.NodeId", "optional": true, "description": "..." } ] } ], "commands": [ { "name": "startAudit", "description": "Start tracking script evaluations. This will initiate `auditUpdate` events followed by a final `auditComplete` event.", "parameters": [ { "name": "test", "type": "string", "optional": true, "description": "Suite or test to run. All tests if missing. Example: `Accessibility.Images`" }, ], "results": [ { "name": "auditIdentifier", "type": "integer", "Identifier that can be used to cancel this audit." } ], }, { "name": "cancelAudit", "description": "Cancel an active audit." "parameters": [ { "name": "auditIdentifier", "type": "integer", "Identifier that can be used to cancel this audit." } ] } ], "events": [ { "name": "auditUpdate", "description": "Individual audit test completions.", "parameters": [ { "name": "event", "$ref": "Event" } ] }, { "name": "auditComplete", "description": "Audit completed.", "parameters": [ { "name": "auditIdentifier", "type": "integer", "Identifier that can be used to cancel this audit." } ] } ] } > Source/WebCore/WebCore.xcodeproj/project.pbxproj:16594 > 2981CAA5131822EC00D12F2A /* AccessibilityObject.cpp */, > + F643AE451E314D6E00BE59FA /* AccessibilityAuditObject.cpp */, > + F643AE461E314D6E00BE59FA /* AccessibilityAuditObject.h */, You should sort these in the Xcode sidebar alphabetically. > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:1 > +// This should use the WebKit Copyright like all the other files in WebCore. The one at the top of Source/WebCore/accessibility/AccessibilityAttachment.cpp is a great example. > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:11 > +#include "config.h" > +#include "AccessibilityAuditObject.h" Style: Keep these two includes at the top (config.h is necessary, and AccessibilityAuditObject is this file) but add a newline between these and the rest. That is the style we use everywhere else. Again AccessibilityAttachment is a good example. <https://webkit.org/code-style-guidelines/#include-statements> > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:28 > + > + Style: Just one newline. > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:29 > +AccessibilityAuditObject AccessibilityAuditObject::setSample(Element* elm) Style: Use full names. So "element" instead of "elm". <https://webkit.org/code-style-guidelines/#names-full-words> > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:67 > + return (is<AccessibilityObject>(*m_axObject) && m_axObject != NULL); Style: Use !ptr instead of comparing to nullptr. <https://webkit.org/code-style-guidelines/#zero-comparison> Also this code look hazardous. First it dereferences m_axObject and then it checks if it was not null. Something is wrong with this logic. Perhaps the null check should have been first. > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:76 > + Ref<Inspector::Protocol::DOM::AXAuditResult> result = Inspector::Protocol::DOM::AXAuditResult::create() > + .setExists(exists) > + .release(); Style: Indent is 4 spaces, this has 3. <https://webkit.org/code-style-guidelines/#indentation-4-spaces> Style: Likewise the chained functions should be indented: Ref<Inspector::Protocol::DOM::AXAuditResult> result = Inspector::Protocol::DOM::AXAuditResult::create() .setExists(exists) .release(); > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:80 > + result->setTitle("aria-labelledby is misspelled as aria-labeledby."); > + result->setHint("The correct spelling is 'aria-labelledby'"); Nit: These setFoo(WTFString) can use ASCIILiteral() for slightly more optimized String construction: result->setTitle(ASCIILiteral("aria-labelledby is misspelled as aria-labeledby."); --- I wonder if we should instead be sending error codes to the frontend so that it can provide localized error message strings. > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:135 > + result->setTitle("'"+instance->m_computedRole+"' must have accessibility label."); Style: Place spaces around binary operators: <https://webkit.org/code-style-guidelines/#spacing-binary-ternary-op> > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:193 > + > + > + > + > +// Webcore namespace > +} Style: tighten this up. > Source/WebCore/accessibility/AccessibilityAuditObject.h:2 > + * Copyright (C) 2012, Google Inc. All rights reserved. Did this file come from Google or is it one we have added? > Source/WebCore/accessibility/AccessibilityAuditObject.h:34 > +#include "HTMLCollection.h" > +#include "AccessibilityObject.h" > +#include "InspectorWebAgentBase.h" > +#include "AXObjectCache.h" Nit: These should be sorted. <https://webkit.org/code-style-guidelines/#include-statements> > Source/WebCore/accessibility/AccessibilityAuditObject.h:51 > +class AuditObject { > + public: > + void runTest(void (*func)()); > +}; > +class AccessibilityAuditObject final: public AuditObject { > + > + public: Style: Unexpected indentation: <https://webkit.org/code-style-guidelines/#classes> Style: "final" should have a space after it. > Source/WebCore/accessibility/AccessibilityAuditObject.h:67 > + static Ref<Inspector::Protocol::DOM::AXAuditResult> checkForMispelledAriaLabelledBy(AccessibilityAuditObject* instance); I don't actually see these being used in this patch. > Source/WebCore/accessibility/AccessibilityAuditObject.h:85 > + Element* m_sample; > + RefPtr<Inspector::Protocol::DOM::Node> m_sampleNodeObject; > + AXObjectCache* m_axObjectCache; > + AccessibilityObject* m_axObject; > + String m_computedRole; > + String m_axLabel; > + String m_ariaRole; > + AccessibilityRole m_axRole; > + AccessibilityAuditObject* m_instancePtr; > + int m_nodeId; > + int m_liveRegionCount; We should be initializing all primitive members. Otherwise they end up with random values in C++. For pointers it is often best to initialize them to { nullptr }. For integers like these, { 0 }. > Source/WebCore/inspector/InspectorDOMAgent.cpp:911 > + if (!m_document) errorString = ASCIILiteral("Cannot get document"); Style: Each statement should have its own line. <https://webkit.org/code-style-guidelines/#linebreaking-multiple-statements> In this case you would want to return after setting the error: if (!m_document) { errorString = ASCIILiteral("..."); return; } > Source/WebInspectorUI/UserInterface/Main.html:32 > - > + <link rel="stylesheet" href="Views/AuditReportContentView.css"> Stylesheets can go anywhere. Put this alphabetically in the list below. But leave the newline between the External stylesheets and this one. > Source/WebInspectorUI/UserInterface/Views/AuditReportContentView.css:2 > + * Copyright (C) 2013 Apple Inc. All rights reserved. Nit: These dates should all be 2017! > Source/WebInspectorUI/UserInterface/Views/AuditReportContentView.js:30 > + WebInspector.AuditReportContentView = class extends WebInspector.ContentView > + { > + constructor() > + { > + super(); Crazy indentation is happening. There might be tabs. Either way it is very difficult to review the contents when spacing is inconsistent. You can skim the Web Inspector style guide to find some things that apply here: https://trac.webkit.org/wiki/WebInspectorCodingStyleGuide > Source/JavaScriptCore/inspector/protocol/DOM.json:124 > ] I'm not sure how this file showed up multiple times. > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:1 > +/* Same thing, this file should not have shown up multiple times?! > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:13 > + * 3. Neither the name of Apple Inc. ("Apple") nor the names of This is the old license. We should be using the 2 paragraph version. See Source/WebCore/accessibility/AccessibilityAttachment.cpp. > Source/WebInspectorUI/UserInterface/Views/AuditReportContentView.js:163 > + if (toShow) { Style: Single statement conditionals do not contain braces: <https://webkit.org/code-style-guidelines/#braces-one-line>
Comment on attachment 302914 [details] Patch for arch. and design review. View in context: https://bugs.webkit.org/attachment.cgi?id=302914&action=review I've seen same files multiple times? Not sure if I commented on the outdated files? > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:4 > +// Copyright text? > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:17 > +AccessibilityAuditObject AccessibilityAuditObject::runTest(TestcasePointer TestFunction) Why don't you return AccessibilityAuditObject* ? > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:29 > +AccessibilityAuditObject AccessibilityAuditObject::setSample(Element* elm) ditto > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:32 > + configureAXInfoForElement(); I think we can do configureAXInfoForElement(element) to be more clear. > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:58 > +bool AccessibilityAuditObject::createAXObject() From the name, seems like this function should return an object. > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:61 > + m_axObject = m_axObjectCache->getOrCreate(m_sample); I think we should check nullptr for m_axObjectCache. > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:67 > + return (is<AccessibilityObject>(*m_axObject) && m_axObject != NULL); we should do null check first. return (m_axObject && is<AccessibilityObject>(*m_axObject)); > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:74 > + Ref<Inspector::Protocol::DOM::AXAuditResult> result = Inspector::Protocol::DOM::AXAuditResult::create() Wrong indention > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:84 > + } Seems this portion of code is similar to all the test cases. Maybe you can create a help function like: testResult(String title, String hint, LogLevel level, Node* node); > Source/WebCore/accessibility/AccessibilityAuditObject.h:2 > + * Copyright (C) 2012, Google Inc. All rights reserved. Google? > Source/WebCore/inspector/InspectorDOMAgent.cpp:894 > + if(is<HTMLHeadElement>(elm) || is<HTMLMetaElement>(elm) || is<HTMLScriptElement>(elm) || is<HTMLBodyElement>(elm) || is<HTMLHtmlElement>(elm) || is<HTMLTitleElement>(elm)) continue; Can you add a comment or move this check into a helper function so it's more clear for why we are ignoring these cases. > Source/WebCore/inspector/InspectorDOMAgent.cpp:906 > + return axResults; This is a little bit strange to me. You are creating an auditObject with an axResults object, and modifying the axResults object within the auditObject class but returning it here. To me I think the AccessibilityAuditObject should be simple enough to just store the ax information related to each element. And maybe you should have a test manager class that contains different test cases and has runTest function that takes in AccessibilityAuditObject object and returns a result array. So that maybe in future you want to test some cases that requires multiple elements you can easily do that by subclassing your test manager class and AccessibilityAuditObject class. > Source/WebCore/inspector/InspectorDOMAgent.cpp:909 > +void InspectorDOMAgent::getAXAuditReport(ErrorString& errorString, RefPtr<Inspector::Protocol::Array<Inspector::Protocol::DOM::AXAuditResult>>& axResults) Why can't this method combined with the one above?
I marked the bogus patch as obsolete.
Created attachment 303086 [details] 2nd patch (consolidated) This is a patch to gather review. Copy and interfaces are FPO. This patch includes some of the quicker fixes from above reviews. No update yet on architectural feedbacks.
Comment on attachment 303086 [details] 2nd patch (consolidated) View in context: https://bugs.webkit.org/attachment.cgi?id=303086&action=review Overall looking really good. Thanks Aaron. > Source/JavaScriptCore/inspector/protocol/DOM.json:128 > + "description": "An Accessibility Audit Result.", Since this is changing from AXAuditResult result to AuditResult, please update the other protocol descriptions and some of the method names, too. ("accessibility issue" -> "issue" etc) > Source/JavaScriptCore/inspector/protocol/DOM.json:199 > + "name": "getAXAuditReport", ditto > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:66 > +void AccessibilityAuditObject::configureAccessibilityInfoForElement() > +{ > + if (createdAccessibilityObject()) { > + m_instance->m_computedRole = m_axObject->computedRoleString(); > + m_instance->m_axLabel = m_axObject->computedLabel(); > + m_instance->m_ariaRole = m_sample->getAttribute("role"); > + m_instance->m_axRole = (m_ariaRole.isEmpty()) ? m_axObject->roleValue() : m_axObject->ariaRoleToWebCoreRole(m_ariaRole); > + } > +} Is there risk that these values could be stale during subsequent audits? IOW, user runs initial audit, corrects an error (e.g. adds a label), and then a subsequent audit still throws an error b/c the cached computed label is empty? > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:128 > + ASCIILiteral("Image elements must have an 'alt' attribute."), > + ASCIILiteral("Consider adding an 'alt' attribute that labels the images. If the image is decorative and does not need a label, include the attribute with an empty value, use alt=\"\"."), > + ASCIILiteral(""), > + Inspector::Protocol::DOM::AuditResult::LogLevel::Warning This one could be an Error rather than Warning. It's a validation error and an accessibility error. > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:141 > + ASCIILiteral("'0' indicates that an element is included in the tabbing order. A value of '-1' indicates that the element is not included in the tabbing order, but it is programatically focusable. All other values will require manual ordering of the tab order and can create a confusing user experience."), Good explanation. > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:152 > +Ref<Inspector::Protocol::DOM::AuditResult> AccessibilityAuditObject::testLabel(AccessibilityAuditObject* instance) > +{ > + AccessibilityRole axRole = instance->m_axRole; > + bool isFailed = (instance->isAccessibilityObject() && (axRole == HeadingRole || axRole == WebCoreLinkRole || axRole == ButtonRole) && instance->m_axLabel.isEmpty()); This is another type of test that could be broken into a suite... For each of arrayRolesRequiringLabel, run this check. This could be another that uses CSS selectors: e.g. "*:not([aria-busy=true]) [role=<roleName>]" > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:188 > + ASCIILiteral("There are too many live regions on the page."), Instead of "too many" try a less subjective explanation like "three or more" or m_liveRegionCount. > Source/WebCore/accessibility/AccessibilityAuditObject.h:74 > + static Ref<Inspector::Protocol::DOM::AuditResult> testMisspelledAriaLabelledBy(AccessibilityAuditObject* instance); > + static Ref<Inspector::Protocol::DOM::AuditResult> testImgAltText(AccessibilityAuditObject* instance); > + static Ref<Inspector::Protocol::DOM::AuditResult> testTabIndex(AccessibilityAuditObject* instance); > + static Ref<Inspector::Protocol::DOM::AuditResult> testLabel(AccessibilityAuditObject* instance); > + static Ref<Inspector::Protocol::DOM::AuditResult> testLinkHrefAndLabel(AccessibilityAuditObject* instance); > + static Ref<Inspector::Protocol::DOM::AuditResult> testLiveRegionCount(AccessibilityAuditObject* instance); Look for ways to combine similar functionality. Something like the testSimpleSelector method mentioned above may allow you to reduce code complexity and make it easier for more web developers (those w/o C++ experience) to contribute simpler reusable test cases. > Source/WebCore/inspector/InspectorDOMAgent.cpp:899 > + if (is<HTMLHeadElement>(element) > + || is<HTMLMetaElement>(element) > + || is<HTMLScriptElement>(element) > + || is<HTMLBodyElement>(element) > + || is<HTMLHtmlElement>(element) > + || is<HTMLTitleElement>(element)) continue; You could probably simplify this to any hidden element? Maybe anything w/o a RenderObject? > Source/WebInspectorUI/UserInterface/Base/Main.js:456 > - WebInspector.TimelineTabContentView, > + WebInspector.TimelineTabContentView Unrelated? May be good to leave since trailing commas are legal in ES6 and recommended as a best practice in many languages.
Comment on attachment 303086 [details] 2nd patch (consolidated) View in context: https://bugs.webkit.org/attachment.cgi?id=303086&action=review I don't like the current nomenclature of tests and objects in the backend. It's vague and more commonly refers to actual tests of WebKit (which, btw, you will need to write at some point for this protocol change). Here's what I propose: - An AuditRule is a single runnable audit (currently this is AccessibilityAuditObject's methods). Its inputs and outputs are variable, and may not even be a class–in your example, AccessibilityAuditObject's test* methods would each be a "rule", though it's not really necessary to make a class for each little test. - An AuditRuleGroup is a premade set of AuditRules that should run together and are requested by the frontend over the protocol. The group should be topical, so that its contents may change and be improved in future releases. - An AuditResult, which contains a payload of data (either per-group or per-rule, i don't know) - An AuditReport, which is a collection of AuditResults that were requested together. AuditResults are keyed by the AuditRuleGroup. > Source/JavaScriptCore/inspector/protocol/DOM.json:131 > + { "name": "isFailed", "type": "boolean", "description": "Indicates whether there is an existing AX object for the DOM node. If this is false, all the other properties will be default values." }, No 'is' prefix is necessary. > Source/JavaScriptCore/inspector/protocol/DOM.json:134 > + { "name": "documentation", "type": "string", "optional": true, "description": "A URL that points to a documentation content of the access issue." }, This is a compatibility trap. When later versions of the frontend get released, the backend may still be sending outdated documentation links. A better approach would be to define an enum of AuditRule or AuditRuleGroup identifiers. The frontend can look up its own localization / docs / suggestions based on the audit identifier. When we release a later version of Safari, it will get updated docs/text even if the backend (say, an older iOS device) has not been updated. > Source/JavaScriptCore/inspector/protocol/DOM.json:135 > + { "name": "logLevel", "type": "string", "enum": ["error", "warning", "info", "debug"], "optional": true, "description": "Severity level of the detected issue." }, I'm not sure how meaningful this is. Can't we allow the frontend to decide this? > Source/JavaScriptCore/inspector/protocol/DOM.json:200 > + "parameters": [], We should think a little harder about the command and its forward-compatibility here. It would be better for the frontend to be able to explicitly ask for particular AuditRules / AuditRuleGroups to be run. We don't want a situation where everything will run every time, or where the frontend gets data that it can't use or won't need. Secondly, we want audit results to trickle in incrementally, since some audits may take a long time and we want to provide some high level feedback (# of issues, affected DOM nodes, etc). So, I would recommend something like: /* Add a new enum, AuditRuleGroup. For now there could be a rule group called webkit.aria-roles or whatever it is the rules in this patch are detecting. The specific rules in a group could change per release according to what the backend can do. */ { /* command */ "name": "performAudits", "parameters": /* array of AuditRuleGroup keys */ "returns": /* an audit report id, used to thread subsequent events */ } { /* event */ "name": "auditReportUpdate", "properties": { "reportId": ... "data": ..., } } { /* event */ "name": "auditReportComplete" "properties": { "reportId": ... "data": /* array of report payloads, keyed by AuditRuleGroup */, } } We kinda discussed this command+ event approach in our conversation, but please ask if this doesn't quite make sense. > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:140 > + ASCIILiteral("Attribute 'tabindex' should either be 0 or -1."), Per my explanation in the protocol section, we really really should move these strings out to WebInspectorUI, where they are easier to localize and maintain. As written, this won't be localized at all. > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:146 > + return instance->populateResult(isFailed, &failureMetaData); This design pattern seems unnecessarily complicated and doesn't seem to have any large benefit. The interface for an AuditRuleGroup should be: void run(); // synchronous Ref<Inspector::Protocol::DOM::AuditResult> buildProtocolObjectForResults(); Why can't the AccessibilityAuditObject take the element in its constructor, then have a run() method which populates its members with audit result data? Then, you could have a separate buildProtocolObjectForResults() method that scans the members and builds an appropriate typechecked protocol object. The actual result object would then be a set of flags per element: {element: nodeid, issues: { Issues.TabIndexBad, Issues.LabelMissing, ... }} > Source/WebCore/accessibility/AccessibilityAuditObject.h:60 > + : m_liveRegionCount(0) This is redundant. > Source/WebCore/accessibility/AccessibilityAuditObject.h:94 > + { Don't do this. Put it in the implementation.
Comment on attachment 303086 [details] 2nd patch (consolidated) View in context: https://bugs.webkit.org/attachment.cgi?id=303086&action=review Didn't include some architecture suggestion since I've already commented in my previous review. > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:35 > +AccessibilityAuditObject AccessibilityAuditObject::runTest(TestcasePointer TestFunction) I don't see where we use the return value, maybe this can be void or just return bool for success or not. > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:39 > + bool isFailed; bool isFailed = false; > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:46 > +AccessibilityAuditObject AccessibilityAuditObject::setSample(Element* element, int nodeId) Same here, maybe just return a bool? > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:60 > + if (createdAccessibilityObject()) { We can make createdAccessibilityObject() return an object, so it would be more clear if we do if (AccessibilityObject *object = createdAccessibilityObject()) { // Do things with this object } > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:76 > + m_instance->m_axObject = m_axObjectCache->getOrCreate(m_sample); should check null for m_axObjectCache > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:82 > + return (is<AccessibilityObject>(*m_instance->m_axObject) && m_instance->m_axObject); should be m_instance->m_axObject && is<AccessibilityObject>(*m_instance->m_axObject) > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:89 > + .release(); Can AuditResult construct with a boolean parameter? > Source/WebCore/accessibility/AccessibilityAuditObject.cpp:98 > + if (sizeof(m_instance->m_nodeIdsForFrontend)) should be if (m_instance->m_nodeIdsForFrontend->length()) ? > Source/WebCore/accessibility/AccessibilityAuditObject.h:95 > + m_nodeIdsForFrontend = Inspector::Protocol::Array<Inspector::Protocol::DOM::NodeId>::create(); Seems little strange that reset is actually creating new array
Comment on attachment 303086 [details] 2nd patch (consolidated) View in context: https://bugs.webkit.org/attachment.cgi?id=303086&action=review >> Source/WebCore/accessibility/AccessibilityAuditObject.cpp:35 >> +AccessibilityAuditObject AccessibilityAuditObject::runTest(TestcasePointer TestFunction) > > I don't see where we use the return value, maybe this can be void or just return bool for success or not. The return value is on line 43. It returns the instance to allow for chaining. >> Source/WebCore/accessibility/AccessibilityAuditObject.cpp:46 >> +AccessibilityAuditObject AccessibilityAuditObject::setSample(Element* element, int nodeId) > > Same here, maybe just return a bool? This one is on line 55, also returning an instance for chaining.
Comment on attachment 303086 [details] 2nd patch (consolidated) View in context: https://bugs.webkit.org/attachment.cgi?id=303086&action=review >>> Source/WebCore/accessibility/AccessibilityAuditObject.cpp:46 >>> +AccessibilityAuditObject AccessibilityAuditObject::setSample(Element* element, int nodeId) >> >> Same here, maybe just return a bool? > > This one is on line 55, also returning an instance for chaining. Oh I see, but this seems like a very strange pattern to me. instance.setSomething().runTest1().runTest2()...
(In reply to comment #19) > Comment on attachment 303086 [details] > 2nd patch (consolidated) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=303086&action=review > > >>> Source/WebCore/accessibility/AccessibilityAuditObject.cpp:46 > >>> +AccessibilityAuditObject AccessibilityAuditObject::setSample(Element* element, int nodeId) > >> > >> Same here, maybe just return a bool? > > > > This one is on line 55, also returning an instance for chaining. > > Oh I see, but this seems like a very strange pattern to me. > instance.setSomething().runTest1().runTest2()... This is fairly common in scripting languages like JavaScript... though we don't really use it that much in Web Inspector code aside from protocol object builders.
(In reply to comment #16) > Comment on attachment 303086 [details] > > Here's what I propose: > > - An AuditRule is a single runnable audit (currently this is > AccessibilityAuditObject's methods). Its inputs and outputs are variable, > and may not even be a class–in your example, AccessibilityAuditObject's > test* methods would each be a "rule", though it's not really necessary to > make a class for each little test. > - An AuditRuleGroup is a premade set of AuditRules that should run together > and are requested by the frontend over the protocol. The group should be > topical, so that its contents may change and be improved in future releases. > - An AuditResult, which contains a payload of data (either per-group or > per-rule, i don't know) > - An AuditReport, which is a collection of AuditResults that were requested > together. AuditResults are keyed by the AuditRuleGroup. Let's think on this some more and potentially meet again. One of the long term goals is to have the accessibility portions of an audit be available through both the WebKit Inspector and the native Accessibility Inspector. Additional plumbing work will be required for the latter, but I'm reluctant to make each platform axAudit daemon dependent upon WebInspectorUI. (It's sounds like that's what you are proposing.)
That said, I'm fine having the test data be sent over the protocol as long as there is no code or data duplication. IOW, axAudit daemon and WebInspectorUI ultimately share a single source for both the logic and test data.
Created attachment 304842 [details] Revised draft. This patch removed the concept of "suite" from WebCore and into the Inspector. It also address a lot of the feedback from the 2nd patch. Note that copy and error code are still FPO.
Created attachment 332244 [details] WIP Patch The latest patch for feedback. Thanks!
PS. UI is in the rough.
Created attachment 332253 [details] WIP Patch Latest patch for feedback. UI is in the rough.
Comment on attachment 332253 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332253&action=review There are a lot of style issues in many of the files. I have to stop reading because I have to go to a class, but I hope you take what I've commented so far and look a the rest of the files to make sure you fix them if they appear elsewhere. Some of the classes you've created seem unnecessary or are redundant (TextView is effectively a WI.createMessageTextView, or you could just create an element at the call-site instead). Many of your UI classes don't need to subclass WI.View, as they aren't really large enough or don't have any work that requires the use of our layout() mechanism. I'm not sure what's been discussed with the team at Apple, but I also think that you could redesign some of the model objects to make them a bit more streamlined, such as not subclassing WI.Collection and just using an member variable object instead. > Source/WebInspectorUI/UserInterface/Base/Main.js:972 > +WI.showAuditTab = function(report) I typically like to follow the pattern of any "optional" objects go inside an `options` parameter. I can imagine that this function might be used without a `report` parameter in the future, so I think we might want to change it slightly: WI.showAuditTab = function(options = {}) { ... if (options.reportToShow) tabContentView.showRepresentedObject(report); } > Source/WebInspectorUI/UserInterface/Base/Main.js:1776 > +} Style: missing semicolon > Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:26 > + WI.AuditManager = class AuditManager extends WI.Object Style: extra space before WI.AuditManager > Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:39 > + this.addEventListener(WI.AuditManager.Event.GenerateReport, this._generateAuditReport.bind(this)); This is a very weird pattern. You should just make it a public method `generateAuditReport` and call it instead of dispatching an event. > Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:45 > + get testSuites() > + { > + return this._testSuites; > + } Style: when a getter is this "simple" or straightforward, you can put it on one line: get testSuites() { return this._testSuites; } > Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:58 > + let res = WI.domTreeManager.getAllNodeIds().map(convertToInteger); It seems like the only reason you're converting the id's to an integer is so that you can iterate over them in a for-loop. This isn't valid, especially since the id's of removed nodes aren't reused, so your array might have "holes" in it. You should use a for-of, or change according to my comment on line 65. > Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:65 > + getDocumentNodeId() This function does a lot of unnecessary work, especially since WI.DOMTreeManager already has a WI.DOMNode `_document` member variable, which has an `id`. Any caller that wants to fetch the document should go through `WI.domTreeManager.requestDocument`. > Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:67 > + let ids = this.domNodeIds; This is the only place that `get domeNodeIds` (and `getDOMNodeIds` by extension) is ever called. We don't need to create unnecessary functions for this work. Also, it saves the result to `_domNodeIds`, but that saved value is never used, so you can probably get rd of it. > Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:70 > + if (WI.domTreeManager.nodeForId(ids[i]).nodeType() === Node.DOCUMENT_NODE) See my comment on line 58 > Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:81 > + let allSuites = WI.AuditTests; Instead of asking this to a variable, you can just inline it: for (let [name, test] of Object.entries(WI.AuditTests)) > Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:83 > + for (let suiteName in allSuites) { Style: we remove the brackets around if/for/while/etc when the body is only one line > Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:90 > + return (this._testSuites.items.size === Object.keys(WI.AuditTests).length); Style: the parenthesis around the return are unnecessary > Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:103 > +} Style: missing semicolon > Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:107 > + ReportWasGenerated: "report-was-generated" Style: missing trailing comma > Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:108 > +} Ditto (103) > Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:116 > + getAllNodeIds() See my comment on AuditManager.js:58 > Source/WebInspectorUI/UserInterface/Main.html:242 > + <link rel="stylesheet" href="Views/AuditIssueView.css"> This should be put with the rest of the "Views/*" CSS files above > Source/WebInspectorUI/UserInterface/Main.html:452 > + <script src="Models/AuditCollection.js"></script> This should be put with the rest of the "Models/*" JS files above > Source/WebInspectorUI/UserInterface/Main.html:465 > + <script src="Test/AuditTests.js"></script> We only put files related to actual testing (e.g. LayoutTests) inside the "Test/*" folder. AuditTests.js is more of a base/model file, and could also use a better name to make it clear that it's meant to test the webpage, not some functionality of WebInspector. > Source/WebInspectorUI/UserInterface/Main.html:523 > + <script src="Views/AuditTabContentView.js"></script> This should be put with the rest of the "Views/*" JS files below > Source/WebInspectorUI/UserInterface/Main.html:555 > + <script src="Views/TextView.js"></script> Ditto (523) > Source/WebInspectorUI/UserInterface/Main.html:565 > + <script src="Views/AuditSidebarPanel.js"></script> Please keep this alphabetical. > Source/WebInspectorUI/UserInterface/Main.html:685 > + <script src="Views/AuditContentView.js"></script> Ditto (565) > Source/WebInspectorUI/UserInterface/Models/AuditCollection.js:26 > +WI.AuditCollection = class AuditCollection extends WI.Collection I don't think you need to subclass WI.Collection, especially since you don't make any use of it's events or WI.CollectionContentView. Additionally, since each item seems to have a name, an Object/Map might be better. > Source/WebInspectorUI/UserInterface/Models/AuditCollection.js:30 > + super(); Style: extra indent > Source/WebInspectorUI/UserInterface/Models/AuditCollection.js:49 > + iterate(callback) This can be replaced at each call-site by using a for-of on `get items`: for (let item of collection.items) { ... } > Source/WebInspectorUI/UserInterface/Models/AuditCollection.js:57 > + find(itemName) Ditto (49) > Source/WebInspectorUI/UserInterface/Models/AuditCollection.js:67 > +} Style: missing semicolon > Source/WebInspectorUI/UserInterface/Models/AuditReport.js:1 > +/* This entire file has extra indentation on each line. We use 4 spaces as our indent, and it looks like this file is using tabs. > Source/WebInspectorUI/UserInterface/Models/AuditReport.js:26 > + WI.AuditReport = class AuditReport Style: extra space > Source/WebInspectorUI/UserInterface/Models/AuditReport.js:31 > + throw new Error("WI.AuditReport should be initialized using WI.AuditCollection."); It doesn't seem possible for result to ever not be a WI.AuditCollection, so this can be dropped > Source/WebInspectorUI/UserInterface/Models/AuditReport.js:50 > + this._reportName = item.suiteName; This means that each iteration, `_reportName` will be overridden by the next value. I don't think that's what you want to happen. If it is, this needs to be cleaned up, or at least deserves a comment. > Source/WebInspectorUI/UserInterface/Models/AuditReport.js:101 > +} Style: missing semicolon > Source/WebInspectorUI/UserInterface/Models/AuditResult.js:1 > +/* This entire file has extra indentation on each line. We use 4 spaces as our indent, and it looks like this file is using tabs. > Source/WebInspectorUI/UserInterface/Models/AuditResult.js:26 > + WI.AuditResult = class AuditResult Style: extra space > Source/WebInspectorUI/UserInterface/Models/AuditResult.js:45 > + } > + get failed() Style: missing newline > Source/WebInspectorUI/UserInterface/Models/AuditResult.js:49 > + } > + get nodeIds() Ditto (44) > Source/WebInspectorUI/UserInterface/Models/AuditResult.js:53 > + } > + get name() Ditto (44) > Source/WebInspectorUI/UserInterface/Models/AuditResult.js:57 > + } > + get errorCode() Ditto (44) > Source/WebInspectorUI/UserInterface/Models/AuditResult.js:62 > + get suiteName() Simple getters like this can be put on one line: get suiteName() { return this._suiteName; } This applies to the previous getters (except `get failed`) as well. > Source/WebInspectorUI/UserInterface/Models/AuditResult.js:75 > +} Style: missing semicolon > Source/WebInspectorUI/UserInterface/Models/AuditResult.js:78 > +WI.AuditResult.Passed = true; > +WI.AuditResult.Failed = false; This seems unnecessary. Either use the literal `true`/`false` at the call-site, or turn these into Symbols/strings so that they are more unique/recognizable. > Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:31 > + if (!testSuite instanceof WI.AuditTestSuite) > + throw new Error("An AuditTestCase must belong to a AuditTestSuite."); It doesn't seem possible for result to ever not be a WI.AuditTestSuite, so this can be dropped > Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:42 > + get displayName() Simple getters like this can be put on one line: get displayName() { return this._name; } This applies to the other getters as well. > Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:64 > + if (testCaseOnly) this._runTestCaseOnly = testCaseOnly; Style: put the body of the if on a new line > Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:66 > + this._testMethod.bind(this).call(); this._testMethod.call(this); > Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:75 > + let processResultCallback = (callback) ? callback : this._processResultFromQuerySelectorTest; You can replace this with an `||` let processResultCallback = callback || this._processResultFromQuerySelectorTest; > Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:86 > + Style: unnecessary newline > Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:87 > + if (this._customOnPassed) this._customOnPassed.call(this, auditResult.data); Ditto (64) > Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:92 > + if (this._customOnFailed) this._customOnFailed.call(this, auditResult.data); Ditto (64) > Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:98 > + if (!auditResultData.result instanceof WI.AuditResult) > + throw new Error("Result must be of instance AuditResult"); Ditto (30) > Source/WebInspectorUI/UserInterface/Models/AuditTestSuite.js:26 > + WI.AuditTestSuite = class AuditTestSuite extends WI.AuditCollection Style: extra space > Source/WebInspectorUI/UserInterface/Models/AuditTestSuite.js:47 > + if (typeof(testCase.name) !== "string" || testCase.name === "" || !testCase.name) > + throw new Error("Test name must be a valid string."); > + if (typeof(testCase.errorCode) !== "number") > + throw new Error("Error Code for test must be a valid number."); It doesn't seem possible for the `name` to ever not be a String or `errorCode` to not be a number, so these can be dropped. > Source/WebInspectorUI/UserInterface/Models/AuditTestSuite.js:50 > + let eCode = testCase.errorCode; Style: use the full name, or just inline it later > Source/WebInspectorUI/UserInterface/Views/AuditContentView.js:35 > + Style: extra newline > Source/WebInspectorUI/UserInterface/Views/AuditContentView.js:36 > + this.highlightConfig = {showInfo: true, contentColor: {r: 111, g: 168, b: 220, a: 0.66}}; Style: expand this object into separate lines > Source/WebInspectorUI/UserInterface/Views/AuditContentView.js:38 > + var scopeBarItems = [ NIT: `let` instead of `var` > Source/WebInspectorUI/UserInterface/Views/AuditContentView.js:40 > + new WI.ScopeBarItem(WI.AuditContentView.Scopes.Passed, "Passed", false), Use a WI.UIString for "Passed" > Source/WebInspectorUI/UserInterface/Views/AuditContentView.js:43 > + new WI.ScopeBarItem(WI.AuditContentView.Scopes.Logs, WI.UIString("Logs"), false) Style: add a trailing comma > Source/WebInspectorUI/UserInterface/Views/AuditContentView.js:46 > + this._scopeBar = new WI.ScopeBar("log-scope-bar", scopeBarItems, scopeBarItems[0]); Is this the right identifier? I would imagine it to be something like "audit-result-type". > Source/WebInspectorUI/UserInterface/Views/AuditContentView.js:51 > + } > + get navigationItems() Style: missing newline > Source/WebInspectorUI/UserInterface/Views/AuditContentView.js:55 > + if (WI.isShowingSplitConsole()) > + navigationItems.push(this._showConsoleTabNavigationItem); I think you copied this. It should just be: get navigationItems() { return [this._scopeBar]; } > Source/WebInspectorUI/UserInterface/Views/AuditContentView.js:61 > + get scopeBar() > + { > + return this._scopeBar; > + } This getter never looks to be used. Is it needed? > Source/WebInspectorUI/UserInterface/Views/AuditContentView.js:63 > + _scopeBarSelectionDidChange(evt) Style: use the full parameter name `event` > Source/WebInspectorUI/UserInterface/Views/AuditContentView.js:67 > + } > + _filterMessageElements(resultDetailsViews, scopeBar) Style: missing newline > Source/WebInspectorUI/UserInterface/Views/AuditContentView.js:69 > + let showAll = scopeBar.item(WI.AuditContentView.Scopes.All).selected; NIT: this could be named better, like `selectedShowAll` > Source/WebInspectorUI/UserInterface/Views/AuditIssueView.css:33 > + border-bottom: 1px solid hsl(125, 14%, 84%); Why are you using this specific shade of gray? Can you use one of the colors we already use (such as those in Variables.css)? > Source/WebInspectorUI/UserInterface/Views/AuditIssueView.css:46 > + box-sizing: border-box; This is already applied in Main.css > Source/WebInspectorUI/UserInterface/Views/AuditIssueView.css:94 > +} > +.audit-report .audit-message-subline { Style: missing newline > Source/WebInspectorUI/UserInterface/Views/AuditIssueView.css:100 > +} > +.audit-report .audit-message-text { Ditto (93) > Source/WebInspectorUI/UserInterface/Views/AuditIssueView.js:1 > +/* This entire file has extra indentation on each line. We use 4 spaces as our indent, and it looks like this file is using tabs. > Source/WebInspectorUI/UserInterface/Views/AuditIssueView.js:26 > + WI.AuditIssueView = class AuditIssueView extends WI.View Style: extra space > Source/WebInspectorUI/UserInterface/Views/AuditIssueView.js:34 > + this._children = []; Is it necessary to save the current sections? It looks like you don't use this member variable anywhere. > Source/WebInspectorUI/UserInterface/Views/AuditIssueView.js:39 > + this._rootView = this.element; Why did you reassign this to another member variable? You can just use `this.element`. > Source/WebInspectorUI/UserInterface/Views/AuditIssueView.js:56 > + get children() > + { > + return this._children; > + } Ditto (34) > Source/WebInspectorUI/UserInterface/Views/AuditIssueView.js:61 > + get rootView() > + { > + return this._rootView; > + } Ditto (39)
You should not put this code in UserInterface/Test. Those files are meant for WebInspector tests. Lets create a new directory, UserInterface/Audits for audit related code. I think the general model can be improved a bit. Right now you have tests and their descriptions / results spread out across multiple files, and its very hard to relate each of the pieces. I think you will want to very closely group the test name, title, description, test function, and test result generation. I thought about this for a bit and this is what I came up with. I don't think its perfect, and I didn't fully think through how we would display this in a UI, but I think this is easier to follow: • There should be a AuditTestSuite with an identifier, name and a bunch of AuditTestCases. • Each AuditTestCase should have at least a name, title, and testFunction. Optionally a hint. • AuditTestCase test functions should be asynchronous, to allow for a lot of work. • AuditTestCase test function should produce AuditResults. - For example a test can produce multiple results: 3 errors, 2 warnings, 1 pass • Each AuditResult should include a level, data, and a preferred way to display the result UserInterface/Audits/AuditTestSuite.js: WI.AuditTestSuite = class AuditTestSuite { constructor(identifier, name) { this.identifier = identifier; this.name = name; } static testCaseDescriptors() { WI.NotImplementedError.subclassMustOverride(); } get testCases() { // FIXME: Turn testCaseDescriptors into unique test case instances. return this._testCases; } }; UserInterface/Audits/AuditTestCase.js: WI.AuditTestCase = class AuditTestCase { constructor(suite, name, hint, testFunction) { console.assert(suite instanceof WI.AuditTestSuite); console.assert(name, "Test must have a non-empty name."); console.assert(typeof testFunction === "function"); console.assert(!hint || typeof hint === "string"); this._suite = testSuite; this._name = testName; this._testFunction = testFunction; this._hint = hint || null; ... } get suite() { return this._suite; } get name() { return this._name; } get hint() { return this._hint; } perform() { return this._testFunction(this); } }; UserInterface/Audits/AuditResult.js: WI.AuditResult = class AuditResult { constructor(testCase, level, data, preferredViewConstructor) { console.assert(testCase instanceof WI.AuditTestCase); console.assert(Object.values(WI.AuditResult.Level).includes(level)); console.assert(!preferredViewConstructor || typeof preferredViewConstructor === "function"); this._testCase = testCase; this._level = level; this._data = data || null; this._viewConstructor = preferredViewConstructor || null; } get testCase() { return this._testCase; } get level() { return this._level; } get data() { return this._data; } get preferredViewConstructor() { return this._viewConstructor; } WI.AuditResult.Level = { Log: "log", Error: "error", Warning: "warning", }; Your Accessibility AuditSuite could look something like this: • Notice the test title and hint are right next to the test code now, instead of split across multiple files. • Each test function should be asynchronous, since it may need to do things asynchronously (ask the backend for data...) - Eventually this could even indicate progress UserInterface/Audits/AccessibilityAuditSuite.js: WI.AccessibilityAuditSuite = class AccessibilityAuditSuite extends AuditTestSuite { constructor() { super("accessibility", WI.UIString("Accessibility")); } static testCaseDescriptors() { // FIXME: We may want to localize all of the titles / hints. return [ { name: "testMisspelledAriaLabelledBy", title: "aria-labelledby is misspelled as aria-labeledby.", hint: "The correct spelling is 'aria-labelledby'", async test(testCase) { return suite.testWithQuerySelector(testCase, "[aria-labeledby]"); } }, ... ] } // Public async testWithQuerySelector(testCase, selector) { let doc = await WI.domTreeManager.requestDocument(); let nodeIds = await WI.domTreeManager.querySelectorAll(doc.id, selector); return nodeIds.map((nodeId) => { return new WI.AuditResult(testCase, WI.AuditResult.Error, {nodeId: nodeId}, WI.AccessibilityNodeIssueView); }); } }; Then in the future if we want, we can add other audit suites following the same kind of pattern: UserInterface/Audits/PerformanceAuditSuite.js: WI.LinterAuditSuite = class Linter extends AuditTestSuite { constructor() { super("linter", WI.UIString("Code Linter")); } static testCaseDescriptors() { return [ { name: "lintJavaScript", title: "aria-labelledby is misspelled as aria-labeledby.", hint: "The correct spelling is 'aria-labelledby'", async test(testCase) { // Request all JavaScript resource sources from backend. // Run eslint on the sources, produce a set of Errors and Warnings for each resource. return [...]; } }, ... ] } }; Usage: UI to perform audits: _clickHandlerPerformAudits(event) { let testSuite = new WI.AccessibilityAuditSuite; let testCases = testSuite.testCases(); for (let testCase of testCases) { // Create UI for test suite // Update UI that test is running testCase.perform().then((results) => { // Update UI that test is no longer running. // Display results. }); } } Things that would still need to be made: • When a AuditTestCase produces a set of Results, how do they get displayed? - Should there be one View specified by the AuditTestCase that knows how to display all of the AuditResults for that test? - Should there be on View specified by each AuditResult that knows how to display itself individually? • The Promises.all approach above means you only get an update when ALL tests are done but not when some are done. - This could be improved. Maybe the events approach you have is easier, but I think async/await may be simpler.
> • The Promises.all approach above means you only get an update when ALL > tests are done but not when some are done. > - This could be improved. Maybe the events approach you have is > easier, but I think async/await may be simpler. Err, I removed the Promise.all approach in the end, but a parallel point still exists. One approach is to use promises, the other is to use events. Both work, which one is better is probably just taste.
(In reply to Devin Rousso from comment #27) > Comment on attachment 332253 [details] > WIP Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332253&action=review > > There are a lot of style issues in many of the files. I have to stop > reading because I have to go to a class, but I hope you take what I've > commented so far and look a the rest of the files to make sure you fix them > if they appear elsewhere. For the first round of feedback on a patch like this we should focus on architecture / approach. If large sections of code will change, then commenting on that code is mostly just noise. Once we get the architecture right, we can cleanup the style. But its very easy to get lost reading this patch with a bunch of style/nit comments. -- My main architecture feedback was that the test, title, description, and results are currently spread out in a bunch of places. I'd like to see these pieces grouped better. So adding, updating, debugging, and reading test cases is easier. I tried to draft such an approach in Comment 28. But I got distracted and never really got to take the idea through to completion, so I think there is still room to iterate on it and make it better.
(In reply to Devin Rousso from comment #27) > Comment on attachment 332253 [details] > WIP Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332253&action=review > > There are a lot of style issues in many of the files. I have to stop > reading because I have to go to a class, but I hope you take what I've > commented so far and look a the rest of the files to make sure you fix them > if they appear elsewhere. > There's definitely something in your comments for me to take away. Thanks for taking the time
(In reply to Joseph Pecoraro from comment #28) > You should not put this code in UserInterface/Test. Those files are meant > for WebInspector tests. Lets create a new directory, UserInterface/Audits > for audit related code. > > I think the general model can be improved a bit. Right now you have tests > and their descriptions / results spread out across multiple files, and its > very hard to relate each of the pieces. > > I think you will want to very closely group the test name, title, > description, test function, and test result generation. > > I thought about this for a bit and this is what I came up with. I don't > think its perfect, and I didn't fully think through how we would display > this in a UI, but I think this is easier to follow: > I love this, let me take all of your feed back and work on improvements.
Created attachment 333677 [details] WIP Patch 2 UI is still pending.
Cool! You should attach a picture too.
Created attachment 333944 [details] Screenshot for patch 2
Added.
Awesome! A couple of quick comments from just looking at the screenshot: * Remove the space before the commas in "1 Passed , 4 Failed , 1 Warnings" * Use the proper singular terms in that string, for example "1 Warning". * The text for DOM elements without children should be indented at the same level as those that do. So everything lines up. * The headers should use a darker red and darker yellow for the borders — not grey. This will make them look better. * Similar for the disclosure arrows in the headers. A transparent black instead of grey will blend better.
Comment on attachment 333677 [details] WIP Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=333677&action=review > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:55 > +localizedStrings["%s \u2014 Passed"] = "%s \u2014 Passed"; > +localizedStrings["%s \u2014 Failed"] = "%s \u2014 Failed"; > +localizedStrings["%s \u2014 Warning"] = "%s \u2014 Warning"; Nit: Alphabetical mismatch. Keep this chunk of "%s \u…" strings with the others above. You may also want to include a comment mentioning \u2014 is an em dash. > Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:9 > + async testWithQuerySelector(selector, callback) What other test functions were you thinking of? In addition to this simple selector test, perhaps implement: • a more complex selector chain (recursively run selector n against result or previous selector in the array) E.g. If one, [selector] then this operates as it currently does. If multiple, you chain them together as sub-level selectors... e.g. [selector1, selector2] Recursively run current.querySelectorAll(selector2) for each of selector1 results, instead of on document. • negative selectors test (e.g. no headings on this page) .... This could also be a parameter (match all, match one, match none). • JavaScript-based DOM tests (pass in a test function) • WebCore-based tests (pass node to a callback across the JSON bridge) You don't need to have all these included in v1, but listing the planned ones out (even as comments) will help inform the design. > Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:32 > + name: "testMispelledAriaLabelledBy", misspelled > Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:38 > + title: "aria-labelledby is misspelled as aria-labeledby.", These title and hint strings should be localized. > Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:48 > + name: "testImageAltText", > + errorDetails: { > + code: 11, > + title: "Image elements should have an 'alt' attribute.", This test could be changed or removed for v1. Yes, this is a validation error (save it for a validation audit if we ever add one), but this does not necessarily coincide with an accessibility error. Examples: <img src="foo" aria-label="bar"> would fail this test but have no accessibility impact to the user. Likewise <div hidden><img src="foo"></div> Instead you could add a testImageLabel block that validated a non-empty computed label on images (computedRole, not tagName) that are not ignored (hidden elements, etc.) by accessibility. > Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:61 > + name: "testTabIndex", > + errorDetails: { > + code: 12, > + title: "Attribute 'tabindex' should either be 0 or -1.", Despite the common anti-pattern, there are valid use instances of this. As such we should not default to a warning unless we can be relatively assured they are doing the wrong thing. > Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:71 > + name: "testFormsToHaveControls", Same feedback as the image test above. This is a DOM/markup validation test, not an accessibility test. Probably best to remove it for v1 or rewrite it. > Source/WebInspectorUI/UserInterface/Models/AuditReport.js:80 > + if (!this.failed) > + return WI.UIString("%s \u2014 Passed").format(this._reportName); Since the test name is an identifier, there may be value in a more human readable version of the test name. Have you considered adding these as Loc string keys? localizedStrings["testImageLabel"] = " Test: Image must have a label."; > Source/WebInspectorUI/UserInterface/Test/AuditTests.js:29 > + name: "testMispelledAriaLabelledBy", Misspelled
Comment on attachment 333677 [details] WIP Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=333677&action=review Mostly style notes. >> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:55 >> +localizedStrings["%s \u2014 Warning"] = "%s \u2014 Warning"; > > Nit: Alphabetical mismatch. Keep this chunk of "%s \u…" strings with the others above. You may also want to include a comment mentioning \u2014 is an em dash. This is a generated file. > Source/WebInspectorUI/UserInterface/Base/Main.js:163 > + // (Read: Reviewers this is just to make it easier to build. Will work on hiding in a feature flag.) I'd recommend adding the flag in this patch. It is a trivial to do. See SettingsTabContentView.prototype._createExperimentalSettingsView, and follow the example set by WI.settings.experimentalEnableLayersTab. > Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:35 > + get testSuites() Add "// Public" comment above this property. > Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:37 > + return this._testSuites; Simple properties can be written as a single line. We consider a property to be simple if it performs no logic, and accesses nothing but a single member variable. The following would not be considered simple, even though it is concise: get foo() { return this._items.length === 1; } For properties having both a getter and setter, they should be formatted identically. If one or the other cannot be written on a single line, neither should. > Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:42 > + this._testSuites = this._testSuites.map((suite) => { Simple arrow functions like this can be written on one line, with an implied return: this._testSuites = this._testSuites.map((suite) => new suite); > Source/WebInspectorUI/UserInterface/Main.html:242 > + <link rel="stylesheet" href="Views/AuditIssueView.css"> Stylesheets should be listed alphabetically since they don't have dependencies. > Source/WebInspectorUI/UserInterface/Main.html:553 > + <script src="Views/TextView.js"></script> Script files should be alphabetical within their group, since (by convention) no files in the group depend on one another. > Source/WebInspectorUI/UserInterface/Main.html:683 > + <script src="Views/AuditContentView.js"></script> Ditto. > Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:5 > + super("accessibility", WI.UIString("Accessibility")); Newline after super() call. > Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:18 > + static testCaseDescriptors() Static functions should be listed before public functions. > Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:23 > + test: function() { Use method-style syntax (or whatever it's called): test() { ... } > Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:84 > + Remove newline. > Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:87 > + for (let i = 0; i < formNodeIds.length; i++) { Use for...of. > Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:89 > + Remove newline. > Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:90 > + if (!failedControls.length) Decrease indent. > Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:111 > + Remove newline. > Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:117 > + if(failedElement.length) Space after if, before open parenthesis. > Source/WebInspectorUI/UserInterface/Models/AuditReport.js:43 > + for (let i = 0; i < this._resultData.length; i++) { Use for...of. > Source/WebInspectorUI/UserInterface/Models/AuditReport.js:50 > + Remove the newlines between all of these properties. > Source/WebInspectorUI/UserInterface/Models/AuditReport.js:65 > + get failed() { return !!this._totalFailedResults.length; } Should not be a single line, since it performs logic. > Source/WebInspectorUI/UserInterface/Models/AuditReport.js:91 > + // Public This comment should go before the public properties above. > Source/WebInspectorUI/UserInterface/Models/AuditReport.js:100 > + _sortResultsByLogLevel(result) Add "// Private" before this. > Source/WebInspectorUI/UserInterface/Models/AuditResult.js:41 > + get failed() Add "// Public" before this. > Source/WebInspectorUI/UserInterface/Models/AuditResult.js:54 > + } These should all be one line, with no newlines between. > Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:41 > + get name() { return this._name; } // Public > Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:44 > + _createErrorDataForAuditResult(resultData) // Private > Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:49 > + } Braces are not needed when the body is a single line (applies to all control flow statements). > Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:58 > + Remove newline. > Source/WebInspectorUI/UserInterface/Models/AuditTestSuite.js:43 > + static testCaseDescriptors() { throw WI.NotImplementedError.subclassMustOverride(); } Functions are always written with open-close braces on their own lines. The order of class members should be: - static functions - properties - public function - protected functions - private functions > Source/WebInspectorUI/UserInterface/Models/AuditTestSuite.js:46 > + Remove the newline. > Source/WebInspectorUI/UserInterface/Models/AuditTestSuite.js:48 > + Ditto. > Source/WebInspectorUI/UserInterface/Models/AuditTestSuite.js:51 > + _buildTestCasesFromDescrptors(descriptors) Add '// Private' comment above this function signature. > Source/WebInspectorUI/UserInterface/Models/AuditTestSuite.js:53 > + for (var i = 0; i < descriptors.length; i++) { Prefer let over var. Also should be for...of since the index isn't used. > Source/WebInspectorUI/UserInterface/Models/AuditTestSuite.js:55 > + if (typeof(testCase.name) !== "string" || testCase.name === "" || !testCase.name) The last two check are redundant, since `!"" === true`. > Source/WebInspectorUI/UserInterface/Models/AuditTestSuite.js:59 > + let errorDetails = testCase.errorDetails; You can use destructuring here: let {name, errorDetails, test} = testCase; test = test || () => {}; > Source/WebInspectorUI/UserInterface/Views/AuditContentView.js:55 > + Remove newline. > Source/WebInspectorUI/UserInterface/Views/AuditContentView.js:58 > + layout() Add "// Protected" comment about this method. > Source/WebInspectorUI/UserInterface/Views/AuditContentView.js:66 > + _showReport(event) Add "// Private" comment about this method. > Source/WebInspectorUI/UserInterface/Views/AuditContentView.js:95 > + if (toShow) This branch can be rewritten as: resultDetailsView.element.classList.toggle("filtered-out", !toShow);
Comment on attachment 333677 [details] WIP Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=333677&action=review Addressed some of Tim's UI feedback. Responded to or addressed Matt and James's feedback in the latest patch. >> Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:9 >> + async testWithQuerySelector(selector, callback) > > What other test functions were you thinking of? In addition to this simple selector test, perhaps implement: > > • a more complex selector chain (recursively run selector n against result or previous selector in the array) E.g. If one, [selector] then this operates as it currently does. If multiple, you chain them together as sub-level selectors... e.g. [selector1, selector2] Recursively run current.querySelectorAll(selector2) for each of selector1 results, instead of on document. > > • negative selectors test (e.g. no headings on this page) .... This could also be a parameter (match all, match one, match none). > > • JavaScript-based DOM tests (pass in a test function) > > • WebCore-based tests (pass node to a callback across the JSON bridge) > > You don't need to have all these included in v1, but listing the planned ones out (even as comments) will help inform the design. Recusing and negating selectors will likely be two other public methods implemented similarly to testWithQuerySelector. JS-based DOM test and WebCore-based tests are already in place with this architecture. In the test descriptor below, a user can define a custom test(), in which they can do anything including all of your suggestions. >> Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:38 >> + title: "aria-labelledby is misspelled as aria-labeledby.", > > These title and hint strings should be localized. Noted. Will implement as localized string once the copy is finalized. >> Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:48 >> + title: "Image elements should have an 'alt' attribute.", > > This test could be changed or removed for v1. Yes, this is a validation error (save it for a validation audit if we ever add one), but this does not necessarily coincide with an accessibility error. Examples: <img src="foo" aria-label="bar"> would fail this test but have no accessibility impact to the user. Likewise <div hidden><img src="foo"></div> > > Instead you could add a testImageLabel block that validated a non-empty computed label on images (computedRole, not tagName) that are not ignored (hidden elements, etc.) by accessibility. Removed for now. >> Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:61 >> + title: "Attribute 'tabindex' should either be 0 or -1.", > > Despite the common anti-pattern, there are valid use instances of this. As such we should not default to a warning unless we can be relatively assured they are doing the wrong thing. Since we are not implementing the `info` log level, I think `warning` is fair as long as what you convey is included in the "hint" copy. >> Source/WebInspectorUI/UserInterface/Models/AccessibilityTestSuite.js:71 >> + name: "testFormsToHaveControls", > > Same feedback as the image test above. This is a DOM/markup validation test, not an accessibility test. Probably best to remove it for v1 or rewrite it. Removed for now. >> Source/WebInspectorUI/UserInterface/Models/AuditReport.js:80 >> + return WI.UIString("%s \u2014 Passed").format(this._reportName); > > Since the test name is an identifier, there may be value in a more human readable version of the test name. Have you considered adding these as Loc string keys? > > localizedStrings["testImageLabel"] = " Test: Image must have a label."; sounds like a good idea. Will implement these as localized strings when implementing these tests. >> Source/WebInspectorUI/UserInterface/Models/AuditTestSuite.js:59 >> + let errorDetails = testCase.errorDetails; > > You can use destructuring here: > > let {name, errorDetails, test} = testCase; > test = test || () => {}; for some reason I am able to do `test = ()=>{}` but not `test = test || () => {};`. I got a syntax error, hence leaving the function as is for now.
Created attachment 334479 [details] WIP Patch UI and copy are both pending.
> for some reason I am able to do `test = ()=>{}` but not `test = test || () > => {};`. I got a syntax error, hence leaving the function as is for now. unable*
https://bugs.webkit.org/show_bug.cgi?id=183090