[Webkit-unassigned] [Bug 179494] AX: AOM: Implement AccessibleNode class and support label and role attributes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=179494

--- Comment #11 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 326477
  --> https://bugs.webkit.org/attachment.cgi?id=326477
initial patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:412
> -    auto& roleValue = downcast<Element>(*node).attributeWithoutSynchronization(roleAttr);
> +    auto& roleValue = AccessibleNode::getAOMPropertyOrARIAAttributeStringValue(downcast<Element>(node), AXPropertyValueKeys::kAXRole);

Don't use "get" prefix since it doesn't have an out argument:
https://webkit.org/code-style-guidelines/#names-out-argument

I think it's better to call this function something more simple like AccessibleNode::propertyValueForElement(~)
or AccessibleNode::effectiveValueForElement
since how the value is computed from AOM & ARIA is more of an implementation detail.

Calling this AX property is a problematic too though because "property" is not a well defined term in Web IDL or ARIA.
The spec is super vague about this. Filed https://github.com/WICG/aom/issues/95

> Source/WebCore/accessibility/AccessibleNode.cpp:48
> +void AccessibleNode::ref()
> +{
> +    m_element.ref();
> +}
> +
> +void AccessibleNode::deref()
> +{
> +    m_element.deref();
> +}

Please inline these.

> Source/WebCore/accessibility/AccessibleNode.h:47
> +enum class AXPropertyValueKeys {

This should be singular form: AXPropertyValueKey.
But why is this property value key? Why don't we just call it AXPropertyName instead?
Anyhow, this term needs to be well defined in the spec in the first place.

> Source/WebCore/accessibility/AccessibleNode.h:87
> +    typedef HashMap<AXPropertyValueKeys, PropertyValueVariant, WTF::IntHash<AXPropertyValueKeys>, AXPropertyHashTraits> VariantsMap;

There is no need to typedef this since it's only used once.
In general, adding new typedef would make it harder to decipher code, so we'd using auto wherever possible
and writing the full type elsewhere these days.

> Source/WebCore/accessibility/AccessibleNode.h:94
> +    // This object's owner Element
> +    Element& m_element;

Instead of adding a comment, call this m_ownerElement.

> Source/WebCore/accessibility/AccessibleNode.idl:26
> +// Accessibility Object Model node

This comment is redundant.

> Source/WebCore/accessibility/AccessibleNode.idl:27
> +// Explainer: https://github.com/WICG/aom/blob/master/explainer.md

You need to refer to the explainer. Include it in the change log instead.

> Source/WebCore/accessibility/AccessibleNode.idl:28
> +// Spec: https://wicg.github.io/aom/spec/

Put this in the change log instead.

> Source/WebCore/dom/Element.idl:107
> +    [EnabledAtRuntime=AccessibilityObjectModel] readonly attribute AccessibleNode? accessibleNode;

This should probably have SameObject even though we don't support it yet.
Filed https://github.com/WICG/aom/issues/97 to track the spec bug.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171110/b740ebc0/attachment.html>