Bug 179494 – AX: AOM: Implement AccessibleNode class and support label and role attributes

• Component: Accessibility
• Status: RESOLVED FIXED

[Nan Wang]

Phase 1 of AOM
Implement base structure, new classes. Support label and role properties.

<rdar://problem/35077686>

[Nan Wang]

Created attachment 326474 [details]
initial patch

[Nan Wang]

Created attachment 326477 [details]
initial patch

merge with TOT

[chris fleizach]

Comment on attachment 326474 [details]
initial patch

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

> Source/WebCore/accessibility/AccessibilityListBoxOption.cpp:145
> +    const AtomicString& ariaLabel = getPropertyStringValue(AXPropertyValueKeys::kAXLabel);

can we use a namespace to avoid appending AXPropertyValueKeys:: everywhere?

> Source/WebCore/accessibility/AccessibilityObject.cpp:2151
> +    if (!is<Element>(node))

we should do if (Element* element = this->element())
here

> Source/WebCore/accessibility/AccessibleNode.cpp:83
> +

is there a way to avoid encoding "StringValue" into the name and just have it choose the right type based on return type?

> Source/WebCore/accessibility/AccessibleNode.cpp:84
> +const String AccessibleNode::getAOMPropertyOrARIAAttributeStringValue(Element* element, AXPropertyValueKeys propertyKey)

this name is a bit of a mouthful
what if it was just

elementAttributeStringValue()

then what happens inside is the right thing, rather than the name enumerating the things that might happen inside of it

> Source/WebCore/accessibility/AccessibleNode.cpp:98
> +        return element->attributeWithoutSynchronization(roleAttr);

can we make the switch set the attributeName to a variable, then below that call 

element->attributeWithoutSynchronization(X);

one time

> Source/WebCore/accessibility/AccessibleNode.cpp:104
> +const String AccessibleNode::getStringProperty(Element* element, AXPropertyValueKeys propertyKey)

can this be a String&

[Build Bot]

Comment on attachment 326477 [details]
initial patch

Attachment 326477 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5167242

New failing tests:
accessibility/accessibility-object-model.html
js/dom/dom-static-property-for-in-iteration.html

[Build Bot]

Created attachment 326482 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 326477 [details]
initial patch

Attachment 326477 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5167216

New failing tests:
js/dom/dom-static-property-for-in-iteration.html

[Build Bot]

Created attachment 326483 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Ryosuke Niwa]

Comment on attachment 326477 [details]
initial patch

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

> Source/WebCore/accessibility/AccessibleNode.h:45
> +typedef Variant<
> +    std::nullptr_t,
> +    String,
> +    bool,
> +    int
> +> PropertyValueVariant;

Please put this into a single line.

> Source/WebCore/accessibility/AccessibleNode.h:51
> +enum class AXPropertyValueKeys {
> +    kAXNone,
> +    kAXRole,
> +    kAXLabel
> +};

Please remove k prefix. We don't use that in WebKit's C++ code.
Also, there's no need prefix each value with AX since enum class requires the enum name as prefix whenever using them.

[Build Bot]

Comment on attachment 326477 [details]
initial patch

Attachment 326477 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5167317

New failing tests:
accessibility/accessibility-object-model.html
js/dom/dom-static-property-for-in-iteration.html

[Build Bot]

Created attachment 326488 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Ryosuke Niwa]

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

[Nan Wang]

Comment on attachment 326474 [details]
initial patch

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

>> Source/WebCore/accessibility/AccessibilityListBoxOption.cpp:145
>> +    const AtomicString& ariaLabel = getPropertyStringValue(AXPropertyValueKeys::kAXLabel);
> 
> can we use a namespace to avoid appending AXPropertyValueKeys:: everywhere?

I don't think we can do that for enum class.

>> Source/WebCore/accessibility/AccessibleNode.cpp:83
>> +
> 
> is there a way to avoid encoding "StringValue" into the name and just have it choose the right type based on return type?

I tried template function but I think it gets more complicated if we are going to return value, reference and pointer in future.
I think it's quite clear right now since the clients know what type of return value they want and can call the corresponding functions.

>> Source/WebCore/accessibility/AccessibleNode.cpp:98
>> +        return element->attributeWithoutSynchronization(roleAttr);
> 
> can we make the switch set the attributeName to a variable, then below that call 
> 
> element->attributeWithoutSynchronization(X);
> 
> one time

Added a AOM <-> ARIA map

>> Source/WebCore/accessibility/AccessibleNode.cpp:104
>> +const String AccessibleNode::getStringProperty(Element* element, AXPropertyValueKeys propertyKey)
> 
> can this be a String&

I hit a crash assert saying accessing the AtomicString from different threads if change to String&

