Bug 244954 – AX ITM: Some non-empty data tables are exposed as 0 columns, 0 rows.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Andres Gonzalez]

rdar://98886794

[Radar WebKit Bug Importer]

<rdar://problem/99723702>

[Andres Gonzalez]

Created attachment 462234 [details]
Patch

[chris fleizach]

Comment on attachment 462234 [details]
Patch

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

> COMMIT_MESSAGE:9
> +* Source/WebCore/accessibility/AccessibilityTable.cpp:

can we add a test for this case

> Source/WebCore/accessibility/AccessibilityTable.cpp:77
> +    return !(role == AccessibilityRole::Unknown // No role attribute specified.

can we convert to a switch statement

switch (ariaRoleAtribute())
   case ..
     return true;
  default:
     return false

[Andres Gonzalez]

Created attachment 462257 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #3)
> Comment on attachment 462234 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=462234&action=review
> 
> > COMMIT_MESSAGE:9
> > +* Source/WebCore/accessibility/AccessibilityTable.cpp:
> 
> can we add a test for this case

Done.
> 
> > Source/WebCore/accessibility/AccessibilityTable.cpp:77
> > +    return !(role == AccessibilityRole::Unknown // No role attribute specified.
> 
> can we convert to a switch statement
> 
> switch (ariaRoleAtribute())
>    case ..
>      return true;
>   default:
>      return false

Done.

Also refined AXObjectCache::handleRoleChanged so that we better account for a wider range of arbitrary role changes for tables and rows.

[Tyler Wilcock]

Comment on attachment 462257 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1901
> +    ASSERT(element && oldValue != newValue);

Very minor, but would it be better to either:

  1. Make these two separate asserts (ASSERT(element) and ASSERT(oldValue != newValue))
  2. Or, use ASSERT_WITH_MESSAGE to indicate which of these two conditions fail

Otherwise anybody who hits this assert won't know which of the two conditions caused it.

> Source/WebCore/accessibility/AXObjectCache.cpp:1912
> +        if (auto* parent = object->parentObject()) {

We should be getting `parentObjectUnignored` instead of `parentObject` here, right? When we build the live tree, if an object is ignored, we descend past it and insert its children.

> Source/WebCore/accessibility/AccessibilityTable.cpp:32
> +#include "AXLogger.h"

Why do we need this additional include?

[Andres Gonzalez]

Created attachment 462294 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #6)
> Comment on attachment 462257 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=462257&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1901
> > +    ASSERT(element && oldValue != newValue);
> 
> Very minor, but would it be better to either:
> 
>   1. Make these two separate asserts (ASSERT(element) and ASSERT(oldValue !=
> newValue))
>   2. Or, use ASSERT_WITH_MESSAGE to indicate which of these two conditions
> fail

Changed it to just assert the new condition about the attributes since we don't assert the non-null of Element pointers anywhere else.
> 
> Otherwise anybody who hits this assert won't know which of the two
> conditions caused it.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1912
> > +        if (auto* parent = object->parentObject()) {
> 
> We should be getting `parentObjectUnignored` instead of `parentObject` here,
> right? When we build the live tree, if an object is ignored, we descend past
> it and insert its children.

It is safer here to do the parentObject without having to check the isIgnored state for the ancestors. Another example is in AXObjectCache::handleTextChanged. The isIgnored check should come later into play.

> 
> > Source/WebCore/accessibility/AccessibilityTable.cpp:32
> > +#include "AXLogger.h"
> 
> Why do we need this additional include?

I just have to add it every time I want to log something in this file :-).

[Tyler Wilcock]

> It is safer here to do the parentObject without having to check the
> isIgnored state for the ancestors. Another example is in
> AXObjectCache::handleTextChanged. The isIgnored check should come later into
> play.
OK, well this is probably fine for now because AXObjectCache::handleChildrenChanged dirties the tree all the way to the root. But if we ever change that behavior like I tried to do in:

https://bugs.webkit.org/show_bug.cgi?id=244695 (AX: AXObjectCache::handleChildrenChanged should not dirty the whole tree's children)

Then I think we would need to make this parentObjectUnignored(). But I actually think keeping `parentObject()` here until we make more progress on the aforementioned bug is more efficient (because we don't need to compute is-ignored).

[EWS]

Committed 254418@main (a313324db71f): <https://commits.webkit.org/254418@main>

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