Bug 237014 – AX: Fix accessibility/aria-current-state-changed-notification.html in isolated tree mode

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Tyler Wilcock]

We need to update AXPropertyName::CurrentValue when we get a AXCurrentStateChanged notification.

[Radar WebKit Bug Importer]

<rdar://problem/89270112>

[Tyler Wilcock]

Created attachment 452811 [details]
Patch

[Andres Gonzalez]

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

[Tyler Wilcock]

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

[Andres Gonzalez]

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

[Andres Gonzalez]

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

[Tyler Wilcock]

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

[Andres Gonzalez]

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

[Jonathan Bedard]

Stopped https://ews-build.webkit.org/#/builders/70/builds/984, the 3 failures are already known.

[EWS]

Committed r290309 (247632@main): <https://commits.webkit.org/247632@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452811 [details].