Enable PDF accessibility when RenderFrame's accessibility mode changes.

chrome/browser/pdf/pdf_extension_test.cc

Index: chrome/browser/pdf/pdf_extension_test.cc
diff --git a/chrome/browser/pdf/pdf_extension_test.cc b/chrome/browser/pdf/pdf_extension_test.cc
index 3e11df7829e3d3e63da939418497da44bdc69f4c..6f59998711cef54118ad30685813d0e52b3ce30a 100644
--- a/chrome/browser/pdf/pdf_extension_test.cc
+++ b/chrome/browser/pdf/pdf_extension_test.cc
@@ -48,6 +48,7 @@
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/manifest_handlers/mime_types_handler.h"
 #include "extensions/test/result_catcher.h"
+#include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "url/gurl.h"
@@ -58,6 +59,13 @@
 
 const int kNumberLoadTestParts = 10;
 
+// Using ASSERT_TRUE deliberately instead of ASSERT_EQ or ASSERT_STREQ
+// in order to print a more readable message if the strings differ.
+#define ASSERT_MULTILINE_STREQ(expected, actual) \
+    ASSERT_TRUE(expected == actual) \
+        << "Expected:\n" << expected \
+        << "\n\nActual:\n" << actual
+
 bool GetGuestCallback(content::WebContents** guest_out,
                       content::WebContents* guest) {
   EXPECT_FALSE(*guest_out);
@@ -72,9 +80,15 @@ class PDFExtensionTest : public ExtensionApiTest,
  public:
   ~PDFExtensionTest() override {}
 
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    content::IsolateAllSitesForTesting(command_line);

[raymes, 2016/07/12 01:36:23]

Don't we have site isolation bots that will run all the tests with these flags
enabled? If not, is the preference these days to run all tests with these flags?
Aren't we changing the nature of our test coverage if we do that?

[dmazzoni, 2016/07/13 23:17:53]

My understanding is that when the test is agnostic and you just want to run the
test once with site isolation and once without, you can use those bots.

In this particular case, there are some minor differences to account for, so we
have separate tests.

content::IsolateAllSitesForTesting is used in ~24 files currently, so it doesn't
seem to be deprecated.

Nasko, please correct me if I'm wrong!

[raymes, 2016/07/14 03:15:54]

Should we only enable it in PdfAccessibilityInOOPIF then? I also defer to
Nasko's advice :)

[nasko, 2016/07/14 18:21:44]

Long term the goal is to eliminate the FYI bots that run with
--site-per-process, so having tests that explicitly want to test OOPIF behavior
will be good to enable it explicitly.
That said, enabling it on tests that don't require it will not hurt, so I'm fine
either way you choose.

+  }
+
   void SetUpOnMainThread() override {
     ExtensionApiTest::SetUpOnMainThread();
+    host_resolver()->AddRule("*", "127.0.0.1");

[raymes, 2016/07/12 01:36:23]

How come this is needed?

[dmazzoni, 2016/07/13 23:17:53]

This allows us to use a url of "/cross-site/1.com/pdf/test-bookmarks.pdf" for
the iframe, and instead of trying to visit 1.com, it resolves to localhost.

[raymes, 2016/07/14 03:15:54]

Thanks. This might conflict with
https://codereview.chromium.org/2127383002/diff/140001/chrome/browser/pdf/pdf...
which was just landed, so we should make sure that it still works. Can the rule
be more specific than *?

[nasko, 2016/07/14 18:21:44]

We have the * rule all over the place. What is a good reason to keep it more
restricted?

     ASSERT_TRUE(embedded_test_server()->Start());
+    content::SetupCrossSiteRedirector(embedded_test_server());
   }
 
   void TearDownOnMainThread() override {
@@ -513,17 +527,7 @@ IN_PROC_BROWSER_TEST_F(PDFExtensionTest, PdfZoomWithoutBubble) {
 #endif
 }
 
-IN_PROC_BROWSER_TEST_F(PDFExtensionTest, PdfAccessibility) {
-  content::BrowserAccessibilityState::GetInstance()->EnableAccessibility();
-
-  GURL test_pdf_url(embedded_test_server()->GetURL("/pdf/test-bookmarks.pdf"));
-  content::WebContents* guest_contents = LoadPdfGetGuestContents(test_pdf_url);
-  ASSERT_TRUE(guest_contents);
-
-  WaitForAccessibilityTreeToContainNodeWithName(guest_contents,
-                                                "1 First Section\r\n");
-  ui::AXTreeUpdate ax_tree = GetAccessibilityTreeSnapshot(guest_contents);
-
+static std::string DumpPdfAccessibilityTree(const ui::AXTreeUpdate& ax_tree) {
   // Create a string representation of the tree starting with the embedded
   // object.
   std::string ax_tree_dump;
@@ -549,39 +553,117 @@ IN_PROC_BROWSER_TEST_F(PDFExtensionTest, PdfAccessibility) {
       id_to_indentation[node.child_ids[j]] = indent + 1;
   }
 
-  const char* expected_ax_tree =
-      "embeddedObject\n"
-      "  group\n"
-      "    region 'Page 1'\n"
-      "      paragraph\n"
-      "        staticText '1 First Section\\r\\n'\n"
-      "          inlineTextBox '1 '\n"
-      "          inlineTextBox 'First Section\\r\\n'\n"
-      "      paragraph\n"
-      "        staticText 'This is the first section.\\r\\n1'\n"
-      "          inlineTextBox 'This is the first section.\\r\\n'\n"
-      "          inlineTextBox '1'\n"
-      "    region 'Page 2'\n"
-      "      paragraph\n"
-      "        staticText '1.1 First Subsection\\r\\n'\n"
-      "          inlineTextBox '1.1 '\n"
-      "          inlineTextBox 'First Subsection\\r\\n'\n"
-      "      paragraph\n"
-      "        staticText 'This is the first subsection.\\r\\n2'\n"
-      "          inlineTextBox 'This is the first subsection.\\r\\n'\n"
-      "          inlineTextBox '2'\n"
-      "    region 'Page 3'\n"
-      "      paragraph\n"
-      "        staticText '2 Second Section\\r\\n'\n"
-      "          inlineTextBox '2 '\n"
-      "          inlineTextBox 'Second Section\\r\\n'\n"
-      "      paragraph\n"
-      "        staticText '3'\n"
-      "          inlineTextBox '3'\n";
-
-  // Using ASSERT_TRUE deliberately instead of ASSERT_EQ or ASSERT_STREQ
-  // in order to print a more readable message if the strings differ.
-  ASSERT_TRUE(expected_ax_tree == ax_tree_dump)
-      << "Expected:\n" << expected_ax_tree
-      << "\n\nActual:\n" << ax_tree_dump;
+  return ax_tree_dump;
+}
+
+static const char* kExpectedPDFAXTree =
+    "embeddedObject\n"
+    "  group\n"
+    "    region 'Page 1'\n"
+    "      paragraph\n"
+    "        staticText '1 First Section\\r\\n'\n"
+    "          inlineTextBox '1 '\n"
+    "          inlineTextBox 'First Section\\r\\n'\n"
+    "      paragraph\n"
+    "        staticText 'This is the first section.\\r\\n1'\n"
+    "          inlineTextBox 'This is the first section.\\r\\n'\n"
+    "          inlineTextBox '1'\n"
+    "    region 'Page 2'\n"
+    "      paragraph\n"
+    "        staticText '1.1 First Subsection\\r\\n'\n"
+    "          inlineTextBox '1.1 '\n"
+    "          inlineTextBox 'First Subsection\\r\\n'\n"
+    "      paragraph\n"
+    "        staticText 'This is the first subsection.\\r\\n2'\n"
+    "          inlineTextBox 'This is the first subsection.\\r\\n'\n"
+    "          inlineTextBox '2'\n"
+    "    region 'Page 3'\n"
+    "      paragraph\n"
+    "        staticText '2 Second Section\\r\\n'\n"
+    "          inlineTextBox '2 '\n"
+    "          inlineTextBox 'Second Section\\r\\n'\n"
+    "      paragraph\n"
+    "        staticText '3'\n"
+    "          inlineTextBox '3'\n";
+
+IN_PROC_BROWSER_TEST_F(PDFExtensionTest, PdfAccessibility) {
+  content::BrowserAccessibilityState::GetInstance()->EnableAccessibility();
+
+  GURL test_pdf_url(embedded_test_server()->GetURL("/pdf/test-bookmarks.pdf"));
+  content::WebContents* guest_contents = LoadPdfGetGuestContents(test_pdf_url);
+  ASSERT_TRUE(guest_contents);
+
+  WaitForAccessibilityTreeToContainNodeWithName(guest_contents,
+                                                "1 First Section\r\n");
+  ui::AXTreeUpdate ax_tree = GetAccessibilityTreeSnapshot(guest_contents);
+  std::string ax_tree_dump = DumpPdfAccessibilityTree(ax_tree);
+  ASSERT_MULTILINE_STREQ(kExpectedPDFAXTree, ax_tree_dump);
+}
+
+IN_PROC_BROWSER_TEST_F(PDFExtensionTest, PdfAccessibilityEnableLater) {
+  // In this test, load the PDF file first, with accessibility off.
+  GURL test_pdf_url(embedded_test_server()->GetURL("/pdf/test-bookmarks.pdf"));
+  content::WebContents* guest_contents = LoadPdfGetGuestContents(test_pdf_url);
+  ASSERT_TRUE(guest_contents);
+
+  // Now enable accessibility globally, and assert that the

[raymes, 2016/07/12 01:36:23]

nit: fill 80 chars

[dmazzoni, 2016/07/13 23:17:53]

Done

+  // PDF accessibility tree loads.
+  EnableAccessibilityForWebContents(guest_contents);
+  WaitForAccessibilityTreeToContainNodeWithName(guest_contents,
+                                                "1 First Section\r\n");
+  ui::AXTreeUpdate ax_tree = GetAccessibilityTreeSnapshot(guest_contents);
+  std::string ax_tree_dump = DumpPdfAccessibilityTree(ax_tree);
+  ASSERT_MULTILINE_STREQ(kExpectedPDFAXTree, ax_tree_dump);
+}
+
+bool RetrieveGuestContents(
+    content::WebContents** out_guest_contents,
+    content::WebContents* in_guest_contents) {
+  *out_guest_contents = in_guest_contents;
+  return true;
+}
+
+IN_PROC_BROWSER_TEST_F(PDFExtensionTest, PdfAccessibilityInIframe) {
+  content::BrowserAccessibilityState::GetInstance()->EnableAccessibility();
+  GURL test_iframe_url(embedded_test_server()->GetURL("/pdf/test-iframe.html"));
+  ui_test_utils::NavigateToURL(browser(), test_iframe_url);
+  content::WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  WaitForAccessibilityTreeToContainNodeWithName(contents,
+                                                "1 First Section\r\n");
+
+  content::WebContents* guest_contents = nullptr;
+  content::BrowserPluginGuestManager* guest_manager =
+        contents->GetBrowserContext()->GetGuestManager();
+  guest_manager->ForEachGuest(contents,
+                              base::Bind(&RetrieveGuestContents,
+                                         &guest_contents));
+  ASSERT_TRUE(guest_contents);
+
+  ui::AXTreeUpdate ax_tree = GetAccessibilityTreeSnapshot(guest_contents);

[raymes, 2016/07/12 01:36:23]

Hmm, why can't we get the accessibility tree from the outer web contents? (I'm
curious because it seems like WaitForAccessibilityTreeToContainNodeWithName
works on the outer web contents)

[dmazzoni, 2016/07/13 23:17:53]

It's easy to get a single frame's accessibility tree as an AXTreeUpdate,
which is a pretty simple data structure that's easy to dump in
DumpPdfAccessibilityTree. In order to get the accessibility tree for
the whole page including subframes, we'd need to export a lot more
interfaces in content/public.

I'd be happy to do that if it was necessary to fully test it, but it doesn't
seem important because WaitForAccessibilityTreeToContainNodeWithName already
gives us confidence that the PDF content is reachable from the outermost frame.
The rest of the test asserts that the PDF content is complete and correct.

[raymes, 2016/07/14 03:15:54]

I still don't fully understand. Does
WaitForAccessibilityTreeToContainNodeWithName have access to some of the data
but not all of it (from the outer web contents?). In any case it's not too
important - I trust this is the easiest way to write the test.

[dmazzoni, 2016/07/14 21:04:40]

WaitForAccessibilityTreeToContainNodeWithName is implemented inside
of content and it explores across frame/plugin boundaries - it just only
checks for a single node name, though.

Since the node we're waiting for only appears in the PDF we can be
pretty confident the PDF loaded.

But then GetAccessibilityTreeSnapshot only gets a single frame.
Getting multiple frames would require either some extra processing
to renumber IDs (since IDs are unique only within frames) or
exporting a more complex data structure from content/.

+  std::string ax_tree_dump = DumpPdfAccessibilityTree(ax_tree);
+  ASSERT_MULTILINE_STREQ(kExpectedPDFAXTree, ax_tree_dump);
+}
+
+IN_PROC_BROWSER_TEST_F(PDFExtensionTest, PdfAccessibilityInOOPIF) {
+  content::BrowserAccessibilityState::GetInstance()->EnableAccessibility();
+  GURL test_iframe_url(embedded_test_server()->GetURL(
+      "/pdf/test-cross-site-iframe.html"));
+  ui_test_utils::NavigateToURL(browser(), test_iframe_url);
+  content::WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  WaitForAccessibilityTreeToContainNodeWithName(contents,
+                                                "1 First Section\r\n");
+
+  content::WebContents* guest_contents = nullptr;
+  content::BrowserPluginGuestManager* guest_manager =
+        contents->GetBrowserContext()->GetGuestManager();
+  guest_manager->ForEachGuest(contents,
+                              base::Bind(&RetrieveGuestContents,
+                                         &guest_contents));
+  ASSERT_TRUE(guest_contents);
+
+  ui::AXTreeUpdate ax_tree = GetAccessibilityTreeSnapshot(guest_contents);
+  std::string ax_tree_dump = DumpPdfAccessibilityTree(ax_tree);
+  ASSERT_MULTILINE_STREQ(kExpectedPDFAXTree, ax_tree_dump);
 }