Use is<>() / downcast<>() for Accessibility objects

• Component: Accessibility
• Status: RESOLVED FIXED

[Chris Dumez]

Use is<>() / downcast<>() for Accessibility objects

[Radar WebKit Bug Importer]

<rdar://problem/18510859>

[Chris Dumez]

Created attachment 238998 [details]
Patch

[WebKit Commit Bot]

Attachment 238998 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/AccessibilityListBox.cpp:101:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:58:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:62:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 238998 [details]
Patch

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

Are we really doing this? 
This: is<AccessibilityRenderObject>(obj) does not seem like a nice thing to look at or to read

> Source/WebCore/accessibility/AccessibilityListBox.cpp:-101
> -        if (obj->roleValue() != ListBoxOptionRole)

I don't believe that obj->roleValue() != ListBoxOptionRole is equivalent to is<AccessibilityListBoxOption>(*child)

> Source/WebCore/accessibility/AccessibilityListBox.cpp:138
> +    if (!element || is<HTMLHRElement>(element))

are these things equivalent? it's not clear that they are.

> Source/WebCore/accessibility/AccessibilityObject.h:1011
> +    static bool isType(const WebCore::AccessibilityObject& object) { return object.predicate; } \

any thoughts to making this inline?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2395
> +    if (is<AccessibilitySpinButton>(m_object)) {

are these equivalent? at first glance the naming to me says no

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2559
>      if (m_object->isAccessibilityTable()) {

don't you want to to the is<> check here

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3297
> +        if (!array || !is<AccessibilityListBox>(m_object))

don't know if this is equivalent

[Chris Dumez]

Comment on attachment 238998 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilityListBox.cpp:-101
>> -        if (obj->roleValue() != ListBoxOptionRole)
> 
> I don't believe that obj->roleValue() != ListBoxOptionRole is equivalent to is<AccessibilityListBoxOption>(*child)

AccessibilityListBoxOption::roleValue() seems to be the only override returning ListBoxOptionRole. Am I missing something? Can an object that is not AccessibilityListBoxOption have roleValue() return ListBoxOptionRole somehow?
We are casting to a AccessibilityListBoxOption below so I guess this is not possible.

>> Source/WebCore/accessibility/AccessibilityListBox.cpp:138
>> +    if (!element || is<HTMLHRElement>(element))
> 
> are these things equivalent? it's not clear that they are.

Yes, this is equivalent.

>> Source/WebCore/accessibility/AccessibilityObject.h:1011
>> +    static bool isType(const WebCore::AccessibilityObject& object) { return object.predicate; } \
> 
> any thoughts to making this inline?

What do you mean? This should be inline already since the implementation is inside the class body (isTyoe() is a private member function).

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceSelection.cpp:-153
> -    if (coreSelection->isListBox() || coreSelection->isMenuList()) {

I believe this check was incorrect. if isMenuList() returns true, then we have an AccessibilityMenuList, and it cannot be casted into an AccessibilityListBox. So the cast below would be unsafe. This looks more obvious with the new checks:
if (is<AccessibilityListBox>(coreSelection) || is<AccessibilityMenuList>(coreSelection))
    downcast<AccessibilityListBox>(coreSelection)->doSomething();

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2395
>> +    if (is<AccessibilitySpinButton>(m_object)) {
> 
> are these equivalent? at first glance the naming to me says no

The type cast macro was "ACCESSIBILITY_OBJECT_TYPE_CASTS(AccessibilitySpinButton, isNativeSpinButton())" so we were checking that is object.isNativeSpinButton(), then we could cast to a AccessibilitySpinButton. Am I missing something?

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2559
>>      if (m_object->isAccessibilityTable()) {
> 
> don't you want to to the is<> check here

isAccessibilityTable() is stricter than is<AccessibilityTable>(). is<AccessibilityTable>() is equivalent to isTable(). This part isn't nice yet but could be solved though refactoring. Ideally, this would be something like is<AccessibilityTable>(m_object) && downcast<AccessibilityTable>(*m_object).renderer(), or something similar. Right now, I think the isTable() / isAccessibilityTable() distinction is a bit confusing.

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3297
>> +        if (!array || !is<AccessibilityListBox>(m_object))
> 
> don't know if this is equivalent

I believe it is until I am missing something. I also think it is clearer to have an is<X>() before a downcast<X>(), like we have here. This 1-1 mapping avoids mistakes.

[chris fleizach]

Comment on attachment 238998 [details]
Patch

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

>>> Source/WebCore/accessibility/AccessibilityListBox.cpp:-101
>>> -        if (obj->roleValue() != ListBoxOptionRole)
>> 
>> I don't believe that obj->roleValue() != ListBoxOptionRole is equivalent to is<AccessibilityListBoxOption>(*child)
> 
> AccessibilityListBoxOption::roleValue() seems to be the only override returning ListBoxOptionRole. Am I missing something? Can an object that is not AccessibilityListBoxOption have roleValue() return ListBoxOptionRole somehow?
> We are casting to a AccessibilityListBoxOption below so I guess this is not possible.

It looks like if you use an aria role of role="option" the role can be ListBoxOptionRole, which might not be a AccessibilityListBoxOption from what I can tell.

>>> Source/WebCore/accessibility/AccessibilityObject.h:1011
>>> +    static bool isType(const WebCore::AccessibilityObject& object) { return object.predicate; } \
>> 
>> any thoughts to making this inline?
> 
> What do you mean? This should be inline already since the implementation is inside the class body (isTyoe() is a private member function).

ok

>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceSelection.cpp:-153
>> -    if (coreSelection->isListBox() || coreSelection->isMenuList()) {
> 
> I believe this check was incorrect. if isMenuList() returns true, then we have an AccessibilityMenuList, and it cannot be casted into an AccessibilityListBox. So the cast below would be unsafe. This looks more obvious with the new checks:
> if (is<AccessibilityListBox>(coreSelection) || is<AccessibilityMenuList>(coreSelection))
>     downcast<AccessibilityListBox>(coreSelection)->doSomething();

Not sure what the intention was here but your assessment seems correct

>>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2395
>>> +    if (is<AccessibilitySpinButton>(m_object)) {
>> 
>> are these equivalent? at first glance the naming to me says no
> 
> The type cast macro was "ACCESSIBILITY_OBJECT_TYPE_CASTS(AccessibilitySpinButton, isNativeSpinButton())" so we were checking that is object.isNativeSpinButton(), then we could cast to a AccessibilitySpinButton. Am I missing something?

Ok thanks for double checking. Poorly named classes and functions I guess

>>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2559
>>>      if (m_object->isAccessibilityTable()) {
>> 
>> don't you want to to the is<> check here
> 
> isAccessibilityTable() is stricter than is<AccessibilityTable>(). is<AccessibilityTable>() is equivalent to isTable(). This part isn't nice yet but could be solved though refactoring. Ideally, this would be something like is<AccessibilityTable>(m_object) && downcast<AccessibilityTable>(*m_object).renderer(), or something similar. Right now, I think the isTable() / isAccessibilityTable() distinction is a bit confusing.

Agreed it's a bit too confusing now

>>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3297
>>> +        if (!array || !is<AccessibilityListBox>(m_object))
>> 
>> don't know if this is equivalent
> 
> I believe it is until I am missing something. I also think it is clearer to have an is<X>() before a downcast<X>(), like we have here. This 1-1 mapping avoids mistakes.

ok thanks

[Chris Dumez]

Created attachment 240099 [details]
Patch

[WebKit Commit Bot]

Attachment 240099 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:58:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:62:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

(In reply to comment #4)
> Are we really doing this? 
> This: is<AccessibilityRenderObject>(obj) does not seem like a nice thing to
> look at or to read

Chris F, did you talk to the Chris D about this basic question?

[Darin Adler]

Comment on attachment 240099 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:63
> -    if (!child || (!child->isTableRow() && child->ariaRoleAttribute() != RowRole))
> +    if (!child || (!is<AccessibilityTableRow>(*child) && child->ariaRoleAttribute() != RowRole))
>          return false;
>          
> -    AccessibilityTableRow* row = toAccessibilityTableRow(child);
> -    if (appendedRows.contains(row))
> +    auto& row = downcast<AccessibilityTableRow>(*child);
> +    if (appendedRows.contains(&row))
>          return false;

I don’t understand the relationship of the if statement to the code below. As currently written, if a non-AccessibilityTableRow child has an ariaRoleAttribute of RowRole, it looks like we will cast incorrectly.

> Source/WebCore/accessibility/AccessibilityARIAGridCell.cpp:58
>      if (parent->isAccessibilityTable())

It’s irritating that callsites like this one can’t change to is<AccessibilityTable>. That’s because this also checks if there is a renderer, I believe. Yet that is not at all clear from the name. I think this needs work; it’s not good to have a name like this with hidden extra behavior. It’s also not good that all the callers have to do a downcast in a non-obviously-correct way.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2841
> +    // FIXME: We should use is<>() / downcast<>() for SpinButtonElement.

I agree about downcast<>. I’m not sure why you mention is<>?

> Source/WebCore/accessibility/AccessibilityTable.h:78
> +    // isAccessibilityTable is whether it is exposed as an AccessibilityTable to the platform.

This comment was already a bit confusing, but even more so now that it’s not in context with isTable and isDataTable below. What can we do to mitigate that?

[chris fleizach]

(In reply to comment #9)
> (In reply to comment #4)
> > Are we really doing this? 
> > This: is<AccessibilityRenderObject>(obj) does not seem like a nice thing to
> > look at or to read
> 
> Chris F, did you talk to the Chris D about this basic question?

It looks like the ship has sailed, so I'm on board if WebKit is going in this direction.

There's still some minor coding fixes that I'd like to see made before this lands (plus its failing style checker)

[Chris Dumez]

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #4)
> > > Are we really doing this? 
> > > This: is<AccessibilityRenderObject>(obj) does not seem like a nice thing to
> > > look at or to read
> > 
> > Chris F, did you talk to the Chris D about this basic question?
> 
> It looks like the ship has sailed, so I'm on board if WebKit is going in
> this direction.
> 
> There's still some minor coding fixes that I'd like to see made before this
> lands (plus its failing style checker)

Sure, please let me know which fixes I should do. Regarding the style checker, I believe it is confused in this case as my code style seems correct to the best of my knowledge. It is complaining about multi-line controls even though they are really single-line ones.

[Chris Dumez]

Comment on attachment 240099 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:63
>>          return false;
> 
> I don’t understand the relationship of the if statement to the code below. As currently written, if a non-AccessibilityTableRow child has an ariaRoleAttribute of RowRole, it looks like we will cast incorrectly.

Yes, the cast does look unsafe but I will look further into it.

>> Source/WebCore/accessibility/AccessibilityARIAGridCell.cpp:58
>>      if (parent->isAccessibilityTable())
> 
> It’s irritating that callsites like this one can’t change to is<AccessibilityTable>. That’s because this also checks if there is a renderer, I believe. Yet that is not at all clear from the name. I think this needs work; it’s not good to have a name like this with hidden extra behavior. It’s also not good that all the callers have to do a downcast in a non-obviously-correct way.

Yes, I am planning to refactor this further as discussed earlier with Chris F. Basically, I'd like to make this: "if (is<AccessibilityTable>(*parent) && downcast<AccessibilityTable>(*parent).renderer())". It is admittedly longer but it is clearer IMHO. I was hoping to do this in a follow-up patch if that's OK for everyone.

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2841
>> +    // FIXME: We should use is<>() / downcast<>() for SpinButtonElement.
> 
> I agree about downcast<>. I’m not sure why you mention is<>?

Because there is a spinButtonElement->isSpinButtonElement() 2 lines above that would need to be replaced with an is<>().

>> Source/WebCore/accessibility/AccessibilityTable.h:78
>> +    // isAccessibilityTable is whether it is exposed as an AccessibilityTable to the platform.
> 
> This comment was already a bit confusing, but even more so now that it’s not in context with isTable and isDataTable below. What can we do to mitigate that?

I am planning to get rid of this method as it is confusing (see comment above).

[chris fleizach]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > (In reply to comment #4)
> > > > Are we really doing this? 
> > > > This: is<AccessibilityRenderObject>(obj) does not seem like a nice thing to
> > > > look at or to read
> > > 
> > > Chris F, did you talk to the Chris D about this basic question?
> > 
> > It looks like the ship has sailed, so I'm on board if WebKit is going in
> > this direction.
> > 
> > There's still some minor coding fixes that I'd like to see made before this
> > lands (plus its failing style checker)
> 
> Sure, please let me know which fixes I should do. Regarding the style
> checker, I believe it is confused in this case as my code style seems
> correct to the best of my knowledge. It is complaining about multi-line
> controls even though they are really single-line ones.

I think the ListBoxOptionRole code is the primary concern

------------------------
> AccessibilityListBoxOption::roleValue() seems to be the only override returning ListBoxOptionRole. Am I missing something? Can an object that is not AccessibilityListBoxOption have roleValue() return ListBoxOptionRole somehow?
> We are casting to a AccessibilityListBoxOption below so I guess this is not possible.

It looks like if you use an aria role of role="option" the role can be ListBoxOptionRole, which might not be a AccessibilityListBoxOption from what I can tell.
--------------------

[Chris Dumez]

(In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #9)
> > > > (In reply to comment #4)
> > > > > Are we really doing this? 
> > > > > This: is<AccessibilityRenderObject>(obj) does not seem like a nice thing to
> > > > > look at or to read
> > > > 
> > > > Chris F, did you talk to the Chris D about this basic question?
> > > 
> > > It looks like the ship has sailed, so I'm on board if WebKit is going in
> > > this direction.
> > > 
> > > There's still some minor coding fixes that I'd like to see made before this
> > > lands (plus its failing style checker)
> > 
> > Sure, please let me know which fixes I should do. Regarding the style
> > checker, I believe it is confused in this case as my code style seems
> > correct to the best of my knowledge. It is complaining about multi-line
> > controls even though they are really single-line ones.
> 
> I think the ListBoxOptionRole code is the primary concern
> 
> ------------------------
> > AccessibilityListBoxOption::roleValue() seems to be the only override returning ListBoxOptionRole. Am I missing something? Can an object that is not AccessibilityListBoxOption have roleValue() return ListBoxOptionRole somehow?
> > We are casting to a AccessibilityListBoxOption below so I guess this is not possible.
> 
> It looks like if you use an aria role of role="option" the role can be
> ListBoxOptionRole, which might not be a AccessibilityListBoxOption from what
> I can tell.
> --------------------

Chris, are you referring to the older patch? In my latest patch, I have been a lot more conservative, and AFAIK I haven't replaces any role check by is<>() checks.

[chris fleizach]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > (In reply to comment #11)
> > > > (In reply to comment #9)
> > > > > (In reply to comment #4)
> > > > > > Are we really doing this? 
> > > > > > This: is<AccessibilityRenderObject>(obj) does not seem like a nice thing to
> > > > > > look at or to read
> > > > > 
> > > > > Chris F, did you talk to the Chris D about this basic question?
> > > > 
> > > > It looks like the ship has sailed, so I'm on board if WebKit is going in
> > > > this direction.
> > > > 
> > > > There's still some minor coding fixes that I'd like to see made before this
> > > > lands (plus its failing style checker)
> > > 
> > > Sure, please let me know which fixes I should do. Regarding the style
> > > checker, I believe it is confused in this case as my code style seems
> > > correct to the best of my knowledge. It is complaining about multi-line
> > > controls even though they are really single-line ones.
> > 
> > I think the ListBoxOptionRole code is the primary concern
> > 
> > ------------------------
> > > AccessibilityListBoxOption::roleValue() seems to be the only override returning ListBoxOptionRole. Am I missing something? Can an object that is not AccessibilityListBoxOption have roleValue() return ListBoxOptionRole somehow?
> > > We are casting to a AccessibilityListBoxOption below so I guess this is not possible.
> > 
> > It looks like if you use an aria role of role="option" the role can be
> > ListBoxOptionRole, which might not be a AccessibilityListBoxOption from what
> > I can tell.
> > --------------------
> 
> Chris, are you referring to the older patch? In my latest patch, I have been
> a lot more conservative, and AFAIK I haven't replaces any role check by
> is<>() checks.

Ah, I didn't realize obsolete patches are hidden by default.. I hadn't seen the new patch go up. yea looks ok
thanks

[Chris Dumez]

Comment on attachment 240099 [details]
Patch

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

>>> Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:63
>>>          return false;
>> 
>> I don’t understand the relationship of the if statement to the code below. As currently written, if a non-AccessibilityTableRow child has an ariaRoleAttribute of RowRole, it looks like we will cast incorrectly.
> 
> Yes, the cast does look unsafe but I will look further into it.

Chris F, any thought about this?

[Chris Dumez]

Comment on attachment 240099 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:58
> +    if (!child || (!is<AccessibilityTableRow>(*child) && child->ariaRoleAttribute() != RowRole))

It seems ariaRoleAttribute() can only return RowRole if the object is a AccessibilityTableRow, so the cast below is safe. However, I don't think we can replace this with a is<AccessibilityTableRow>() because not all AccessibilityTableRow objects have RowRole as ariaRoleAttribute().

[chris fleizach]

Comment on attachment 240099 [details]
Patch

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

>>>> Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:63
>>>>          return false;
>>> 
>>> I don’t understand the relationship of the if statement to the code below. As currently written, if a non-AccessibilityTableRow child has an ariaRoleAttribute of RowRole, it looks like we will cast incorrectly.
>> 
>> Yes, the cast does look unsafe but I will look further into it.
> 
> Chris F, any thought about this?

I think if an object has an ARIA role of Row, we are assuming that we'll be guaranteed to be an AccessibilityARIAGridRow, which is a sub-class of AccessibilityTableRow.
I think that's generally a safe assumption to make, but its not very clear.

the case this is trying to protect against I think is where the object is an AccessibilityTableRow, but its roleValue() is something different.

This might be better to write as

|| (!is<AccessibilityTableRow>(*child) && !is<AccessibilityARIAGridRow>(*child))

[Chris Dumez]

Created attachment 240131 [details]
Patch

[Chris Dumez]

(In reply to comment #19)
> Comment on attachment 240099 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240099&action=review
> 
> >>>> Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:63
> >>>>          return false;
> >>> 
> >>> I don’t understand the relationship of the if statement to the code below. As currently written, if a non-AccessibilityTableRow child has an ariaRoleAttribute of RowRole, it looks like we will cast incorrectly.
> >> 
> >> Yes, the cast does look unsafe but I will look further into it.
> > 
> > Chris F, any thought about this?
> 
> I think if an object has an ARIA role of Row, we are assuming that we'll be
> guaranteed to be an AccessibilityARIAGridRow, which is a sub-class of
> AccessibilityTableRow.
> I think that's generally a safe assumption to make, but its not very clear.
> 
> the case this is trying to protect against I think is where the object is an
> AccessibilityTableRow, but its roleValue() is something different.
> 
> This might be better to write as
> 
> || (!is<AccessibilityTableRow>(*child) &&
> !is<AccessibilityARIAGridRow>(*child))

Ok, I made this change before landing, thanks.

[WebKit Commit Bot]

Attachment 240131 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:58:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:62:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 240131 [details]
Patch

Clearing flags on attachment: 240131

Committed r174898: <http://trac.webkit.org/changeset/174898>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.