Bug 222375 – WebAccessibilityObjectWrapper method to retrieve related error message elements should return not-ignored accessibility elements.

• Component: New Bugs
• Status: RESOLVED FIXED

[Andres Gonzalez]

WebAccessibilityObjectWrapper method to retrieve related error message elements should return not-ignored accessibility elements.

[Andres Gonzalez]

Created attachment 421439 [details]
Patch

[chris fleizach]

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

[Andres Gonzalez]

(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.

[chris fleizach]

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?

[Andres Gonzalez]

(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.

[chris fleizach]

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.

[Andres Gonzalez]

Created attachment 421698 [details]
Patch

[chris fleizach]

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

[Andres Gonzalez]

(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.

[Andres Gonzalez]

Created attachment 421700 [details]
Patch

[Andres Gonzalez]

(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.

[EWS]

commit-queue failed to commit attachment 421700 [details] to WebKit repository. To retry, please set cq+ flag again.

[Andres Gonzalez]

Created attachment 421853 [details]
Patch

[EWS]

Committed r273684: <https://commits.webkit.org/r273684>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421853 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74886782>