Bug 234059 – Decouple AXObjectCache handleChildrenChanged and postNotification.

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

[Andres Gonzalez]

Decouple AXObjectCache handleChildrenChanged and postNotification.

[Radar WebKit Bug Importer]

<rdar://problem/86247404>

[Andres Gonzalez]

Created attachment 446484 [details]
Patch

[chris fleizach]

Comment on attachment 446484 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        notification (postNotification), which causes a doble deferral of these

doble > double

> Source/WebCore/ChangeLog:16
> +        added or removed, before properties in the rsulting objects may be

rsulting > resulting

> Source/WebCore/ChangeLog:17
> +        updated. This patch fixes this problem by making handleChildrenchanged

handleChildrenchanged > handleChildrenChanged

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

question here - if we're going up the parent chain telling things we need to update, do we need to process this subtree any further? if a parent has changed won't it recreate the entire subtree?

> Source/WebCore/accessibility/AXObjectCache.cpp:1801
> +        handleChildrenChanged(*get(element->parentNode()));

we probably want to defer this along with the modal change
or maybe deferModalChange should also implicitly handle children changed on the paren

[Andres Gonzalez]

Created attachment 446647 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #3)
> Comment on attachment 446484 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446484&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        notification (postNotification), which causes a doble deferral of these
> 
> doble > double

Fixed.
> 
> > Source/WebCore/ChangeLog:16
> > +        added or removed, before properties in the rsulting objects may be
> 
> rsulting > resulting

Fixed.
> 
> > Source/WebCore/ChangeLog:17
> > +        updated. This patch fixes this problem by making handleChildrenchanged
> 
> handleChildrenchanged > handleChildrenChanged

Fixed.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:995
> > +            childrenChanged(parent->parentObject());
> 
> question here - if we're going up the parent chain telling things we need to
> update, do we need to process this subtree any further? if a parent has
> changed won't it recreate the entire subtree?

You're right. I thought we might need to go up the entire chain checking whether isIgnored had change, but it doesn't seem to be necessary, at least for the test cases we have.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1801
> > +        handleChildrenChanged(*get(element->parentNode()));
> 
> we probably want to defer this along with the modal change
> or maybe deferModalChange should also implicitly handle children changed on
> the paren

Done. thanks!

[Andres Gonzalez]

Created attachment 447090 [details]
Patch

[Andres Gonzalez]

Created attachment 447314 [details]
Patch

[Andres Gonzalez]

Created attachment 451417 [details]
Patch

[Tyler Wilcock]

Comment on attachment 451417 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1049
> +    postPlatformNotification(&object, AXChildrenChanged);

Do we still need to post this notification since we now immediately handle this children changed event by calling AXObjectCache::childrenChanged and AXObjectCache::updateIsolatedTree?

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #9)
> Comment on attachment 451417 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451417&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1049
> > +    postPlatformNotification(&object, AXChildrenChanged);
> 
> Do we still need to post this notification since we now immediately handle
> this children changed event by calling AXObjectCache::childrenChanged and
> AXObjectCache::updateIsolatedTree?

The naming isn't great here, but postPlatformNotification actually just fires the platform notification, as opposed to postNotification which is the one that causes changes in the state of the live tree and consequently in the isolated tree. That's why I see this as a decoupling between the handling the notification which means updating the trees state, and once that is done, notifying clients that something changed, which is postPlatformNotification.

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #10)
> (In reply to Tyler Wilcock from comment #9)
> > Comment on attachment 451417 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=451417&action=review
> > 
> > > Source/WebCore/accessibility/AXObjectCache.cpp:1049
> > > +    postPlatformNotification(&object, AXChildrenChanged);
> > 
> > Do we still need to post this notification since we now immediately handle
> > this children changed event by calling AXObjectCache::childrenChanged and
> > AXObjectCache::updateIsolatedTree?
> 
> The naming isn't great here, but postPlatformNotification actually just
> fires the platform notification, as opposed to postNotification which is the
> one that causes changes in the state of the live tree and consequently in
> the isolated tree. That's why I see this as a decoupling between the
> handling the notification which means updating the trees state, and once
> that is done, notifying clients that something changed, which is
> postPlatformNotification.
Ahh, got it. Nice, yeah this seems like a good change.

[chris fleizach]

Comment on attachment 451417 [details]
Patch

looks good assuming tests pass now

[Tyler Wilcock]

I think somebody clicked the "Retry failed tests" button, but that won't include the latest changes from trunk that speed up the test. A new patch might have to be uploaded.

[Andres Gonzalez]

Created attachment 451687 [details]
Patch

[Andres Gonzalez]

Created attachment 451840 [details]
Patch

[EWS]

Committed r289723 (247208@main): <https://commits.webkit.org/247208@main>

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