AOM Phase 1: Implement boolean type properties Spec: https://wicg.github.io/aom/spec/ <rdar://problem/35077917>
Created attachment 327216 [details] patch
Created attachment 327217 [details] patch fix build
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?
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.
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
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?
(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
(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.
Created attachment 327237 [details] patch Removed redundant "AX" prefix
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
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();
Comment on attachment 327237 [details] patch Clearing flags on attachment: 327237 Committed r224992: <https://trac.webkit.org/changeset/224992>
All reviewed patches have been landed. Closing bug.
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.
(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