232141 2021-10-22 03:56:33 -0700 AX: Notify about children changed earlier in isolated tree mode 2021-11-16 05:42:23 -0800 1 1 1 Unclassified WebKit Accessibility WebKit Nightly Build All All RESOLVED WONTFIX InRadar P2 Normal --- 230259 1 cgarcia webkit-unassigned aboxhall andresg_22 aperez apinheiro cfleizach darin dmazzoni ews-watchlist jcraig jdiggs samuel_white tyler_w webkit-bug-importer oldest_to_newest 1807628 0 cgarcia 2021-10-22 03:56:33 -0700 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. 1807629 1 webkit-bug-importer 2021-10-22 03:56:43 -0700 <rdar://problem/84543935> 1807630 2 442147 cgarcia 2021-10-22 03:58:10 -0700 Created attachment 442147 Patch 1808177 3 442147 darin 2021-10-23 22:44:38 -0700 Comment on attachment 442147 Patch Can this be tested? Generally we expect that code will get broken over time if there’s no test 1808309 4 cgarcia 2021-10-25 00:20:38 -0700 hmm, there are indeed test failures, I thought isolated tree was not enabled in tests. I'll investigate them. 1808365 5 andresg_22 2021-10-25 06:09:44 -0700 (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. 1808375 6 cgarcia 2021-10-25 06:38:06 -0700 (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. 1808386 7 andresg_22 2021-10-25 07:20:55 -0700 (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? 1808765 8 cgarcia 2021-10-25 22:58:51 -0700 (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 1808799 9 cgarcia 2021-10-26 02:08:03 -0700 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. 1810503 10 andresg_22 2021-10-31 07:35:22 -0700 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. 1811104 11 cgarcia 2021-11-02 07:26:36 -0700 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. 1814405 12 aperez 2021-11-12 05:42:22 -0800 (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 :) 1814406 13 cgarcia 2021-11-12 05:59:01 -0800 I'm not in a hurry to land this and there are tests failing in mac that need more investigation. 1815434 14 cgarcia 2021-11-16 04:03:50 -0800 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. 1815435 15 442147 cgarcia 2021-11-16 04:04:06 -0800 Comment on attachment 442147 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. 1815445 16 andresg_22 2021-11-16 05:42:23 -0800 (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! 442147 2021-10-22 03:58:10 -0700 2021-11-16 04:04:06 -0800 Patch wcore-ac-isolated-tree-children-changed.diff text/plain 1855 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAzYzczZGJhMzI5NGEuLmMwYzdhMDE4ZmE4YiAxMDA2NDQKLS0tIGEvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAt MSwzICsxLDE5IEBACisyMDIxLTEwLTIyICBDYXJsb3MgR2FyY2lhIENhbXBvcyAgPGNnYXJjaWFA aWdhbGlhLmNvbT4KKworICAgICAgICBBWDogTm90aWZ5IGFib3V0IGNoaWxkcmVuIGNoYW5nZWQg ZWFybGllciBpbiBpc29sYXRlZCB0cmVlIG1vZGUKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTIzMjE0MQorICAgICAgICA8cmRhcjovL3Byb2JsZW0vODQ1 NDM5MzU+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg SW4gY2FzZSBvZiBpc29sYXRlZCB0cmVlIG1vZGUgdGhlIGNoaWxkcmVuIGNoYW5nZWQgbm90aWZp Y2F0aW9uIGhhcHBlbnMgYWZ0ZXIgdGhlIGRlZmVycmVkIGNhY2hlIHVwZGF0ZSBsaWtlIGluCisg ICAgICAgIG5vbi1pc29sYXRlZCB0cmVlIG1vZGUsIGJ1dCB0aGVuIGl0J3MgZGVmZXJyZWQgYWdh aW4gYnkgdGhlIG5vdGlmaWNhdGlvbiBwb3N0IHRpbWVyLiBUaGlzIGFmZmVjdHMgbGF5b3V0IHRl c3RzCisgICAgICAgIGRvaW5nIHRoaW5ncyBsaWtlIGZvby5hcHBlbmRDaGlsZChiYXIpICsgZm9v LmNoaWxkQXRJbmRleCgwKSwgYmVjYXVzZSBpbiBpc29sYXRlZCB0cmVlIG1vZGUgZm9vLmNoaWxk QXRJbmRleCgwKQorICAgICAgICByZXR1cm5zIG51bGwsIHRoZSBjaGlsZCB3cmFwcGVyIGhhc24n dCBiZWVuIGF0dGFjaGVkIHlldCB3aXRoIHRoZSBheCBpc29sYXRlZCBvYmplY3QuCisKKyAgICAg ICAgKiBhY2Nlc3NpYmlsaXR5L0FYT2JqZWN0Q2FjaGUuY3BwOgorICAgICAgICAoV2ViQ29yZTo6 QVhPYmplY3RDYWNoZTo6cG9zdE5vdGlmaWNhdGlvbik6CisKIDIwMjEtMTAtMjIgIENhcmxvcyBH YXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29tPgogCiAgICAgICAgIFtHVEtdW2ExMXld IExvY2FsaXplZCByb2xlIG5hbWUgZG9lc24ndCB3b3JrIHdpdGggQVRTUEkgZW5hYmxlZApkaWZm IC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9BWE9iamVjdENhY2hlLmNwcCBi L1NvdXJjZS9XZWJDb3JlL2FjY2Vzc2liaWxpdHkvQVhPYmplY3RDYWNoZS5jcHAKaW5kZXggMTgz N2RmMDJlMzcxLi5lZGE3YzA0MDIwNDMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2FjY2Vz c2liaWxpdHkvQVhPYmplY3RDYWNoZS5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvYWNjZXNzaWJp bGl0eS9BWE9iamVjdENhY2hlLmNwcApAQCAtMTE4MCw2ICsxMTgwLDExIEBAIHZvaWQgQVhPYmpl Y3RDYWNoZTo6cG9zdE5vdGlmaWNhdGlvbihBWENvcmVPYmplY3QqIG9iamVjdCwgRG9jdW1lbnQq IGRvY3VtZW50LCBBCiAgICAgaWYgKCFvYmplY3QpCiAgICAgICAgIHJldHVybjsKIAorI2lmIEVO QUJMRShBQ0NFU1NJQklMSVRZX0lTT0xBVEVEX1RSRUUpCisgICAgaWYgKG5vdGlmaWNhdGlvbiA9 PSBBWENoaWxkcmVuQ2hhbmdlZCkKKyAgICAgICAgdXBkYXRlSXNvbGF0ZWRUcmVlKCpvYmplY3Qs IEFYQ2hpbGRyZW5DaGFuZ2VkKTsKKyNlbmRpZgorCiAgICAgbV9ub3RpZmljYXRpb25zVG9Qb3N0 LmFwcGVuZChzdGQ6Om1ha2VfcGFpcihvYmplY3QsIG5vdGlmaWNhdGlvbikpOwogICAgIGlmICgh bV9ub3RpZmljYXRpb25Qb3N0VGltZXIuaXNBY3RpdmUoKSkKICAgICAgICAgbV9ub3RpZmljYXRp b25Qb3N0VGltZXIuc3RhcnRPbmVTaG90KDBfcyk7Cg==