Summary: | AX: AOM: Implement boolean type properties | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nan Wang <n_wang> | ||||||||
Component: | Accessibility | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, cdumez, cfleizach, commit-queue, dmazzoni, esprehn+autocc, ews-watchlist, jcraig, jdiggs, joepeck, kondapallykalyan, n_wang, rniwa, samuel_white, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=179255 | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 179255 | ||||||||||
Attachments: |
|
Description
Nan Wang
2017-11-09 11:31:30 PST
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 |