WebAccessibilityObjectWrapper method to retrieve related error message elements should return not-ignored accessibility elements.
Created attachment 421439 [details] Patch
Comment on attachment 421439 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421439&action=review > Source/WebCore/accessibility/AccessibilityObjectInterface.h:1578 > + if (child) pretty sure child should always be non nil here right? > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1878 > + ASSERT(element); shouldn't we always have an element from the vector? > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2913 > + for (const auto& element : errorElements) { do we need to do this for Mac? I think they have code to handle descendant output correctly
(In reply to chris fleizach from comment #2) > Comment on attachment 421439 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=421439&action=review > > > Source/WebCore/accessibility/AccessibilityObjectInterface.h:1578 > > + if (child) > > pretty sure child should always be non nil here right? Hard to say because several classes override updateChildrenIfNecessary() and m_children is a Vector<RefPtr>. If we want to enforce that children()[i] is not null, we should probably change m_children to be Vector<Ref>. > > > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1878 > > + ASSERT(element); > > shouldn't we always have an element from the vector? The value is a RefPtr, which should be not null, thus the ASSERT. > > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2913 > > + for (const auto& element : errorElements) { > > do we need to do this for Mac? I think they have code to handle descendant > output correctly Probably, but thought it wouldn't hurt to return elements that are actually not ignored. And keeps some consistency between Mac and iOS implementations. We can revisit when Mac clients implement this support.
Comment on attachment 421439 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421439&action=review >>> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1878 >>> + ASSERT(element); >> >> shouldn't we always have an element from the vector? > > The value is a RefPtr, which should be not null, thus the ASSERT. I feel like we probably don't need to worry about this. are we doing this anywhere else we access elements? >>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2913 >>> + for (const auto& element : errorElements) { >> >> do we need to do this for Mac? I think they have code to handle descendant output correctly > > Probably, but thought it wouldn't hurt to return elements that are actually not ignored. And keeps some consistency between Mac and iOS implementations. We can revisit when Mac clients implement this support. I'm actually worried that VO may do the wrong thing in this case because its different from other examples. Does this already work on MacOS or do they need to implement this attribute?
(In reply to chris fleizach from comment #4) > Comment on attachment 421439 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=421439&action=review > > >>> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1878 > >>> + ASSERT(element); > >> > >> shouldn't we always have an element from the vector? > > > > The value is a RefPtr, which should be not null, thus the ASSERT. > > I feel like we probably don't need to worry about this. are we doing this > anywhere else we access elements? I think we do a fair amount of AXObject* null checks, when passing parameters around, indirectly when using is<classname>, etc. It is just better to ensure you don't dereference a null pointer. In this case is just an assert to ensure that the method errorMessageElements() doesn't rotten. > > >>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2913 > >>> + for (const auto& element : errorElements) { > >> > >> do we need to do this for Mac? I think they have code to handle descendant output correctly > > > > Probably, but thought it wouldn't hurt to return elements that are actually not ignored. And keeps some consistency between Mac and iOS implementations. We can revisit when Mac clients implement this support. > > I'm actually worried that VO may do the wrong thing in this case because its > different from other examples. Does this already work on MacOS or do they > need to implement this attribute? It doesn't work with VO on the Mac, and the AXErrorMessageElements is not supported yet.
Comment on attachment 421439 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421439&action=review >>>>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2913 >>>>> + for (const auto& element : errorElements) { >>>> >>>> do we need to do this for Mac? I think they have code to handle descendant output correctly >>> >>> Probably, but thought it wouldn't hurt to return elements that are actually not ignored. And keeps some consistency between Mac and iOS implementations. We can revisit when Mac clients implement this support. >> >> I'm actually worried that VO may do the wrong thing in this case because its different from other examples. Does this already work on MacOS or do they need to implement this attribute? > > It doesn't work with VO on the Mac, and the AXErrorMessageElements is not supported yet. ok, I think it should be a bit more like the other Mac methods that return the referenced element, rather than diving down into the group.
Created attachment 421698 [details] Patch
Comment on attachment 421698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421698&action=review > Source/WebCore/accessibility/AccessibilityObjectInterface.h:1578 > + if (child) I know we had this conversation before, but I just don't think we ever have the array of children returning us nil elements. so I don't think this check is necessary
(In reply to chris fleizach from comment #6) > Comment on attachment 421439 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=421439&action=review > > >>>>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2913 > >>>>> + for (const auto& element : errorElements) { > >>>> > >>>> do we need to do this for Mac? I think they have code to handle descendant output correctly > >>> > >>> Probably, but thought it wouldn't hurt to return elements that are actually not ignored. And keeps some consistency between Mac and iOS implementations. We can revisit when Mac clients implement this support. > >> > >> I'm actually worried that VO may do the wrong thing in this case because its different from other examples. Does this already work on MacOS or do they need to implement this attribute? > > > > It doesn't work with VO on the Mac, and the AXErrorMessageElements is not supported yet. > > ok, I think it should be a bit more like the other Mac methods that return > the referenced element, rather than diving down into the group. Reverted the change for Mac, we dive into the group only for iOS.
Created attachment 421700 [details] Patch
(In reply to chris fleizach from comment #8) > Comment on attachment 421698 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=421698&action=review > > > Source/WebCore/accessibility/AccessibilityObjectInterface.h:1578 > > + if (child) > > I know we had this conversation before, but I just don't think we ever have > the array of children returning us nil elements. so I don't think this check > is necessary Ok, removed null check.
commit-queue failed to commit attachment 421700 [details] to WebKit repository. To retry, please set cq+ flag again.
Created attachment 421853 [details] Patch
Committed r273684: <https://commits.webkit.org/r273684> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421853 [details].
<rdar://problem/74886782>