RESOLVED FIXED 99547
AX: Notification should be sent when accessibilityIsIgnored changes
https://bugs.webkit.org/show_bug.cgi?id=99547
Summary AX: Notification should be sent when accessibilityIsIgnored changes
Dominic Mazzoni
Reported 2012-10-16 23:27:44 PDT
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.
Attachments
Patch (27.56 KB, patch)
2012-10-17 00:12 PDT, Dominic Mazzoni
no flags
Patch (48.64 KB, patch)
2012-10-22 14:32 PDT, Dominic Mazzoni
no flags
Patch (50.83 KB, patch)
2012-10-23 01:35 PDT, Dominic Mazzoni
no flags
Patch (55.43 KB, patch)
2012-10-23 11:07 PDT, Dominic Mazzoni
no flags
Patch (59.68 KB, patch)
2012-10-23 13:37 PDT, Dominic Mazzoni
no flags
Patch for landing (59.69 KB, patch)
2012-10-25 11:57 PDT, Dominic Mazzoni
no flags
Patch for landing (59.79 KB, patch)
2012-10-26 14:16 PDT, Dominic Mazzoni
no flags
Dominic Mazzoni
Comment 1 2012-10-17 00:12:00 PDT
WebKit Review Bot
Comment 2 2012-10-17 04:05:45 PDT
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 3 2012-10-17 23:55:32 PDT
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
Comment 4 2012-10-18 10:11:07 PDT
(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
Comment 5 2012-10-18 16:35:12 PDT
(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
Comment 6 2012-10-22 14:32:04 PDT
Dominic Mazzoni
Comment 7 2012-10-23 01:35:20 PDT
WebKit Review Bot
Comment 8 2012-10-23 01:37:26 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Dominic Mazzoni
Comment 9 2012-10-23 01:50:32 PDT
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 10 2012-10-23 05:36:22 PDT
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 11 2012-10-23 09:06:13 PDT
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
Comment 12 2012-10-23 11:07:51 PDT
Dominic Mazzoni
Comment 13 2012-10-23 11:21:57 PDT
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
Comment 14 2012-10-23 13:37:09 PDT
(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
Comment 15 2012-10-23 13:37:19 PDT
Dominic Mazzoni
Comment 16 2012-10-24 15:21:51 PDT
Chris, I think I addressed your concerns - how does this look now?
Dominic Mazzoni
Comment 17 2012-10-24 15:22:13 PDT
How does this latest patch look?
chris fleizach
Comment 18 2012-10-24 16:46:34 PDT
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
Comment 19 2012-10-24 16:55:30 PDT
(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
Comment 20 2012-10-24 16:57:13 PDT
(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
Comment 21 2012-10-24 16:59:45 PDT
> 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
Comment 22 2012-10-24 17:02:53 PDT
(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
Comment 23 2012-10-24 23:27:13 PDT
(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
Comment 24 2012-10-25 11:57:00 PDT
Created attachment 170704 [details] Patch for landing
WebKit Review Bot
Comment 25 2012-10-25 12:31:39 PDT
Comment on attachment 170704 [details] Patch for landing Clearing flags on attachment: 170704 Committed r132514: <http://trac.webkit.org/changeset/132514>
WebKit Review Bot
Comment 26 2012-10-25 12:31:45 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 27 2012-10-25 18:02:18 PDT
Re-opened since this is blocked by bug 100440
Dominic Mazzoni
Comment 28 2012-10-26 14:16:55 PDT
Created attachment 171004 [details] Patch for landing
WebKit Review Bot
Comment 29 2012-10-26 14:58:13 PDT
Comment on attachment 171004 [details] Patch for landing Clearing flags on attachment: 171004 Committed r132699: <http://trac.webkit.org/changeset/132699>
WebKit Review Bot
Comment 30 2012-10-26 14:58:19 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.