Bug 258223

<rdar://problem/110919883>

Created attachment 466732 [details]
Patch

Created attachment 466735 [details]
Patch

Created attachment 466738 [details]
Patch

Created attachment 466739 [details]
Patch

Comment on attachment 466739 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:698
> +    if (is<RenderTableRow>(renderer) || isAccessibilityTableRow(node))

do we get anonymous table rows too? or anonymous tables we need to handle?

> Source/WebCore/accessibility/AXObjectCache.cpp:1242
> +void AXObjectCache::handleRecomputeSlotGrid(AccessibilityTable& axTable)

is slot grid the best terminology for this? what do you mean with "slot" in this case?

> Source/WebCore/accessibility/AccessibilityTable.cpp:171
> +    if (!renderTable)

is this always going to be a data table? or will people use flex/contents on table and not be a data table?

> Source/WebCore/accessibility/AccessibilityTable.cpp:303
> +            if (renderTable->hBorderSpacing() > 0 && renderTable->vBorderSpacing() > 0

is this a downgrade to use pointers here? is it possible that this pointer becomes nil during its usage?

> Source/WebCore/accessibility/AccessibilityTable.cpp:527
> +            // Step 17 and 18: Let current cell be the next td or th element child in the tr element being processed.

are these steps no implemented? what about step 16?

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:48
> +    , m_axColIndexFromRow(-1)

do we need this initialization here if we set it in the .h file?

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:165
> +        for (int i = 0; i < 2 && current; i++) {

can you have a comment for why the 2

Created attachment 466740 [details]
Patch

(In reply to chris fleizach from comment #6)
> Comment on attachment 466739 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=466739&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:698
> > +    if (is<RenderTableRow>(renderer) || isAccessibilityTableRow(node))
> 
> do we get anonymous table rows too? or anonymous tables we need to handle?
I think the answer is yes to both, but I'm planning on handling those more comprehensively in a follow-up patch. I believe anonymous rows are messing up parent-child hierarchies, making non-standard-display-value tables inaccessible even with this patch.

> > Source/WebCore/accessibility/AXObjectCache.cpp:1242
> > +void AXObjectCache::handleRecomputeSlotGrid(AccessibilityTable& axTable)
> 
> is slot grid the best terminology for this? what do you mean with "slot" in
> this case?
I use "slot" as is defined here:

https://html.spec.whatwg.org/multipage/tables.html#concept-slots

What do you think about m_cellSlots? Any other ideas? I think I like m_cellSlots better than m_slotGrid, but open to other ideas if you have them.

> > Source/WebCore/accessibility/AccessibilityTable.cpp:171
> > +    if (!renderTable)
> 
> is this always going to be a data table? or will people use flex/contents on
> table and not be a data table?
Yeah you're right, we should probably modify this "is data table" algorithm to not require render-tree structure, since I'm guessing flex/contents/inline-block/etc tables can be layout tables too. That will take a bit more time investment, so will get to it first thing next week.

> > Source/WebCore/accessibility/AccessibilityTable.cpp:303
> > +            if (renderTable->hBorderSpacing() > 0 && renderTable->vBorderSpacing() > 0
> 
> is this a downgrade to use pointers here? is it possible that this pointer
> becomes nil during its usage?
Not a downgrade. `renderTable` used to be a raw reference (RenderTable&), and now it is a WeakPtr. If the WeakPtr becomes nullptr because the RenderTable is destroyed, we will get a nullptr crash. But that same possibility exists prior to this patch, and it would be a use-after-free crash when accessing the RenderTable&, not a nullptr crash. So this is an improvement.

> > Source/WebCore/accessibility/AccessibilityTable.cpp:527
> > +            // Step 17 and 18: Let current cell be the next td or th element child in the tr element being processed.
> 
> are these steps no implemented? what about step 16?
They are implemented by allowing the for-loop to spin. I added to the comment to clarify this. I also added a comment indicating that step 16 is handled below.

> > Source/WebCore/accessibility/AccessibilityTableCell.cpp:165
> > +        for (int i = 0; i < 2 && current; i++) {
> 
> can you have a comment for why the 2
Added!

Created attachment 466791 [details]
Patch

Created attachment 466792 [details]
Patch

Comment on attachment 466792 [details]
Patch

Thanks for making those updates!

Committed 265411@main (0604c50219a2): <https://commits.webkit.org/265411@main>

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