Skip to content

GitLab

T
tangled
Commit 777cefdf authored by Ben Wells's avatar Ben Wells Committed by Commit Bot
Browse files
Options

Revert "Test any widget activated in a browser test for accessibility violations" 

This reverts commit 0a869fb4.

Reason for revert: New checker is causing errors on bot 
e.g. https://ci.chromium.org/buildbot/chromium.memory/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/26753

from the failure: The following view violates DoesViewHaveAccessibilityErrors():
browser/ui/views/frame/BrowserRootView > ui/views/window/NonClientView > BrowserView > TopContainerView > ToolbarView > View > MenuButton (id 1008)
- Focusable View has no accessible name or placeholder, and the name attribute does not use kAttributeExplicitlyEmpty.

Original change's description:
> Test any widget activated in a browser test for accessibility violations
> 
> Use ChromeViewsDelegate/WidgetObserver combination to ensure that all
> widgets are tested for accessibility as they become hidden or visible
> in a browsertest.
> 
> The test goes through the View entire subtree and ensure any focusable
> View has an accessible name.
> 
> Bug: 819350
> Change-Id: I213e02bcfb1ec64aefc528cbda6a8a2a1d7bd0d0
> Reviewed-on: https://chromium-review.googlesource.com/951933
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Trent Apted <tapted@chromium.org>
> Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#545904}

TBR=sky@chromium.org,tapted@chromium.org,aleventhal@chromium.org

