Show the password bubble even if the browser window has no focus.

Show the password bubble even if the browser window has no focus.

The bubble itself steals no focus. It's very important to show it for the new Credential Manager API. The user can be presented with the account chooser or the auto sign-in toast. The auto sign-in toast should stay visible while the browser isn't activated. Then it can be closed by timeout.

BUG=468281
Committed: https://crrev.com/bc82a04abd421cbe2ba76b0cb6899e34def04c2d
Cr-Commit-Position: refs/heads/master@{#321795}

[vasilii, 2015-03-18 13:15:28]

vasilii@chromium.org changed reviewers:
+ mkwst@chromium.org, sky@chromium.org

[vasilii, 2015-03-18 13:16:03]

Hi guys,

please review the CL.

[sky, 2015-03-18 16:04:17]

Will showing the password bubble take focus in anyway? If the browser window
doesn't have focus and you show the bubble, how is it closed?

[vasilii, 2015-03-18 16:08:17]

On 2015/03/18 16:04:17, sky wrote:
> Will showing the password bubble take focus in anyway? If the browser window
> doesn't have focus and you show the bubble, how is it closed?

No, the bubble is opened always without focus.
We have an observer implemented. It closes the bubble when the user clicks or
types in the web_contents.

[sky, 2015-03-18 20:57:46]

Doesn't the bubble also close if the hosting window loses focus? In
this case wouldn't that mean the bubble could be up for a very long
time?

On Wed, Mar 18, 2015 at 9:08 AM,  <vasilii@chromium.org> wrote:
> On 2015/03/18 16:04:17, sky wrote:
>>
>> Will showing the password bubble take focus in anyway? If the browser
>> window
>> doesn't have focus and you show the bubble, how is it closed?
>
>
> No, the bubble is opened always without focus.
> We have an observer implemented. It closes the bubble when the user clicks
> or
> types in the web_contents.
>
> https://codereview.chromium.org/1016003002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vasilii, 2015-03-19 09:12:52]

On 2015/03/18 20:57:46, sky wrote:
> Doesn't the bubble also close if the hosting window loses focus? In
> this case wouldn't that mean the bubble could be up for a very long
> time?
> 
> On Wed, Mar 18, 2015 at 9:08 AM,  <mailto:vasilii@chromium.org> wrote:
> > On 2015/03/18 16:04:17, sky wrote:
> >>
> >> Will showing the password bubble take focus in anyway? If the browser
> >> window
> >> doesn't have focus and you show the bubble, how is it closed?
> >
> >
> > No, the bubble is opened always without focus.
> > We have an observer implemented. It closes the bubble when the user clicks
> > or
> > types in the web_contents.
> >
> > https://codereview.chromium.org/1016003002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Indeed, the bubble closes when it loses focus. It can be up for a very long
time. For the auto sign-in bubble we have 5 seconds timeout. The account chooser
should stay til user selects a credential or dismisses the bubble.

[Mike West, 2015-03-19 12:52:33]

On 2015/03/19 at 09:12:52, vasilii wrote:
> On 2015/03/18 20:57:46, sky wrote:
> > Doesn't the bubble also close if the hosting window loses focus? In
> > this case wouldn't that mean the bubble could be up for a very long
> > time?
> > 
> > On Wed, Mar 18, 2015 at 9:08 AM,  <mailto:vasilii@chromium.org> wrote:
> > > On 2015/03/18 16:04:17, sky wrote:
> > >>
> > >> Will showing the password bubble take focus in anyway? If the browser
> > >> window
> > >> doesn't have focus and you show the bubble, how is it closed?
> > >
> > >
> > > No, the bubble is opened always without focus.
> > > We have an observer implemented. It closes the bubble when the user clicks
> > > or
> > > types in the web_contents.
> > >
> > > https://codereview.chromium.org/1016003002/
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Indeed, the bubble closes when it loses focus. It can be up for a very long
time. For the auto sign-in bubble we have 5 seconds timeout. The account chooser
should stay til user selects a credential or dismisses the bubble.

We should come back to the toast; the goal is to inform the user that her
credentials have been shared with a site. If a site waits until she's switched
to a different window (by popping up a window in front of the current window,
for instance), it could grab credentials without her knowledge if the toast
fades before she finishes working in the popup.

Perhaps we could start the timer once the window is active in a followup CL.

The behavior LGTM, but I defer to sky@ for the implementation.

[sky, 2015-03-19 19:24:39]

Seems to me if the user needs to see this toast then you need
something that makes sure it's stacked at the top of the screen.

  -Scott

On Thu, Mar 19, 2015 at 5:52 AM,  <mkwst@chromium.org> wrote:
> On 2015/03/19 at 09:12:52, vasilii wrote:
>>
>> On 2015/03/18 20:57:46, sky wrote:
>> > Doesn't the bubble also close if the hosting window loses focus? In
>> > this case wouldn't that mean the bubble could be up for a very long
>> > time?
>> >
>> > On Wed, Mar 18, 2015 at 9:08 AM,  <mailto:vasilii@chromium.org> wrote:
>> > > On 2015/03/18 16:04:17, sky wrote:
>> > >>
>> > >> Will showing the password bubble take focus in anyway? If the browser
>> > >> window
>> > >> doesn't have focus and you show the bubble, how is it closed?
>> > >
>> > >
>> > > No, the bubble is opened always without focus.
>> > > We have an observer implemented. It closes the bubble when the user
>> > > clicks
>> > > or
>> > > types in the web_contents.
>> > >
>> > > https://codereview.chromium.org/1016003002/
>> >
>> > To unsubscribe from this group and stop receiving emails from it, send
>> > an
>
> email
>>
>> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>> Indeed, the bubble closes when it loses focus. It can be up for a very
>> long
>
> time. For the auto sign-in bubble we have 5 seconds timeout. The account
> chooser
> should stay til user selects a credential or dismisses the bubble.
>
> We should come back to the toast; the goal is to inform the user that her
> credentials have been shared with a site. If a site waits until she's
> switched
> to a different window (by popping up a window in front of the current
> window,
> for instance), it could grab credentials without her knowledge if the toast
> fades before she finishes working in the popup.
>
> Perhaps we could start the timer once the window is active in a followup CL.
>
> The behavior LGTM, but I defer to sky@ for the implementation.
>
> https://codereview.chromium.org/1016003002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vasilii, 2015-03-19 20:03:43]

On 2015/03/19 19:24:39, sky wrote:
> Seems to me if the user needs to see this toast then you need
> something that makes sure it's stacked at the top of the screen.
> 
>   -Scott
> 
> On Thu, Mar 19, 2015 at 5:52 AM,  <mailto:mkwst@chromium.org> wrote:
> > On 2015/03/19 at 09:12:52, vasilii wrote:
> >>
> >> On 2015/03/18 20:57:46, sky wrote:
> >> > Doesn't the bubble also close if the hosting window loses focus? In
> >> > this case wouldn't that mean the bubble could be up for a very long
> >> > time?
> >> >
> >> > On Wed, Mar 18, 2015 at 9:08 AM,  <mailto:vasilii@chromium.org> wrote:
> >> > > On 2015/03/18 16:04:17, sky wrote:
> >> > >>
> >> > >> Will showing the password bubble take focus in anyway? If the browser
> >> > >> window
> >> > >> doesn't have focus and you show the bubble, how is it closed?
> >> > >
> >> > >
> >> > > No, the bubble is opened always without focus.
> >> > > We have an observer implemented. It closes the bubble when the user
> >> > > clicks
> >> > > or
> >> > > types in the web_contents.
> >> > >
> >> > > https://codereview.chromium.org/1016003002/
> >> >
> >> > To unsubscribe from this group and stop receiving emails from it, send
> >> > an
> >
> > email
> >>
> >> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> >
> >> Indeed, the bubble closes when it loses focus. It can be up for a very
> >> long
> >
> > time. For the auto sign-in bubble we have 5 seconds timeout. The account
> > chooser
> > should stay til user selects a credential or dismisses the bubble.
> >
> > We should come back to the toast; the goal is to inform the user that her
> > credentials have been shared with a site. If a site waits until she's
> > switched
> > to a different window (by popping up a window in front of the current
> > window,
> > for instance), it could grab credentials without her knowledge if the toast
> > fades before she finishes working in the popup.
> >
> > Perhaps we could start the timer once the window is active in a followup CL.
> >
> > The behavior LGTM, but I defer to sky@ for the implementation.
> >
> > https://codereview.chromium.org/1016003002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

What do you mean? We don't want to pop up an Os message box.
It's not only about the toast. The account chooser UI should be visible too.
Do you want us to come up with a completely different UI element?

[sky, 2015-03-19 23:07:30]

I think you need to raise the issue with UX and ask for input. If
showing the toast is important and the window isn't the foreground
than I suspect it shouldn't expire.

  -Scott

On Thu, Mar 19, 2015 at 1:03 PM,  <vasilii@chromium.org> wrote:
> On 2015/03/19 19:24:39, sky wrote:
>>
>> Seems to me if the user needs to see this toast then you need
>> something that makes sure it's stacked at the top of the screen.
>
>
>>    -Scott
>
>
>> On Thu, Mar 19, 2015 at 5:52 AM,  <mailto:mkwst@chromium.org> wrote:
>> > On 2015/03/19 at 09:12:52, vasilii wrote:
>> >>
>> >> On 2015/03/18 20:57:46, sky wrote:
>> >> > Doesn't the bubble also close if the hosting window loses focus? In
>> >> > this case wouldn't that mean the bubble could be up for a very long
>> >> > time?
>> >> >
>> >> > On Wed, Mar 18, 2015 at 9:08 AM,  <mailto:vasilii@chromium.org>
>> >> > wrote:
>> >> > > On 2015/03/18 16:04:17, sky wrote:
>> >> > >>
>> >> > >> Will showing the password bubble take focus in anyway? If the
>> >> > >> browser
>> >> > >> window
>> >> > >> doesn't have focus and you show the bubble, how is it closed?
>> >> > >
>> >> > >
>> >> > > No, the bubble is opened always without focus.
>> >> > > We have an observer implemented. It closes the bubble when the user
>> >> > > clicks
>> >> > > or
>> >> > > types in the web_contents.
>> >> > >
>> >> > > https://codereview.chromium.org/1016003002/
>> >> >
>> >> > To unsubscribe from this group and stop receiving emails from it,
>> >> > send
>> >> > an
>> >
>> > email
>> >>
>> >> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>> >
>> >
>> >> Indeed, the bubble closes when it loses focus. It can be up for a very
>> >> long
>> >
>> > time. For the auto sign-in bubble we have 5 seconds timeout. The account
>> > chooser
>> > should stay til user selects a credential or dismisses the bubble.
>> >
>> > We should come back to the toast; the goal is to inform the user that
>> > her
>> > credentials have been shared with a site. If a site waits until she's
>> > switched
>> > to a different window (by popping up a window in front of the current
>> > window,
>> > for instance), it could grab credentials without her knowledge if the
>> > toast
>> > fades before she finishes working in the popup.
>> >
>> > Perhaps we could start the timer once the window is active in a followup
>> > CL.
>> >
>> > The behavior LGTM, but I defer to sky@ for the implementation.
>> >
>> > https://codereview.chromium.org/1016003002/
>
>
>> To unsubscribe from this group and stop receiving emails from it, send an
>
> email
>>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
> What do you mean? We don't want to pop up an Os message box.
> It's not only about the toast. The account chooser UI should be visible too.
> Do you want us to come up with a completely different UI element?
>
> https://codereview.chromium.org/1016003002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vasilii, 2015-03-20 16:16:18]

I changed the CL. Now we don't close the auto sign-in toast by timeout if the
browser isn't active.
Please review.

[Mike West, 2015-03-20 16:31:17]

On 2015/03/20 at 16:16:18, vasilii wrote:
> I changed the CL. Now we don't close the auto sign-in toast by timeout if the
browser isn't active.
> Please review.

This sounds pretty reasonable to me, and the implementation LGTM.

[vasilii, 2015-03-20 17:11:22]

On 2015/03/20 16:31:17, Mike West wrote:
> On 2015/03/20 at 16:16:18, vasilii wrote:
> > I changed the CL. Now we don't close the auto sign-in toast by timeout if
the
> browser isn't active.
> > Please review.
> 
> This sounds pretty reasonable to me, and the implementation LGTM.

The test didn't pass. I hope I fixed it.

[sky, 2015-03-20 19:52:26]

https://codereview.chromium.org/1016003002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1016003002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:271: };
niT: DISALLOW...

https://codereview.chromium.org/1016003002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:287: Browser*
browser =
Because this is all async is it possible browser is null?

https://codereview.chromium.org/1016003002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:305: if
(active)
if (active && !timer_.IsRunning()) ?

[vasilii, 2015-03-23 09:27:51]

https://codereview.chromium.org/1016003002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1016003002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:271: };
On 2015/03/20 19:52:26, sky wrote:
> niT: DISALLOW...

Done.

https://codereview.chromium.org/1016003002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:287: Browser*
browser =
On 2015/03/20 19:52:26, sky wrote:
> Because this is all async is it possible browser is null?

I don't think so. If it happens then everything crashes much earlier in
BrowserCommandController::ExecuteCommandWithDisposition(). This constructor is
actually synchronous.

https://codereview.chromium.org/1016003002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:305: if
(active)
On 2015/03/20 19:52:26, sky wrote:
> if (active && !timer_.IsRunning()) ?

Done.

[sky, 2015-03-23 15:56:50]

LGTM

[vasilii, 2015-03-23 16:16:25]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2015-03-23 16:16:25]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1016003002/#ps60001
(title: "addressed the comments from sky@")

[commit-bot: I haz the power, 2015-03-23 16:16:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1016003002/60001

[commit-bot: I haz the power, 2015-03-23 17:01:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-23 17:02:12]

Patchset 4 (id:??) landed as
https://crrev.com/bc82a04abd421cbe2ba76b0cb6899e34def04c2d
Cr-Commit-Position: refs/heads/master@{#321795}