[webkit-reviews] review denied: [Bug 127447] Web Inspector: AX: Accessibility Node Inspection : [Attachment 223506] patch with test coverage
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Feb 7 15:49:57 PST 2014
chris fleizach <cfleizach at apple.com> has denied review:
Bug 127447: Web Inspector: AX: Accessibility Node Inspection
https://bugs.webkit.org/show_bug.cgi?id=127447
Attachment 223506: patch with test coverage
https://bugs.webkit.org/attachment.cgi?id=223506&action=review
------- Additional Comments from chris fleizach <cfleizach at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=223506&action=review
> LayoutTests/accessibility/roles-computedRoleString-expected.txt:2
> +This tests that native elements and ARIA overrides result in the same ARIA
computed role, regardless of platform.
You should add an entry in TestExpectation in GTK marking this as failing until
computedRoleString is implemented
> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2067
> +- (NSString*)computedRoleString
need a space before the *
> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2113
> + if ([attributeName isEqualToString: @"AXAriaRole"])
This should be AXARIARole and no space after the ":" even though other lines
are doing that.
> Source/WebCore/inspector/InspectorDOMAgent.cpp:879
> + Element* element = assertElement(errorString, nodeId);
Is there a reason you need to get the element here? you don't see to use it.
It seems just getting the node, and checking if that's nil should be good
enough
> Source/WebCore/inspector/InspectorDOMAgent.cpp:1412
> +PassRefPtr<TypeBuilder::DOM::AccessibilityProperties>
InspectorDOMAgent::buildObjectForAccessibilityProperties(Node* node)
you should probably ASSERT(node) in here, and
if (!node)
return nullptr
> Source/WebCore/inspector/InspectorDOMAgent.cpp:1424
> + String label = "";
i don't think you need to initialize these strings to anything
> Source/WebCore/inspector/InspectorDOMAgent.cpp:1429
> + isAccessibilityObject = true;
this should probably check accessibilityIsIgnored() to determine true/false
> Source/WebCore/inspector/protocol/DOM.json:55
> + "description": "A structure holding EventListener properties."
this seems unrelated to this patch
> Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:598
> + NSString* computedRoleString = descriptionOfValue([m_element
accessibilityAttributeValue:@"AXAriaRole"], m_element);
* on the wrong side
> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:642
> + NSString* computedRoleString = descriptionOfValue([m_element
accessibilityAttributeValue:@"AXAriaRole"], m_element);
* on wrong side
More information about the webkit-reviews
mailing list