Fix issue where iframe content wasn't included in the accessibility tree.

Fix issue where iframe content wasn't included in the accessibility tree.

Have RenderWidget notify the accessibility object when its widget changes,
otherwise the AccessibilityRenderObject for the iframe might have a stale
reference to the previous widget. (This wouldn't crash, it'd just result in
an empty subtree until some other event triggered it to update.)

Also, remove a check that made isAttachment return false if an iframe
element had an ARIA role. I don't believe an ARIA role on an iframe should
ever have the effect of hiding its contents - it's unlikely that's what the web
author wanted, and if the web author really wants that they can use
aria-hidden.

This allows us to re-enable this test, that was previously
failing flakily because of this bug: loading-iframe-updates-axtree.html

BUG=242811
R=aboxhall@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151615

[eseidel, 2013-05-22 06:27:57]

Sadness.  All of this widget code is sadness.  Setting the Widget causes a
layout of the subtree of teh widget, no?  Shouldn't that update the AX tree?

Curious why other ports didn't need this?

[dmazzoni, 2013-05-22 06:48:45]

On 2013/05/22 06:27:57, eseidel wrote:
> Sadness.  All of this widget code is sadness.

Is there a plan to get rid of the concepts of widgets and attachments from core?
In your ideal world, what renderer would an iframe get, and how would it differ
from a RenderWidget?

> Setting the Widget causes a
> layout of the subtree of teh widget, no?  Shouldn't that update the AX tree?

The issue here is that the AccessibilityRenderObject corresponding to the iframe
element doesn't realize that its "child" (the scroll view for the frame) has
changed. A layout of the iframe's subtree alone wouldn't fix that. However, as
soon as any element in that subtree calls childrenChanged (which happens when
any node is added, deleted, hidden, shown, and more...) that will propagate up
the parent chain and cause this object to re-check its children.

So in practice this usually fixes itself - which is why I didn't catch the bug
"in the wild" until now.

> Curious why other ports didn't need this?

Good question. The Mac port doesn't create an accessibility node for the iframe
element, it marks it as "ignored" and just attaches the scroll view in its place
- so it sidesteps this bug. Windows accessibility needs the iframe element to be
exposed and we found it to be harmless to expose it on Mac - well, aside from
this bug.

[dmazzoni, 2013-05-22 06:51:25]

On 2013/05/22 06:48:45, Dominic Mazzoni wrote:
> > Curious why other ports didn't need this?

I'll file a WebKit bug anyway, after this change lands. It may still be a useful
fix for ATK or some other port, I'm not sure.

[eseidel, 2013-05-22 07:15:49]

I was assuming by the test being fail for us, but presumably passingd for other
webkit ports that other webkit ports didn't need this. :)

[aboxhall, 2013-05-22 16:05:10]

https://codereview.chromium.org/15674002/diff/1/Source/core/rendering/RenderW...
File Source/core/rendering/RenderWidget.cpp (right):

https://codereview.chromium.org/15674002/diff/1/Source/core/rendering/RenderW...
Source/core/rendering/RenderWidget.cpp:208: if (AXObjectCache* cache =
document()->existingAXObjectCache())
Can you help me understand under what circumstances the cache would or would not
exist? Or is this just a defensive check?

[dmazzoni, 2013-05-22 16:28:13]

https://codereview.chromium.org/15674002/diff/1/Source/core/rendering/RenderW...
File Source/core/rendering/RenderWidget.cpp (right):

https://codereview.chromium.org/15674002/diff/1/Source/core/rendering/RenderW...
Source/core/rendering/RenderWidget.cpp:208: if (AXObjectCache* cache =
document()->existingAXObjectCache())
On 2013/05/22 16:05:10, aboxhall wrote:
> Can you help me understand under what circumstances the cache would or would
not
> exist? Or is this just a defensive check?

The cache shouldn't exist if accessibility is disabled in this
renderer, or if this document is not yet attached to the page.

We could check AXObjectCache::accessibilityEnabled() - however
if we did that we'd probably still want to call
existingAXObjectCache and check if it exists, because there
are some corner cases where accessibility is enabled but we
don't want an AXObjectCache for one particular document (like
if that document is not yet attached to the page).

So most of the time, code that needs to update the accessibility
tree can just do so if there's an AXObjectCache for the current
document and not otherwise. In just a few places we carefully
create the cache if it doesn't already exist.

[aboxhall, 2013-05-22 16:34:32]

lgtm

Thanks for the explanation!

[dmazzoni, 2013-05-23 14:59:57]

@eseidel - i need an lgtm from an owner still, could you approve?

[dmazzoni, 2013-05-27 22:43:08]

Ping - I still need a review from an owner.

[commit-bot: I haz the power, 2013-05-29 18:31:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/15674002/1

[commit-bot: I haz the power, 2013-05-29 19:06:29]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-05-30 14:45:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/15674002/1

[commit-bot: I haz the power, 2013-05-30 16:53:38]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[dmazzoni, 2013-06-02 22:13:31]

Committed patchset #2 manually as r151615 (presubmit successful).