Bug 249521

Created attachment 464083 [details] Patch

(In reply to chris fleizach from comment #1) > Created attachment 464083 [details] > Patch In the malformed table, which element is causing the problem? <tbody role="list"> or <tr aria-hidden="false" role="presentation"> --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp @@ -353,6 +353,11 @@ void AXIsolatedTree::collectNodeChangesForSubtree(AXCoreObject& axObject) axChildrenIDs.uncheckedAppend(axChild->objectID()); collectNodeChangesForSubtree(*axChild); + + // Sometimes a parent has children, but those children have different parents (like in malformed tables). + // We need to get those parents into the subtree collection so they have isolated nodes too. + if (auto* parent = axChild->parentObjectUnignored(); parent && parent->objectID() != axObject.objectID() && !m_nodeMap.contains(parent->objectID())) + collectNodeChangesForSubtree(*parent); } I think this can create an infinite recursion hang in a case like: <parent id="parent1" aria-owns="child2"> <child id="child1"/> </parent> <parent id="parent2" aria-owns="child1"> <child id="child2"/> </parent> If both parent1 and parent2 are unignored and for some reason not in the iso tree.

Comment on attachment 464083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=464083&action=review > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:359 > + if (auto* parent = axChild->parentObjectUnignored(); parent && parent->objectID() != axObject.objectID() && !m_nodeMap.contains(parent->objectID())) I'm worried this will cause performance issues. Computing both parentObject() and accessibilityIsIgnored() can be extremely expensive (especially for tables), and this is one of our hottest codepaths. I recognize we need to do this somehow, but I do wonder if there is some cheaper way to detect this edgecase. Or maybe we could measure the effects of this patch and determine if there is any meaningful performance hit. > LayoutTests/accessibility/table-element-retrieval-with-bad-rows.html:12 > + <td><ul><li id="test"><button id="button"></li></ul></td> Formatting this differently might make the parent-hierarchy being tested easier to parse: <td> <ul> <li id="test"> <button id="button"> </li> </ul> </td>

(In reply to Tyler Wilcock from comment #3) > I'm worried this will cause performance issues. Computing both > parentObject() and accessibilityIsIgnored() can be extremely expensive > (especially for tables), and this is one of our hottest codepaths. I > recognize we need to do this somehow, but I do wonder if there is some > cheaper way to detect this edgecase. Or maybe we could measure the effects > of this patch and determine if there is any meaningful performance hit. This adds 3% to the runtime of collectNodeChangesForSubtree. So for example, on Alaska air, collectNodeChangesForSubtree ran for a total of .1s. This addition added an extra .004s

Created attachment 464113 [details] Patch

Created attachment 464145 [details] Patch

Comment on attachment 464145 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=464145&action=review > Source/WebCore/accessibility/AccessibilityTable.cpp:136 > + || (tableElement->tHead() && tableElement->tHead()->renderer()) > + || (tableElement->tFoot() && tableElement->tHead()->renderer()) To fix this bug, is it required to start requiring renderers for tHead() and tFoot()? This precludes adding display:contents to these elements, which I think people could validly do while still expecting table AX semantics (but if you disagree let's talk about it). We would likely be the only browser implementing this rule. If our answer is yes, you have a typo. (tableElement->tFoot() && tableElement->tHead()->renderer()) Should be: (tableElement->tFoot() && tableElement->tFoot()->renderer()) > Source/WebCore/accessibility/AccessibilityTable.cpp:175 > + auto* topSection = table.topSection(); > + // If the tbody has any non-group role, then don't make this a data table. The author probably wants to use the role inside the <tbody>. > + if (AccessibilityObject* topSectionObject = axObjectCache()->getOrCreate(topSection)) { The auto* topSection local variable seems unnecessary. What about: // If the tbody has any non-group role, then don't make this a data table. The author probably wants to use the role inside the <tbody>. if (auto* topSection = axObjectCache()->getOrCreate(table.topSection())) { > LayoutTests/accessibility/table-ignored-with-role-on-tbody.html:8 > +<div id="content"> This id="content" isn't necessary. In fact, you might be able to get rid of this div entirely? > LayoutTests/accessibility/table-ignored-with-role-on-tbody.html:11 > + <tr aria-hidden="false" role="presentation"> I think this tr should be indented one more level since it's inside the tbody. > LayoutTests/accessibility/table-ignored-with-role-on-tbody.html:14 > + <li id="test"> This id="test" isn't necessary. > LayoutTests/accessibility/table-ignored-with-role-on-tbody.html:27 > + var button; var table; This could be `var button, table;` > LayoutTests/accessibility/table-ignored-with-role-on-tbody.html:29 > + window.jsTestIsAsync = true; We shouldn't need to make this test async since there are no dynamic JS changes.

(In reply to chris fleizach from comment #6) > Created attachment 464145 [details] > Patch > If a role is put on a <tbody> is breaks the parent/child hierarchy for tables. is breaks -> it breaks > This also exposed an issue with ITM where if we do have a mismatch in parent/child, then its possible then its possible -> then it's possible * LayoutTests/accessibility/table-ignored-with-role-on-tbody-expected.txt: Added. * LayoutTests/accessibility/table-ignored-with-role-on-tbody.html: Added. * Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp: (WebCore::AXIsolatedTree::collectNodeChangesForSubtree): The files/functions section of the comment is out of date, so I would remove it completely since it has no relevant info. --- a/Source/WebCore/accessibility/AccessibilityTable.cpp +++ b/Source/WebCore/accessibility/AccessibilityTable.cpp - if (!tableElement->summary().isEmpty() || tableElement->tHead() || tableElement->tFoot() || tableElement->caption()) + if (!tableElement->summary().isEmpty() + || (tableElement->tHead() && tableElement->tHead()->renderer()) + || (tableElement->tFoot() && tableElement->tHead()->renderer()) If we are checking for renderer(), the line above should be + || (tableElement->tFoot() && tableElement->tFoot()->renderer()) Do we need to check for renderer()? --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp + // Sometimes a parent has children, but those children have different parents (like in malformed tables). + // We need to get those parents into the subtree collection so they have isolated nodes too. + if (axObject.isTable()) { + auto* parent = axChild->parentObjectUnignored(); + if (parent && parent->objectID() != axObject.objectID() && !m_nodeMap.contains(parent->objectID())) + collectNodeChangesForSubtree(*parent); + } Since the table/tbody issue is fixed with the change to AccessibilityTable::isDataTable, I would not make this change here. Instead, we can add an ASSERT to identify other scenarios where this could happen. Otherwise this would be hiding an underlying bug that may or may not manifest in ITM only.

Created attachment 464324 [details] Patch

(In reply to chris fleizach from comment #9) > Created attachment 464324 [details] > Patch A couple of typos in the commit message. I'm assuming the check for renderer() in the thead and tfoot is needed cause it didn't change, I'm fine with it since we require that in many other places, will need to rework all at once for display:contents for tables. No need to R? again if you decide to make these corrections.

Created attachment 464377 [details] Patch

Created attachment 464381 [details] Patch

Created attachment 464394 [details] Patch

Committed 258633@main (9c513c530205): <https://commits.webkit.org/258633@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 464394 [details].