Fixed a11y tab-handling for language settings

chrome/browser/ui/webui/options/language_options_interactive_uitest.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/prefs/pref_service.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/chrome_pages.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/url_constants.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/browser_test_utils.h"
+#include "grit/generated_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/events/test/event_generator.h"
+
+namespace language_options_ui_test {
+
+namespace {
+
+// TODO(hcarmona): Figure out a better way of getting this constant.
+const char kAcceptLanguages[] = "intl.accept_languages";
+
+// This class will test the language options settings.
+// We need to emulate button pushing because we are testing accessibility,
+// this means that this is part of interactive_ui_tests.
+class LanguageOptionsInteractiveTest : public InProcessBrowserTest {
+ public:
+  LanguageOptionsInteractiveTest() {}
+
+ protected:
+  // This method will navigate to the language settings page and show
+  // a subset of languages from the list of available languages that is not
+  // the default.
+  bool NavigateToLanguagePage() {
+    const GURL& url = chrome::GetSettingsUrl(chrome::kLanguageOptionsSubPage);

[groby-ooo-7-16, 2014/08/12 23:57:27]

Make this non-ref. GetSettingsUrl returns an object, not a reference.

[hcarmona, 2014/08/13 20:24:39]

Done.

+    ui_test_utils::NavigateToURL(browser(), url);
+    browser()->profile()->GetPrefs()->SetString(
+        kAcceptLanguages, "en-US,es,fr");
+    return true;
+  }
+
+  bool GetActiveElementId(std::string *element_id) {
+    content::RenderFrameHost* active_frame = NULL;
+    if (!GetActiveFrame(active_frame))
+      return false;
+
+    return content::ExecuteScriptAndExtractString(
+        active_frame,
+        "domAutomationController.send(document.activeElement.id);",
+        element_id);
+  }
+
+  // Will get the active frame. Returns false on error.
+  bool GetActiveFrame(content::RenderFrameHost* &active_frame) {

[groby-ooo-7-16, 2014/08/12 23:57:27]

No outparams via ref, sorry :(  - Here and elsewhere.

[hcarmona, 2014/08/13 20:24:39]

Done.

+    content::WebContents* web_contents = NULL;
+    if (GetActiveWebContents(web_contents)) {

[groby-ooo-7-16, 2014/08/12 23:57:27]

If this "should never happen", it's OK to just get the web_contents and not
check, if the failure case returns NULL. (Having a crashing test will still tell
us there's a bug)

[hcarmona, 2014/08/13 20:24:39]

Done.

+      active_frame = web_contents->GetFocusedFrame();
+      return active_frame;
+    }
+
+    return false;
+  }
+
+  // Will get the active web contents. Returns false on error.
+  bool GetActiveWebContents(content::WebContents* &web_contents) {
+    if (browser()) {
+      if (TabStripModel* tab_strip_model = browser()->tab_strip_model()) {
+        web_contents = tab_strip_model->GetActiveWebContents();
+        return web_contents;
+      }
+    }
+
+    return false;
+  }
+
+  // Press and release a key in a particular window. Returns false on error.
+  bool PressKey(ui::KeyboardCode key_code) {
+    gfx::NativeWindow native_window = NULL;
+    if (!GetNativeWindow(native_window))

[groby-ooo-7-16, 2014/08/12 23:57:27]

NULL check not needed if this should always be not NULL.

[hcarmona, 2014/08/13 20:24:39]

Done.

+      return false;
+
+    ui::test::EventGenerator eventGen(native_window);
+    eventGen.PressKey(key_code, ui::EF_NONE);
+    eventGen.ReleaseKey(key_code, ui::EF_NONE);
+    return true;
+  }
+
+ protected:
+  // Will get the native window. Returns false on error.
+  bool GetNativeWindow(gfx::NativeWindow &native_window) {

[groby-ooo-7-16, 2014/08/12 23:57:27]

Chrome's code standard doesn't allow out parameters via references. You want
either a pointer as out param, or you could just return by value - it's OK to
check gfx::NativeWindow against NULL.

[hcarmona, 2014/08/13 20:24:39]

Done.

+    if (browser()) {
+      if (BrowserWindow* window = browser()->window()) {
+        native_window = window->GetNativeWindow();
+        return true;
+      }
+    }
+
+    return false;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(LanguageOptionsInteractiveTest);
+};
+
+}  // namespace
+
+// This test will verify that after calling NavigateToLanguagePage
+// we are in the correct page.
+// TODO(hcarmona):
+//   Is this test necessary? All other tests will fail if this one fails.

[groby-ooo-7-16, 2014/08/12 23:57:28]

If this is a precondition, I'd handle this in the test's SetUp and just ASSERT
the conditions - side effect, all further tests will also already be navigated
to the language page.

[hcarmona, 2014/08/13 20:24:39]

Done.

+//   How to handle change in title format? (that would break this test)

[groby-ooo-7-16, 2014/08/12 23:57:27]

Don't worry about that - it's up to the person changing the title format.

[hcarmona, 2014/08/13 20:24:39]

Acknowledged.

+IN_PROC_BROWSER_TEST_F(LanguageOptionsInteractiveTest, TestNavigateToLanguage) {
+  ASSERT_TRUE(NavigateToLanguagePage());
+
+  content::WebContents* web_contents = NULL;
+  ASSERT_TRUE(GetActiveWebContents(web_contents));
+
+  base::string16 expected_title = l10n_util::GetStringFUTF16(
+      IDS_OPTIONS_TAB_TITLE,
+      l10n_util::GetStringUTF16(IDS_SETTINGS_TITLE),
+      l10n_util::GetStringUTF16(IDS_OPTIONS_SETTINGS_LANGUAGES_DIALOG_TITLE));
+
+  EXPECT_EQ(expected_title, web_contents->GetTitle());
+}
+
+// This test will verify that the appropriate languages are available.
+IN_PROC_BROWSER_TEST_F(LanguageOptionsInteractiveTest, TestMockLanguages) {
+  ASSERT_TRUE(NavigateToLanguagePage());
+
+  content::RenderFrameHost* active_frame = NULL;
+  ASSERT_TRUE(GetActiveFrame(active_frame));
+
+  int language_count = 0;
+  EXPECT_TRUE(content::ExecuteScriptAndExtractInt(
+      active_frame,
+      "var count = 0;"
+      "for (var i = 0; i < document.activeElement.childElementCount; ++i) {"
+      "  if (document.activeElement.children[i].className == 'deletable-item')"
+      "    ++count;"
+      "}"
+      "domAutomationController.send(count);",
+      &language_count));
+
+  // this test will fail until I can add languages
+  EXPECT_EQ(3, language_count);
+
+  std::string languages = "";
+  EXPECT_TRUE(content::ExecuteScriptAndExtractString(
+      active_frame,
+      "domAutomationController.send(document.activeElement.textContent);",
+      &languages));
+
+  // this test will fail until I can add languages
+  EXPECT_EQ("English (United States)SpanishFrench", languages);
+}
+
+// TODO(hcarmona): There is an error that needs to be addressed:
+//   Cannot get a GdkWindow for a key event.
+//   Cannot translate a XKeyEvent to a GdkEvent.

[groby-ooo-7-16, 2014/08/12 23:57:27]

Might want to ping yukishiino@ about this.

[hcarmona, 2014/08/13 20:24:39]

Found another way to send keyboard events that gets rid of these errors and also
works on my mac.

On 2014/08/12 23:57:27, groby wrote:

+// Test appearst to work even though there are these errors
+// This test will validate that the language webui is accessible through
+// the keyboard.
+IN_PROC_BROWSER_TEST_F(LanguageOptionsInteractiveTest, HOCTestAccessibility) {
+  ASSERT_TRUE(NavigateToLanguagePage());
+
+  // verify that the list is focused by default
+  std::string original_id;
+  ASSERT_TRUE(GetActiveElementId(&original_id));
+  EXPECT_EQ("language-options-list", original_id);
+
+  // press tab to select the next element
+  ASSERT_TRUE(PressKey(ui::VKEY_TAB));
+
+  // make sure that the next element is the button that is next in the tab order

[groby-ooo-7-16, 2014/08/12 23:57:27]

Can you instead just test that it's moved on to a new element? Otherwise this'll
break as soon as somebody changes tab order.

[hcarmona, 2014/08/13 20:24:39]

Kept this expectation the same because we want to make sure we're not tabbing to
another element inside the list with a different id.

On 2014/08/12 23:57:27, groby wrote:

+  std::string new_id;
+  ASSERT_TRUE(GetActiveElementId(&new_id));
+  EXPECT_EQ("language-options-add-button", new_id);
+}
+
+}  // namespace language_options_ui_test