Bug 232141 – AX: Notify about children changed earlier in isolated tree mode

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Accessibility

[Carlos Garcia Campos]

In case of isolated tree mode the children changed notification happens after the deferred cache update like in non-isolated tree mode, but then it's deferred again by the notification post timer. This affects layout tests doing things like foo.appendChild(bar) + foo.childAtIndex(0), because in isolated tree mode foo.childAtIndex(0) returns null, the child wrapper hasn't been attached yet with the ax isolated object.

[Radar WebKit Bug Importer]

<rdar://problem/84543935>

[Carlos Garcia Campos]

Created attachment 442147 [details]
Patch

[Darin Adler]

Comment on attachment 442147 [details]
Patch

Can this be tested? Generally we expect that code will get broken over time if there’s no test

[Carlos Garcia Campos]

hmm, there are indeed test failures, I thought isolated tree was not enabled in tests. I'll investigate them.

[Andres Gonzalez]

(In reply to Carlos Garcia Campos from comment #2)
> Created attachment 442147 [details]
> Patch

+        non-isolated tree mode, but then it's deferred again by the notification post timer. This affects layout tests
+        doing things like foo.appendChild(bar) + foo.childAtIndex(0), because in isolated tree mode foo.childAtIndex(0)
+        returns null, the child wrapper hasn't been attached yet with the ax isolated object.
+

That is true for all properties, not only for children. I believe we have to make the LayoutTests async, i.e., waitFor foo.childAtIndex(0) is not null in your example above.

[Carlos Garcia Campos]

(In reply to Andres Gonzalez from comment #5)
> (In reply to Carlos Garcia Campos from comment #2)
> > Created attachment 442147 [details]
> > Patch
> 
> +        non-isolated tree mode, but then it's deferred again by the
> notification post timer. This affects layout tests
> +        doing things like foo.appendChild(bar) + foo.childAtIndex(0),
> because in isolated tree mode foo.childAtIndex(0)
> +        returns null, the child wrapper hasn't been attached yet with the
> ax isolated object.
> +
> 
> That is true for all properties, not only for children. I believe we have to
> make the LayoutTests async, i.e., waitFor foo.childAtIndex(0) is not null in
> your example above.

We are already notifying early other properties, though. I wonder how the tests pass in mac with isolated tree enabled.

[Andres Gonzalez]

(In reply to Carlos Garcia Campos from comment #6)
> (In reply to Andres Gonzalez from comment #5)
> > (In reply to Carlos Garcia Campos from comment #2)
> > > Created attachment 442147 [details]
> > > Patch
> > 
> > +        non-isolated tree mode, but then it's deferred again by the
> > notification post timer. This affects layout tests
> > +        doing things like foo.appendChild(bar) + foo.childAtIndex(0),
> > because in isolated tree mode foo.childAtIndex(0)
> > +        returns null, the child wrapper hasn't been attached yet with the
> > ax isolated object.
> > +
> > 
> > That is true for all properties, not only for children. I believe we have to
> > make the LayoutTests async, i.e., waitFor foo.childAtIndex(0) is not null in
> > your example above.
> 
> We are already notifying early other properties, though. I wonder how the
> tests pass in mac with isolated tree enabled.

Where are we notifying early other properties?

[Carlos Garcia Campos]

(In reply to Andres Gonzalez from comment #7)
> (In reply to Carlos Garcia Campos from comment #6)
> > (In reply to Andres Gonzalez from comment #5)
> > > (In reply to Carlos Garcia Campos from comment #2)
> > > > Created attachment 442147 [details]
> > > > Patch
> > > 
> > > +        non-isolated tree mode, but then it's deferred again by the
> > > notification post timer. This affects layout tests
> > > +        doing things like foo.appendChild(bar) + foo.childAtIndex(0),
> > > because in isolated tree mode foo.childAtIndex(0)
> > > +        returns null, the child wrapper hasn't been attached yet with the
> > > ax isolated object.
> > > +
> > > 
> > > That is true for all properties, not only for children. I believe we have to
> > > make the LayoutTests async, i.e., waitFor foo.childAtIndex(0) is not null in
> > > your example above.
> > 
> > We are already notifying early other properties, though. I wonder how the
> > tests pass in mac with isolated tree enabled.
> 
> Where are we notifying early other properties?

https://github.com/WebKit/WebKit/blob/main/Source/WebCore/accessibility/AXObjectCache.cpp#L1476
https://github.com/WebKit/WebKit/blob/main/Source/WebCore/accessibility/AXObjectCache.cpp#L1515
https://github.com/WebKit/WebKit/blob/main/Source/WebCore/accessibility/AXObjectCache.cpp#L1723
https://github.com/WebKit/WebKit/blob/main/Source/WebCore/accessibility/AXObjectCache.cpp#L1774

[Carlos Garcia Campos]

I'm having the same problem with AXValueChanged, I need to do an early isolated tree update for test accessibility/range-alter-by-step.html to work (in addition to bug #232298). Without that, when the value is queried after an increment/decrement, the isolated tree still contains the previous value.

[Andres Gonzalez]

We cannot update the isolated tree in AXObjectCache::postNotification because it can be called in the middle of a layout, in which case the render tree is not ready for all the queries accessibility will make, or it can trigger a layout. That's why we have moved most or all notification handling to deferred handlers. This is hinted by the comment in postNotification that reads:

    // Get an accessibility object that already exists. One should not be created here
    // because a render update may be in progress and creating an AX object can re-trigger a layout

updateIsolatedTree may create new AX objects and query for lots of properties that the render tree is not ready to compute.

There may be some properties that could be updated early like some ARIA attributes, because they won't cause any changes to the render tree. But we need to special case those.

[Carlos Garcia Campos]

hmm, I don't think childrenChanged can be called in the middle of a layout, since render tree mutation is disallowed by layout, no? We can probably add an assert to ensure it's never called in the middle of a layout.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #11)
> hmm, I don't think childrenChanged can be called in the middle of a layout,
> since render tree mutation is disallowed by layout, no? We can probably add
> an assert to ensure it's never called in the middle of a layout.

This seems to be the case, IIUC — but I am not super familiar with the related
code, so I will tentatively r+ this, and we can wait for example until the
Monday before landing in case Andres (or anybody else) still has concerns :)

[Carlos Garcia Campos]

I'm not in a hurry to land this and there are tests failing in mac that need more investigation.

[Carlos Garcia Campos]

I see some tests have already been rewritten to use async apis to make them pass in isolated tree mode, so maybe this won't be needed in the end. I'm going to close this for now. Once I finish the ATSPI implementation I'll check if there are still tests failing because of this and if it's possible to rewrite them all or reconsider this.

[Carlos Garcia Campos]

Comment on attachment 442147 [details]
Patch

I see some tests have already be rewritten to use async apis to make them pass in isolated tree mode, so maybe this won't be needed in the end.

[Andres Gonzalez]

(In reply to Carlos Garcia Campos from comment #15)
> Comment on attachment 442147 [details]
> Patch
> 
> I see some tests have already be rewritten to use async apis to make them
> pass in isolated tree mode, so maybe this won't be needed in the end.

That's right, Carlos, we are actively working in making all necessary tests async. If you find problematic tests in ATSPI, let me know and will work together to address them. Thanks!