[Nan Wang]

Created attachment 326527 [details]
patch

update based on review.

[Nan Wang]

Created attachment 326563 [details]
patch

fix build?

[chris fleizach]

Comment on attachment 326563 [details]
patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:13261
> +		DE5F854A1FA1ABBE006DB63A /* UnifiedSource17-mm.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = "UnifiedSource17-mm.mm"; sourceTree = "<group>"; };

all this stuff seems unnecessary. can you undo

[Nan Wang]

(In reply to chris fleizach from comment #15)
> Comment on attachment 326563 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326563&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:13261
> > +		DE5F854A1FA1ABBE006DB63A /* UnifiedSource17-mm.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = "UnifiedSource17-mm.mm"; sourceTree = "<group>"; };
> 
> all this stuff seems unnecessary. can you undo

Ok

[Nan Wang]

Created attachment 326609 [details]
patch

update project file

[Ryosuke Niwa]

Did you see my review comments?

[Nan Wang]

(In reply to Ryosuke Niwa from comment #18)
> Did you see my review comments?

Yes I think I've addressed those.

[Nan Wang]

Ryosuke, can you take another look when you have a chance? Thanks!

[Ryosuke Niwa]

Comment on attachment 326609 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityListBoxOption.cpp:145
> -    const AtomicString& ariaLabel = getAttribute(aria_labelAttr);
> +    const AtomicString& ariaLabel = getPropertyStringValue(AXPropertyName::Label);

Again, please don't use "get" prefix. That should be only used when there is an out-argument,
or we're forced to use it because it's exposed as Web API.

> Source/WebCore/accessibility/AccessibilityObject.cpp:2148
> +bool AccessibilityObject::hasPropertyValue(AXPropertyName propertyKey) const

We should call this hasProperty. "value" doesn't clarify anything beyond that.

> Source/WebCore/accessibility/AccessibilityObject.cpp:2155
> +const String AccessibilityObject::getPropertyStringValue(AXPropertyName propertyKey) const

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

Also, since this function returns the string value of a property
it's probably more appropriate to call it stringValueForProperty.

> Source/WebCore/accessibility/AccessibleNode.cpp:40
> +static ARIAAttributeMap* gARIAAttributeMap = nullptr;

We should use NeverDestroyed inside ariaAttributeMap.
Please see canAttachAuthorShadowRoot in Element.cpp and follow the same style.
r- because of this.

> Source/WebCore/accessibility/AccessibleNode.cpp:66
> +static bool isPropertyValueTypeString(AXPropertyName propertyName)

Is this function supposed to return true when the property value is a string?
Then it should recalled isPropertyValueString instead.

> Source/WebCore/accessibility/AccessibleNode.cpp:77
> +bool AccessibleNode::hasPropertyValue(Element* element, AXPropertyName propertyName)

Again, this should be called hasProperty.
Please make this function take Element& instead.

> Source/WebCore/accessibility/AccessibleNode.cpp:82
> +    if (AccessibleNode* accessibleNode = element->existingAccessibleNode()) {

Use auto*.

> Source/WebCore/accessibility/AccessibleNode.cpp:83
> +        if (accessibleNode->m_axPropertyMapping.contains(propertyName))

Why not just m_propertyMap? That naming matches ariaAttributeMap() better.
It certainly shouldn't have "ax" prefix since everything in AccessibleNode is related to accessibility by definition.

> Source/WebCore/accessibility/AccessibleNode.cpp:87
> +    // Fall back on the equivalent ARIA attribute.

This comment repeats what the code says. Please remove.

> Source/WebCore/accessibility/AccessibleNode.cpp:90
> +    

Nit: Trailing whitespaces.

> Source/WebCore/accessibility/AccessibleNode.cpp:94
> +const PropertyValueVariant AccessibleNode::valueForProperty(Element* element, AXPropertyName propertyName)

Please make this take Element& instead.

> Source/WebCore/accessibility/AccessibleNode.cpp:99
> +    if (AccessibleNode* accessibleNode = element->existingAccessibleNode()) {

Use auto*.

> Source/WebCore/accessibility/AccessibleNode.cpp:103
> +    

Nit: Trailing whitespaces.

> Source/WebCore/accessibility/AccessibleNode.cpp:109
> +    if (!element)

Use Element&.

> Source/WebCore/accessibility/AccessibleNode.cpp:119
> +    

Nit: Trailing whitespaces

> Source/WebCore/accessibility/AccessibleNode.h:67
> +    

Nit: Trailing whitespace.

> Source/WebCore/accessibility/AccessibleNode.h:70
> +    

Nit: Trailing whitespace.

> Source/WebCore/accessibility/AccessibleNode.h:73
> +    

Nit: Trailing whitespace.

> Source/WebCore/accessibility/AccessibleNode.h:76
> +    

Nit: Trailing whitespace.

> Source/WebCore/accessibility/AccessibleNode.h:87
> +    HashMap<AXPropertyName, PropertyValueVariant, WTF::IntHash<AXPropertyName>, AXPropertyHashTraits> m_axPropertyMapping;

Again, this should be called m_propertyMap.

> Source/WebCore/dom/Element.cpp:3697
> +    

Nit: Trailing whitespace.

> Source/WebCore/dom/Element.cpp:3708
> +    

Nit: Trailing whitespace.

> LayoutTests/accessibility/accessibility-object-model.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Please use modern DOCTYPE: <!DOCTYPE html>

[Nan Wang]

Created attachment 326895 [details]
patch

Thanks for the review!
Addressed review comments.

[Ryosuke Niwa]

Please update the patch for trunk.

[Nan Wang]

(In reply to Ryosuke Niwa from comment #23)
> Please update the patch for trunk.

working on it

[Nan Wang]

Created attachment 326899 [details]
patch

update

[Ryosuke Niwa]

Comment on attachment 326899 [details]
patch

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

r=me assuming you can remove gc() call in the test.

> Source/WebCore/accessibility/AccessibilityObject.cpp:2161
>      

Nit: Trailing whitespaces.

> Source/WebCore/accessibility/AccessibilityObject.h:895
> +    

Nit: Trailing whitespaces.

> Source/WebCore/accessibility/AccessibleNode.cpp:36
> +    

Nit: Trailing whitespaces.

> Source/WebCore/accessibility/AccessibleNode.cpp:43
> +struct AttributeEntry {
> +    AXPropertyName name;
> +    QualifiedName ariaAttribute;
> +};

There is no need to declare this time separately like this. Just declare it as you declare the variable as in:
const struct AttributeEntry {
    AXPropertyName name;
    QualifiedName ariaAttribute;
} attributes[] = {
...
};
Or you could also use std::pair<AXPropertyName, QualifiedName> also.

> Source/WebCore/accessibility/AccessibleNode.cpp:93
> +    

Nit: Trailing whitespaces.

> Source/WebCore/accessibility/AccessibleNode.cpp:118
> +        if (m_propertyMap.contains(propertyName))
> +            m_propertyMap.remove(propertyName);

Just call m_propertyMap.remove(propertyName).
There is no need to check contains(propertyName).

> Source/WebCore/accessibility/AccessibleNode.cpp:147
> +    

Nit: Trailing whitespaces.

> Source/WebCore/accessibility/AccessibleNode.h:39
> +typedef Variant<std::nullptr_t, String, bool, int> PropertyValueVariant;

When is nullptr_t used?

> Source/WebCore/accessibility/AccessibleNode.h:89
> +    

Nit: Trailing whitespaces.

> Source/WebCore/dom/Element.h:555
> +    

Nit: Trailing whitespaces.

> Source/WebCore/dom/Element.idl:106
> +    // Accessibility Object Model

There's no need to add this comment. The attribute is guarded with a runtime flag named AccessibilityObjectModel.
Please remove.

> Source/WebCore/dom/ElementRareData.h:113
> +    

Nit: Trailing whitespaces.

> Source/WebCore/dom/ElementRareData.h:150
> +    

Nit: Trailing whitespaces.

> Source/WebCore/page/RuntimeEnabledFeatures.h:227
> +    

Nit: Trailing whitespaces.

> Source/WebCore/page/RuntimeEnabledFeatures.h:350
> +    

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:24
> +        

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:26
> +        

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:34
> +    

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:42
> +    

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:49
> +        

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:54
> +    

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:57
> +        

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:61
> +        

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:67
> +        

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:72
> +    

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:77
> +        

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:83
> +    

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:86
> +        

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:92
> +        

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:98
> +    

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:108
> +        

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:114
> +        gc();

Why gc()? Something is not right if you need to call gc() to make the test run property.

> LayoutTests/accessibility/accessibility-object-model.html:117
> +    

Nit: Trailing whitespaces.

[Nan Wang]

Comment on attachment 326899 [details]
patch

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

>> Source/WebCore/accessibility/AccessibleNode.h:39
>> +typedef Variant<std::nullptr_t, String, bool, int> PropertyValueVariant;
> 
> When is nullptr_t used?

will remove.

>> LayoutTests/accessibility/accessibility-object-model.html:114
>> +        gc();
> 
> Why gc()? Something is not right if you need to call gc() to make the test run property.

Sure I will remove this.

[Nan Wang]

Committed r224871: <https://trac.webkit.org/changeset/224871>

[Nan Wang]

Addressed review comments and removed gc() in test.