Bug 244954 - AX ITM: Some non-empty data tables are exposed as 0 columns, 0 rows.
Summary: AX ITM: Some non-empty data tables are exposed as 0 columns, 0 rows.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-09-08 17:54 PDT by Andres Gonzalez
Modified: 2022-09-12 19:02 PDT (History)
15 users (show)

See Also:


Attachments
Patch (5.36 KB, patch)
2022-09-09 09:31 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (19.18 KB, patch)
2022-09-10 14:27 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (19.17 KB, patch)
2022-09-12 11:43 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2022-09-08 17:54:41 PDT
rdar://98886794
Comment 1 Radar WebKit Bug Importer 2022-09-08 17:54:57 PDT
<rdar://problem/99723702>
Comment 2 Andres Gonzalez 2022-09-09 09:31:28 PDT
Created attachment 462234 [details]
Patch
Comment 3 chris fleizach 2022-09-09 09:47:48 PDT
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
Comment 4 Andres Gonzalez 2022-09-10 14:27:59 PDT
Created attachment 462257 [details]
Patch
Comment 5 Andres Gonzalez 2022-09-10 14:32:00 PDT
(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.
Comment 6 Tyler Wilcock 2022-09-12 10:36:05 PDT
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?
Comment 7 Andres Gonzalez 2022-09-12 11:43:59 PDT
Created attachment 462294 [details]
Patch
Comment 8 Andres Gonzalez 2022-09-12 11:58:00 PDT
(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 :-).
Comment 9 Tyler Wilcock 2022-09-12 12:17:49 PDT
> 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).
Comment 10 EWS 2022-09-12 19:02:04 PDT
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].