Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
553133002:
[Mac] Add text-editing accelerators to the User Manager. (Closed)
|
Created:
6 years, 3 months ago by noms (inactive) Modified:
6 years, 3 months ago Reviewers:
groby-ooo-7-16 CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[Mac] Add text-editing accelerators to the User Manager.
More specifically, add Cmd+A and Cmd+V for making entering the password less sucky.
BUG=394575
TEST=Start Chrome with --enable-new-profile-management. Sign in a profile,
and from the avatar bubble, click on "Lock. In the User Manager that
shows up, Chrome shortcuts like Cmd+N and Cmd+T shouldn't work, but in
the password field of the locked pod things like Cmd+A and Cmd+V should.
Committed: https://crrev.com/6a1af5b47462c0ffd3e79041ceeff68bab395b84
Cr-Commit-Position: refs/heads/master@{#295580}
Patch Set 1 : #
Total comments: 5
Patch Set 2 : windows change not needed :/ #
Total comments: 1
Patch Set 3 : rebase #Patch Set 4 : possibly moar better? #
Total comments: 3
Patch Set 5 : fix whitespace #
Total comments: 2
Patch Set 6 : moar better fo' sho' #
Total comments: 2
Patch Set 7 : fixed comment and useless include #Patch Set 8 : fix include #Messages
Total messages: 21 (5 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
noms@chromium.org changed reviewers: + groby@chromium.org, sky@chromium.org
Hi Rachel and Scott, This CL allows non-chrome accelerators to trickle down to the User Manager's web view. I hope this makes sense -- I've tested it and it does do what it's supposed to (allows things like Ctrl+A and C, doesn't allow Ctrl+N or Ctrl+T), but it looks too simple to be right. Please take a look. Thanks!
https://codereview.chromium.org/553133002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/user_manager_view.cc (right): https://codereview.chromium.org/553133002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/user_manager_view.cc:177: if (accelerator.key_code() == ui::VKEY_W) { How do you end up here for other accelerator types? AFAICT you only register control-w.
https://codereview.chromium.org/553133002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/553133002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:63: if (commandId == -1 || commandId == IDC_CLOSE_WINDOW || No support for IDC_CLOSE_TAB? (which is cmd-w. CLOSE_WINDOW is cmd-shift-w, IIRC)
https://codereview.chromium.org/553133002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/553133002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:63: if (commandId == -1 || commandId == IDC_CLOSE_WINDOW || According to https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... It looks like Cmd+W. (Also, Cmd+W works, Cmd+Shit+W doesn't work in the User Manager) On 2014/09/10 16:38:21, groby wrote: > No support for IDC_CLOSE_TAB? (which is cmd-w. CLOSE_WINDOW is cmd-shift-w, > IIRC) https://codereview.chromium.org/553133002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/user_manager_view.cc (right): https://codereview.chromium.org/553133002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/user_manager_view.cc:177: if (accelerator.key_code() == ui::VKEY_W) { Oh weird, you're right. This seems to work without any changes. I probably had broken it first, which is why this no-op appeared to work. Thanks for catching it :/ On 2014/09/10 16:21:15, sky wrote: > How do you end up here for other accelerator types? AFAICT you only register > control-w.
I tried to run locally, but of course "service is not available" when I want to log in. Is there a secret invocation needed to make sync work, since it's not Chrome but Chromium? Also, a bunch of questions: 1) Oddly, even with the flag, I don't get new profiles. chrome://flags shows it in the "default" state. Forcing it via flags works, though. 2) I can still create profiles that are not signed in. Do I not get to lock these? I didn't see a lock entry in the avatar bubble. 3) Should menu entries be greyed out if they don't work? 4) Have you tried if the tab context menu for "close tab" is disabled? 4) What happens with other menu entries - specifically, bookmarks? https://codereview.chromium.org/553133002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/553133002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:63: if (commandId == -1 || commandId == IDC_CLOSE_WINDOW || On 2014/09/10 16:55:09, Monica Dinculescu wrote: > According to > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > It looks like Cmd+W. (Also, Cmd+W works, Cmd+Shit+W doesn't work in the User > Manager) Sigh. It turns out that the "Close window" entry in the OSX menu doesn't have a command attached - https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... Cmd-W is definitely "Close Tab", but you're right, it's mapped to IDC_CLOSE_WINDOW. Why that works is beyond me... > > On 2014/09/10 16:38:21, groby wrote: > > No support for IDC_CLOSE_TAB? (which is cmd-w. CLOSE_WINDOW is cmd-shift-w, > > IIRC) > https://codereview.chromium.org/553133002/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/553133002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:68: [window_ redispatchKeyEvent:event.os_event]; Technically, valid commandIds should be handled via [[NSApp mainMenu] performKeyEquivalent:event] The difference is subtle - if you redispatch to |window_|, the event is first offered to performKeyEquivalent on |window_|. If that can't handle the event, it's handed off to [NSApp mainMenu]. That only matters if there's an extension command handler for the key event - which means extension keypresses are still allowed through. I don't think you want that. I love key handling ;(
Answers inline! On 2014/09/10 20:06:40, groby wrote: > I tried to run locally, but of course "service is not available" when I want to > log in. Is there a secret invocation needed to make sync work, since it's not > Chrome but Chromium? Yup! You need to set up your API keys. Sorry! Here are the instructions: http://www.chromium.org/developers/how-tos/api-keys > 1) Oddly, even with the flag, I don't get new profiles. chrome://flags shows it > in the "default" state. Forcing it via flags works, though. For things like Canary/Beta we force push the flag by default, through Finch. This means you need to explicitly set it to "Enabled" in your local build (or pass --enable-new-profile-management) > 2) I can still create profiles that are not signed in. Do I not get to lock > these? I didn't see a lock entry in the avatar bubble. Correct. Locking requires you to unlock them with your Gaia password. If you're not signed in, you don't have that. We're not adding a pin or anything like that at the moment either... > 3) Should menu entries be greyed out if they don't work? If Locking isn't available, then the button doesn't appear in the avatar bubble. If you are seeing it greyed out, there's a CL by mlerman@ that fixed it recently (it was because we don't have your password hash stored. You might have to disconnect and reconnect your sync account to ungrey it out. This is still in development so we don't have the greatest error ui right now) > 4) Have you tried if the tab context menu for "close tab" is disabled? > 4) What happens with other menu entries - specifically, bookmarks? The context menu for the web contents is disabled altogether. I don't know what you mean by other menu entries... :( > https://codereview.chromium.org/553133002/diff/40001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): > > https://codereview.chromium.org/553133002/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:63: if (commandId == -1 || > commandId == IDC_CLOSE_WINDOW || > On 2014/09/10 16:55:09, Monica Dinculescu wrote: > > According to > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > It looks like Cmd+W. (Also, Cmd+W works, Cmd+Shit+W doesn't work in the User > > Manager) > > Sigh. It turns out that the "Close window" entry in the OSX menu doesn't have a > command attached - > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > Cmd-W is definitely "Close Tab", but you're right, it's mapped to > IDC_CLOSE_WINDOW. Why that works is beyond me... > > > > On 2014/09/10 16:38:21, groby wrote: > > > No support for IDC_CLOSE_TAB? (which is cmd-w. CLOSE_WINDOW is cmd-shift-w, > > > IIRC) > > > > https://codereview.chromium.org/553133002/diff/60001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): > > https://codereview.chromium.org/553133002/diff/60001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:68: [window_ > redispatchKeyEvent:event.os_event]; > Technically, valid commandIds should be handled via [[NSApp mainMenu] > performKeyEquivalent:event] > > The difference is subtle - if you redispatch to |window_|, the event is first > offered to performKeyEquivalent on |window_|. If that can't handle the event, > it's handed off to [NSApp mainMenu]. > > That only matters if there's an extension command handler for the key event - > which means extension keypresses are still allowed through. I don't think you > want that. > > I love key handling ;(
noms@chromium.org changed reviewers: - sky@chromium.org
> or pass --enable-new-profile-management Nope. Doesn't work. Only forcing it in chrome://flags works. >> 3) Should menu entries be greyed out if they don't work? I mean entries in the application menu. I.e. in File/Edit/View/History/Bookmarks.... It'd look odd if i.e. Bookmarks entries are still active, but get ignored. > don't know what you mean by other menu entries... Entries in the application menu. I'm not sure what'd happen if you hit a bookmark menu entry while the bubble is up. I'd try, but I can't log in. On Wed, Sep 10, 2014 at 1:14 PM, <noms@chromium.org> wrote: > Answers inline! > > On 2014/09/10 20:06:40, groby wrote: > >> I tried to run locally, but of course "service is not available" when I >> want >> > to > >> log in. Is there a secret invocation needed to make sync work, since it's >> not >> Chrome but Chromium? >> > Yup! You need to set up your API keys. Sorry! Here are the instructions: > http://www.chromium.org/developers/how-tos/api-keys > > 1) Oddly, even with the flag, I don't get new profiles. chrome://flags >> shows >> > it > >> in the "default" state. Forcing it via flags works, though. >> > For things like Canary/Beta we force push the flag by default, through > Finch. > This > means you need to explicitly set it to "Enabled" in your local build (or > pass > --enable-new-profile-management) > > 2) I can still create profiles that are not signed in. Do I not get to >> lock >> these? I didn't see a lock entry in the avatar bubble. >> > Correct. Locking requires you to unlock them with your Gaia password. If > you're > not signed in, you don't have that. We're not adding a pin or anything > like that > > at the moment either... > > 3) Should menu entries be greyed out if they don't work? >> > If Locking isn't available, then the button doesn't appear in the avatar > bubble. > If you are seeing it greyed out, there's a CL by mlerman@ that fixed it > recently > (it was because we don't have your password hash stored. You might have to > disconnect and reconnect your sync account to ungrey it out. This is still > in > development so we don't have the greatest error ui right now) > > 4) Have you tried if the tab context menu for "close tab" is disabled? >> 4) What happens with other menu entries - specifically, bookmarks? >> > The context menu for the web contents is disabled altogether. I don't know > what > you > mean by other menu entries... :( > > > > > > https://codereview.chromium.org/553133002/diff/40001/ > chrome/browser/ui/cocoa/profiles/user_manager_mac.mm > >> File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): >> > > > https://codereview.chromium.org/553133002/diff/40001/ > chrome/browser/ui/cocoa/profiles/user_manager_mac.mm#newcode63 > >> chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:63: if (commandId >> == -1 >> > || > >> commandId == IDC_CLOSE_WINDOW || >> On 2014/09/10 16:55:09, Monica Dinculescu wrote: >> > According to >> > >> > > https://code.google.com/p/chromium/codesearch#chromium/ > src/chrome/browser/ui/cocoa/accelerators_cocoa.mm&l=69 > >> > It looks like Cmd+W. (Also, Cmd+W works, Cmd+Shit+W doesn't work in the >> User >> > Manager) >> > > Sigh. It turns out that the "Close window" entry in the OSX menu doesn't >> have >> > a > >> command attached - >> > > https://code.google.com/p/chromium/codesearch#chromium/ > src/chrome/browser/ui/cocoa/accelerators_cocoa.mm&l=24 > > Cmd-W is definitely "Close Tab", but you're right, it's mapped to >> IDC_CLOSE_WINDOW. Why that works is beyond me... >> > >> > On 2014/09/10 16:38:21, groby wrote: >> > > No support for IDC_CLOSE_TAB? (which is cmd-w. CLOSE_WINDOW is >> > cmd-shift-w, > >> > > IIRC) >> > >> > > > https://codereview.chromium.org/553133002/diff/60001/ > chrome/browser/ui/cocoa/profiles/user_manager_mac.mm > >> File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): >> > > > https://codereview.chromium.org/553133002/diff/60001/ > chrome/browser/ui/cocoa/profiles/user_manager_mac.mm#newcode68 > >> chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:68: [window_ >> redispatchKeyEvent:event.os_event]; >> Technically, valid commandIds should be handled via [[NSApp mainMenu] >> performKeyEquivalent:event] >> > > The difference is subtle - if you redispatch to |window_|, the event is >> first >> offered to performKeyEquivalent on |window_|. If that can't handle the >> event, >> it's handed off to [NSApp mainMenu]. >> > > That only matters if there's an extension command handler for the key >> event - >> which means extension keypresses are still allowed through. I don't think >> you >> want that. >> > > I love key handling ;( >> > > > https://codereview.chromium.org/553133002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/553133002/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/553133002/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:68: isTextEditingCommand = cmdKey && (keyChar == 'a' || keyChar == 'v'); So I don't like this very much, but ui::VKEY_A has a different value than 'a' because Mac and Windows. Is there a less gross way? I can't find non-test examples of this anywhere in Chrome. :( Key event code is not my favourite code.
https://codereview.chromium.org/553133002/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/553133002/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:68: isTextEditingCommand = cmdKey && (keyChar == 'a' || keyChar == 'v'); On 2014/09/17 19:45:46, Monica Dinculescu wrote: > So I don't like this very much, but ui::VKEY_A has a different value than 'a' > because Mac and Windows. Yes, there is - If you only had the NSEvent, KeyboardCodeFromNSEvent But you have a NativeWebKeyboardEvent - event.windowsKeyCode works well. (Or should, at least ;) > Is there a less gross way? I can't find non-test examples of this anywhere in > Chrome. :( > > Key event code is not my favourite code. I'm so tempted to mumble something about "in my day", "contact chatter", and lawns to get off of :)
https://codereview.chromium.org/553133002/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/553133002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:68: isTextEditingCommand = cmdKey && (keyChar == 'a' || keyChar == 'v'); nit: Get rid of modifiers and cmdKey, just the test for command key here. You can even sidestep the whole platform side of things and just test (event.modifiers & WebInputEvent::MetaKey) Sorry for not mentioning that earlier.
https://codereview.chromium.org/553133002/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/553133002/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:68: isTextEditingCommand = cmdKey && (keyChar == 'a' || keyChar == 'v'); Gah, I should've looked at NativeWebKeyboardEvent. Sorry :( On 2014/09/17 21:42:44, groby wrote: > On 2014/09/17 19:45:46, Monica Dinculescu wrote: > > So I don't like this very much, but ui::VKEY_A has a different value than 'a' > > because Mac and Windows. > > Yes, there is - If you only had the NSEvent, KeyboardCodeFromNSEvent > > But you have a NativeWebKeyboardEvent - event.windowsKeyCode works well. (Or > should, at least ;) > > > Is there a less gross way? I can't find non-test examples of this anywhere in > > Chrome. :( > > > > Key event code is not my favourite code. > > I'm so tempted to mumble something about "in my day", "contact chatter", and > lawns to get off of :) https://codereview.chromium.org/553133002/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/553133002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:68: isTextEditingCommand = cmdKey && (keyChar == 'a' || keyChar == 'v'); On 2014/09/18 06:35:11, groby wrote: > nit: Get rid of modifiers and cmdKey, just the test for command key here. You > can even sidestep the whole platform side of things and just test > (event.modifiers & WebInputEvent::MetaKey) > > Sorry for not mentioning that earlier. AMAZING! Done.
LGTM % nit https://codereview.chromium.org/553133002/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/553133002/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:18: #include "third_party/WebKit/public/web/WebInputEvent.h" nit: transitively included by native_web_keyboard_event.h, and its base class. I don't think it's needed.
Yay! Thanks so much! https://codereview.chromium.org/553133002/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/553133002/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:18: #include "third_party/WebKit/public/web/WebInputEvent.h" On 2014/09/18 20:59:56, groby wrote: > nit: transitively included by native_web_keyboard_event.h, and its base class. I > don't think it's needed. Done.
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/553133002/180001
Message was sent while issue was closed.
Committed patchset #8 (id:180001) as f46d2429b17dca0aa141db00b0b2afa1cd2d3a06
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/6a1af5b47462c0ffd3e79041ceeff68bab395b84 Cr-Commit-Position: refs/heads/master@{#295580} |
