237014 2022-02-21 19:04:33 -0800 AX: Fix accessibility/aria-current-state-changed-notification.html in isolated tree mode 2022-02-22 09:07:14 -0800 1 1 1 Unclassified WebKit Accessibility WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 tyler_w tyler_w aboxhall andresg_22 apinheiro cfleizach dmazzoni ews-watchlist jbedard jcraig jdiggs samuel_white webkit-bug-importer oldest_to_newest 1844509 0 tyler_w 2022-02-21 19:04:33 -0800 We need to update AXPropertyName::CurrentValue when we get a AXCurrentStateChanged notification. 1844510 1 webkit-bug-importer 2022-02-21 19:04:45 -0800 <rdar://problem/89270112> 1844511 2 452811 tyler_w 2022-02-21 19:06:45 -0800 Created attachment 452811 Patch 1844653 3 andresg_22 2022-02-22 05:28:47 -0800 (In reply to Tyler Wilcock from comment #2) > Created attachment 452811 [details] > Patch --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp + case AXPropertyName::CurrentValue: + propertyMap.set(AXPropertyName::CurrentValue, axObject.currentValue().isolatedCopy()); + break; Do we need to also clear the state in the object that is loosing current? Or we get separate notifications for each. 1844701 4 tyler_w 2022-02-22 07:54:36 -0800 (In reply to Andres Gonzalez from comment #3) > (In reply to Tyler Wilcock from comment #2) > > Created attachment 452811 [details] > > Patch > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > > + case AXPropertyName::CurrentValue: > + propertyMap.set(AXPropertyName::CurrentValue, > axObject.currentValue().isolatedCopy()); > + break; > > Do we need to also clear the state in the object that is loosing current? Or > we get separate notifications for each. (In reply to Andres Gonzalez from comment #3) > (In reply to Tyler Wilcock from comment #2) > > Created attachment 452811 [details] > > Patch > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > > + case AXPropertyName::CurrentValue: > + propertyMap.set(AXPropertyName::CurrentValue, > axObject.currentValue().isolatedCopy()); > + break; > > Do we need to also clear the state in the object that is loosing current? Or > we get separate notifications for each. We get an AXCurrentStateChanged notification for each individual element that changes aria-current, so assuming correct authoring we would get separate notifications for each -- one gaining current status, and the other losing it. 1844708 5 andresg_22 2022-02-22 08:04:28 -0800 (In reply to Tyler Wilcock from comment #4) > (In reply to Andres Gonzalez from comment #3) > > (In reply to Tyler Wilcock from comment #2) > > > Created attachment 452811 [details] > > > Patch > > > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > > > > + case AXPropertyName::CurrentValue: > > + propertyMap.set(AXPropertyName::CurrentValue, > > axObject.currentValue().isolatedCopy()); > > + break; > > > > Do we need to also clear the state in the object that is loosing current? Or > > we get separate notifications for each. > (In reply to Andres Gonzalez from comment #3) > > (In reply to Tyler Wilcock from comment #2) > > > Created attachment 452811 [details] > > > Patch > > > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > > > > + case AXPropertyName::CurrentValue: > > + propertyMap.set(AXPropertyName::CurrentValue, > > axObject.currentValue().isolatedCopy()); > > + break; > > > > Do we need to also clear the state in the object that is loosing current? Or > > we get separate notifications for each. > We get an AXCurrentStateChanged notification for each individual element > that changes aria-current, so assuming correct authoring we would get > separate notifications for each -- one gaining current status, and the other > losing it. Is the test covering that? 1844710 6 andresg_22 2022-02-22 08:07:01 -0800 (In reply to Tyler Wilcock from comment #4) > (In reply to Andres Gonzalez from comment #3) > > (In reply to Tyler Wilcock from comment #2) > > > Created attachment 452811 [details] > > > Patch > > > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > > > > + case AXPropertyName::CurrentValue: > > + propertyMap.set(AXPropertyName::CurrentValue, > > axObject.currentValue().isolatedCopy()); > > + break; > > > > Do we need to also clear the state in the object that is loosing current? Or > > we get separate notifications for each. > (In reply to Andres Gonzalez from comment #3) > > (In reply to Tyler Wilcock from comment #2) > > > Created attachment 452811 [details] > > > Patch > > > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > > > > + case AXPropertyName::CurrentValue: > > + propertyMap.set(AXPropertyName::CurrentValue, > > axObject.currentValue().isolatedCopy()); > > + break; > > > > Do we need to also clear the state in the object that is loosing current? Or > > we get separate notifications for each. > We get an AXCurrentStateChanged notification for each individual element > that changes aria-current, so assuming correct authoring we would get > separate notifications for each -- one gaining current status, and the other > losing it. (In reply to Andres Gonzalez from comment #5) > (In reply to Tyler Wilcock from comment #4) > > (In reply to Andres Gonzalez from comment #3) > > > (In reply to Tyler Wilcock from comment #2) > > > > Created attachment 452811 [details] > > > > Patch > > > > > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > > > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > > > > > > + case AXPropertyName::CurrentValue: > > > + propertyMap.set(AXPropertyName::CurrentValue, > > > axObject.currentValue().isolatedCopy()); > > > + break; > > > > > > Do we need to also clear the state in the object that is loosing current? Or > > > we get separate notifications for each. > > (In reply to Andres Gonzalez from comment #3) > > > (In reply to Tyler Wilcock from comment #2) > > > > Created attachment 452811 [details] > > > > Patch > > > > > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > > > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > > > > > > + case AXPropertyName::CurrentValue: > > > + propertyMap.set(AXPropertyName::CurrentValue, > > > axObject.currentValue().isolatedCopy()); > > > + break; > > > > > > Do we need to also clear the state in the object that is loosing current? Or > > > we get separate notifications for each. > > We get an AXCurrentStateChanged notification for each individual element > > that changes aria-current, so assuming correct authoring we would get > > separate notifications for each -- one gaining current status, and the other > > losing it. > > Is the test covering that? It wasn't important before because we would query the live object, but it is important now in ITM. 1844714 7 tyler_w 2022-02-22 08:11:01 -0800 > > Is the test covering that? > > It wasn't important before because we would query the live object, but it is > important now in ITM. It does. The expectations exercised by the test are: Initial state item2: page item3: false Update item2 aria-current to false item2: false item3: false Update item3 aria-current to page item2: false item3: page 1844715 8 andresg_22 2022-02-22 08:15:25 -0800 (In reply to Tyler Wilcock from comment #7) > > > Is the test covering that? > > > > It wasn't important before because we would query the live object, but it is > > important now in ITM. > It does. The expectations exercised by the test are: > > Initial state > item2: page > item3: false > > Update item2 aria-current to false > item2: false > item3: false > > Update item3 aria-current to page > item2: false > item3: page Great, thanks! 1844721 9 jbedard 2022-02-22 08:22:48 -0800 Stopped https://ews-build.webkit.org/#/builders/70/builds/984, the 3 failures are already known. 1844735 10 ews-feeder 2022-02-22 09:07:10 -0800 Committed r290309 (247632@main): <https://commits.webkit.org/247632@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 452811. 452811 2022-02-21 19:06:45 -0800 2022-02-22 09:07:12 -0800 Patch bug-237014-20220221210644.patch text/plain 2834 tyler_w U3VidmVyc2lvbiBSZXZpc2lvbjogMjkwMTMzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMzE2MGQwOTNkYzMxNzc0 ZjUzMzEzYzE2NDVkNWJiOThhODdjNGE0MC4uY2E3NjIyMzBhYjU1ZjFkZDYzNDllNGRkM2E0NTU5 ODUzMzk0YzJlOCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDIyLTAyLTIxICBUeWxl ciBXaWxjb2NrICA8dHlsZXJfd0BhcHBsZS5jb20+CisKKyAgICAgICAgQVg6IEZpeCBhY2Nlc3Np YmlsaXR5L2FyaWEtY3VycmVudC1zdGF0ZS1jaGFuZ2VkLW5vdGlmaWNhdGlvbi5odG1sIGluIGlz b2xhdGVkIHRyZWUgbW9kZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9MjM3MDE0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgV2UgbmVlZCB0byB1cGRhdGUgQVhQcm9wZXJ0eU5hbWU6OkN1cnJlbnRWYWx1ZSB3 aGVuIHdlIGdldCBhIEFYQ3VycmVudFN0YXRlQ2hhbmdlZCBub3RpZmljYXRpb24uCisKKyAgICAg ICAgRml4ZXMgYWNjZXNzaWJpbGl0eS9hcmlhLWN1cnJlbnQtc3RhdGUtY2hhbmdlZC1ub3RpZmlj YXRpb24uaHRtbCBpbiBpc29sYXRlZCB0cmVlIG1vZGUuCisKKyAgICAgICAgKiBhY2Nlc3NpYmls aXR5L0FYT2JqZWN0Q2FjaGUuY3BwOgorICAgICAgICAoV2ViQ29yZTo6QVhPYmplY3RDYWNoZTo6 dXBkYXRlSXNvbGF0ZWRUcmVlKToKKyAgICAgICAgKiBhY2Nlc3NpYmlsaXR5L2lzb2xhdGVkdHJl ZS9BWElzb2xhdGVkVHJlZS5jcHA6CisKIDIwMjItMDItMTggIFBlbmcgTGl1ICA8cGVuZy5saXU2 QGFwcGxlLmNvbT4KIAogICAgICAgICBbaU9TXVtNb2Rlcm4gTWVkaWEgQ29udHJvbHNdIEFkZCBh biBhbHRlcm5hdGl2ZSBvdmVyZmxvdyBpY29uCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9h Y2Nlc3NpYmlsaXR5L0FYT2JqZWN0Q2FjaGUuY3BwIGIvU291cmNlL1dlYkNvcmUvYWNjZXNzaWJp bGl0eS9BWE9iamVjdENhY2hlLmNwcAppbmRleCAwMzM5YTdkNzc4ZDhmMDljMjU5NzhjMDk3MGFm MzE3MmExNTNmNTQzLi4wZmUxNDNjMmI4ZWEyOTUxNzUyNjYxMmM5ZjZhMjg2ZWMyZWYzMDgwIDEw MDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L0FYT2JqZWN0Q2FjaGUuY3Bw CisrKyBiL1NvdXJjZS9XZWJDb3JlL2FjY2Vzc2liaWxpdHkvQVhPYmplY3RDYWNoZS5jcHAKQEAg LTMzNTUsNiArMzM1NSw5IEBAIHZvaWQgQVhPYmplY3RDYWNoZTo6dXBkYXRlSXNvbGF0ZWRUcmVl KGNvbnN0IFZlY3RvcjxzdGQ6OnBhaXI8UmVmUHRyPEFYQ29yZU9iamVjCiAgICAgICAgIGNhc2Ug QVhDaGVja2VkU3RhdGVDaGFuZ2VkOgogICAgICAgICAgICAgdHJlZS0+dXBkYXRlTm9kZVByb3Bl cnR5KCpub3RpZmljYXRpb24uZmlyc3QsIEFYUHJvcGVydHlOYW1lOjpJc0NoZWNrZWQpOwogICAg ICAgICAgICAgYnJlYWs7CisgICAgICAgIGNhc2UgQVhDdXJyZW50U3RhdGVDaGFuZ2VkOgorICAg ICAgICAgICAgdHJlZS0+dXBkYXRlTm9kZVByb3BlcnR5KCpub3RpZmljYXRpb24uZmlyc3QsIEFY UHJvcGVydHlOYW1lOjpDdXJyZW50VmFsdWUpOworICAgICAgICAgICAgYnJlYWs7CiAgICAgICAg IGNhc2UgQVhEaXNhYmxlZFN0YXRlQ2hhbmdlZDoKICAgICAgICAgICAgIHRyZWUtPnVwZGF0ZU5v ZGVQcm9wZXJ0eSgqbm90aWZpY2F0aW9uLmZpcnN0LCBBWFByb3BlcnR5TmFtZTo6Q2FuU2V0Rm9j dXNBdHRyaWJ1dGUpOwogICAgICAgICAgICAgdHJlZS0+dXBkYXRlTm9kZVByb3BlcnR5KCpub3Rp ZmljYXRpb24uZmlyc3QsIEFYUHJvcGVydHlOYW1lOjpJc0VuYWJsZWQpOwpkaWZmIC0tZ2l0IGEv U291cmNlL1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9pc29sYXRlZHRyZWUvQVhJc29sYXRlZFRyZWUu Y3BwIGIvU291cmNlL1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9pc29sYXRlZHRyZWUvQVhJc29sYXRl ZFRyZWUuY3BwCmluZGV4IGRkNGEyODU3ZDA2NWIyNWRmNzdmY2IwMDNkMTA4ZjRhZjM4ODhhYjQu LjI3Yzg4OWFlNjc3ZTJkMzczMjFjYzRlNzMwMDM4ZjIyZGY4NTBiMzEgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9XZWJDb3JlL2FjY2Vzc2liaWxpdHkvaXNvbGF0ZWR0cmVlL0FYSXNvbGF0ZWRUcmVlLmNw cAorKysgYi9Tb3VyY2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L2lzb2xhdGVkdHJlZS9BWElzb2xh dGVkVHJlZS5jcHAKQEAgLTMyMSw2ICszMjEsOSBAQCB2b2lkIEFYSXNvbGF0ZWRUcmVlOjp1cGRh dGVOb2RlUHJvcGVydHkoY29uc3QgQVhDb3JlT2JqZWN0JiBheE9iamVjdCwgQVhQcm9wZXJ0eQog ICAgIGNhc2UgQVhQcm9wZXJ0eU5hbWU6OkNhblNldEZvY3VzQXR0cmlidXRlOgogICAgICAgICBw cm9wZXJ0eU1hcC5zZXQoQVhQcm9wZXJ0eU5hbWU6OkNhblNldEZvY3VzQXR0cmlidXRlLCBheE9i amVjdC5jYW5TZXRGb2N1c0F0dHJpYnV0ZSgpKTsKICAgICAgICAgYnJlYWs7CisgICAgY2FzZSBB WFByb3BlcnR5TmFtZTo6Q3VycmVudFZhbHVlOgorICAgICAgICBwcm9wZXJ0eU1hcC5zZXQoQVhQ cm9wZXJ0eU5hbWU6OkN1cnJlbnRWYWx1ZSwgYXhPYmplY3QuY3VycmVudFZhbHVlKCkuaXNvbGF0 ZWRDb3B5KCkpOworICAgICAgICBicmVhazsKICAgICBjYXNlIEFYUHJvcGVydHlOYW1lOjpJc0No ZWNrZWQ6CiAgICAgICAgIHByb3BlcnR5TWFwLnNldChBWFByb3BlcnR5TmFtZTo6SXNDaGVja2Vk LCBheE9iamVjdC5pc0NoZWNrZWQoKSk7CiAgICAgICAgIGJyZWFrOwo=