...
<rdar://problem/115604027>
Created attachment 467706 [details] Patch
Created attachment 467723 [details] Patch
(In reply to Tyler Wilcock from comment #3) > Created attachment 467723 [details] > Patch --- a/Source/WebCore/accessibility/AXObjectCache.cpp +++ b/Source/WebCore/accessibility/AXObjectCache.cpp + for (AXID axID : m_deferredRemoveReplacedObjects) { + if (!markedRelationsDirty) { + // If the replaced object was part of any relation, we need to make sure the relations are updated. + if (m_relations.contains(axID)) + markRelationsDirty(); + else if (auto* axObject = objectForID(axID); axObject && !axObject->identifierAttribute().isEmpty()) + markRelationsDirty(); Do we need to update the relations even if the object being removed wasn't previously related? Wouldn't it be simpler to do this in AXObjectCache::onRendererCreated: if (m_relations.contains(axID)) relationsNeedUpdate(true); + // Contrary to the name "AXSelectedCellsChanged", this notification can be posted on a cell who has changed + // selected state, not just on table or grid who has changed its selected cells. case AXSelectedCellsChanged: That actually seems a bug to me, we already have a selected state that applies to several types of objects, so I don't see why we would need SelectedCellsChange with a target cell, the target should be a table/grid container. Could this comment be a FIXME then? --- a/Source/WebCore/accessibility/AXObjectCache.h +++ b/Source/WebCore/accessibility/AXObjectCache.h - HashSet<AXID> m_deferredRemovedObjects; + // An object can be "replaced" when we create an AX object from the backing element before it has + // attached a renderer, but then want to replace it with a new AX object after the renderer has been attached. + HashSet<AXID> m_deferredRemoveReplacedObjects; Can we call this just m_replacedObjects? --- a/Source/WebCore/accessibility/AccessibilityObjectInterface.h +++ b/Source/WebCore/accessibility/AccessibilityObjectInterface.h +inline AXCoreObject* AXCoreObject::activeDescendant() const +{ + auto activeDescendants = relatedObjects(AXRelationType::ActiveDescendant); + ASSERT(activeDescendants.size() <= 1); + if (!activeDescendants.isEmpty()) + return activeDescendants[0].get(); + return nullptr; I'd prefer the more succinct: return activeDescendants.size() ? activeDescendants[0].get() : nullptr; +inline AXCoreObject::AccessibilityChildrenVector AXCoreObject::selectedCells() +{ ... We need to add an AXCoreObjectImpl.cpp to keep adding these to the header file.
Created attachment 467740 [details] Patch
> + for (AXID axID : m_deferredRemoveReplacedObjects) { > + if (!markedRelationsDirty) { > + // If the replaced object was part of any relation, we need to > make sure the relations are updated. > + if (m_relations.contains(axID)) > + markRelationsDirty(); > + else if (auto* axObject = objectForID(axID); axObject && > !axObject->identifierAttribute().isEmpty()) > + markRelationsDirty(); > > Do we need to update the relations even if the object being removed wasn't > previously related? Fixed! > Wouldn't it be simpler to do this in AXObjectCache::onRendererCreated: > > if (m_relations.contains(axID)) > relationsNeedUpdate(true); We discussed this morning, but the reason I don't dirty relations directly in onRendererCreated is in case it's called multiple times in a row. Doing it in performDeferredCacheUpdate allows us to dirty relations as few times as possible. > + // Contrary to the name "AXSelectedCellsChanged", this notification > can be posted on a cell who has changed > + // selected state, not just on table or grid who has changed its > selected cells. > case AXSelectedCellsChanged: > > That actually seems a bug to me, we already have a selected state that > applies to several types of objects, so I don't see why we would need > SelectedCellsChange with a target cell, the target should be a table/grid > container. Could this comment be a FIXME then? I agree that this behavior is not ideal. Changed to a FIXME. > + HashSet<AXID> m_deferredRemoveReplacedObjects; > > Can we call this just m_replacedObjects? Fixed. > I'd prefer the more succinct: > > return activeDescendants.size() ? activeDescendants[0].get() : nullptr; Fixed. > +inline AXCoreObject::AccessibilityChildrenVector > AXCoreObject::selectedCells() > +{ > ... > > We need to add an AXCoreObjectImpl.cpp to keep adding these to the header > file. Definitely. Sounds like you may have already started on this, so let's move everything in a follow-up patch.
Committed 268122@main (7a0745034449): <https://commits.webkit.org/268122@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 467740 [details].