Bug 244695

Summary: AX: AXObjectCache::handleChildrenChanged should not dirty the whole tree's children
Product: WebKit Reporter: Tyler Wilcock <tyler_w>
Component: AccessibilityAssignee: 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:
Description Flags
Patch ews-feeder: commit-queue-

Tyler Wilcock
Reported 2022-09-01 17:56:05 PDT
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.
Attachments
Patch (46.81 KB, patch)
2022-09-01 18:54 PDT, Tyler Wilcock 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2022-09-01 17:56:21 PDT
<rdar://problem/99466565>
Tyler Wilcock
Comment 2 2022-09-01 18:54:15 PDT
Created attachment 462084 [details] Patch
chris fleizach
Comment 3 2022-09-01 22:05:15 PDT
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
Andres Gonzalez
Comment 4 2022-09-02 09:22:59 PDT
(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?
Andres Gonzalez
Comment 5 2022-09-02 10:38:33 PDT
(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.
Andres Gonzalez
Comment 6 2022-09-02 10:52:44 PDT
(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>.
Note You need to log in before you can comment on or make changes to this bug.