New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 800270 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug
Team-Accessibility

Blocking:
issue 594887



Sign in to add a comment

Consider removing spoken_feedback_notification_ in ash::AccessibilityController

Project Member Reported by warx@chromium.org, Jan 9 2018

Issue description

It is there when we migrate Is{Set}SpokenFeedbackEnabled from AccessibilityManager, due to the reason that OnBrailleDisplayStateChanged we need different notification visibility type. However, |spoken_feedback_notification_| is not a reliable design.


 

Comment 1 by warx@chromium.org, Jan 9 2018

There are also some ash/ side callers which need A11Y_NOTIFICATION_SHOW.
Cc: est...@chromium.org
Naively it seems like there should be a separate code path that explicitly shows a notification, rather than making it an optional side effect of turning on spoken feedback.

+estade just FYI since he's looked at notifications recently.

Comment 3 by est...@chromium.org, Jan 10 2018

I dunno what your plan is to fix it, but this comment seems like it would necessitate changes that would encompass removing that member:

// TODO(crbug.com/594887): Fix for mash by moving pref into ash.

> [...] rather than making it an optional side effect of turning on spoken feedback

From my reading of the code it seems like it's done this way because it no-ops when the pref is unchanged.
Components: UI>Accessibility

Comment 5 by warx@chromium.org, Mar 2 2018

Cc: dmazz...@chromium.org
In ash, the only one place that SetSpokenFeedbackEnabled call doesn't trigger notification is here: https://cs.chromium.org/chromium/src/ash/system/tray_accessibility.cc?sq=package:chromium&l=346

I wonder whether we can always make ChromeVox enabled showing notification, including above "clicked on system tray menu option" case. It doesn't look bad to me with notification bubble shown on top of system tray bubble after I clicked on the tray menu.

If we could do so, it would be pretty straightforward to remove spoken_feedback_notification_.

Comment 6 by warx@chromium.org, Mar 3 2018

Cc: lpalmaro@chromium.org
+lpalmaro for #5, since it is a PM level decision I believe.
Project Member

Comment 7 by bugdroid1@chromium.org, Mar 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/60e79030f2646daa279646c9ca79219110996131

commit 60e79030f2646daa279646c9ca79219110996131
Author: Qiang Xu <warx@google.com>
Date: Thu Mar 29 08:18:31 2018

cros: remove AccessibilityNotificationType from chrome

Chrome/ never calls A11Y_NOTIFICATION_SHOW after refactoring, thus
ash::AccessibilityNotificationType is not needed for chrome/.

TBR=sky@chromium.org

Bug:  800270 
Test: covered by tests and emulator tests
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Ic5b7b669074a5a00fe41dcbb814769358ae39f72
Reviewed-on: https://chromium-review.googlesource.com/982578
Commit-Queue: Qiang Xu <warx@google.com>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546767}
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/ash/accelerators/spoken_feedback_toggler.cc
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/ash/accessibility/accessibility_controller.cc
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/ash/accessibility/accessibility_controller.h
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/ash/accessibility/touch_exploration_manager.cc
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/ash/public/cpp/accessibility_types.h
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/ash/system/tray_accessibility.cc
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/ash/system/tray_accessibility_unittest.cc
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/chrome/browser/chromeos/accessibility/accessibility_manager.cc
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/chrome/browser/chromeos/accessibility/accessibility_manager.h
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/chrome/browser/chromeos/accessibility/accessibility_manager_browsertest.cc
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/chrome/browser/chromeos/accessibility/chromevox_panel.cc
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/chrome/browser/chromeos/accessibility/chromevox_panel.h
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/chrome/browser/chromeos/accessibility/magnification_manager.cc
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge_browsertest.cc
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/chrome/browser/chromeos/login/wizard_controller_browsertest.cc
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/chrome/browser/chromeos/system/tray_accessibility_browsertest.cc
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/chrome/browser/policy/policy_browsertest.cc
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/chrome/browser/resources/chromeos/chromevox/testing/chromevox_e2e_test_base.js
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/chrome/browser/ui/ash/volume_controller_browsertest.cc
[modify] https://crrev.com/60e79030f2646daa279646c9ca79219110996131/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Apr 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/82504ea9b271ba1f784f6c110b06cc286a17e2e8

commit 82504ea9b271ba1f784f6c110b06cc286a17e2e8
Author: Qiang Xu <warx@google.com>
Date: Tue Apr 17 05:19:08 2018

cros: move ShowAccessibilityNotification to A11yController

changes:
Remove ShowAccessibilityNotification in AccessibilityObserver. That is
only used in TrayAccessibility, while TrayAccessibility should be pure
views code. Move the function and tests to AccessibilityController.

Bug:  800270 
Test: covered by tests
Change-Id: I0551c8b36d1a4bfb030b786bd78d610627113828
Reviewed-on: https://chromium-review.googlesource.com/1007891
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Qiang Xu <warx@google.com>
Cr-Commit-Position: refs/heads/master@{#551253}
[modify] https://crrev.com/82504ea9b271ba1f784f6c110b06cc286a17e2e8/ash/accessibility/accessibility_controller.cc
[modify] https://crrev.com/82504ea9b271ba1f784f6c110b06cc286a17e2e8/ash/accessibility/accessibility_controller.h
[modify] https://crrev.com/82504ea9b271ba1f784f6c110b06cc286a17e2e8/ash/accessibility/accessibility_controller_unittest.cc
[modify] https://crrev.com/82504ea9b271ba1f784f6c110b06cc286a17e2e8/ash/accessibility/accessibility_observer.h
[modify] https://crrev.com/82504ea9b271ba1f784f6c110b06cc286a17e2e8/ash/system/tray_accessibility.cc
[modify] https://crrev.com/82504ea9b271ba1f784f6c110b06cc286a17e2e8/ash/system/tray_accessibility.h
[modify] https://crrev.com/82504ea9b271ba1f784f6c110b06cc286a17e2e8/ash/system/tray_accessibility_unittest.cc

Comment 9 by warx@chromium.org, Apr 17 2018

Status: Fixed (was: Assigned)
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2dbd34cb1732bce41b442ec3926e14c3640fdd3e

commit 2dbd34cb1732bce41b442ec3926e14c3640fdd3e
Author: Qiang Xu <warx@google.com>
Date: Tue Apr 17 18:17:51 2018

cros: remove braille_display_connected_

changes:
Follow up with crrev.com/c/1007891, |braille_display_connected_| is no
longer needed by TrayAccessibility. We don't need it for tray
visibility either, since when connected it always guaranteed that spoken
feedback is enabled, so that spoken feedback enabled state could reflect
on behalf of braille display connected state.

Bug:  800270 
Test: covered by tests
Change-Id: I33f0d7b9d53a41e7ff1ed5160207b9d5fb603188
Reviewed-on: https://chromium-review.googlesource.com/1014746
Commit-Queue: Qiang Xu <warx@google.com>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551398}
[modify] https://crrev.com/2dbd34cb1732bce41b442ec3926e14c3640fdd3e/ash/accessibility/accessibility_controller.cc
[modify] https://crrev.com/2dbd34cb1732bce41b442ec3926e14c3640fdd3e/ash/accessibility/accessibility_controller.h
[modify] https://crrev.com/2dbd34cb1732bce41b442ec3926e14c3640fdd3e/ash/system/tray_accessibility.cc

Sign in to add a comment