Bug 99547 – AX: Notification should be sent when accessibilityIsIgnored changes

• Component: Accessibility
• Status: RESOLVED FIXED

[Dominic Mazzoni]

There are several dynamic changes that can happen to an element that can cause it to become ignored or unignored for accessibility, including:

* Gaining/losing an ARIA role
* Gaining/losing a title or aria-label
* Gaining/losing inner text / content
* Becoming visible or invisible/hidden

On some platforms like Chromium, a "children changed" notification should be fired on its parent element when this happens, causing it to recompute its number of (unignored) children and notify the platform immediately.

On Mac, the "children changed" notification isn't needed, but this potentially affects live regions. There are probably similar cases where live regions aren't announcing everything they should because a notification isn't being generated.

[Dominic Mazzoni]

Created attachment 169102 [details]
Patch

[WebKit Review Bot]

Comment on attachment 169102 [details]
Patch

Attachment 169102 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14388574

New failing tests:
fast/dom/Window/orphaned-frame-access.html
fast/storage/storage-detached-iframe.html

[chris fleizach]

Comment on attachment 169102 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169102&action=review

My main concern is what kind of performance implication will there be by recalculating axIsIgnored(). Is it basically the same due to our live region calculations or are there other impacts

> Source/WebCore/accessibility/AXObjectCache.cpp:331
> +    newObj->setLastKnownIsIgnored(newObj->accessibilityIsIgnored());

Naming might be more clear if it was

cachedIsIgnoredValue() or

isIgnoredCachedValue() or

can this calculation be done when the element is created or init'd()

there's a number of other element creation routines in this file that I see you haven't touched.

> Source/WebCore/accessibility/AXObjectCache.cpp:720
> +        recomputeIsIgnored(obj);

does this method have to be in AXObjectCache, or can it be a method in AXObject?
Maybe there could be just one "update" method, and inside of that would be updateAXRole() and updateIsIgnored()

> Source/WebCore/accessibility/AXObjectCache.cpp:738
> +    else if (attrName == aria_labelAttr || attrName == aria_labeledbyAttr)

aria-labeledby can also be spelled aria-labelledby, so we should probably take care of that case here too

> Source/WebCore/accessibility/AXObjectCache.cpp:747
> +        childrenChanged(element->parentNode());

it looks like we need not call this on parentNode() anymore since the childrenChanged method looks at the element itself now to recompute isIgnored values

> Source/WebCore/accessibility/AXObjectCache.cpp:751
> +    recomputeIsIgnored(get(element));

is it necessary to call this on every aria-attribute change? it seems like we can limit it to the relevant ones

> Source/WebCore/accessibility/AXObjectCache.h:111
> +    void recomputeIsIgnored(AccessibilityObject*);

it looks like this version could be private

> Source/WebCore/accessibility/AXObjectCache.h:249
> +inline void AXObjectCache::handleAttributeChanged(const QualifiedName& attrName, Element* element) { }

probably want to remove the argument names here

> Source/WebCore/accessibility/AccessibilityObject.h:685
> +    void setLastKnownIsIgnored(bool);

i think we can put the implementations here in the header

> Source/WebCore/accessibility/AccessibilityTable.cpp:133
> +    table->recalcSectionsIfNeeded();

what's going on with this change?

> Source/WebCore/rendering/RenderBlock.cpp:1032
> +        document()->axObjectCache()->recomputeIsIgnored(this);

so were these two cases where childrenChanged() should have been called but wasn't?

[Dominic Mazzoni]

