Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2234823003:
Make the mute button in the Ash system menu keyboard focusable (Closed)
|
Created:
4 years, 4 months ago by tdanderson Modified:
4 years, 4 months ago CC:
chromium-reviews, kalyank, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake the mute button in the Ash system menu keyboard focusable
Make the mute button in the Ash system menu
keyboard focusable, and assign it the accessible
name of "Mute" for ChromeVox spoken feedback.
BUG=626464
TEST=manual
Committed: https://crrev.com/61303af2fe0a016b69e2dfd1b8a752326d605d40
Cr-Commit-Position: refs/heads/master@{#411355}
Patch Set 1 #
Total comments: 2
Patch Set 2 : override GetAccessibleState #
Messages
Total messages: 25 (13 generated)
The CQ bit was checked by tdanderson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tdanderson@chromium.org changed reviewers: + jamescook@chromium.org
James, can you please take a look?
On 2016/08/10 14:03:01, tdanderson wrote: > James, can you please take a look? Also, I can't find any documentation on strings - please let me know if there is anything else I need to do, e.g., in order for the string to get translated to all languages.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dtseng@chromium.org changed reviewers: + dtseng@chromium.org
Drive-by Thanks for fixing! https://codereview.chromium.org/2234823003/diff/1/ash/common/system/audio/vol... File ash/common/system/audio/volume_view.cc (right): https://codereview.chromium.org/2234823003/diff/1/ash/common/system/audio/vol... ash/common/system/audio/volume_view.cc:63: SetAccessibleName( Could you override View::GetAccessibleState(AXViewState* state) and set the following: state->role = ui::AX_ROLE_TOGGLE_BUTTON; and state->AddStateFlag(ui::AX_STATE_PRESSED); if audio is muted in addition to state->name
LGTM assuming dtseng's comment addressed. (And thanks for the drive-by -- I wouldn't have known to recommend that.) You don't need to do anything to get the new string translated. The TPMs run some tools around branch time to pick up new strings and get them translated.
Hi David, I have a question for you below: https://codereview.chromium.org/2234823003/diff/1/ash/common/system/audio/vol... File ash/common/system/audio/volume_view.cc (right): https://codereview.chromium.org/2234823003/diff/1/ash/common/system/audio/vol... ash/common/system/audio/volume_view.cc:63: SetAccessibleName( On 2016/08/10 15:41:02, David Tseng wrote: > Could you override View::GetAccessibleState(AXViewState* state) and set the > following: > state->role = ui::AX_ROLE_TOGGLE_BUTTON; > and > state->AddStateFlag(ui::AX_STATE_PRESSED); > if audio is muted > in addition to > state->name If I'm understanding the code correctly, it looks like all of that is handled by views::ToggleImageButton::GetAccessibleState(), so I don't think that VolumeButton needs to override GetAccessibleState() too. Can you please clarify?
On 2016/08/10 20:24:34, tdanderson wrote: > Hi David, I have a question for you below: > > https://codereview.chromium.org/2234823003/diff/1/ash/common/system/audio/vol... > File ash/common/system/audio/volume_view.cc (right): > > https://codereview.chromium.org/2234823003/diff/1/ash/common/system/audio/vol... > ash/common/system/audio/volume_view.cc:63: SetAccessibleName( > On 2016/08/10 15:41:02, David Tseng wrote: > > Could you override View::GetAccessibleState(AXViewState* state) and set the > > following: > > state->role = ui::AX_ROLE_TOGGLE_BUTTON; > > and > > state->AddStateFlag(ui::AX_STATE_PRESSED); > > if audio is muted > > in addition to > > state->name > > If I'm understanding the code correctly, it looks like all of that is handled by > views::ToggleImageButton::GetAccessibleState(), so I don't think that > VolumeButton needs to override GetAccessibleState() too. Can you please clarify? Are you hearing ChromeVox say "mute button, not pressed" and "mute button, pressed"? And, are you seeing this spoken when you click the button? If not, then the VolumeButton is doing something else to indicate toggled vs untoggled so you do need to handle it. Note the call to NotifyAccessibilityEvent in ToggleImageButton::SetToggled. The core issue is that subclasses don't actually do the same thing to indicate toggled or not, so the base impl is only a guess. Ideally, subclasses would actually use ToggleImageButton::SetToggled.
On 2016/08/10 23:05:08, David Tseng wrote: > On 2016/08/10 20:24:34, tdanderson wrote: > > Hi David, I have a question for you below: > > > > > https://codereview.chromium.org/2234823003/diff/1/ash/common/system/audio/vol... > > File ash/common/system/audio/volume_view.cc (right): > > > > > https://codereview.chromium.org/2234823003/diff/1/ash/common/system/audio/vol... > > ash/common/system/audio/volume_view.cc:63: SetAccessibleName( > > On 2016/08/10 15:41:02, David Tseng wrote: > > > Could you override View::GetAccessibleState(AXViewState* state) and set the > > > following: > > > state->role = ui::AX_ROLE_TOGGLE_BUTTON; > > > and > > > state->AddStateFlag(ui::AX_STATE_PRESSED); > > > if audio is muted > > > in addition to > > > state->name > > > > If I'm understanding the code correctly, it looks like all of that is handled > by > > views::ToggleImageButton::GetAccessibleState(), so I don't think that > > VolumeButton needs to override GetAccessibleState() too. Can you please > clarify? > > Are you hearing ChromeVox say "mute button, not pressed" and "mute button, > pressed"? And, are you seeing this spoken when you click the button? > > If not, then the VolumeButton is doing something else to indicate toggled vs > untoggled so you do need to handle it. Note the call to NotifyAccessibilityEvent > in ToggleImageButton::SetToggled. > > The core issue is that subclasses don't actually do the same thing to indicate > toggled or not, so the base impl is only a guess. Ideally, subclasses would > actually use ToggleImageButton::SetToggled. Another suggestion: if you want to avoid overriding GetAccessibleState, add another string "muted" and set that as the accessible name when the control is toggled. That only leaves ensuring ChromeVox reports the new state when you press enter or click the button.
The CQ bit was checked by tdanderson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
David, thanks for the feedback. I think I understand what you mean now. Please take a look at Patch Set 2.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Patched it in and looks good.
The CQ bit was checked by tdanderson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2234823003/#ps20001 (title: "override GetAccessibleState")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Make the mute button in the Ash system menu keyboard focusable Make the mute button in the Ash system menu keyboard focusable, and assign it the accessible name of "Mute" for ChromeVox spoken feedback. BUG=626464 TEST=manual ========== to ========== Make the mute button in the Ash system menu keyboard focusable Make the mute button in the Ash system menu keyboard focusable, and assign it the accessible name of "Mute" for ChromeVox spoken feedback. BUG=626464 TEST=manual Committed: https://crrev.com/61303af2fe0a016b69e2dfd1b8a752326d605d40 Cr-Commit-Position: refs/heads/master@{#411355} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/61303af2fe0a016b69e2dfd1b8a752326d605d40 Cr-Commit-Position: refs/heads/master@{#411355} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
