Bug 232503 – Move handling of ChildrenChanged notifications out of the AccessibilityObjects into AXObjectCache.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Andres Gonzalez]

Move handling of ChildrenChanged notifications out of the AccessibilityObjects into AXObjectCache.

[Radar WebKit Bug Importer]

<rdar://problem/84820154>

[Andres Gonzalez]

Created attachment 442856 [details]
Patch

[chris fleizach]

Comment on attachment 442856 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442856&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:992
> +        childrenChanged(object->parentObject());

if the text changed for an object, do we really need to notify the parent? or is it enough to recalculate just the children?

> Source/WebCore/accessibility/AXObjectCache.cpp:1147
> +        // FIXME: the above childrenChanged adds the object to m_deferredChildrenChangedList but there is no updateIsolatedTree after that children changed is handled in performDeferredCacheUpdate.

is this fix relevant to put here, or should be put this in preferomDeferredCache update ?

> Source/WebCore/accessibility/AccessibilityObject.h:649
> +    bool lastKnownIsIgnoredValue();

can we make this const

> Source/WebCore/accessibility/AccessibilityObject.h:651
> +    bool hasIgnoredValueChanged();

can we make this const

[Andres Gonzalez]

(In reply to chris fleizach from comment #3)
> Comment on attachment 442856 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442856&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:992
> > +        childrenChanged(object->parentObject());
> 
> if the text changed for an object, do we really need to notify the parent?
> or is it enough to recalculate just the children?

Before it was:

    if (object->parentObjectIfExists())
        object->notifyIfIgnoredValueChanged();

And AccessibilityObject::notifyIfIgnoredValueChanged was doing:

    if (lastKnownIsIgnoredValue() != isIgnored) {
        if (AXObjectCache* cache = axObjectCache())
            cache->childrenChanged(parentObject());

That is equivalent to what we are doing now:

    if (object->parentObjectIfExists() && object->hasIgnoredValueChanged())
        childrenChanged(object->parentObject());

So I'm preserving what it was there before for some time.

As for the need of this children changed notification on the parent, it is the kind of question that this patch allows to ask because it makes it evident. There was a reason why this extra notification was added, but it is not clear to me.

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1147
> > +        // FIXME: the above childrenChanged adds the object to m_deferredChildrenChangedList but there is no updateIsolatedTree after that children changed is handled in performDeferredCacheUpdate.
> 
> is this fix relevant to put here, or should be put this in
> preferomDeferredCache update ?

Replaced this comment with two new FIXME comments that better explain the issue with isolated tree updates.
> 
> > Source/WebCore/accessibility/AccessibilityObject.h:649
> > +    bool lastKnownIsIgnoredValue();
> 
> can we make this const

No because it updates m_lastKnownIsIgnoredValue.
> 
> > Source/WebCore/accessibility/AccessibilityObject.h:651
> > +    bool hasIgnoredValueChanged();
> 
> can we make this const

No because it updates m_lastKnownIsIgnoredValue. The function is actually, has it changed since last time I checked? and hence it can update when it checks.

[Andres Gonzalez]

Created attachment 442913 [details]
Patch

[EWS]

Committed r285092 (243734@main): <https://commits.webkit.org/243734@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442913 [details].

[Don Olmstead]

This ended up breaking the PlayStation build where !ENABLE(ACCESSIBILITY). There's no EWS for it so I'm fixing the issue over in https://bugs.webkit.org/show_bug.cgi?id=232590

Just a reminder to double check to see if there's a !ENABLE(ACCESSIBILITY) definition when modifying signatures here.