Bug 159422

Summary: AX: Image attachment in email does not show up in AX tree
Product: WebKit Reporter: chris fleizach <cfleizach>
Component: AccessibilityAssignee: chris fleizach <cfleizach>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, commit-queue, dmazzoni, esprehn+autocc, glenn, jcraig, jdiggs, kondapallykalyan, mario, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 9   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
none
patch none

Description chris fleizach 2016-07-05 08:50:34 PDT
It appears that if an image is loaded after a delay, the AX tree doesn't get updated

<rdar://problem/26338151>
Comment 1 chris fleizach 2016-07-05 09:00:20 PDT
Created attachment 282796 [details]
patch
Comment 2 chris fleizach 2016-07-05 14:11:17 PDT
Joanie, are you able to review?
Comment 3 Joanmarie Diggs (irc: joanie) 2016-07-05 15:33:36 PDT
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?
Comment 4 Joanmarie Diggs (irc: joanie) 2016-07-05 15:53:38 PDT
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.
Comment 5 chris fleizach 2016-07-05 15:53:58 PDT
(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?
Comment 6 chris fleizach 2016-07-05 15:54:24 PDT
(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
Comment 7 chris fleizach 2016-07-05 16:10:25 PDT
Created attachment 282824 [details]
patch

Updated patch with a layout test that should work for all platforms
Comment 8 Joanmarie Diggs (irc: joanie) 2016-07-05 16:28:17 PDT
(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!
Comment 9 chris fleizach 2016-07-05 17:24:02 PDT
(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!
Comment 10 WebKit Commit Bot 2016-07-05 17:42:21 PDT
Comment on attachment 282824 [details]
patch

Clearing flags on attachment: 282824

Committed r202841: <http://trac.webkit.org/changeset/202841>
Comment 11 WebKit Commit Bot 2016-07-05 17:42:25 PDT
All reviewed patches have been landed.  Closing bug.