rdar://98886794
<rdar://problem/99723702>
Created attachment 462234 [details] Patch
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
Created attachment 462257 [details] Patch
(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 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?
Created attachment 462294 [details] Patch
(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 :-).
> 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).
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].