AXPropertyName::IsEnabled is dependent on an object's disabled state, so it needs to be re-computed when that changes.
<rdar://problem/87683948>
Created attachment 449340 [details] Patch
Comment on attachment 449340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449340&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:3385 > + tree->updateNodeProperty(*notification.first, AXPropertyName::IsEnabled); Why do we have two methods that seem to do the same work? Can we combine those?
(In reply to chris fleizach from comment #3) > Comment on attachment 449340 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449340&action=review > > > Source/WebCore/accessibility/AXObjectCache.cpp:3385 > > + tree->updateNodeProperty(*notification.first, AXPropertyName::IsEnabled); > > Why do we have two methods that seem to do the same work? Can we combine > those? Agreed — I noticed this too (https://bugs.webkit.org/show_bug.cgi?id=235246#c6). Refactoring it is on my to-do list.
Unless you'd prefer I do that with this patch?
(In reply to Tyler Wilcock from comment #2) > Created attachment 449340 [details] > Patch --- a/LayoutTests/accessibility/dynamic-attribute-changes-should-update-isenabled.html +++ a/LayoutTests/accessibility/dynamic-attribute-changes-should-update-isenabled.html + function logAndEval(expression) { + debug(expression); + eval(expression); + } Can't we use evalAndLog instead of adding a new one? --- a/LayoutTests/resources/accessibility-helper.js +++ a/LayoutTests/resources/accessibility-helper.js The additions here are clever, but I'm not sure that we should do this since a real client it is not going to wait for layouts to finish. We should fix the core to prevent the scenario we are avoiding here from happening.
(In reply to Andres Gonzalez from comment #6) > (In reply to Tyler Wilcock from comment #2) > > Created attachment 449340 [details] > > Patch > > --- > a/LayoutTests/accessibility/dynamic-attribute-changes-should-update- > isenabled.html > +++ > a/LayoutTests/accessibility/dynamic-attribute-changes-should-update- > isenabled.html > > + function logAndEval(expression) { > + debug(expression); > + eval(expression); > + } > > Can't we use evalAndLog instead of adding a new one? Didn't know this existed, thanks! Will update. > --- a/LayoutTests/resources/accessibility-helper.js > +++ a/LayoutTests/resources/accessibility-helper.js > > The additions here are clever, but I'm not sure that we should do this since > a real client it is not going to wait for layouts to finish. We should fix > the core to prevent the scenario we are avoiding here from happening. It's fine that a client wouldn't wait for layouts (or anything else) -- they would get the right behavior in this test scenario regardless. The problem WaitForStableLayout solves is a testing one, in that we don't want to get the right behavior purely by timing of layouts re-computing entire objects with the correct value, we want to make sure we're issuing the correct `updateNodeProperty`. WaitForStableLayout guarantees we're testing the latter behavior. Without WaitForStableLayout (or some similar mechanism), the test added in this patch would pass with and without the patch, which isn't correct.
Created attachment 449405 [details] Patch
(In reply to Tyler Wilcock from comment #7) > (In reply to Andres Gonzalez from comment #6) > > (In reply to Tyler Wilcock from comment #2) > > > Created attachment 449340 [details] > > > Patch > > > > --- > > a/LayoutTests/accessibility/dynamic-attribute-changes-should-update- > > isenabled.html > > +++ > > a/LayoutTests/accessibility/dynamic-attribute-changes-should-update- > > isenabled.html > > > > + function logAndEval(expression) { > > + debug(expression); > > + eval(expression); > > + } > > > > Can't we use evalAndLog instead of adding a new one? > Didn't know this existed, thanks! Will update. > > > --- a/LayoutTests/resources/accessibility-helper.js > > +++ a/LayoutTests/resources/accessibility-helper.js > > > > The additions here are clever, but I'm not sure that we should do this since > > a real client it is not going to wait for layouts to finish. We should fix > > the core to prevent the scenario we are avoiding here from happening. > It's fine that a client wouldn't wait for layouts (or anything else) -- they > would get the right behavior in this test scenario regardless. The problem > WaitForStableLayout solves is a testing one, in that we don't want to get > the right behavior purely by timing of layouts re-computing entire objects > with the correct value, we want to make sure we're issuing the correct > `updateNodeProperty`. WaitForStableLayout guarantees we're testing the > latter behavior. Without WaitForStableLayout (or some similar mechanism), > the test added in this patch would pass with and without the patch, which > isn't correct. Ok, I see your point. Still think that it would be preferable not to have to do this. If we update one node property, we should see that the isolated tree updates just that property for that node, nothing else. The fact that there are more updates happening is the actual problem. So in that sense, this is a test workaround to test a property update. But we shouldn't need to do this at all if we were updating the isolated tree properly in the first place, i.e., not doing other spurious updates when a single property of a DOM node is updated.
> Ok, I see your point. Still think that it would be preferable not to have to > do this. If we update one node property, we should see that the isolated > tree updates just that property for that node, nothing else. The fact that > there are more updates happening is the actual problem. So in that sense, > this is a test workaround to test a property update. But we shouldn't need > to do this at all if we were updating the isolated tree properly in the > first place, i.e., not doing other spurious updates when a single property > of a DOM node is updated. I do generally agree with you, but I think the layouts causing this issue are generated via our `debug` statements, which change the DOM dynamically, rather than the property updates. Specifically, because the `debug` statements append above the test content, each one will cause the test content to shift lower, requiring layout, and causing the issue WaitForStableLayout solves. I don't think updating the tree in this type of layout is "wrong", just something we need to workaround for some tests. I theorize that this also wouldn't be a problem if `debug` and test output methods buffered output in memory until test completion rather than printing immediately. This behavior has caused us confusion in the past, too. But that's not a completely easy problem to solve either, since only async tests have an explicit test ending point in finishJSTest(). We would also need to figure out how to dump this buffer when the test runner tells the test to die via timeout, or crash (not even sure how that would work).
(In reply to Tyler Wilcock from comment #10) > > Ok, I see your point. Still think that it would be preferable not to have to > > do this. If we update one node property, we should see that the isolated > > tree updates just that property for that node, nothing else. The fact that > > there are more updates happening is the actual problem. So in that sense, > > this is a test workaround to test a property update. But we shouldn't need > > to do this at all if we were updating the isolated tree properly in the > > first place, i.e., not doing other spurious updates when a single property > > of a DOM node is updated. > I do generally agree with you, but I think the layouts causing this issue > are generated via our `debug` statements, which change the DOM dynamically, > rather than the property updates. Specifically, because the `debug` > statements append above the test content, each one will cause the test > content to shift lower, requiring layout, and causing the issue > WaitForStableLayout solves. I don't think updating the tree in this type of > layout is "wrong", just something we need to workaround for some tests. > > I theorize that this also wouldn't be a problem if `debug` and test output > methods buffered output in memory until test completion rather than printing > immediately. This behavior has caused us confusion in the past, too. But > that's not a completely easy problem to solve either, since only async tests > have an explicit test ending point in finishJSTest(). We would also need to > figure out how to dump this buffer when the test runner tells the test to > die via timeout, or crash (not even sure how that would work). We only updateIsolatedTree for certain notifications, and AXLayoutComplete is not one of them. Why are we updating the isolated tree when debug adds text to the page?
> We only updateIsolatedTree for certain notifications, and AXLayoutComplete > is not one of them. Why are we updating the isolated tree when debug adds > text to the page? I debugged it and hit a breakpoint where we calculate the correct value thanks to layout caused by `debug`. Here's a stacktrace, but the TLDR is that we get an AXChildrenChanged notification. Then, in AXIsolatedTree::updateChildren, we hit the "last resort" case: if (updatedChild || removals.size()) { // Make the children IDs of the isolated object to be the same as the AXObject's. Locker locker { m_changeLogLock }; updateChildrenIDs(axAncestor->objectID(), WTFMove(axChildrenIDs)); } else { // Nothing was updated. As a last resort, update the subtree. updateSubtree(*axAncestor); } And axAncestor is the web area: (objectID 19 (identifierAttribute ) (roleValue WebArea) (address 0x1030362a0) (wrapper 0x139684120) (parentObject 18) (remoteParentObject 0x1396b6190)) * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1 frame #0: 0x0000000148a54a74 WebCore`WebCore::AXIsolatedObject::initializeAttributeData(this=0x00000001052b2e00, coreObject=0x0000000105089940, isRoot=false) at AXIsolatedObject.cpp:99:17 frame #1: 0x0000000148a53fb4 WebCore`WebCore::AXIsolatedObject::AXIsolatedObject(this=0x00000001052b2e00, object=0x0000000105089940, tree=0x0000000105236a00, parentID=(m_identifier = 22)) at AXIsolatedObject.cpp:48:9 frame #2: 0x0000000148a5bcb8 WebCore`WebCore::AXIsolatedObject::AXIsolatedObject(this=0x00000001052b2e00, object=0x0000000105089940, tree=0x0000000105236a00, parentID=(m_identifier = 22)) at AXIsolatedObject.cpp:45:1 frame #3: 0x0000000148a5bd10 WebCore`WebCore::AXIsolatedObject::create(object=0x0000000105089940, tree=0x0000000105236a00, parentID=(m_identifier = 22)) at AXIsolatedObject.cpp:57:26 frame #4: 0x0000000148a69510 WebCore`WebCore::AXIsolatedTree::createSubtree(this=0x0000000105236a00, axObject=0x0000000105089940, parentID=(m_identifier = 22), attachWrapper=false) at AXIsolatedTree.cpp:214:19 frame #5: 0x0000000148a69758 WebCore`WebCore::AXIsolatedTree::createSubtree(this=0x0000000105236a00, axObject=0x0000000105089800, parentID=(m_identifier = 19), attachWrapper=false) at AXIsolatedTree.cpp:235:22 frame #6: 0x0000000148a69758 WebCore`WebCore::AXIsolatedTree::createSubtree(this=0x0000000105236a00, axObject=0x0000000105088f40, parentID=(m_identifier = 18), attachWrapper=false) at AXIsolatedTree.cpp:235:22 frame #7: 0x0000000148a68644 WebCore`WebCore::AXIsolatedTree::generateSubtree(this=0x0000000105236a00, axObject=0x0000000105088f40, axParent=0x0000000105203d80, attachWrapper=false) at AXIsolatedTree.cpp:197:19 frame #8: 0x0000000148a6a768 WebCore`WebCore::AXIsolatedTree::updateSubtree(this=0x0000000105236a00, axObject=0x0000000105088f40) at AXIsolatedTree.cpp:304:5 frame #9: 0x0000000148a6b0c8 WebCore`WebCore::AXIsolatedTree::updateChildren(this=0x0000000105236a00, axObject=0x00000001050896c0) at AXIsolatedTree.cpp:375:9 * frame #10: 0x000000014897fa18 WebCore`WebCore::AXObjectCache::updateIsolatedTree(this=0x000000010512e6e0, notifications={ size = 1, capacity = 1 }) at AXObjectCache.cpp:3416:23 frame #11: 0x0000000148977954 WebCore`WebCore::AXObjectCache::notificationPostTimerFired(this=0x000000010512e6e0) at AXObjectCache.cpp:1126:5
I think to your point, AXChildrenChanged is not the same as layout, so the WaitForStableLayout abstraction in my patch is probably wrong.
(In reply to Tyler Wilcock from comment #12) > > We only updateIsolatedTree for certain notifications, and AXLayoutComplete > > is not one of them. Why are we updating the isolated tree when debug adds > > text to the page? > I debugged it and hit a breakpoint where we calculate the correct value > thanks to layout caused by `debug`. Here's a stacktrace, but the TLDR is > that we get an AXChildrenChanged notification. Then, in > AXIsolatedTree::updateChildren, we hit the "last resort" case: > > if (updatedChild || removals.size()) { > // Make the children IDs of the isolated object to be the same as the > AXObject's. > Locker locker { m_changeLogLock }; > updateChildrenIDs(axAncestor->objectID(), WTFMove(axChildrenIDs)); > } else { > // Nothing was updated. As a last resort, update the subtree. > updateSubtree(*axAncestor); > } > > And axAncestor is the web area: > > (objectID 19 > (identifierAttribute ) > (roleValue WebArea) > (address 0x1030362a0) > (wrapper 0x139684120) > (parentObject 18) > (remoteParentObject 0x1396b6190)) > > * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1 > frame #0: 0x0000000148a54a74 > WebCore`WebCore::AXIsolatedObject:: > initializeAttributeData(this=0x00000001052b2e00, > coreObject=0x0000000105089940, isRoot=false) at AXIsolatedObject.cpp:99:17 > frame #1: 0x0000000148a53fb4 > WebCore`WebCore::AXIsolatedObject::AXIsolatedObject(this=0x00000001052b2e00, > object=0x0000000105089940, tree=0x0000000105236a00, parentID=(m_identifier = > 22)) at AXIsolatedObject.cpp:48:9 > frame #2: 0x0000000148a5bcb8 > WebCore`WebCore::AXIsolatedObject::AXIsolatedObject(this=0x00000001052b2e00, > object=0x0000000105089940, tree=0x0000000105236a00, parentID=(m_identifier = > 22)) at AXIsolatedObject.cpp:45:1 > frame #3: 0x0000000148a5bd10 > WebCore`WebCore::AXIsolatedObject::create(object=0x0000000105089940, > tree=0x0000000105236a00, parentID=(m_identifier = 22)) at > AXIsolatedObject.cpp:57:26 > frame #4: 0x0000000148a69510 > WebCore`WebCore::AXIsolatedTree::createSubtree(this=0x0000000105236a00, > axObject=0x0000000105089940, parentID=(m_identifier = 22), > attachWrapper=false) at AXIsolatedTree.cpp:214:19 > frame #5: 0x0000000148a69758 > WebCore`WebCore::AXIsolatedTree::createSubtree(this=0x0000000105236a00, > axObject=0x0000000105089800, parentID=(m_identifier = 19), > attachWrapper=false) at AXIsolatedTree.cpp:235:22 > frame #6: 0x0000000148a69758 > WebCore`WebCore::AXIsolatedTree::createSubtree(this=0x0000000105236a00, > axObject=0x0000000105088f40, parentID=(m_identifier = 18), > attachWrapper=false) at AXIsolatedTree.cpp:235:22 > frame #7: 0x0000000148a68644 > WebCore`WebCore::AXIsolatedTree::generateSubtree(this=0x0000000105236a00, > axObject=0x0000000105088f40, axParent=0x0000000105203d80, > attachWrapper=false) at AXIsolatedTree.cpp:197:19 > frame #8: 0x0000000148a6a768 > WebCore`WebCore::AXIsolatedTree::updateSubtree(this=0x0000000105236a00, > axObject=0x0000000105088f40) at AXIsolatedTree.cpp:304:5 > frame #9: 0x0000000148a6b0c8 > WebCore`WebCore::AXIsolatedTree::updateChildren(this=0x0000000105236a00, > axObject=0x00000001050896c0) at AXIsolatedTree.cpp:375:9 > * frame #10: 0x000000014897fa18 > WebCore`WebCore::AXObjectCache::updateIsolatedTree(this=0x000000010512e6e0, > notifications={ size = 1, capacity = 1 }) at AXObjectCache.cpp:3416:23 > frame #11: 0x0000000148977954 > WebCore`WebCore::AXObjectCache:: > notificationPostTimerFired(this=0x000000010512e6e0) at > AXObjectCache.cpp:1126:5 Yes, that's the real bug, we shouldn't be re-generating the web area subtree as a result of calling debug.
Created attachment 449753 [details] Patch
https://bugs.webkit.org/show_bug.cgi?id=235389 has fixed the root issue, so this new patch leaves just the new test and the small AXPropertyName::IsEnabled handling that has already been r+'d by Chris and also looked over by Andres. Committing.
Committed r288425 (246316@main): <https://commits.webkit.org/246316@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449753 [details].