Bug 179494 - AX: AOM: Implement AccessibleNode class and support label and role attributes
Summary: AX: AOM: Implement AccessibleNode class and support label and role attributes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 179255
  Show dependency treegraph
 
Reported: 2017-11-09 11:23 PST by Nan Wang
Modified: 2017-11-15 10:36 PST (History)
5 users (show)

See Also:


Attachments
initial patch (59.79 KB, patch)
2017-11-09 12:04 PST, Nan Wang
no flags Details | Formatted Diff | Diff
initial patch (84.53 KB, patch)
2017-11-09 12:29 PST, Nan Wang
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (2.20 MB, application/zip)
2017-11-09 13:34 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (3.09 MB, application/zip)
2017-11-09 13:42 PST, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-elcapitan (3.25 MB, application/zip)
2017-11-09 14:17 PST, Build Bot
no flags Details
patch (86.46 KB, patch)
2017-11-09 18:30 PST, Nan Wang
no flags Details | Formatted Diff | Diff
patch (86.09 KB, patch)
2017-11-09 23:22 PST, Nan Wang
no flags Details | Formatted Diff | Diff
patch (59.85 KB, patch)
2017-11-10 11:19 PST, Nan Wang
rniwa: review-
Details | Formatted Diff | Diff
patch (59.20 KB, patch)
2017-11-14 11:17 PST, Nan Wang
no flags Details | Formatted Diff | Diff
patch (59.08 KB, patch)
2017-11-14 11:44 PST, Nan Wang
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nan Wang 2017-11-09 11:23:12 PST
Phase 1 of AOM
Implement base structure, new classes. Support label and role properties.

<rdar://problem/35077686>
Comment 1 Nan Wang 2017-11-09 12:04:09 PST
Created attachment 326474 [details]
initial patch
Comment 2 Nan Wang 2017-11-09 12:29:53 PST
Created attachment 326477 [details]
initial patch

merge with TOT
Comment 3 chris fleizach 2017-11-09 12:34:24 PST
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&
Comment 4 Build Bot 2017-11-09 13:34:43 PST
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
Comment 5 Build Bot 2017-11-09 13:34:45 PST
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
Comment 6 Build Bot 2017-11-09 13:42:36 PST
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
Comment 7 Build Bot 2017-11-09 13:42:37 PST
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
Comment 8 Ryosuke Niwa 2017-11-09 13:55:08 PST
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.
Comment 9 Build Bot 2017-11-09 14:17:56 PST
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
Comment 10 Build Bot 2017-11-09 14:17:57 PST
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
Comment 11 Ryosuke Niwa 2017-11-09 16:47:43 PST
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.
Comment 12 Nan Wang 2017-11-09 18:28:52 PST
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&
Comment 13 Nan Wang 2017-11-09 18:30:38 PST
Created attachment 326527 [details]
patch

update based on review.
Comment 14 Nan Wang 2017-11-09 23:22:30 PST
Created attachment 326563 [details]
patch

fix build?
Comment 15 chris fleizach 2017-11-10 10:37:41 PST
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
Comment 16 Nan Wang 2017-11-10 10:44:54 PST
(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
Comment 17 Nan Wang 2017-11-10 11:19:42 PST
Created attachment 326609 [details]
patch

update project file
Comment 18 Ryosuke Niwa 2017-11-10 11:38:57 PST
Did you see my review comments?
Comment 19 Nan Wang 2017-11-10 11:39:54 PST
(In reply to Ryosuke Niwa from comment #18)
> Did you see my review comments?

Yes I think I've addressed those.
Comment 20 Nan Wang 2017-11-13 12:25:03 PST
Ryosuke, can you take another look when you have a chance? Thanks!
Comment 21 Ryosuke Niwa 2017-11-13 20:35:28 PST
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>
Comment 22 Nan Wang 2017-11-14 11:17:49 PST
Created attachment 326895 [details]
patch

Thanks for the review!
Addressed review comments.
Comment 23 Ryosuke Niwa 2017-11-14 11:29:44 PST
Please update the patch for trunk.
Comment 24 Nan Wang 2017-11-14 11:32:14 PST
(In reply to Ryosuke Niwa from comment #23)
> Please update the patch for trunk.

working on it
Comment 25 Nan Wang 2017-11-14 11:44:13 PST
Created attachment 326899 [details]
patch

update
Comment 26 Ryosuke Niwa 2017-11-14 21:11:01 PST
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.
Comment 27 Nan Wang 2017-11-14 23:47:53 PST
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.
Comment 28 Nan Wang 2017-11-14 23:56:07 PST
Committed r224871: <https://trac.webkit.org/changeset/224871>
Comment 29 Nan Wang 2017-11-14 23:56:56 PST
Addressed review comments and removed gc() in test.