Change-Id: I4acc057b19df35a9350ae04efa06e5f6b69b2e88
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 819350
Reviewed-on: https://chromium-review.googlesource.com/981634Reviewed-by: default avatarBen Wells <benwells@chromium.org>
Commit-Queue: Ben Wells <benwells@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546020}
parent fc982567
Hide whitespace changes
Inline Side-by-side
Showing with 0 additions and 257 deletions
chrome/test/BUILD.gn
View file @ 777cefdf
......@@ -250,8 +250,6 @@ static_library("test_support") {
public_deps += [ "//ui/views:test_support" ]
sources += [
"../browser/ui/views/media_router/app_menu_test_api.h",
"views/accessibility_checker.cc",
"views/accessibility_checker.h",
"views/chrome_views_test_base.cc",
"views/chrome_views_test_base.h",
"views/scoped_macviews_browser_mode.cc",
......@@ -2652,11 +2650,6 @@ test("unit_tests") {
"../../third_party/zlib/google/zip_unittest.cc",
]
if (toolkit_views) {
# Test accessibility checks that audit any UI opened in a browser test.
sources += [ "views/accessibility_checker_unittest.cc" ]
}
if (is_win) {
assert(toolkit_views)
sources += [
......
chrome/test/base/in_process_browser_test.cc
View file @ 777cefdf
......@@ -96,10 +96,6 @@
#include "extensions/browser/extension_api_frame_id_map.h"
#endif
#if defined(TOOLKIT_VIEWS)
#include "chrome/test/views/accessibility_checker.h"
#endif
namespace {
// Passed as value of kTestType.
......@@ -150,10 +146,6 @@ InProcessBrowserTest::InProcessBrowserTest()
#if defined(OS_CHROMEOS)
DefaultAshEventGeneratorDelegate::GetInstance();
#endif
#if defined(TOOLKIT_VIEWS)
accessibility_checker_ = std::make_unique<AccessibilityChecker>();
#endif
}
InProcessBrowserTest::~InProcessBrowserTest() {
......
chrome/test/base/in_process_browser_test.h
View file @ 777cefdf
......@@ -41,10 +41,6 @@ class ScopedCOMInitializer;
#endif // defined(OS_WIN)
} // namespace base
#if defined(TOOLKIT_VIEWS)
class AccessibilityChecker;
#endif
class Browser;
class Profile;
#if defined(OS_MACOSX)
......@@ -275,10 +271,6 @@ class InProcessBrowserTest : public content::BrowserTestBase {
#if defined(OS_WIN)
std::unique_ptr<base::win::ScopedCOMInitializer> com_initializer_;
#endif
#if defined(TOOLKIT_VIEWS)
std::unique_ptr<AccessibilityChecker> accessibility_checker_;
#endif
};
#endif // CHROME_TEST_BASE_IN_PROCESS_BROWSER_TEST_H_
chrome/test/views/accessibility_checker.cc deleted 100644 → 0
View file @ fc982567
// Copyright 2018 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 "chrome/test/views/accessibility_checker.h"
#include "base/strings/string_util.h"
#include "base/strings/stringprintf.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/accessibility/ax_node_data.h"
#include "ui/views/accessibility/view_accessibility.h"
#include "ui/views/widget/native_widget_delegate.h"
#include "ui/views/widget/widget.h"
namespace {
using ax::mojom::NameFrom;
using ax::mojom::Role;
using ax::mojom::State;
using ax::mojom::StringAttribute;
// Return helpful string for identifying a view.
// Includes the view class of every view in the ancestor chain, root first.
// Also provides the id.
// For example:
// BrowserView > OmniboxView (id 3).
std::string GetViewDebugString(const views::View* view) {
// Get classes of ancestors.
std::vector<std::string> classes;
for (const views::View* ancestor = view; ancestor;
ancestor = ancestor->parent())
classes.insert(classes.begin(), ancestor->GetClassName());
return base::JoinString(classes, " > ") +
base::StringPrintf(" (id %d)", view->id());
}
bool DoesViewHaveAccessibleNameOrLabelError(ui::AXNodeData* data) {
// Focusable nodes must have an accessible name, otherwise screen reader users
// will not know what they landed on. For example, the reload button should
// have an accessible name of "Reload".
// Exceptions:
// 1) Textfields can set the placeholder string attribute.
// 2) Explicitly setting the name to "" is allowed if the view uses
// AXNodedata.SetNameExplicitlyEmpty().
// It has a name, we're done.
if (!data->GetStringAttribute(StringAttribute::kName).empty())
return false;
// Text fields are allowed to have a placeholder instead.
if (data->role == Role::kTextField &&
!data->GetStringAttribute(StringAttribute::kPlaceholder).empty())
return false;
// Finally, a view is allowed to explicitly state that it has no name.
if (data->GetNameFrom() == NameFrom::kAttributeExplicitlyEmpty)
return false;
// Has an error -- no name or placeholder, and not explicitly empty.
return true;
}
bool DoesViewHaveAccessibilityErrors(views::View* view,
std::string* error_message) {
views::ViewAccessibility& view_ax = view->GetViewAccessibility();
ui::AXNodeData node_data;
// Get accessible node data from view_ax instead of view, because some
// additional fields are processed and set there.
view_ax.GetAccessibleNodeData(&node_data);
std::string violations;
// No checks for unfocusable items yet.
if (node_data.HasState(State::kFocusable)) {
if (DoesViewHaveAccessibleNameOrLabelError(&node_data)) {
violations +=
"\n- Focusable View has no accessible name or placeholder, and the "
"name attribute does not use kAttributeExplicitlyEmpty.";
}
if (node_data.HasState(State::kInvisible))
violations += "\n- Focusable View should not be invisible.";
}
if (violations.empty())
return false; // No errors.
*error_message =
"The following view violates DoesViewHaveAccessibilityErrors():\n" +
GetViewDebugString(view) + violations;
return true;
}
bool DoesViewHaveAccessibilityErrorsRecursive(views::View* view,
std::string* error_message) {
if (DoesViewHaveAccessibilityErrors(view, error_message))
return true;
for (int i = 0; i < view->child_count(); ++i) {
if (DoesViewHaveAccessibilityErrorsRecursive(view->child_at(i),
error_message))
return true;
}
return false; // All views in this subtree passed all checker.
}
} // namespace
void AddFailureOnWidgetAccessibilityError(views::Widget* widget) {
std::string error_message;
if (widget->widget_delegate() && !widget->IsClosed() &&
widget->GetRootView() &&
DoesViewHaveAccessibilityErrorsRecursive(widget->GetRootView(),
&error_message)) {
ADD_FAILURE() << error_message;
}
}
AccessibilityChecker::AccessibilityChecker() : scoped_observer_(this) {}
AccessibilityChecker::~AccessibilityChecker() {
DCHECK(!scoped_observer_.IsObservingSources());
}
void AccessibilityChecker::OnBeforeWidgetInit(
views::Widget::InitParams* params,
views::internal::NativeWidgetDelegate* delegate) {
ChromeViewsDelegate::OnBeforeWidgetInit(params, delegate);
views::Widget* widget = delegate->AsWidget();
if (widget)
scoped_observer_.Add(widget);
}
void AccessibilityChecker::OnWidgetDestroying(views::Widget* widget) {
scoped_observer_.Remove(widget);
}
void AccessibilityChecker::OnWidgetVisibilityChanged(views::Widget* widget,
bool visible) {
// Test widget for accessibility errors both as it becomes visible or hidden,
// in order to catch more errors. For example, to catch errors in the download
// shelf we must check the browser window as it is hidden, because the shelf
// is not visible when the browser window first appears.
AddFailureOnWidgetAccessibilityError(widget);
}
chrome/test/views/accessibility_checker.h deleted 100644 → 0
View file @ fc982567
// Copyright 2018 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.
#ifndef CHROME_TEST_VIEWS_ACCESSIBILITY_CHECKER_H_
#define CHROME_TEST_VIEWS_ACCESSIBILITY_CHECKER_H_
#include "base/scoped_observer.h"
#include "chrome/browser/ui/views/chrome_views_delegate.h"
#include "ui/views/widget/widget_observer.h"
// Runs UI accessibility checks on |widget|.
// Adds a gtest failure if any check fails.
// Callers are not expected to assert/expect on failure.
void AddFailureOnWidgetAccessibilityError(views::Widget* widget);
// Observe the creation of all widgets and ensure their view subtrees are
// checked for accessibility violations when they become visible or hidden.
//
// Accessibility violations will add a gtest failure.
class AccessibilityChecker : public ChromeViewsDelegate,
public views::WidgetObserver {
public:
AccessibilityChecker();
~AccessibilityChecker() override;
// ChromeViewsDelegate:
void OnBeforeWidgetInit(
views::Widget::InitParams* params,
views::internal::NativeWidgetDelegate* delegate) override;
// views::WidgetObserver:
void OnWidgetDestroying(views::Widget* widget) override;
void OnWidgetVisibilityChanged(views::Widget* widget, bool visible) override;
private:
ScopedObserver<views::Widget, WidgetObserver> scoped_observer_;
DISALLOW_COPY_AND_ASSIGN(AccessibilityChecker);
};
#endif // CHROME_TEST_VIEWS_ACCESSIBILITY_CHECKER_H_
chrome/test/views/accessibility_checker_unittest.cc deleted 100644 → 0
View file @ fc982567
// Copyright 2018 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 "chrome/test/views/accessibility_checker.h"
#include "base/strings/utf_string_conversions.h"
#include "testing/gtest/include/gtest/gtest-spi.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/views/controls/button/image_button.h"
#include "ui/views/test/views_test_base.h"
#include "ui/views/view.h"
#include "ui/views/widget/widget.h"
typedef views::ViewsTestBase AccessibilityCheckerTest;
// Test that a view that is not accessible will fail the accessibility audit.
TEST_F(AccessibilityCheckerTest, VerifyAccessibilityCheckerFailAndPass) {
// Create containing widget.
views::Widget widget;
views::Widget::InitParams params =
views::Widget::InitParams(views::Widget::InitParams::TYPE_WINDOW);
params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
params.bounds = gfx::Rect(0, 0, 650, 650);
params.context = GetContext();
widget.Init(params);
widget.Show();
// Add the button.
views::ImageButton* button = new views::ImageButton(nullptr);
widget.GetContentsView()->AddChildView(button);
// Accessibility test should pass as it is focusable but has a name.
button->SetFocusBehavior(views::View::FocusBehavior::ALWAYS);
button->SetAccessibleName(base::ASCIIToUTF16("Some name"));
AddFailureOnWidgetAccessibilityError(&widget);
// Accessibility test should pass as it has no name but is not focusable.
button->SetFocusBehavior(views::View::FocusBehavior::NEVER);
button->SetAccessibleName(base::ASCIIToUTF16(""));
AddFailureOnWidgetAccessibilityError(&widget);
// Accessibility test should fail as it has no name and is focusable.
button->SetFocusBehavior(views::View::FocusBehavior::ALWAYS);
EXPECT_NONFATAL_FAILURE(AddFailureOnWidgetAccessibilityError(&widget),
"Accessibility");
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment