Revert "Improvements to ViewAccessibility::OverrideIsEnabled"

This reverts commit 2d8cc01.

Reason for revert: Suspected cause of build breakage https://cr-buildbucket.appspot.com/build/8855175821513127424

Original change's description:
> Improvements to ViewAccessibility::OverrideIsEnabled
>
> This patch makes the following improvements to ViewAccessibility::OverrideIsEnabled:
> 1. Changes the method name from its original name to make it more similar to View::GetIsEnabled.
> 2. Adds an accessor, similar to other special overridable attributes.
> 3. Views that are disabled but are accessibility enabled should also
> be marked as focusable for consistency with the rest of the codebase.
> 4. Marking an accessibility view as disabled while its associated View
> is disabled will no longer cause a DCHECK.
> 5. Marking an accessibility view as readonly will not be affected by its
> accessibility enabled state.
>
> See original patch at https://crrev.com/c/2576342
>
> R=​dmazzoni@chromium.org, aleventhal@chromium.org, andrewxu@chromium.org
>
> AX-Relnotes: n/a.
> Change-Id: Id2673625d4dfe42b49dc1203de5a44aa0f278b02
> Bug: 1150503
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2583706
> Auto-Submit: Nektarios Paisios <nektar@chromium.org>
> Commit-Queue: David Black <dmblack@google.com>
> Reviewed-by: David Black <dmblack@google.com>
> Reviewed-by: Andrew Xu <andrewxu@chromium.org>
> Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#854125}

TBR=dmazzoni@chromium.org,nektar@chromium.org,dmblack@google.com,aleventhal@chromium.org,andrewxu@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com

Change-Id: I114c46bc24f5b00faaa9191ee01e6f372be662b6
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1150503
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2695150
Reviewed-by: Jeevan Shikaram <jshikaram@chromium.org>
Commit-Queue: Jeevan Shikaram <jshikaram@chromium.org>
Cr-Commit-Position: refs/heads/master@{#854141}

ash/clipboard/clipboard_history_menu_model_adapter.cc

@@ -207,7 +207,7 @@ void ClipboardHistoryMenuModelAdapter::RemoveMenuItemWithCommandId(
 
   // Disabling `item_view_to_delete` is more like implementation details.
   // So do not expose it to users.
-  view_accessibility.OverrideIsEnabled(true);
+  view_accessibility.OverrideViewEnablingState(true);
 
   // Specify `item_view_to_delete`'s position in the set. Without calling
   // `OverridePosInSet()`, the menu's size after deletion may be announced.

ui/views/accessibility/view_accessibility.cc

@@ -249,18 +249,8 @@ void ViewAccessibility::GetAccessibleNodeData(ui::AXNodeData* data) const {
   if (ViewAccessibility::IsAccessibilityFocusable())
     data->AddState(ax::mojom::State::kFocusable);
 
-  if (is_enabled_) {
-    if (*is_enabled_) {
-      // Take into account the possibility that the View is marked as readonly
-      // but enabled. In other words, we can't just remove all restrictions,
-      // unless the View is explicitly marked as disabled. Note that readonly is
-      // another restriction state in addition to enabled and disabled, (see
-      // ax::mojom::Restriction).
-      if (data->GetRestriction() == ax::mojom::Restriction::kDisabled)
-        data->SetRestriction(ax::mojom::Restriction::kNone);
-    } else {
-      data->SetRestriction(ax::mojom::Restriction::kDisabled);
-    }
+  if (custom_data_.HasIntAttribute(ax::mojom::IntAttribute::kRestriction)) {
+    data->SetRestriction(custom_data_.GetRestriction());
   } else if (!view_->GetEnabled()) {
     data->SetRestriction(ax::mojom::Restriction::kDisabled);
   }
@@ -298,21 +288,16 @@ bool ViewAccessibility::IsAccessibilityFocusable() const {
   // be focusable, if there is test coverage, such a situation will cause a test
   // failure.
   return view_->GetFocusBehavior() != View::FocusBehavior::NEVER &&
-         ViewAccessibility::IsAccessibilityEnabled() && view_->IsDrawn() &&
-         !is_ignored_;
+         view_->GetEnabled() && view_->IsDrawn() && !is_ignored_;
 }
 
 bool ViewAccessibility::IsFocusedForTesting() const {
   return view_->HasFocus() && !focused_virtual_child_;
 }
 
-void ViewAccessibility::SetPopupFocusOverride() {
-  NOTIMPLEMENTED();
-}
+void ViewAccessibility::SetPopupFocusOverride() {}
 
-void ViewAccessibility::EndPopupFocusOverride() {
-  NOTIMPLEMENTED();
-}
+void ViewAccessibility::EndPopupFocusOverride() {}
 
 void ViewAccessibility::FireFocusAfterMenuClose() {
   NotifyAccessibilityEvent(ax::mojom::Event::kFocusAfterMenuClose);
@@ -368,6 +353,19 @@ void ViewAccessibility::OverrideIsIgnored(bool value) {
   is_ignored_ = value;
 }
 
+void ViewAccessibility::OverrideViewEnablingState(bool enabled) {
+  // Cannot use `AXNodeData::SetRestriction()` which does not store
+  // `ax::mojom::Restriction::kNone`.
+
+  if (custom_data_.HasIntAttribute(ax::mojom::IntAttribute::kRestriction))
+    custom_data_.RemoveIntAttribute(ax::mojom::IntAttribute::kRestriction);
+
+  custom_data_.AddIntAttribute(
+      ax::mojom::IntAttribute::kRestriction,
+      enabled ? static_cast<int>(ax::mojom::Restriction::kNone)
+              : static_cast<int>(ax::mojom::Restriction::kDisabled));
+}
+
 bool ViewAccessibility::IsIgnored() const {
   // TODO(nektar): Make this method non-virtual and implement as follows:
   // ui::AXNodeData out_data;
@@ -376,21 +374,6 @@ bool ViewAccessibility::IsIgnored() const {
   return is_ignored_;
 }
 
-void ViewAccessibility::OverrideIsEnabled(bool enabled) {
-  // Cannot store this value in `custom_data_` because
-  // `AXNodeData::AddIntAttribute` will DCHECK if you add an IntAttribute that
-  // is equal to kNone. Adding an IntAttribute that is equal to kNone is
-  // ambiguous, since it is unclear what would be the difference between doing
-  // this and not adding the attribute at all.
-  is_enabled_ = enabled;
-}
-
-bool ViewAccessibility::IsAccessibilityEnabled() const {
-  if (is_enabled_)
-    return *is_enabled_;
-  return view_->GetEnabled();
-}
-
 void ViewAccessibility::OverrideBounds(const gfx::RectF& bounds) {
   custom_data_.relative_bounds.bounds = bounds;
 }

ui/views/accessibility/view_accessibility.h

@@ -10,7 +10,6 @@
 #include <vector>
 
 #include "base/callback_forward.h"
-#include "base/optional.h"
 #include "base/strings/string16.h"
 #include "build/build_config.h"
 #include "build/chromeos_buildflags.h"
@@ -83,7 +82,7 @@ class VIEWS_EXPORT ViewAccessibility {
   // needs to be turned on.
   virtual bool IsAccessibilityFocusable() const;
 
-  // Used for testing. Returns true if this view is considered focused.
+  // Returns true if this view is considered focused.
   virtual bool IsFocusedForTesting() const;
 
   // Call when this is the active descendant of a popup view that temporarily
@@ -122,19 +121,9 @@ class VIEWS_EXPORT ViewAccessibility {
   // Hides this View from the accessibility tree that is exposed to platform
   // APIs.
   void OverrideIsIgnored(bool value);
+  void OverrideViewEnablingState(bool enabled);
   virtual bool IsIgnored() const;
 
-  // Marks this View either as enabled or disabled (grayed out) in the
-  // accessibility tree and ignores the View's real enabled state. Does not
-  // affect the View's focusable state (see "IsAccessibilityFocusable()").
-  // Screen readers make a special announcement when an item is disabled.
-  //
-  // It might not be advisable to mark a View as enabled in the accessibility
-  // tree, whilst the real View is actually disabled, because such a View will
-  // not respond to user actions.
-  void OverrideIsEnabled(bool enabled);
-  virtual bool IsAccessibilityEnabled() const;
-
   void OverrideBounds(const gfx::RectF& bounds);
   void OverrideDescribedBy(View* described_by_view);
   void OverrideHasPopup(const ax::mojom::HasPopup has_popup);
@@ -255,10 +244,6 @@ class VIEWS_EXPORT ViewAccessibility {
   // "presentational".
   bool is_ignored_;
 
-  // Used to override the View's enabled state in case we need to mark the View
-  // as enabled or disabled only in the accessibility tree.
-  base::Optional<bool> is_enabled_ = base::nullopt;
-
   // Used by the Views system to help some assistive technologies, such as
   // screen readers, transition focus from one widget to another.
   Widget* next_focus_ = nullptr;

ui/views/accessibility/view_ax_platform_node_delegate.cc

@@ -177,7 +177,7 @@ void ViewAXPlatformNodeDelegate::FireFocusAfterMenuClose() {
   ui::AXPlatformNodeBase* focused_node =
       static_cast<ui::AXPlatformNodeBase*>(ax_platform_node_);
   // Continue to drill down focused nodes to get to the "deepest" node that is
-  // focused. This is not necessarily a view. It could be web content.
+  // focused. This is not necessarily a view - it could be web content.
   while (focused_node) {
     ui::AXPlatformNodeBase* deeper_focus = static_cast<ui::AXPlatformNodeBase*>(
         ui::AXPlatformNode::FromNativeViewAccessible(focused_node->GetFocus()));
@@ -434,7 +434,7 @@ bool ViewAXPlatformNodeDelegate::IsChildOfLeaf() const {
 }
 
 gfx::NativeViewAccessible ViewAXPlatformNodeDelegate::GetNSWindow() {
-  NOTIMPLEMENTED() << "Should only be called on Mac.";
+  NOTREACHED() << "Should only be called on Mac.";
   return nullptr;
 }
 
@@ -482,10 +482,6 @@ bool ViewAXPlatformNodeDelegate::IsInvisibleOrIgnored() const {
   return IsIgnored() || !view()->GetVisible();
 }
 
-bool ViewAXPlatformNodeDelegate::IsAccessibilityEnabled() const {
-  return GetData().GetRestriction() != ax::mojom::Restriction::kDisabled;
-}
-
 bool ViewAXPlatformNodeDelegate::IsFocused() const {
   return GetFocus() == GetNativeObject();
 }

ui/views/accessibility/view_ax_platform_node_delegate.h

@@ -9,7 +9,6 @@
 
 #include <vector>
 
-#include "base/optional.h"
 #include "base/strings/string16.h"
 #include "build/build_config.h"
 #include "ui/accessibility/ax_enums.mojom-forward.h"
@@ -51,7 +50,6 @@ class ViewAXPlatformNodeDelegate : public ViewAccessibility,
   void EndPopupFocusOverride() override;
   void FireFocusAfterMenuClose() override;
   bool IsIgnored() const override;
-  bool IsAccessibilityEnabled() const override;
   gfx::NativeViewAccessible GetNativeObject() const override;
   void NotifyAccessibilityEvent(ax::mojom::Event event_type) override;
 #if defined(OS_APPLE)

ui/views/accessibility/view_ax_platform_node_delegate_unittest.cc

@@ -386,7 +386,7 @@ TEST_F(ViewAXPlatformNodeDelegateTest, InvisibleViews) {
       label_accessibility()->GetData().HasState(ax::mojom::State::kInvisible));
 }
 
-TEST_F(ViewAXPlatformNodeDelegateTest, SetFocus) {
+TEST_F(ViewAXPlatformNodeDelegateTest, WritableFocus) {
   // Make |button_| focusable, and focus/unfocus it via
   // ViewAXPlatformNodeDelegate.
   button_->SetFocusBehavior(View::FocusBehavior::ALWAYS);
@@ -400,18 +400,9 @@ TEST_F(ViewAXPlatformNodeDelegateTest, SetFocus) {
   EXPECT_EQ(nullptr, button_->GetFocusManager()->GetFocusedView());
   EXPECT_EQ(nullptr, button_accessibility()->GetFocus());
 
-  // If the button is not focusable at all, or if it is disabled for
-  // accessibility, SetFocused() should return false.
+  // If not focusable at all, SetFocused() should return false.
   button_->SetEnabled(false);
   EXPECT_FALSE(SetFocused(button_accessibility(), true));
-  button_->SetEnabled(true);
-
-  button_accessibility()->OverrideIsEnabled(false);
-  EXPECT_FALSE(SetFocused(button_accessibility(), true));
-
-  EXPECT_FALSE(button_accessibility()->IsAccessibilityFocusable());
-  button_accessibility()->OverrideIsEnabled(true);
-  EXPECT_TRUE(button_accessibility()->IsAccessibilityFocusable());
 }
 
 TEST_F(ViewAXPlatformNodeDelegateTest, GetAuthorUniqueIdDefault) {
@@ -763,50 +754,6 @@ TEST_F(ViewAXPlatformNodeDelegateTest, TreeNavigationWithIgnoredViews) {
             child_view_4->GetPreviousSibling());
 }
 
-TEST_F(ViewAXPlatformNodeDelegateTest, OverrideIsEnabled) {
-  // Initially, the button should be enabled.
-  EXPECT_TRUE(button_accessibility()->IsAccessibilityEnabled());
-  EXPECT_TRUE(button_accessibility()->IsAccessibilityFocusable());
-
-  button_->SetEnabled(false);
-  EXPECT_FALSE(button_accessibility()->IsAccessibilityEnabled());
-  EXPECT_FALSE(button_accessibility()->IsAccessibilityFocusable());
-
-  button_->SetEnabled(true);
-  EXPECT_TRUE(button_accessibility()->IsAccessibilityEnabled());
-  EXPECT_TRUE(button_accessibility()->IsAccessibilityFocusable());
-
-  // `ViewAccessibility::OverrideIsEnabled` should have priority over
-  // `View::SetEnabled`.
-  button_accessibility()->OverrideIsEnabled(false);
-  EXPECT_FALSE(button_accessibility()->IsAccessibilityEnabled());
-  EXPECT_FALSE(button_accessibility()->IsAccessibilityFocusable());
-
-  button_->SetEnabled(false);
-  EXPECT_FALSE(button_accessibility()->IsAccessibilityEnabled());
-  EXPECT_FALSE(button_accessibility()->IsAccessibilityFocusable());
-  button_accessibility()->OverrideIsEnabled(true);
-  EXPECT_TRUE(button_accessibility()->IsAccessibilityEnabled());
-  EXPECT_TRUE(button_accessibility()->IsAccessibilityFocusable());
-
-  // Initially, the label should be enabled. It should never be focusable
-  // because it is not an interactive control like the button.
-  EXPECT_TRUE(label_accessibility()->IsAccessibilityEnabled());
-  EXPECT_FALSE(label_accessibility()->IsAccessibilityFocusable());
-
-  label_->SetEnabled(false);
-  EXPECT_FALSE(label_accessibility()->IsAccessibilityEnabled());
-  EXPECT_FALSE(label_accessibility()->IsAccessibilityFocusable());
-
-  label_accessibility()->OverrideIsEnabled(true);
-  EXPECT_TRUE(label_accessibility()->IsAccessibilityEnabled());
-  EXPECT_FALSE(label_accessibility()->IsAccessibilityFocusable());
-
-  label_accessibility()->OverrideIsEnabled(false);
-  EXPECT_FALSE(label_accessibility()->IsAccessibilityEnabled());
-  EXPECT_FALSE(label_accessibility()->IsAccessibilityFocusable());
-}
-
 TEST_F(ViewAXPlatformNodeDelegateTest, OverrideHasPopup) {
   View::Views view_ids = SetUpExtraViews();