Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
890143005:
[Mac] Allow "Enter" to activate the buttons in the new avatar bubble. (Closed)
|
Created:
5 years, 10 months ago by noms (inactive) Modified:
5 years, 10 months ago Reviewers:
groby-ooo-7-16 CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Mac] Allow "Enter" to activate the buttons in the new avatar bubble.
Despite this not being the OSX way, users are complaining that Enter doesn't
activate the buttons in the menu, because it used to. I think the easiest
way is to wrap all buttons in a view that responds to Enter, and activates
the button accordingly.
BUG=454467
TEST=Start Chrome with --enable-new-avatar-menu. Open the avatar bubble and
tab through items. Pressing enter should perform the button's action.
Committed: https://crrev.com/7286b9d038d6d9f95578ced6de7ebaa788970d1d
Cr-Commit-Position: refs/heads/master@{#314265}
Patch Set 1 #Patch Set 2 : unbreak tests #
Total comments: 4
Patch Set 3 : simplify code #Messages
Total messages: 18 (4 generated)
noms@chromium.org changed reviewers: + groby@chromium.org
Hiya, We get a surprising amount of "Enter doesn't work so keyboard wasn't tested" comments for the new avatar bubble. Since buttons don't really want to all respond to Enter, I've wrapped the ones that should get activated by Enter into an NSView, which knows how to handle it on -keyDown. Please take a look. Thanks! Bet you missed them avatar bubble CLs! :)
I really hate to be that person, but this completely violates OSX UI guidelines. It's *wrong*. Quoth the HIG[1]: "Usually the rightmost button or the Cancel button is the default button. A default button has color to let users know that when they press Return or Enter, the default button is activated." So, my apologies, but NOT LGTM :( [1] https://developer.apple.com/library/mac/documentation/UserExperience/Conceptu...
If we're considering key accessibility: What about * Always having a focused item * Making "edit" key-accessible * Making the picture-taking accessible? https://codereview.chromium.org/890143005/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/890143005/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:801: @interface BackgroundColorHoverButtonContainer : NSView { Why not just subclass BackgroundColorHoverButton, and only override keyDown? Saves us from adding more views to the hierarchy. https://codereview.chromium.org/890143005/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:821: // On enter, activate the wrapped button. Do we care about the state of modifier keys? Menus don't, so this is correct - but I wanted to make sure it's intentional :)
Oh, and withdrawing the NOT part above.
> * Always having a focused item UI is pretty against this as it looks ugly-ish (the blue border looks very jarring) > * Making "edit" key-accessible > * Making the picture-taking accessible? These were originally both accessible. We took them off so that within one tab you get to Switch Person, and thus have to do less tabbing to switch users (otherwise you'd have a minimum of two tabs, and photo/name editing is a very, very rare action). The other alternative would be to start tabbing in the middle of the list of buttons, and that's kind of a really weird experience too. > https://codereview.chromium.org/890143005/diff/20001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): > > https://codereview.chromium.org/890143005/diff/20001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:801: @interface > BackgroundColorHoverButtonContainer : NSView { > Why not just subclass BackgroundColorHoverButton, and only override keyDown? > Saves us from adding more views to the hierarchy. > > https://codereview.chromium.org/890143005/diff/20001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:821: // On enter, > activate the wrapped button. > Do we care about the state of modifier keys? Menus don't, so this is correct - > but I wanted to make sure it's intentional :)
On 2015/02/02 22:05:01, Monica Dinculescu wrote: > > * Always having a focused item > UI is pretty against this as it looks ugly-ish (the blue border looks very > jarring) If our focus looks jarring, maybe we should make it less jarring? ;) > > * Making "edit" key-accessible > > * Making the picture-taking accessible? > > These were originally both accessible. We took them off so that within one tab > you get to Switch Person, > and thus have to do less tabbing to switch users (otherwise you'd have a minimum > of two tabs, and > photo/name editing is a very, very rare action). The other alternative would be > to start tabbing > in the middle of the list of buttons, and that's kind of a really weird > experience too. I assure you, when you're vision impaired the experience is much weirder without keyboard accessibility. (Yes, not this CL - but *please* keep this in mind) > > > > https://codereview.chromium.org/890143005/diff/20001/chrome/browser/ui/cocoa/... > > File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): > > > > > https://codereview.chromium.org/890143005/diff/20001/chrome/browser/ui/cocoa/... > > chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:801: @interface > > BackgroundColorHoverButtonContainer : NSView { > > Why not just subclass BackgroundColorHoverButton, and only override keyDown? > > Saves us from adding more views to the hierarchy. > > > > > https://codereview.chromium.org/890143005/diff/20001/chrome/browser/ui/cocoa/... > > chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:821: // On > enter, > > activate the wrapped button. > > Do we care about the state of modifier keys? Menus don't, so this is correct - > > but I wanted to make sure it's intentional :)
https://codereview.chromium.org/890143005/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/890143005/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:801: @interface BackgroundColorHoverButtonContainer : NSView { On 2015/02/02 21:57:33, groby wrote: > Why not just subclass BackgroundColorHoverButton, and only override keyDown? > Saves us from adding more views to the hierarchy. Ah, duh. I misunderstood my old CL when I tried to do this (but that used performKeyEquivalent) Don't even need to subclass it, all these buttons should respond to enter. https://codereview.chromium.org/890143005/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:821: // On enter, activate the wrapped button. On 2015/02/02 21:57:33, groby wrote: > Do we care about the state of modifier keys? Menus don't, so this is correct - > but I wanted to make sure it's intentional :) I don't think we do, nope.
The CQ bit was checked by groby@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/890143005/40001
On 2015/02/03 00:57:25, groby wrote: > lgtm Woooo thank you!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (None)
The CQ bit was checked by noms@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/890143005/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7286b9d038d6d9f95578ced6de7ebaa788970d1d Cr-Commit-Position: refs/heads/master@{#314265} |