(In reply to comment #3)
> (From update of attachment 169102 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169102&action=review
> 
> My main concern is what kind of performance implication will there be by recalculating axIsIgnored(). Is it basically the same due to our live region calculations or are there other impacts

That's true, and I'd like to call it much less often. Right now it's quite hot - when a screen reader queries a single object, axIsIgnored gets called several times under the hood - and it calls several other expensive functions.

However, when a node actually changes, it's important to recalculate it, and we're not always doing that now.

Let me investigate the total number of times it's being called - maybe I can count the number of times it's called across all layout tests.

> > Source/WebCore/accessibility/AXObjectCache.cpp:331
> > +    newObj->setLastKnownIsIgnored(newObj->accessibilityIsIgnored());
> 
> can this calculation be done when the element is created or init'd()

It's problematic to call it from within init(), because the wrapper hasn't been created yet, and that messes things up. That's why I do it in AXObjectCache.

> there's a number of other element creation routines in this file that I see you haven't touched.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:720
> > +        recomputeIsIgnored(obj);
> 
> does this method have to be in AXObjectCache, or can it be a method in AXObject?
> Maybe there could be just one "update" method, and inside of that would be updateAXRole() and updateIsIgnored()
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:738
> > +    else if (attrName == aria_labelAttr || attrName == aria_labeledbyAttr)
> 
> aria-labeledby can also be spelled aria-labelledby, so we should probably take care of that case here too
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:747
> > +        childrenChanged(element->parentNode());
> 
> it looks like we need not call this on parentNode() anymore since the childrenChanged method looks at the element itself now to recompute isIgnored values

Good point. I think I want a elementChanged method that calls childrenChanged on the parent if needed.

> > Source/WebCore/accessibility/AccessibilityTable.cpp:133
> > +    table->recalcSectionsIfNeeded();
> 
> what's going on with this change?

Calling accessibilityIsIgnored immediately after every AccessibilityObject is created exposed a previously undiscovered bug. This was crashing in table->firstBody() because the table sections hadn't been initialized yet.

> > Source/WebCore/rendering/RenderBlock.cpp:1032
> > +        document()->axObjectCache()->recomputeIsIgnored(this);
> 
> so were these two cases where childrenChanged() should have been called but wasn't?

Yes, these are specifically related to this logic in axIsIgnored:

    if (m_renderer->isBlockFlow() && m_renderer->childrenInline() && !canSetFocusAttribute())
        return !toRenderBlock(m_renderer)->firstLineBox() && !mouseButtonListener();

In essence, this is making a render block ignored if it doesn't have any children. So a div with no child elements and no inner text is ignored. But then if you add inner text, it's now unignored, but no notification was firing. (Same in reverse if you set the inner text to blank.)

[chris fleizach]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 169102 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=169102&action=review
> > 
> > My main concern is what kind of performance implication will there be by recalculating axIsIgnored(). Is it basically the same due to our live region calculations or are there other impacts
> 
> That's true, and I'd like to call it much less often. Right now it's quite hot - when a screen reader queries a single object, axIsIgnored gets called several times under the hood - and it calls several other expensive functions.
> 
> However, when a node actually changes, it's important to recalculate it, and we're not always doing that now.

I have actually thought that maybe we should cache accessibilityIsIgnored if we can cover all the cases that it will change.

> 
> Let me investigate the total number of times it's being called - maybe I can count the number of times it's called across all layout tests.
> 
> > > Source/WebCore/accessibility/AXObjectCache.cpp:331
> > > +    newObj->setLastKnownIsIgnored(newObj->accessibilityIsIgnored());
> > 
> > can this calculation be done when the element is created or init'd()
> 
> It's problematic to call it from within init(), because the wrapper hasn't been created yet, and that messes things up. That's why I do it in AXObjectCache.
> 
> > there's a number of other element creation routines in this file that I see you haven't touched.
> > 
> > > Source/WebCore/accessibility/AXObjectCache.cpp:720
> > > +        recomputeIsIgnored(obj);
> > 
> > does this method have to be in AXObjectCache, or can it be a method in AXObject?
> > Maybe there could be just one "update" method, and inside of that would be updateAXRole() and updateIsIgnored()
> > 
> > > Source/WebCore/accessibility/AXObjectCache.cpp:738
> > > +    else if (attrName == aria_labelAttr || attrName == aria_labeledbyAttr)
> > 
> > aria-labeledby can also be spelled aria-labelledby, so we should probably take care of that case here too
> > 
> > > Source/WebCore/accessibility/AXObjectCache.cpp:747
> > > +        childrenChanged(element->parentNode());
> > 
> > it looks like we need not call this on parentNode() anymore since the childrenChanged method looks at the element itself now to recompute isIgnored values
> 
> Good point. I think I want a elementChanged method that calls childrenChanged on the parent if needed.
> 
> > > Source/WebCore/accessibility/AccessibilityTable.cpp:133
> > > +    table->recalcSectionsIfNeeded();
> > 
> > what's going on with this change?
> 
> Calling accessibilityIsIgnored immediately after every AccessibilityObject is created exposed a previously undiscovered bug. This was crashing in table->firstBody() because the table sections hadn't been initialized yet.
> 
> > > Source/WebCore/rendering/RenderBlock.cpp:1032
> > > +        document()->axObjectCache()->recomputeIsIgnored(this);
> > 
> > so were these two cases where childrenChanged() should have been called but wasn't?
> 
> Yes, these are specifically related to this logic in axIsIgnored:
> 
>     if (m_renderer->isBlockFlow() && m_renderer->childrenInline() && !canSetFocusAttribute())
>         return !toRenderBlock(m_renderer)->firstLineBox() && !mouseButtonListener();
> 
> In essence, this is making a render block ignored if it doesn't have any children. So a div with no child elements and no inner text is ignored. But then if you add inner text, it's now unignored, but no notification was firing. (Same in reverse if you set the inner text to blank.)

[Dominic Mazzoni]

Created attachment 169987 [details]
Patch

[Dominic Mazzoni]

Created attachment 170082 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Dominic Mazzoni]

Comment on attachment 169102 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169102&action=review

>>>> Source/WebCore/accessibility/AXObjectCache.cpp:331
>>>> +    newObj->setLastKnownIsIgnored(newObj->accessibilityIsIgnored());
>>> 
>>> Naming might be more clear if it was
>>> 
>>> cachedIsIgnoredValue() or
>>> 
>>> isIgnoredCachedValue() or
>>> 
>>> can this calculation be done when the element is created or init'd()
>>> 
>>> there's a number of other element creation routines in this file that I see you haven't touched.
>> 
>> It's problematic to call it from within init(), because the wrapper hasn't been created yet, and that messes things up. That's why I do it in AXObjectCache.
> 
> 

OK, I renamed it cachedIsIgnoredValue().

I tried computing it on init, but found:
1. This causes accessibilityIsIgnored to be called quite a bit more often - with no benefit.
2. It causes more AccessibilityObjects to get created (since accessibilityIsIgnored walks up the parent chain) and breaks some non-accessibility tests because the extra AccessibilityObjects hold references to objects that the test is trying to destroy.

So, I think it's best to just cache accessibilityIsIgnored the first time it's requested. To do this I made it a ternary.

What do you think of reusing the AccessibilityObjectInclusion enum here?

>>> Source/WebCore/accessibility/AXObjectCache.cpp:720
>>> +        recomputeIsIgnored(obj);
>> 
>> does this method have to be in AXObjectCache, or can it be a method in AXObject?
>> Maybe there could be just one "update" method, and inside of that would be updateAXRole() and updateIsIgnored()
> 
> Good point. I think I want a elementChanged method that calls childrenChanged on the parent if needed.

OK, I added a notifyIfIgnoredValueChanged to AccessibilityObject.h, and made AXObjectCache::recomputeIsIgnored a 1-liner. (I still think it's better to have it in AXObjectCache so that RenderBlock.cpp only deals with AXObjectCache and doesn't need to deal with AccessibilityObjects.)

>> Source/WebCore/accessibility/AXObjectCache.cpp:738
>> +    else if (attrName == aria_labelAttr || attrName == aria_labeledbyAttr)
> 
> aria-labeledby can also be spelled aria-labelledby, so we should probably take care of that case here too

Good idea, done.

>> Source/WebCore/accessibility/AXObjectCache.cpp:747
>> +        childrenChanged(element->parentNode());
> 
> it looks like we need not call this on parentNode() anymore since the childrenChanged method looks at the element itself now to recompute isIgnored values

I really thing calling it on parentNode is correct; when a node is created or destroyed we call childrenChanged on the parent, so if a node is hidden or shown I think the same thing should happen.

Yes, it's true that childrenChanged would get called on the parent indirectly, but the total pattern of notifications would be inconsistent.

>> Source/WebCore/accessibility/AXObjectCache.cpp:751
>> +    recomputeIsIgnored(get(element));
> 
> is it necessary to call this on every aria-attribute change? it seems like we can limit it to the relevant ones

You're right, only a handful can change accessibilityIsIgnored. I tightened it up.

I really want some sort of notification when any ARIA attribute changes, so I went ahead and added an additional notification as a catch-all for any other ARIA attribute changing, along with a couple of tests for that.

>> Source/WebCore/accessibility/AXObjectCache.h:111
>> +    void recomputeIsIgnored(AccessibilityObject*);
> 
> it looks like this version could be private

Done

>> Source/WebCore/accessibility/AXObjectCache.h:249
>> +inline void AXObjectCache::handleAttributeChanged(const QualifiedName& attrName, Element* element) { }
> 
> probably want to remove the argument names here

Done

>> Source/WebCore/accessibility/AccessibilityObject.h:685
>> +    void setLastKnownIsIgnored(bool);
> 
> i think we can put the implementations here in the header

They ended up nontrivial after the last changes, so probably better to leave them in the source file.

[WebKit Review Bot]

Comment on attachment 170082 [details]
Patch

Attachment 170082 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14486922

New failing tests:
fast/forms/time-multiple-fields/time-multiple-fields-ax-value-changed-notification.html
fast/forms/date-multiple-fields/date-multiple-fields-ax-value-changed-notification.html

[chris fleizach]

Comment on attachment 170082 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170082&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:552
> +    postNotification(obj, obj->document(), AXObjectCache::AXTitleOrDescriptionChanged, true);

I think the name of this notification should be something more like AXAccessibleNameChanged, instead of title or description which are essential Mac concepts.

Also it seems like the method name "contentChanged" should match the name of the notification being posted (maybe the notification is AXContentChanged)

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2681
> +                childObject->setCachedIsIgnoredValue(isIgnored);

i'm worried that there are now hard to follow rules about when to set the cached ignored value. I see it being called in 4 places, all relatively different (FocusedUIElementForPage, addHiddenChildren, notifyParentOnChange, AXRenderObject::addChildren).

I feel like it will be easy to add some method that forgets to call this method which will cause problems

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2720
> +    for (RefPtr<AccessibilityObject> obj = firstChild(); obj; obj = obj->nextSibling()) {

i don't like how this reduplicates the addChild() logic again in render object, after it was moved out into NodeObject

[Dominic Mazzoni]

Created attachment 170197 [details]
Patch

[Dominic Mazzoni]

Comment on attachment 170082 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170082&action=review

>> Source/WebCore/accessibility/AXObjectCache.cpp:552
>> +    postNotification(obj, obj->document(), AXObjectCache::AXTitleOrDescriptionChanged, true);
> 
> I think the name of this notification should be something more like AXAccessibleNameChanged, instead of title or description which are essential Mac concepts.
> 
> Also it seems like the method name "contentChanged" should match the name of the notification being posted (maybe the notification is AXContentChanged)

Sounds good. I switched them to AXTextChanged and textChanged(), respectively - that sound good? (I think "accessible" is implied in both cases.)

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2681
>> +                childObject->setCachedIsIgnoredValue(isIgnored);
> 
> i'm worried that there are now hard to follow rules about when to set the cached ignored value. I see it being called in 4 places, all relatively different (FocusedUIElementForPage, addHiddenChildren, notifyParentOnChange, AXRenderObject::addChildren).
> 
> I feel like it will be easy to add some method that forgets to call this method which will cause problems

Well, the worst thing that will happen if this is done incorrectly is that a node will change
and it won't send a notification. We'll have to add more tests over time to catch this.

I agree it'd be cleaner to just call it once on initialization and then whenever we think
it might have changed. Doing this broke just two tests, let me explain why in more detail:

1. accessibilityIsIgnored walks up the parent chain, creating AccessibilityObjects as it goes
2. When an AccessibilityScrollView is created for an iframe, it holds a reference to the ScrollView,
   so the ScrollView isn't actually deleted right away when other references to it go away.
3. These two tests are trying to check for things that happen when an iframe is deleted. The test
   breaks because removing all references to the iframe doesn't cause it to be deleted with the
   AccessibilityScrollView still around.

Let me see if I can make AccessibilityScrollView hold a weak reference to the ScrollView. If not,
maybe I can add a method to accessibilityController to disable accessibility for those tests.

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2720
>> +    for (RefPtr<AccessibilityObject> obj = firstChild(); obj; obj = obj->nextSibling()) {
> 
> i don't like how this reduplicates the addChild() logic again in render object, after it was moved out into NodeObject

Oops, I wasn't paying attention when merging. I meant to change the code in AccessibilityNodeObject. Fixed.

[Dominic Mazzoni]

(In reply to comment #13)
> >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2681
> >> +                childObject->setCachedIsIgnoredValue(isIgnored);
> > 
> > i'm worried that there are now hard to follow rules about when to set the cached ignored value. I see it being called in 4 places, all relatively different (FocusedUIElementForPage, addHiddenChildren, notifyParentOnChange, AXRenderObject::addChildren).

I made AccessibilityScrollView hold a weak reference to the ScrollView, then I was able to initialize m_cachedIsIgnoredValue just once when an object is created, and all tests pass. How does that look?

[Dominic Mazzoni]

Created attachment 170221 [details]
Patch

[Dominic Mazzoni]

Chris, I think I addressed your concerns - how does this look now?

[Dominic Mazzoni]

How does this latest patch look?

[chris fleizach]

Comment on attachment 170221 [details]
Patch

is detach() already called on AXScrollView somewhere in ScrollView?

AXOtherAriaAttributeChanged is a weird name because it doesn't really tell you what it includes or what it doesn't. Maybe I'd like it better if it was just AriaAttributeChanged and it fired for any aria attribute... Or maybe there's a better name we can choose

thanks

[Dominic Mazzoni]

(In reply to comment #18)
> AXOtherAriaAttributeChanged is a weird name because it doesn't really tell you what it includes or what it doesn't. Maybe I'd like it better if it was just AriaAttributeChanged and it fired for any aria attribute... Or maybe there's a better name we can choose

Hmm, I don't really want duplicate notifications - i.e. AriaAttribute changed and ValueChanged fired for the same change.

Should I just add a separate notification for every possible Aria attribute now? It'd be a bit tedious but it'd be very clear, and it'd make it easy to do the platform mapping or handle special cases in the future.

[chris fleizach]

(In reply to comment #19)
> (In reply to comment #18)
> > AXOtherAriaAttributeChanged is a weird name because it doesn't really tell you what it includes or what it doesn't. Maybe I'd like it better if it was just AriaAttributeChanged and it fired for any aria attribute... Or maybe there's a better name we can choose
> 
> Hmm, I don't really want duplicate notifications - i.e. AriaAttribute changed and ValueChanged fired for the same change.
> 
> Should I just add a separate notification for every possible Aria attribute now? It'd be a bit tedious but it'd be very clear, and it'd make it easy to do the platform mapping or handle special cases in the future.

No I don't think that's a good solution either. Maybe just remove the word Other.

Any idea about these scroll view question?

[Dominic Mazzoni]

> No I don't think that's a good solution either. Maybe just remove the word Other.

OK.

> Any idea about these scroll view question?

I thought I addressed them? What didn't I answer?

[chris fleizach]

(In reply to comment #21)
> > No I don't think that's a good solution either. Maybe just remove the word Other.
> 
> OK.
> 
> > Any idea about these scroll view question?
> 
> I thought I addressed them? What didn't I answer?

this one

is detach() already called on AXScrollView somewhere in ScrollView?

[Dominic Mazzoni]

(In reply to comment #22)
> is detach() already called on AXScrollView somewhere in ScrollView?

Sorry I missed this. Yes, it's already called, though it's maybe not obvious:

An AccessibilityScrollView is actually only created for a FrameView, which is a subclass of ScrollView (see AXObjectCache::getOrCreate(Widget*)). The FrameView destructor already calls AXObjectCache::remove(Widget*), which calls detach() on the object.

[Dominic Mazzoni]

Created attachment 170704 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 170704 [details]
Patch for landing

Clearing flags on attachment: 170704

Committed r132514: <http://trac.webkit.org/changeset/132514>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 100440

[Dominic Mazzoni]

Created attachment 171004 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 171004 [details]
Patch for landing

Clearing flags on attachment: 171004

Committed r132699: <http://trac.webkit.org/changeset/132699>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.