Bug 232503 - Move handling of ChildrenChanged notifications out of the AccessibilityObjects into AXObjectCache.
Summary: Move handling of ChildrenChanged notifications out of the AccessibilityObject...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-29 12:43 PDT by Andres Gonzalez
Modified: 2021-11-01 16:08 PDT (History)
11 users (show)

See Also:


Attachments
Patch (26.13 KB, patch)
2021-10-29 13:16 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (26.68 KB, patch)
2021-10-30 11:53 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2021-10-29 12:43:29 PDT
Move handling of ChildrenChanged notifications out of the AccessibilityObjects into AXObjectCache.
Comment 1 Radar WebKit Bug Importer 2021-10-29 12:43:43 PDT
<rdar://problem/84820154>
Comment 2 Andres Gonzalez 2021-10-29 13:16:33 PDT
Created attachment 442856 [details]
Patch
Comment 3 chris fleizach 2021-10-29 14:20:46 PDT
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
Comment 4 Andres Gonzalez 2021-10-30 11:47:53 PDT
(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.
Comment 5 Andres Gonzalez 2021-10-30 11:53:54 PDT
Created attachment 442913 [details]
Patch
Comment 6 EWS 2021-10-31 06:41:41 PDT
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].
Comment 7 Don Olmstead 2021-11-01 16:08:24 PDT
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.