Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2488373002:
Expose caps lock state as an accessible alert (Closed)
|
Created:
4 years, 1 month ago by David Tseng Modified:
4 years, 1 month ago CC:
chromium-reviews, alemate+watch_chromium.org, sadrul, oshima+watch_chromium.org, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, kalyank, arv+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExpose caps lock state as an accessible alert
- handle the output the same way we do for other difficult alert cases. Since the caps lock ui is transient in some cases, this ended up being the cleanest and simplest way
- clean up alert output in ChromeVox. Namely, we no longer need to ensure non-interruption since we queue by default.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/3e14f24d679b76741b6c0ca0bc1510d16febe672
Cr-Commit-Position: refs/heads/master@{#432443}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address feedback. #Patch Set 3 : Rebqase #
Messages
Total messages: 27 (14 generated)
Description was changed from ========== Expose cap lock state as an accessible alert - handle the output the same way we do for other difficult alert cases. Since the caps lock ui is transient in some cases, this ended up being the cleanest and simplest way - clean up alert output in ChromeVox. Namely, we no longer need to ensure non-interruption since we queue by default. BUG= ========== to ========== Expose cap lock state as an accessible alert - handle the output the same way we do for other difficult alert cases. Since the caps lock ui is transient in some cases, this ended up being the cleanest and simplest way - clean up alert output in ChromeVox. Namely, we no longer need to ensure non-interruption since we queue by default. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Expose cap lock state as an accessible alert - handle the output the same way we do for other difficult alert cases. Since the caps lock ui is transient in some cases, this ended up being the cleanest and simplest way - clean up alert output in ChromeVox. Namely, we no longer need to ensure non-interruption since we queue by default. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Expose caps lock state as an accessible alert - handle the output the same way we do for other difficult alert cases. Since the caps lock ui is transient in some cases, this ended up being the cleanest and simplest way - clean up alert output in ChromeVox. Namely, we no longer need to ensure non-interruption since we queue by default. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dtseng@chromium.org changed reviewers: + dmazzoni@chromium.org, oshima@chromium.org
oshima for ash, dmazzoni for cvox
lgtm with a suggestion https://codereview.chromium.org/2488373002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js (left): https://codereview.chromium.org/2488373002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:401: speak: '!doNotInterrupt $role $descendants $state' Are you deleting !doNotInterrupt on purpose? I thought this was the type of thing it was useful for https://codereview.chromium.org/2488373002/diff/1/chrome/browser/ui/ash/chrom... File chrome/browser/ui/ash/chrome_shell_delegate.cc (right): https://codereview.chromium.org/2488373002/diff/1/chrome/browser/ui/ash/chrom... chrome/browser/ui/ash/chrome_shell_delegate.cc:285: case ash::A11Y_ALERT_CAPS_ON: { How about refactoring this as a concise switch that gets the message_id of the alert text, and then at the end of the switch just have one call to HandleAlert.
lgtm https://codereview.chromium.org/2488373002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/tray_caps_lock.cc (right): https://codereview.chromium.org/2488373002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/tray_caps_lock.cc:171: A11Y_ALERT_CAPS_OFF); nit: WmShell::Get()->acces..->TriggerAccessibilityAlert( enabled ? A11Y_ALERT_CAPS_ON : A11Y_ALERT_CAPS_OFF); https://codereview.chromium.org/2488373002/diff/1/chrome/browser/ui/ash/chrom... File chrome/browser/ui/ash/chrome_shell_delegate.cc (right): https://codereview.chromium.org/2488373002/diff/1/chrome/browser/ui/ash/chrom... chrome/browser/ui/ash/chrome_shell_delegate.cc:285: case ash::A11Y_ALERT_CAPS_ON: { On 2016/11/10 22:15:18, dmazzoni wrote: > How about refactoring this as a concise switch that > gets the message_id of the alert text, and then at the > end of the switch just have one call to HandleAlert. +1
https://codereview.chromium.org/2488373002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/tray_caps_lock.cc (right): https://codereview.chromium.org/2488373002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/tray_caps_lock.cc:171: A11Y_ALERT_CAPS_OFF); On 2016/11/10 22:41:40, oshima wrote: > nit: > > WmShell::Get()->acces..->TriggerAccessibilityAlert( > enabled ? A11Y_ALERT_CAPS_ON : A11Y_ALERT_CAPS_OFF); Done. https://codereview.chromium.org/2488373002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js (left): https://codereview.chromium.org/2488373002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:401: speak: '!doNotInterrupt $role $descendants $state' On 2016/11/10 22:15:18, dmazzoni wrote: > Are you deleting !doNotInterrupt on purpose? I thought > this was the type of thing it was useful for > It was when we were flushing by default. Ever since we queued up things (and category flushed) by default, this is less necessary. https://codereview.chromium.org/2488373002/diff/1/chrome/browser/ui/ash/chrom... File chrome/browser/ui/ash/chrome_shell_delegate.cc (right): https://codereview.chromium.org/2488373002/diff/1/chrome/browser/ui/ash/chrom... chrome/browser/ui/ash/chrome_shell_delegate.cc:285: case ash::A11Y_ALERT_CAPS_ON: { On 2016/11/10 22:15:18, dmazzoni wrote: > How about refactoring this as a concise switch that > gets the message_id of the alert text, and then at the > end of the switch just have one call to HandleAlert. Done.
The CQ bit was checked by dtseng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2488373002/#ps20001 (title: "Address feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for ash/common/system/chromeos/tray_caps_lock.cc:
While running git apply --index -p1;
error: patch failed: ash/common/system/chromeos/tray_caps_lock.cc:4
error: ash/common/system/chromeos/tray_caps_lock.cc: patch does not apply
Patch: ash/common/system/chromeos/tray_caps_lock.cc
Index: ash/common/system/chromeos/tray_caps_lock.cc
diff --git a/ash/common/system/chromeos/tray_caps_lock.cc
b/ash/common/system/chromeos/tray_caps_lock.cc
index
003b9dd7372b947f8122ac37836c4e8d2d9f71fb..91d54cdc4299aa5991d3aa108a2261a856ba6a2f
100644
--- a/ash/common/system/chromeos/tray_caps_lock.cc
+++ b/ash/common/system/chromeos/tray_caps_lock.cc
@@ -4,6 +4,7 @@
#include "ash/common/system/chromeos/tray_caps_lock.h"
+#include "ash/common/accessibility_delegate.h"
#include "ash/common/material_design/material_design_controller.h"
#include "ash/common/system/tray/actionable_view.h"
#include "ash/common/system/tray/fixed_sized_image_view.h"
@@ -161,6 +162,10 @@ TrayCapsLock::~TrayCapsLock() {
void TrayCapsLock::OnCapsLockChanged(bool enabled) {
caps_lock_enabled_ = enabled;
+ // Send an a11y alert.
+ WmShell::Get()->accessibility_delegate()->TriggerAccessibilityAlert(
+ enabled ? A11Y_ALERT_CAPS_ON : A11Y_ALERT_CAPS_OFF);
+
if (tray_view())
tray_view()->SetVisible(caps_lock_enabled_);
The CQ bit was checked by dtseng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2488373002/#ps40001 (title: "Rebqase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for ash/common/accessibility_types.h:
While running git apply --index -p1;
error: patch failed: ash/common/accessibility_types.h:14
error: ash/common/accessibility_types.h: patch does not apply
Patch: ash/common/accessibility_types.h
Index: ash/common/accessibility_types.h
diff --git a/ash/common/accessibility_types.h b/ash/common/accessibility_types.h
index
2339ceca88fb7868dadb07bedd0aec278ce4d84a..26093aa84c3275f39f5880918aabf49763eb38fd
100644
--- a/ash/common/accessibility_types.h
+++ b/ash/common/accessibility_types.h
@@ -14,6 +14,8 @@ enum AccessibilityNotificationVisibility {
enum AccessibilityAlert {
A11Y_ALERT_NONE,
+ A11Y_ALERT_CAPS_ON,
+ A11Y_ALERT_CAPS_OFF,
A11Y_ALERT_WINDOW_NEEDED,
A11Y_ALERT_WINDOW_OVERVIEW_MODE_ENTERED
};
Message was sent while issue was closed.
Description was changed from ========== Expose caps lock state as an accessible alert - handle the output the same way we do for other difficult alert cases. Since the caps lock ui is transient in some cases, this ended up being the cleanest and simplest way - clean up alert output in ChromeVox. Namely, we no longer need to ensure non-interruption since we queue by default. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Expose caps lock state as an accessible alert - handle the output the same way we do for other difficult alert cases. Since the caps lock ui is transient in some cases, this ended up being the cleanest and simplest way - clean up alert output in ChromeVox. Namely, we no longer need to ensure non-interruption since we queue by default. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/3e14f24d679b76741b6c0ca0bc1510d16febe672 Cr-Commit-Position: refs/heads/master@{#432443} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3e14f24d679b76741b6c0ca0bc1510d16febe672 Cr-Commit-Position: refs/heads/master@{#432443} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
