Password "key" icon in the omnibox should be focusable. If the bubble is open then it automatically…

Password "key" icon in the omnibox should be focusable. If the bubble is open then it automatically passes its focus to the bubble.

BUG=392835
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287768

[vasilii, 2014-08-05 16:15:41]

Hi Mike,

One more patch for you please.

[Mike West, 2014-08-05 16:41:55]

Thanks. One thing to check.

https://codereview.chromium.org/440903002/diff/1/chrome/browser/ui/views/pass...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right):

https://codereview.chromium.org/440903002/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:162: // Makes
the bubble foreground window.
Nit: "the foreground window"

https://codereview.chromium.org/440903002/diff/1/chrome/browser/ui/views/pass...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (right):

https://codereview.chromium.org/440903002/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:95: if
(active())
You need to check reverse here. Otherwise shift-tab is broken.

[Mike West, 2014-08-05 16:42:24]

https://codereview.chromium.org/440903002/diff/1/chrome/browser/ui/views/pass...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (right):

https://codereview.chromium.org/440903002/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:23:
SetFocusable(true);
What's the difference?

[vasilii, 2014-08-06 09:14:01]

https://codereview.chromium.org/440903002/diff/1/chrome/browser/ui/views/pass...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right):

https://codereview.chromium.org/440903002/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:162: // Makes
the bubble foreground window.
On 2014/08/05 16:41:55, Mike West wrote:
> Nit: "the foreground window"

Done.

https://codereview.chromium.org/440903002/diff/1/chrome/browser/ui/views/pass...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (right):

https://codereview.chromium.org/440903002/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:23:
SetFocusable(true);
On 2014/08/05 16:42:24, Mike West wrote:
> What's the difference?

AccessibilityFocusable means focusable for accessibility user's only. Now it's
just focusable for everybody.

https://codereview.chromium.org/440903002/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:95: if
(active())
On 2014/08/05 16:41:55, Mike West wrote:
> You need to check reverse here. Otherwise shift-tab is broken.

Shift-Tab is fine. The focus goes to the bubble to the default button.

[Mike West, 2014-08-06 09:17:46]

https://codereview.chromium.org/440903002/diff/1/chrome/browser/ui/views/pass...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (right):

https://codereview.chromium.org/440903002/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:95: if
(active())
On 2014/08/06 09:14:01, vasilii wrote:
> On 2014/08/05 16:41:55, Mike West wrote:
> > You need to check reverse here. Otherwise shift-tab is broken.
> 
> Shift-Tab is fine. The focus goes to the bubble to the default button.

If I shift-tab out of the bubble, what happens? Perhaps I've misunderstood the
tab order, but if it's something like:

Omnibox Button -> Bubble

Then tabbing out of the bubble would land on the omnibox icon, which would put
me back in the bubble, right?

[Mike West, 2014-08-06 09:25:15]

LGTM.

As discussed, I think we need to verify that there's some sort of announcement
to the user that they've entered a modal context by tabbing into the bubble.
CCing Alice who might have some helpful tips for us in that regard.

I'll send you an email separately, Alice. :)

[vasilii, 2014-08-06 09:26:33]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-08-06 09:27:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vasilii@chromium.org/440903002/20001

[commit-bot: I haz the power, 2014-08-06 09:41:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/37916)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/43116)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-06 09:42:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-06 09:42:13]

Try jobs failed on following builders:
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[vasilii, 2014-08-06 09:48:08]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-08-06 09:49:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vasilii@chromium.org/440903002/40001

[commit-bot: I haz the power, 2014-08-06 13:08:50]

Change committed as 287768