Bug 266680 – AX: Grids/tables are not resilient to invalid Row->Cell hierarchy

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

[Joshua Hoffman]

Tables are not resistant to invalid Row->Cell hierarchies, such as when an unignored object is used as a container around cells in a row. We should be resilient to these incorrect, but common, authoring paradigms.

[Radar WebKit Bug Importer]

<rdar://problem/119911669>

[Joshua Hoffman]

rdar://118938262

[Joshua Hoffman]

rdar://118938262

[Joshua Hoffman]

Created attachment 469138 [details]
Patch

[Joshua Hoffman]

Created attachment 469139 [details]
Patch

[Joshua Hoffman]

Created attachment 469140 [details]
Patch

[Joshua Hoffman]

Created attachment 469150 [details]
Patch

[Tyler Wilcock]

Comment on attachment 469150 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:4399
> +bool AccessibilityObject::hasTableRowParent() const

We're checking more than one layer above us here, so "hasTableRowAncestor" might be more appropriate? Also that name in general seems misleading, since we have all of these conditions to break before answering the question "has table row ancestor".

> LayoutTests/accessibility/grid-row-authoring-error-ignored.html:101
> +    debug(output)

Missing semicolon after debug(). Also, in the past when we've added ancestor flags, I've always tried to add a test verifying they work even after dynamic tree changes. That might be nice to do here too? One idea, maybe we can move #grid3's draggable div to the end of the body. Before doing so, the div should be ignored due to the changes in your patch. After moving it, it probably should be unignored with a role of group.

[Joshua Hoffman]

Comment on attachment 469150 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilityObject.cpp:4399
>> +bool AccessibilityObject::hasTableRowParent() const
> 
> We're checking more than one layer above us here, so "hasTableRowAncestor" might be more appropriate? Also that name in general seems misleading, since we have all of these conditions to break before answering the question "has table row ancestor".

Good point—maybe "isIgnoredInTable" would fit better?

>> LayoutTests/accessibility/grid-row-authoring-error-ignored.html:101
>> +    debug(output)
> 
> Missing semicolon after debug(). Also, in the past when we've added ancestor flags, I've always tried to add a test verifying they work even after dynamic tree changes. That might be nice to do here too? One idea, maybe we can move #grid3's draggable div to the end of the body. Before doing so, the div should be ignored due to the changes in your patch. After moving it, it probably should be unignored with a role of group.

Good call, I'll add that in to this test

[Tyler Wilcock]

Comment on attachment 469150 [details]
Patch

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

>>> Source/WebCore/accessibility/AccessibilityObject.cpp:4399
>>> +bool AccessibilityObject::hasTableRowParent() const
>> 
>> We're checking more than one layer above us here, so "hasTableRowAncestor" might be more appropriate? Also that name in general seems misleading, since we have all of these conditions to break before answering the question "has table row ancestor".
> 
> Good point—maybe "isIgnoredInTable" would fit better?

Maybe "ignoredByRowAncestor"?

> Source/WebCore/accessibility/AccessibilityObject.cpp:4401
> +    auto* ancestor = Accessibility::findAncestor<AccessibilityObject>(*this, false, [] (const AccessibilityObject& object) {

"const AccessibilityObject& ancestor" is more descriptive than "object" here.

> Source/WebCore/accessibility/AccessibilityObject.cpp:4403
> +        // If an object has a table cell ancestor (before a table row), that is a cell's contents, so don't ignore it.
> +        return object.isTableCell() || object.isTableRow() || object.isTable();

Might be nice to have a comment explaining why we can stop iterating when we encounter a table, just as you explain why we can stop when we encounter a cell.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1234
> +    // Objects that are between table rows and cells should be ignored.

Might be nice to explain why, e.g. "...should be ignored, or else the table hierarchy will be incorrect, preventing the table content from being accessible to ATs."

[Joshua Hoffman]

Created attachment 469151 [details]
Patch

[Joshua Hoffman]

(In reply to Tyler Wilcock from comment #10)
> Comment on attachment 469150 [details]
> Patch

Addressed your last two comments in the latest patch

[Tyler Wilcock]

Comment on attachment 469151 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1237
> +        if (!isTableCell() && ignoredByRowAncestor())

Another idea to short-circuit this traversal — we don't need to do this for objects that !canHaveChildren(), right? For such objects, if they're for some reason placed between a row and a cell (it would be very, very bad authoring), the table structure is already going to be broken, since they'll prevent the cells from being exposed?

[Tyler Wilcock]

(In reply to Tyler Wilcock from comment #13)
> Comment on attachment 469151 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=469151&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1237
> > +        if (!isTableCell() && ignoredByRowAncestor())
> 
> Another idea to short-circuit this traversal — we don't need to do this for
> objects that !canHaveChildren(), right? For such objects, if they're for
> some reason placed between a row and a cell (it would be very, very bad
> authoring), the table structure is already going to be broken, since they'll
> prevent the cells from being exposed?

i.e., I'm wondering if this is both still correct and also more performant:

if (!isTableCell() && canHaveChildren() && ignoredByRowAncestor())
    return true;

[Joshua Hoffman]

(In reply to Tyler Wilcock from comment #14)
> (In reply to Tyler Wilcock from comment #13)
> > Comment on attachment 469151 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=469151&action=review
> > 
> > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1237
> > > +        if (!isTableCell() && ignoredByRowAncestor())
> > 
> > Another idea to short-circuit this traversal — we don't need to do this for
> > objects that !canHaveChildren(), right? For such objects, if they're for
> > some reason placed between a row and a cell (it would be very, very bad
> > authoring), the table structure is already going to be broken, since they'll
> > prevent the cells from being exposed?
> 
> i.e., I'm wondering if this is both still correct and also more performant:
> 
> if (!isTableCell() && canHaveChildren() && ignoredByRowAncestor())
>     return true;

Yep, that would still be correct! We should include this both here and in the traversal itself.

[Joshua Hoffman]

(In reply to Joshua Hoffman from comment #15)
> (In reply to Tyler Wilcock from comment #14)
> > (In reply to Tyler Wilcock from comment #13)
> > > Comment on attachment 469151 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=469151&action=review
> > > 
> > > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1237
> > > > +        if (!isTableCell() && ignoredByRowAncestor())
> > > 
> > > Another idea to short-circuit this traversal — we don't need to do this for
> > > objects that !canHaveChildren(), right? For such objects, if they're for
> > > some reason placed between a row and a cell (it would be very, very bad
> > > authoring), the table structure is already going to be broken, since they'll
> > > prevent the cells from being exposed?
> > 
> > i.e., I'm wondering if this is both still correct and also more performant:
> > 
> > if (!isTableCell() && canHaveChildren() && ignoredByRowAncestor())
> >     return true;
> 
> Yep, that would still be correct! We should include this both here and in
> the traversal itself.

Actually, ignore the part about puttting it inside the traversal. We wouldn't descend into something that !canHaveChildren, so we would never hit that in there.

[Joshua Hoffman]

Created attachment 469153 [details]
Patch

[Joshua Hoffman]

Created attachment 469157 [details]
Patch

[EWS]

Committed 272447@main (1a8243f4e81e): <https://commits.webkit.org/272447@main>

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