Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
471703002:
[Mac] Add accessibility features to the new avatar menu (Closed)
|
Created:
6 years, 4 months ago by noms (inactive) Modified:
6 years, 4 months ago Reviewers:
groby-ooo-7-16 CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
Description[Mac] Add accessibility features to the new avatar menu
BUG=401834
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290055
Patch Set 1 : #
Total comments: 15
Patch Set 2 : rebase #Patch Set 3 : rachel comments #
Total comments: 2
Patch Set 4 : fix a bunch of rebase mishaps >_< #
Messages
Total messages: 15 (0 generated)
Hi Rachel, I've started adding accessibility features to the new avatar menu. Everything works great with VoiceOver on, but despite all the controls setting -canBecomeKeyView:YES, I can't actually tab through any of them. I think I am missing something re: making things tabbable :(
Is the Avatar thingy by any chance a bubble? In which case it's a window, in which case it needs to actively participate in key handling :) See https://codereview.chromium.org/265693009/ or https://codereview.chromium.org/378693003 :)
Hmm, it is a bubble, but just adding that won't help. Would you mind reviewing this as it is, and I will add the tabbing in a separate CL? It has strings, so I'd like to get it in by tomorrow :(
Sorry for the flood of comments :( https://codereview.chromium.org/471703002/diff/20001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/471703002/diff/20001/chrome/app/generated_res... chrome/app/generated_resources.grd:11216: + <message name="IDS_PROFILES_NEW_AVATAR_MENU_ACCESSIBLE_NAME" desc="Title of the new avatar bubble menu for accessibility"> These seem to be _new_ strings. Wasn't string freeze two weeks ago? Are UI-leads good with this? https://codereview.chromium.org/471703002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right): https://codereview.chromium.org/471703002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:20: #include "ui/base/l10n/l10n_util.h" Do you need this? https://codereview.chromium.org/471703002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:276: - (void)updateAccessibilityValue:(BOOL)hasError { That's a horrible name, sorry. How about setAccessibleAccountButtonTitle, or something that makes clear _what_ value to set? https://codereview.chromium.org/471703002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:277: base::string16 buttonTitle = base::SysNSStringToUTF16([button_ title]); Silly question - why get this outside the if(), since that's the only place it's used? https://codereview.chromium.org/471703002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:280: accessibilitySetOverrideValue:l10n_util::GetNSStringF( Why not do all this magic in the cell - that way it's guaranteed to happen when you change the error status? https://codereview.chromium.org/471703002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/471703002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:463: - (void)accessibilityPerformAction:(NSString*)action; No need to declare any of them - they're all implemented by subclasses. https://codereview.chromium.org/471703002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:708: - (BOOL)acceptsFirstResponder { Technically, a control's -acceptsFirstResponder is driven by the cell's -refusesFirstResponder. (And that usually results in YES). Are you sure you need these?
Also, yes, I'm fine with implementing tabbing later - but how do you test this works?
Enable VoiceOver (In the accessibility menu), and them Cmd+F5 turns it on (and most importantly turns it off. It's super noisy, but you can tab between things in Chrome and it reads them out to you! On 2014/08/15 00:39:02, groby wrote: > Also, yes, I'm fine with implementing tabbing later - but how do you test this > works?
Accessibility tabbing works, normal tabbing doesn't. Sigh, bubbles :( On 2014/08/15 00:57:36, Monica Dinculescu wrote: > Enable VoiceOver (In the accessibility menu), and them Cmd+F5 turns it on (and > most importantly turns it off. It's super noisy, but you > can tab between things in Chrome and it reads them out to you! > > On 2014/08/15 00:39:02, groby wrote: > > Also, yes, I'm fine with implementing tabbing later - but how do you test this > > works?
Wait, what? How is accessibility tabbing different from normal tabbing? (I hate bubbles. Everybody hates bubbles! ;)
So yeah, accessibility tabbing works differently. I think VoiceOver is a wizard and knows about all the native controls in all the things, whether you want it or not. Which is great. Regular tabbing: still postponed (mostly because I really want these strings in) https://codereview.chromium.org/471703002/diff/20001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/471703002/diff/20001/chrome/app/generated_res... chrome/app/generated_resources.grd:11216: + <message name="IDS_PROFILES_NEW_AVATAR_MENU_ACCESSIBLE_NAME" desc="Title of the new avatar bubble menu for accessibility"> My understand was that feature freeze was 2 days ago, string freeze is...today :/ On 2014/08/15 00:38:35, groby wrote: > These seem to be _new_ strings. Wasn't string freeze two weeks ago? Are UI-leads > good with this? https://codereview.chromium.org/471703002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right): https://codereview.chromium.org/471703002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:20: #include "ui/base/l10n/l10n_util.h" On 2014/08/15 00:38:35, groby wrote: > Do you need this? Done. https://codereview.chromium.org/471703002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:276: - (void)updateAccessibilityValue:(BOOL)hasError { On 2014/08/15 00:38:35, groby wrote: > That's a horrible name, sorry. How about setAccessibleAccountButtonTitle, or > something that makes clear _what_ value to set? Acknowledged. https://codereview.chromium.org/471703002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:277: base::string16 buttonTitle = base::SysNSStringToUTF16([button_ title]); No good reason On 2014/08/15 00:38:35, groby wrote: > Silly question - why get this outside the if(), since that's the only place it's > used? https://codereview.chromium.org/471703002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:280: accessibilitySetOverrideValue:l10n_util::GetNSStringF( Hmm, because the cell didn't have the title but that was not actually a thing. Done! On 2014/08/15 00:38:35, groby wrote: > Why not do all this magic in the cell - that way it's guaranteed to happen when > you change the error status? https://codereview.chromium.org/471703002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/471703002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:463: - (void)accessibilityPerformAction:(NSString*)action; Argh, sorry. I never know when to declare and when to not declare methods that come from Cocoa. Is it "Cocoa.h declares everything so you never need to declare anything in there?" :( On 2014/08/15 00:38:35, groby wrote: > No need to declare any of them - they're all implemented by subclasses. https://codereview.chromium.org/471703002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:708: - (BOOL)acceptsFirstResponder { No. I think I just hoped these would make regular tabbing work. They don't. On 2014/08/15 00:38:35, groby wrote: > Technically, a control's -acceptsFirstResponder is driven by the cell's > -refusesFirstResponder. (And that usually results in YES). > > Are you sure you need these?
LGTM w/ nit https://codereview.chromium.org/471703002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/471703002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:463: - (void)accessibilityPerformAction:(NSString*)action; On 2014/08/15 19:26:47, Monica Dinculescu wrote: > Argh, sorry. I never know when to declare and when to not declare methods that > come from Cocoa. Is it "Cocoa.h declares everything so you never need to declare > anything in there?" :( > > On 2014/08/15 00:38:35, groby wrote: > > No need to declare any of them - they're all implemented by subclasses. > This is one of the few places ObjC pretends to not be verbose. If a subclass declared it, you're good to go. (Not that ObjC is fooling anybody there...) https://codereview.chromium.org/471703002/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/471703002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1601: // TODO(noms): Use the correct incognito icon when it's available. nit: Pleaze to be filing a bug?
https://codereview.chromium.org/471703002/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/471703002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1601: // TODO(noms): Use the correct incognito icon when it's available. Gah, sorry, bad rebase. Right icon is already in use. Done. On 2014/08/15 19:52:07, groby wrote: > nit: Pleaze to be filing a bug?
The CQ bit was checked by noms@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/471703002/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
Message was sent while issue was closed.
Committed patchset #4 (100001) as 290055 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
