Bug 159422 – AX: Image attachment in email does not show up in AX tree

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

[chris fleizach]

It appears that if an image is loaded after a delay, the AX tree doesn't get updated

<rdar://problem/26338151>

[chris fleizach]

Created attachment 282796 [details]
patch

[chris fleizach]

Joanie, are you able to review?

[Joanmarie Diggs]

Comment on attachment 282796 [details]
patch

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

I assume images are a missed/unhandled case and that other elements don't suffer from the same problem?

One additional, code-specific question inline.

> Source/WebCore/rendering/RenderImage.cpp:256
> +    if (UNLIKELY(AXObjectCache::accessibilityEnabled())) {

What does using this check buy us here (both the macro and accessibilityEnabled())? It looks like more often than not, we're not doing either before calling existingAXObjectCache(). Are images special? Or should this check be (re)moved?

[Joanmarie Diggs]

Chris: One more thing *if* the response to my questions is going to result in a new patch. On my platform, the before count is 0 and the after count is 1. (ATK doesn't do StaticTextRole children which don't have properties necessitating their own accessible object.)

If the answers to my questions won't lead to a new patch, I'll do a patch for this change after your patch lands. Otherwise, a platform check would be cool.

[chris fleizach]

(In reply to comment #3)
> Comment on attachment 282796 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282796&action=review
> 
> I assume images are a missed/unhandled case and that other elements don't
> suffer from the same problem?

It appears that the loading of the image does not trigger any thing in AX to update its tree

> 
> One additional, code-specific question inline.
> 
> > Source/WebCore/rendering/RenderImage.cpp:256
> > +    if (UNLIKELY(AXObjectCache::accessibilityEnabled())) {
> 
> What does using this check buy us here (both the macro and
> accessibilityEnabled())? It looks like more often than not, we're not doing
> either before calling existingAXObjectCache(). Are images special? Or should
> this check be (re)moved?

This is the way that's it used elsewhere in WebCore code. I don't know how much it buys us, but for the general user case I suppose this if check is slightly faster


(In reply to comment #3)
> Comment on attachment 282796 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282796&action=review
> 
> I assume images are a missed/unhandled case and that other elements don't
> suffer from the same problem?
> 
> One additional, code-specific question inline.
> 
> > Source/WebCore/rendering/RenderImage.cpp:256
> > +    if (UNLIKELY(AXObjectCache::accessibilityEnabled())) {
> 
> What does using this check buy us here (both the macro and
> accessibilityEnabled())? It looks like more often than not, we're not doing
> either before calling existingAXObjectCache(). Are images special? Or should
> this check be (re)moved?

[chris fleizach]

(In reply to comment #4)
> Chris: One more thing *if* the response to my questions is going to result
> in a new patch. On my platform, the before count is 0 and the after count is
> 1. (ATK doesn't do StaticTextRole children which don't have properties
> necessitating their own accessible object.)
> 
> If the answers to my questions won't lead to a new patch, I'll do a patch
> for this change after your patch lands. Otherwise, a platform check would be
> cool.

Ok, I can do that

[chris fleizach]

Created attachment 282824 [details]
patch

Updated patch with a layout test that should work for all platforms

[Joanmarie Diggs]

(In reply to comment #5)
 
> This is the way that's it used elsewhere in WebCore code. I don't know how
> much it buys us, but for the general user case I suppose this if check is
> slightly faster

Fair enough re slightly faster. And it's used in RenderBlockLineLayout.cpp.

From a quick grep, there are quite a few more places where it's not used. If it does make a difference, it might be worth adding the check to Document::existingAXObjectCache(). (In a new bug; not this one.)

(In reply to comment #6)
 
> Ok, I can do that

You rock. Thanks!

[chris fleizach]

(In reply to comment #8)
> (In reply to comment #5)
>  
> > This is the way that's it used elsewhere in WebCore code. I don't know how
> > much it buys us, but for the general user case I suppose this if check is
> > slightly faster
> 
> Fair enough re slightly faster. And it's used in RenderBlockLineLayout.cpp.
> 
> From a quick grep, there are quite a few more places where it's not used. If
> it does make a difference, it might be worth adding the check to
> Document::existingAXObjectCache(). (In a new bug; not this one.)

https://bugs.webkit.org/show_bug.cgi?id=159445

> 
> (In reply to comment #6)
>  
> > Ok, I can do that
> 
> You rock. Thanks!

[WebKit Commit Bot]

Comment on attachment 282824 [details]
patch

Clearing flags on attachment: 282824

Committed r202841: <http://trac.webkit.org/changeset/202841>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.