Added battery level and time to the status tray's accessible name.

chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc

 // Copyright (c) 2012 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/browser/ui/views/accessibility/accessibility_event_router_views.h"
 
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/memory/singleton.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/accessibility/accessibility_extension_api.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "content/public/browser/notification_service.h"
 #include "ui/accessibility/ax_view_state.h"
 #include "ui/views/controls/menu/menu_item_view.h"
 #include "ui/views/controls/menu/submenu_view.h"
 #include "ui/views/controls/tree/tree_view.h"
 #include "ui/views/focus/view_storage.h"
 #include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
 
 using views::FocusManager;
 
 AccessibilityEventRouterViews::AccessibilityEventRouterViews()
-    : most_recent_profile_(NULL) {
+    : most_recent_profile_(NULL),
+      most_recent_view_(NULL) {
   // Register for notification when profile is destroyed to ensure that all
   // observers are detatched at that time.
   registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED,
                  content::NotificationService::AllSources());
 }
 
 AccessibilityEventRouterViews::~AccessibilityEventRouterViews() {
 }
 
 // static
@@ skipping 116 matching lines @@
   most_recent_profile_ = profile;
 
   if (type == ui::AX_EVENT_MENU_START ||
       type == ui::AX_EVENT_MENU_POPUP_START ||
       type == ui::AX_EVENT_MENU_END ||
       type == ui::AX_EVENT_MENU_POPUP_END) {
     SendMenuNotification(view, type, profile);
     return;
   }
 
+  // The highest focusable view is processed to handle cases where there are

[sky, 2014/07/11 15:44:02]

It's not clear to me why you want the highest focusable view? If you hover over
|view| isn't |view| the interesting view and not some ancestor? Also, if you are
going to look for a focusable ancestor shouldn't you stop at the first one?

I suspect you want the first ancestor (including view) that would handle the
mouse event, but I'm not sure there. How is this code called?

[dmazzoni, 2014/07/11 16:11:38]

If a view is focusable, it's usually atomic wrt accessibility.

The example this is fixing is the status tray button. The button itself is
focusable and has proper accessibility attributes, and if you focus it with the
keyboard it does the right thing.

It also has several child views - the battery, time, some icons - but when
hovering over these views it doesn't make sense to announce them separately,
because they're all just part of the same button.

[sky, 2014/07/11 19:06:08]

What mechanism does the status tray button use to ensure only it shows active?
Hit targetting? Seems like this code should do the same.

[evy, 2014/07/12 00:32:50]

Aha. If I go to the next highest accessibility focusable view, it works. Does
this implementation seem okay to you?

+  // views inside other focusable views (e.g. hover for date text on the
+  //  status bar) and the status bar information should be read instead

[sky, 2014/07/11 15:44:02]

nit: you've got an extra space at the beginning.

[evy, 2014/07/11 18:15:10]

Done.

+  // of the specific button inside. Here |view| is updated its highest
+  // focusable ancestor.
+  views::View* temp_view = view;
+  while (temp_view->parent()) {
+    temp_view = temp_view->parent();
+    if (temp_view->IsFocusable()) {

[dmazzoni, 2014/07/10 23:25:58]

Nit: in C++, braces aren't needed when the "if" condition and the statement each
fit on one line.

[evy, 2014/07/11 18:15:10]

Done.

+      view = temp_view;
+    }
+  }
+
+  // Since multiple items could share a highest focusable view, these items
+  // could all dispatch the same accessibility hover events, which isn't
+  // necessary.
+  if (type == ui::AX_EVENT_HOVER && most_recent_view_ == view)
+    return;
+  most_recent_view_ = view;

[sky, 2014/07/11 15:44:02]

I don't see this ever set to NULL. What if most_recent_view_ is deleted?

[dmazzoni, 2014/07/11 16:11:38]

It should be harmless because all we ever do is test whether or not
most_recent_view_ is equal to the current view.

Would you prefer ViewStorage here instead, or is this fine, maybe with just a
comment indicating that most_recent_view_ may be invalid and we should only use
it for equality comparison.

[sky, 2014/07/11 19:06:08]

Two concerns: there is nothing stopping the pointer from getting recycled, and
since the member is a view there is nothing stopping code added later on
attempting to use the member as a View and not realizing it may be deleted.
I prefer view storage.

[evy, 2014/07/12 00:32:50]

Done.

+
   ui::AXViewState state;
   view->GetAccessibleState(&state);
 
   if (type == ui::AX_EVENT_ALERT &&
       !(state.role == ui::AX_ROLE_ALERT ||
         state.role == ui::AX_ROLE_WINDOW)) {
     SendAlertControlNotification(view, type, profile);
     return;
   }
 
@@ skipping 369 matching lines @@
     return base::UTF16ToUTF8(state.name);
 
   for (int i = 0; i < view->child_count(); ++i) {
     views::View* child = view->child_at(i);
     std::string result = RecursiveGetStaticText(child);
     if (!result.empty())
       return result;
   }
   return std::string();
 }