Bug 237014 - AX: Fix accessibility/aria-current-state-changed-notification.html in isolated tree mode
Summary: AX: Fix accessibility/aria-current-state-changed-notification.html in isolate...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tyler Wilcock
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-21 19:04 PST by Tyler Wilcock
Modified: 2022-02-22 09:07 PST (History)
11 users (show)

See Also:


Attachments
Patch (2.77 KB, patch)
2022-02-21 19:06 PST, Tyler Wilcock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tyler Wilcock 2022-02-21 19:04:33 PST
We need to update AXPropertyName::CurrentValue when we get a AXCurrentStateChanged notification.
Comment 1 Radar WebKit Bug Importer 2022-02-21 19:04:45 PST
<rdar://problem/89270112>
Comment 2 Tyler Wilcock 2022-02-21 19:06:45 PST
Created attachment 452811 [details]
Patch
Comment 3 Andres Gonzalez 2022-02-22 05:28:47 PST
(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.
Comment 4 Tyler Wilcock 2022-02-22 07:54:36 PST
(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.
Comment 5 Andres Gonzalez 2022-02-22 08:04:28 PST
(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?
Comment 6 Andres Gonzalez 2022-02-22 08:07:01 PST
(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.
Comment 7 Tyler Wilcock 2022-02-22 08:11:01 PST
> > 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
Comment 8 Andres Gonzalez 2022-02-22 08:15:25 PST
(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!
Comment 9 Jonathan Bedard 2022-02-22 08:22:48 PST
Stopped https://ews-build.webkit.org/#/builders/70/builds/984, the 3 failures are already known.
Comment 10 EWS 2022-02-22 09:07:10 PST
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].