AOM Phase 1: Implement number type properties Spec: https://wicg.github.io/aom/spec/ <rdar://problem/35077947>
Created attachment 327295 [details] patch
Created attachment 327302 [details] patch Fix GTK build
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?
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?
Created attachment 327318 [details] patch Update from review and use auto if possible
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
(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?
Created attachment 327320 [details] patch update
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() ?
(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
Committed r225030: <https://trac.webkit.org/changeset/225030>
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.