Fix accessibility with out-of-process iframes.

content/browser/accessibility/site_per_process_accessibility_browsertest.cc

Index: content/browser/accessibility/site_per_process_accessibility_browsertest.cc
diff --git a/content/browser/accessibility/site_per_process_accessibility_browsertest.cc b/content/browser/accessibility/site_per_process_accessibility_browsertest.cc
index bf9c189a202c3e799a298c35aaa38893cbc5d8fd..4dc6906cac1b5a1cd2f2480ce0b20c570e4668f9 100644
--- a/content/browser/accessibility/site_per_process_accessibility_browsertest.cc
+++ b/content/browser/accessibility/site_per_process_accessibility_browsertest.cc
@@ -58,10 +58,6 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessAccessibilityBrowserTest,
   // Enable full accessibility for all current and future WebContents.
   BrowserAccessibilityState::GetInstance()->EnableAccessibility();
 
-  AccessibilityNotificationWaiter main_frame_accessibility_waiter(
-      shell(), AccessibilityModeComplete,
-      ui::AX_EVENT_LOAD_COMPLETE);
-
   host_resolver()->AddRule("*", "127.0.0.1");
   ASSERT_TRUE(test_server()->Start());
   GURL main_url(test_server()->GetURL("files/site_per_process_main.html"));
@@ -78,6 +74,8 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessAccessibilityBrowserTest,
   NavigateFrameToURL(child, http_url);
 
   // Load cross-site page into iframe.
+  RenderFrameHostImpl* child_rfh =
+      child->render_manager()->current_frame_host();
   GURL::Replacements replace_host;
   GURL cross_site_url(test_server()->GetURL("files/title2.html"));
   replace_host.SetHostStr("foo.com");
@@ -89,9 +87,15 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessAccessibilityBrowserTest,
   SiteInstance* site_instance = child->current_frame_host()->GetSiteInstance();
   EXPECT_NE(shell()->web_contents()->GetSiteInstance(), site_instance);
 
-  // Wait for the accessibility tree from the main frame to load.
-  // Because we created the AccessibilityNotificationWaiter before accessibility
-  // was enabled, we're guaranteed to get a LOAD_COMPLETE event.
+  // Wait until the iframe completes the swap.
+  RenderFrameHostDeletedObserver rfhdo(child_rfh);
+  rfhdo.WaitUntilDeleted();
+
+  // Waits for the event that modifies the iframe in the accessibility tree.
+  // This event is sent immediatelly after the swap.
+  AccessibilityNotificationWaiter main_frame_accessibility_waiter(

[dmazzoni, 2015/05/28 19:12:10]

Be careful about constructing the waiter too late. If the children changed event
is received before you construct the waiter, it will hang and the test will time
out. If you construct the waiter before the children changed event could
possibly get sent, then if it's already been received when you call
WaitForNotification it will just exit immediately.

Children changed is a pretty generic event, to make this totally robust you may
need to add a loop that keeps waiting until you get the event on the node you
want and it actually adds the correct child. Maybe it won't matter if all of
these pages are really basic HTML, but where this would bite you is if a page
loads dynamically, or even if it just loads slowly - we could get a partial
accessibility tree, then later it updates to add the rest of the tree and the
update could include a children changed event.

[lfg, 2015/05/29 19:23:29]

That's why I had the RFHostDeletedObserver right above it, since the
CHILDREN_CHANGED is sent right after the RFHostdeletedObserver triggers.

There's another issue though, watching the RFHostDeletedObserver is racy because
the child_rfh can be deleted in the message loop that waits for the
NavigateFrametoURL, depending on how both renderers are scheduled. I'm still not
sure how to handle this event, I'll think more about this, if you have
suggestions let me know.

[dmazzoni, 2015/05/29 19:37:23]

Yes, but I believe it's possible that the accessibility event could be received
before rfhdo.WaitUntilDeleted() returns, so I suggest sandwiching it like this:

AccessibilityNotificationWaiter main_frame_accessibility_waiter(
    shell(), AccessibilityModeComplete,
    ui::AX_EVENT_CHILDREN_CHANGED);

RenderFrameHostDeletedObserver rfhdo(child_rfh);
rfhdo.WaitUntilDeleted();

main_frame_accessibility_waiter.WaitForNotification();
I'd do it the same way. Construct the RenderFrameHostDeletedObserver before the
code that triggers the deletion.

+      shell(), AccessibilityModeComplete,
+      ui::AX_EVENT_CHILDREN_CHANGED);
   main_frame_accessibility_waiter.WaitForNotification();
 
   RenderFrameHostImpl* main_frame = static_cast<RenderFrameHostImpl*>(
@@ -130,16 +134,8 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessAccessibilityBrowserTest,
   EXPECT_EQ(ui::AX_ROLE_GROUP, ax_group->GetRole());
   ASSERT_EQ(2U, ax_group->PlatformChildCount());
 
-  BrowserAccessibility* ax_iframe = ax_group->PlatformGetChild(0);
-  EXPECT_EQ(ui::AX_ROLE_IFRAME, ax_iframe->GetRole());

[dmazzoni, 2015/05/28 19:12:10]

We definitely want the IFRAME element to be in the tree.

As I suggested in another thread, let's rewrite
BrowserAccessibility::PlatformGetChild and
BrowserAccessibility::PlatformChildCount so that the child tree gets added as a
child of the iframe element rather than replacing what was previously the
document element.

[lfg, 2015/05/29 19:23:29]

Done.

-  ASSERT_EQ(1U, ax_iframe->PlatformChildCount());
-
-  BrowserAccessibility* ax_scroll_area = ax_iframe->PlatformGetChild(0);
-  EXPECT_EQ(ui::AX_ROLE_SCROLL_AREA, ax_scroll_area->GetRole());

[dmazzoni, 2015/05/28 19:12:10]

I'm okay with losing the scroll area. I'm actually planning to get rid of that
role completely when I get around to it, it's a holdover from WebKit that we
don't really need.

[lfg, 2015/05/29 19:23:29]

Acknowledged.

-  ASSERT_EQ(1U, ax_scroll_area->PlatformChildCount());
-
   BrowserAccessibility* ax_child_frame_root =
-      ax_scroll_area->PlatformGetChild(0);
+      ax_group->PlatformGetChild(0);
   EXPECT_EQ(ui::AX_ROLE_ROOT_WEB_AREA, ax_child_frame_root->GetRole());
   ASSERT_EQ(1U, ax_child_frame_root->PlatformChildCount());
   EXPECT_EQ("Title Of Awesomeness",
@@ -156,7 +152,7 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessAccessibilityBrowserTest,
   ASSERT_EQ(0U, ax_child_frame_static_text->PlatformChildCount());
 
   // Last, check that the parent of the child frame root is correct.
-  EXPECT_EQ(ax_child_frame_root->GetParent(), ax_scroll_area);
+  EXPECT_EQ(ax_child_frame_root->GetParent(), ax_group);
 }
 
 }  // namespace content