Bug 232141

<rdar://problem/84543935>

Created attachment 442147 [details] Patch

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

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

(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.

(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.

(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?

(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

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.

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.

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.

(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 :)

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

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.

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.

(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!