Bug 179496 – AX: AOM: Implement boolean type properties

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Nan Wang]

AOM Phase 1:
Implement boolean type properties

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

<rdar://problem/35077917>

[Nan Wang]

Created attachment 327216 [details]
patch

[Nan Wang]

Created attachment 327217 [details]
patch

fix build

[chris fleizach]

Comment on attachment 327217 [details]
patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:120
> +static const Seconds accessibilityFocusAXModalNodeNotificationInterval { 50_ms };

I think the AX might be redundant, since we're already in the AX code base. I think that can just be removed
that comment applies to everything below here too

> Source/WebCore/accessibility/AccessibilityObject.cpp:2166
> +    return std::nullopt;

should we just return false here instead of using optionals?
do we check null opt anywhere?

> Source/WebCore/accessibility/AccessibleNode.cpp:124
> +    default:

we should remove the default case so we know everything is handled always

> Source/WebCore/accessibility/AccessibleNode.cpp:127
> +}

then we return false here

> LayoutTests/accessibility/mac/AOM-bool-properties.html:59
> +        shouldBeNull("node.accessibleNode.atomic");

is this expected behavior? or should it return false?

[Nan Wang]

Comment on attachment 327217 [details]
patch

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

>> Source/WebCore/accessibility/AXObjectCache.cpp:120
>> +static const Seconds accessibilityFocusAXModalNodeNotificationInterval { 50_ms };
> 
> I think the AX might be redundant, since we're already in the AX code base. I think that can just be removed
> that comment applies to everything below here too

Ok I will change this except for AXHidden since we do have another isHidden() check

>> Source/WebCore/accessibility/AccessibilityObject.cpp:2166
>> +    return std::nullopt;
> 
> should we just return false here instead of using optionals?
> do we check null opt anywhere?

Yes we check this at all the places to make sure the property is actually being set

>> Source/WebCore/accessibility/AccessibleNode.cpp:127
>> +}
> 
> then we return false here

Ok

>> LayoutTests/accessibility/mac/AOM-bool-properties.html:59
>> +        shouldBeNull("node.accessibleNode.atomic");
> 
> is this expected behavior? or should it return false?

Yes by default all the property value should be null.

[Nan Wang]

Comment on attachment 327217 [details]
patch

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

>>> Source/WebCore/accessibility/AccessibleNode.cpp:127
>>> +}
>> 
>> then we return false here
> 
> Ok

Actually this is giving an error "17 enumeration values not handled in switch:" if without the default case

[chris fleizach]

Comment on attachment 327217 [details]
patch

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

>>>> Source/WebCore/accessibility/AccessibleNode.cpp:127
>>>> +}
>>> 
>>> then we return false here
>> 
>> Ok
> 
> Actually this is giving an error "17 enumeration values not handled in switch:" if without the default case

what are the unhandled values? all the string values?

[Nan Wang]

(In reply to chris fleizach from comment #6)
> Comment on attachment 327217 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327217&action=review
> 
> >>>> Source/WebCore/accessibility/AccessibleNode.cpp:127
> >>>> +}
> >>> 
> >>> then we return false here
> >> 
> >> Ok
> > 
> > Actually this is giving an error "17 enumeration values not handled in switch:" if without the default case
> 
> what are the unhandled values? all the string values?

enum class items

[Nan Wang]

(In reply to Nan Wang from comment #7)
> (In reply to chris fleizach from comment #6)
> > Comment on attachment 327217 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=327217&action=review
> > 
> > >>>> Source/WebCore/accessibility/AccessibleNode.cpp:127
> > >>>> +}
> > >>> 
> > >>> then we return false here
> > >> 
> > >> Ok
> > > 
> > > Actually this is giving an error "17 enumeration values not handled in switch:" if without the default case
> > 
> > what are the unhandled values? all the string values?
> 
> enum class items

AXPropertyName::None, AXPropertyName::Autocomplete, etc.

[Nan Wang]

Created attachment 327237 [details]
patch

Removed redundant "AX" prefix

[chris fleizach]

Comment on attachment 327237 [details]
patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3200
> +        return [NSNumber numberWithBool:m_object->ariaIsMultiline()];

there must be iOS changes required too right?

> LayoutTests/accessibility/mac/AOM-bool-properties.html:144
> +    function testRequired() {

missing tests for 
Atomic
Busy

[Nan Wang]

Comment on attachment 327237 [details]
patch

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

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3200
>> +        return [NSNumber numberWithBool:m_object->ariaIsMultiline()];
> 
> there must be iOS changes required too right?

I only added this to support test and the test is only available on Mac. 
I think we might need to expose more stuff in future to make this test cross platform. Right now I just used the macOS attribute names to test most properties.

>> LayoutTests/accessibility/mac/AOM-bool-properties.html:144
>> +    function testRequired() {
> 
> missing tests for 
> Atomic
> Busy

Atomic is covered in testClearBooleanProperty().
and there is a testBusy();

[WebKit Commit Bot]

Comment on attachment 327237 [details]
patch

Clearing flags on attachment: 327237

Committed r224992: <https://trac.webkit.org/changeset/224992>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Comment on attachment 327237 [details]
patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:2885
> +            std::optional<bool> hiddenValue = AccessibleNode::effectiveBoolValueForElement(downcast<Element>(*testNode), AXPropertyName::Hidden);

Use auto in these cases.

> Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:85
> +    std::optional<bool> multiSelectable = boolValueForProperty(AXPropertyName::Multiselectable);

Use auto.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:628
> +        if (std::optional<bool> disabled = object->boolValueForProperty(AXPropertyName::Disabled)) {

Use auto.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:707
> +    std::optional<bool> multiSelectable = boolValueForProperty(AXPropertyName::Multiselectable);

Use auto.

[Nan Wang]

(In reply to Ryosuke Niwa from comment #14)
> Comment on attachment 327237 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327237&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:2885
> > +            std::optional<bool> hiddenValue = AccessibleNode::effectiveBoolValueForElement(downcast<Element>(*testNode), AXPropertyName::Hidden);
> 
> Use auto in these cases.
> 
> > Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:85
> > +    std::optional<bool> multiSelectable = boolValueForProperty(AXPropertyName::Multiselectable);
> 
> Use auto.
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:628
> > +        if (std::optional<bool> disabled = object->boolValueForProperty(AXPropertyName::Disabled)) {
> 
> Use auto.
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:707
> > +    std::optional<bool> multiSelectable = boolValueForProperty(AXPropertyName::Multiselectable);
> 
> Use auto.

Ok I will update these in https://bugs.webkit.org/show_bug.cgi?id=179497