Bug 179497 – AX: AOM: Implement number type properties

• Component: Accessibility
• Status: RESOLVED FIXED

[Nan Wang]

AOM Phase 1:
Implement number type properties

Spec: https://wicg.github.io/aom/spec/

<rdar://problem/35077947>

[Nan Wang]

Created attachment 327295 [details]
patch

[Nan Wang]

Created attachment 327302 [details]
patch

Fix GTK build

[chris fleizach]

Comment on attachment 327302 [details]
patch

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

> Source/WebCore/accessibility/AccessibleNode.cpp:203
> +void AccessibleNode::setProperty(bool shouldRemove, PropertyValueVariant&& value, AXPropertyName propertyName)

can we remove the first argument and just check if value.has_value inside the method?

It seems more natural to have the PropertyName be the first argument, and the value be the second?

> Source/WebCore/accessibility/AccessibleNode.cpp:279
> +        return element.attributeWithoutSynchronization(ariaAttributeMap().get(propertyName)).toInt();

toUnsigned()

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

should this be float instead of double?

[Nan Wang]

Comment on attachment 327302 [details]
patch

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

>> Source/WebCore/accessibility/AccessibleNode.cpp:203
>> +void AccessibleNode::setProperty(bool shouldRemove, PropertyValueVariant&& value, AXPropertyName propertyName)
> 
> can we remove the first argument and just check if value.has_value inside the method?
> 
> It seems more natural to have the PropertyName be the first argument, and the value be the second?

Ok I added a template function to cover the optional values
template<typename T> void setOptionalProperty(AXPropertyName, std::optional<T>);
And also setStringProperty()

>> Source/WebCore/accessibility/AccessibleNode.cpp:279
>> +        return element.attributeWithoutSynchronization(ariaAttributeMap().get(propertyName)).toInt();
> 
> toUnsigned()

There's no toUnsigned() on AtomicString, so I cast it to String and use toUInt()

>> Source/WebCore/accessibility/AccessibleNode.h:39
>> +typedef Variant<std::nullptr_t, String, bool, int, unsigned, double> PropertyValueVariant;
> 
> should this be float instead of double?

This is to match the JS API interface:
attribute double?

[Nan Wang]

Created attachment 327318 [details]
patch

Update from review and use auto if possible

[chris fleizach]

Comment on attachment 327318 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:2183
> +float AccessibilityObject::floatValueForProperty(AXPropertyName propertyKey) const

this is dealing with floatValues not double though

[Nan Wang]

(In reply to chris fleizach from comment #6)
> Comment on attachment 327318 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327318&action=review
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:2183
> > +float AccessibilityObject::floatValueForProperty(AXPropertyName propertyKey) const
> 
> this is dealing with floatValues not double though

I can change this to double too?

[Nan Wang]

Created attachment 327320 [details]
patch

update

[chris fleizach]

Comment on attachment 327320 [details]
patch

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

> Source/WebCore/accessibility/AccessibleNode.cpp:290
> +        const String& value =  element.attributeWithoutSynchronization(ariaAttributeMap().get(propertyName));

extra space after =

> Source/WebCore/accessibility/AccessibleNode.cpp:304
> +        return element.attributeWithoutSynchronization(ariaAttributeMap().get(propertyName)).toFloat();

toDouble() ?

[Nan Wang]

(In reply to chris fleizach from comment #9)
> Comment on attachment 327320 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327320&action=review
> 
> > Source/WebCore/accessibility/AccessibleNode.cpp:290
> > +        const String& value =  element.attributeWithoutSynchronization(ariaAttributeMap().get(propertyName));
> 
> extra space after =
> 
> > Source/WebCore/accessibility/AccessibleNode.cpp:304
> > +        return element.attributeWithoutSynchronization(ariaAttributeMap().get(propertyName)).toFloat();
> 
> toDouble() ?

Ok, updated

[Nan Wang]

Committed r225030: <https://trac.webkit.org/changeset/225030>

[Ryosuke Niwa]

Comment on attachment 327320 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:2171
> +    if (Element* element = this->element())

Use auto in these cases.

> Source/WebCore/accessibility/AccessibilityObject.cpp:2178
> +    if (Element* element = this->element())

Ditto.

> Source/WebCore/accessibility/AccessibilityObject.cpp:2185
> +    if (Element* element = this->element())

Ditto.