Summary: | AX: Image attachment in email does not show up in AX tree | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||||
Component: | Accessibility | Assignee: | 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
chris fleizach
2016-07-05 08:50:34 PDT
Created attachment 282796 [details]
patch
Joanie, are you able to review? 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: 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. (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? (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 Created attachment 282824 [details]
patch
Updated patch with a layout test that should work for all platforms
(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! (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 on attachment 282824 [details] patch Clearing flags on attachment: 282824 Committed r202841: <http://trac.webkit.org/changeset/202841> All reviewed patches have been landed. Closing bug. |