Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2676563003:
ChromeVox: Disable following a11y events after screen off alert is received (Closed)
|
Created:
3 years, 10 months ago by Qiang(Joe) Xu Modified:
3 years, 10 months ago CC:
chromium-reviews, sadrul, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, kalyank Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChromeVox: Disable following a11y events after screen off alert is received
BUG=686899
TEST=turning the screen off works fine. For turning the screen on, the "alert screen on" is missing if chromvox needs to talk back the screen focus for the first time.
Review-Url: https://codereview.chromium.org/2676563003
Cr-Commit-Position: refs/heads/master@{#448324}
Committed: https://chromium.googlesource.com/chromium/src/+/38b110ec3981062d81c3e61a3a0508eee0f2f1c2
Patch Set 1 #
Total comments: 2
Patch Set 2 : SetAutomationManagerEnabled #
Total comments: 6
Patch Set 3 : based on comments from James #
Total comments: 1
Patch Set 4 : nits #Messages
Total messages: 19 (9 generated)
Description was changed from ========== ChromeVox: Disable following a11y events after screen off alert is received BUG=686899 ========== to ========== ChromeVox: Disable following a11y events after screen off alert is received BUG=686899 TEST= ==========
warx@chromium.org changed reviewers: + dtseng@chromium.org
Description was changed from ========== ChromeVox: Disable following a11y events after screen off alert is received BUG=686899 TEST= ========== to ========== ChromeVox: Disable following a11y events after screen off alert is received BUG=686899 TEST=turning the screen off works fine. For turning the screen on, the "alert screen on" is missing if chromvox needs to talk back the screen focus. ==========
Hi David, please take a look, thanks!
https://codereview.chromium.org/2676563003/diff/1/chrome/browser/ui/aura/acce... File chrome/browser/ui/aura/accessibility/automation_manager_aura.cc (right): https://codereview.chromium.org/2676563003/diff/1/chrome/browser/ui/aura/acce... chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:101: if (IsScreenOffAlert(text)) I wouldn't want to check against this string every single time an alert gets fired. How about adding AutomationManagerAura to subscribe to screen on/off notifications?
https://codereview.chromium.org/2676563003/diff/1/chrome/browser/ui/aura/acce... File chrome/browser/ui/aura/accessibility/automation_manager_aura.cc (right): https://codereview.chromium.org/2676563003/diff/1/chrome/browser/ui/aura/acce... chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:101: if (IsScreenOffAlert(text)) On 2017/02/03 00:27:15, David Tseng wrote: > I wouldn't want to check against this string every single time an alert gets > fired. > > How about adding AutomationManagerAura to subscribe to screen on/off > notifications? Screen on/off info may come from chromeos::power_manager_client. Power button event is processed in ash/. I feel it has code dependency issue and timing issue. Another way is ChromeShellDelegate::TriggerAccessibilityAlert to pass in the screen on/off information of the alert. WDYT?
Description was changed from ========== ChromeVox: Disable following a11y events after screen off alert is received BUG=686899 TEST=turning the screen off works fine. For turning the screen on, the "alert screen on" is missing if chromvox needs to talk back the screen focus. ========== to ========== ChromeVox: Disable following a11y events after screen off alert is received BUG=686899 TEST=turning the screen off works fine. For turning the screen on, the "alert screen on" is missing if chromvox needs to talk back the screen focus for the first time. ==========
warx@chromium.org changed reviewers: + jamescook@chromium.org
David, ptal as we discussed. +James for owner review. Thanks!
lgtm
https://codereview.chromium.org/2676563003/diff/20001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/chrome_shell_delegate.cc (right): https://codereview.chromium.org/2676563003/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_shell_delegate.cc:312: if (screen_off_alert) Can you just check alert == ash::A11Y_ALERT_SCREEN_OFF instead of introducing a new local variable? Also, this code needs comments about why you are doing this. It's not obvious to me from reading the code why you are turning things on and off. https://codereview.chromium.org/2676563003/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_shell_delegate.cc:349: AutomationManagerAura* manager = AutomationManagerAura::GetInstance(); optional: DCHECK(context); so the reader knows it cannot be null. https://codereview.chromium.org/2676563003/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_shell_delegate.cc:350: enabled ? manager->Enable(context) : manager->Disable(); Use if (enabled) not a trigraph.
Hi James, new patch ready, ptal, thanks! https://codereview.chromium.org/2676563003/diff/20001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/chrome_shell_delegate.cc (right): https://codereview.chromium.org/2676563003/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_shell_delegate.cc:312: if (screen_off_alert) On 2017/02/04 00:00:38, James Cook wrote: > Can you just check alert == ash::A11Y_ALERT_SCREEN_OFF instead of introducing a > new local variable? > > Also, this code needs comments about why you are doing this. It's not obvious to > me from reading the code why you are turning things on and off. Done. https://codereview.chromium.org/2676563003/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_shell_delegate.cc:349: AutomationManagerAura* manager = AutomationManagerAura::GetInstance(); On 2017/02/04 00:00:38, James Cook wrote: > optional: DCHECK(context); so the reader knows it cannot be null. Done. https://codereview.chromium.org/2676563003/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_shell_delegate.cc:350: enabled ? manager->Enable(context) : manager->Disable(); On 2017/02/04 00:00:38, James Cook wrote: > Use if (enabled) not a trigraph. Done.
LGTM https://codereview.chromium.org/2676563003/diff/40001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/chrome_shell_delegate.cc (right): https://codereview.chromium.org/2676563003/diff/40001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_shell_delegate.cc:352: DCHECK(context); super nit: Do this as the first line of the function so that it's obvious it's a precondition check.
The CQ bit was checked by warx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org, jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2676563003/#ps60001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486403537976260,
"parent_rev": "c05b743bc7e42d73fe6e571496269037077dbb4f", "commit_rev":
"38b110ec3981062d81c3e61a3a0508eee0f2f1c2"}
Message was sent while issue was closed.
Description was changed from ========== ChromeVox: Disable following a11y events after screen off alert is received BUG=686899 TEST=turning the screen off works fine. For turning the screen on, the "alert screen on" is missing if chromvox needs to talk back the screen focus for the first time. ========== to ========== ChromeVox: Disable following a11y events after screen off alert is received BUG=686899 TEST=turning the screen off works fine. For turning the screen on, the "alert screen on" is missing if chromvox needs to talk back the screen focus for the first time. Review-Url: https://codereview.chromium.org/2676563003 Cr-Commit-Position: refs/heads/master@{#448324} Committed: https://chromium.googlesource.com/chromium/src/+/38b110ec3981062d81c3e61a3a05... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/38b110ec3981062d81c3e61a3a05... |
