<html>
    <head>
      <base href="https://bugs.webkit.org/">
    </head>
    <body>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - AX: AOM: Implement AccessibleNode class and support label and role attributes"
   href="https://bugs.webkit.org/show_bug.cgi?id=179494#c11">Comment # 11</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - AX: AOM: Implement AccessibleNode class and support label and role attributes"
   href="https://bugs.webkit.org/show_bug.cgi?id=179494">bug 179494</a>
              from <span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span></b>
        <pre>Comment on <span class=""><a href="attachment.cgi?id=326477&action=diff" name="attach_326477" title="initial patch">attachment 326477</a> <a href="attachment.cgi?id=326477&action=edit" title="initial patch">[details]</a></span>
initial patch

View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=326477&action=review">https://bugs.webkit.org/attachment.cgi?id=326477&action=review</a>

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

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

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 <a href="https://github.com/WICG/aom/issues/95">https://github.com/WICG/aom/issues/95</a>

<span class="quote">> Source/WebCore/accessibility/AccessibleNode.cpp:48
> +void AccessibleNode::ref()
> +{
> +    m_element.ref();
> +}
> +
> +void AccessibleNode::deref()
> +{
> +    m_element.deref();
> +}</span >

Please inline these.

<span class="quote">> Source/WebCore/accessibility/AccessibleNode.h:47
> +enum class AXPropertyValueKeys {</span >

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.

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

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.

<span class="quote">> Source/WebCore/accessibility/AccessibleNode.h:94
> +    // This object's owner Element
> +    Element& m_element;</span >

Instead of adding a comment, call this m_ownerElement.

<span class="quote">> Source/WebCore/accessibility/AccessibleNode.idl:26
> +// Accessibility Object Model node</span >

This comment is redundant.

<span class="quote">> Source/WebCore/accessibility/AccessibleNode.idl:27
> +// Explainer: <a href="https://github.com/WICG/aom/blob/master/explainer.md">https://github.com/WICG/aom/blob/master/explainer.md</a></span >

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

<span class="quote">> Source/WebCore/accessibility/AccessibleNode.idl:28
> +// Spec: <a href="https://wicg.github.io/aom/spec/">https://wicg.github.io/aom/spec/</a></span >

Put this in the change log instead.

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

This should probably have SameObject even though we don't support it yet.
Filed <a href="https://github.com/WICG/aom/issues/97">https://github.com/WICG/aom/issues/97</a> to track the spec bug.</pre>
        </div>
      </p>


      <hr>
      <span>You are receiving this mail because:</span>

      <ul>
          <li>You are the assignee for the bug.</li>
      </ul>
    </body>
</html>