| Summary: | AX: AXObjectCache::handleChildrenChanged should not dirty the whole tree's children | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tyler Wilcock <tyler_w> | ||||
| Component: | Accessibility | Assignee: | Tyler Wilcock <tyler_w> | ||||
| Status: | NEW --- | ||||||
| Severity: | Normal | CC: | aboxhall, andresg_22, apinheiro, cdumez, cfleizach, changseok, dmazzoni, esprehn+autocc, ews-watchlist, gyuyoung.kim, jcraig, jdiggs, kangil.han, samuel_white, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
Created attachment 462084 [details]
Patch
Comment on attachment 462084 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=462084&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:1077 > + object.setNeedsToUpdateChildren(); why do we have a needs update children and a needs update subtree? does updating children force a full subtree update? > LayoutTests/accessibility/insert-children-assert.html:60 > + testOutput += "Got fullscren cancellation. Verifying expected value occurence count.\n"; fullscreen occurrence (In reply to Tyler Wilcock from comment #2) > Created attachment 462084 [details] > Patch --- a/Source/WebCore/accessibility/AXObjectCache.cpp +++ a/Source/WebCore/accessibility/AXObjectCache.cpp @@ -1092,9 +1101,6 @@ void AXObjectCache::handleChildrenChanged(AccessibilityObject& object) // If this object is an ARIA text control, notify that its value changed. if (parent->isNonNativeTextControl()) { postNotification(parent, parent->document(), AXValueChanged); - - // Do not let any ancestor of an editable object update its children. - shouldUpdateParent = false; Why is it ok to remove this? (In reply to Tyler Wilcock from comment #2) > Created attachment 462084 [details] > Patch --- a/Source/WebCore/html/HTMLDetailsElement.cpp +++ a/Source/WebCore/html/HTMLDetailsElement.cpp @@ -160,11 +160,21 @@ void HTMLDetailsElement::parseAttribute(const QualifiedName& name, const AtomStr void HTMLDetailsElement::toggleOpen() { + auto* axObjectCache = document().existingAXObjectCache(); + if (axObjectCache) { + // Changing openAttr will cause this AX object to be deleted and re-created, so inform the object's + // parent of the change before toggling it. + if (auto* axObject = axObjectCache->get(this)) + axObjectCache->childrenChanged(axObject->parentObjectUnignored()); + } + It seems to me that this should be done in the AXObjectCache code, not here, along the handling of the AXExpandedChanged notification that is triggered blow. (In reply to Tyler Wilcock from comment #2) > Created attachment 462084 [details] > Patch --- a/LayoutTests/accessibility/image-with-alt-and-map.html +++ a/LayoutTests/accessibility/image-with-alt-and-map.html +<body> + +<body id="body" role="group"> An extra <body>. |
AXObjectCache::handleChildrenChanged does this: bool shouldUpdateParent = true; for (auto* parent = &object; parent; parent = parent->parentObjectIfExists()) { if (shouldUpdateParent) parent->setNeedsToUpdateChildren(); ... } Which seems wasteful. We should probably not dirty the entire ancestry of every changed object when it's just the children of target object that have changed.