Suppress Autofill popup if it would be outside the viewport.

Suppress Autofill popup if it would be outside the viewport.

This is not a problem for the usual autofill popup, since th focused field will move the viewport. However, it is relevant to the separate "Form Not Secure" popup that is shown for autofilled fields on HTTP pages at page load.

TEST=
1) Enable chrome://flags#enable-http-form-warning
2) Store a password on http://rsolomakhin.github.io/autofill/ under "Name/Password"
3) Reload the page with so that the password field and credit card field are both in the viewport. Verify that "Login Not Secure" appears (see just-inside-viewport-bottom.png at crbug.com/678713#c11)
4) Resize the browser so that the password field is below the viewport and reload. Verify that "Login Not Secure" does *not* (see just-outside-viewport-bottom.png at crbug.com/678713#c11)

BUG=678713

[lgarron, 2017-01-17 23:37:31]

Description was changed from

==========
Suppress Autofill popup if it would be outside the viewport.

This is not a problem for the usual autofill popup, since th focused field will
move the viewport. However, it is relevant to the separate "Form Not Secure"
popup that is shown for autofilled fields on HTTP pages at page load.

BUG=678713
==========

to

==========
Suppress Autofill popup if it would be outside the viewport.

This is not a problem for the usual autofill popup, since th focused field will
move the viewport. However, it is relevant to the separate "Form Not Secure"
popup that is shown for autofilled fields on HTTP pages at page load.

TEST=
1) Enable chrome://flags#enable-http-form-warning
2) Store a password on http://rsolomakhin.github.io/autofill/ under
"Name/Password"
3) Reload the page with so that the password field and credit card field are
both in the viewport. Verify that "Login Not Secure" appears (see
just-inside-viewport-bottom.png at crbug.com/678713#c11)
4) Resize the browser so that the password field is below the viewport and
reload. Verify that "Login Not Secure" does *not* (see
just-outside-viewport-bottom.png at crbug.com/678713#c11)

BUG=678713
==========

[lgarron, 2017-01-17 23:37:34]

lgarron@chromium.org changed reviewers:
+ estade@chromium.org

[lgarron, 2017-01-17 23:37:34]

estade@, could you review?
(If this is not the appropriate place for such a fix, could you suggest where
else to look?)

See crbug.com/678713#c11 for screenshots.

[Evan Stade, 2017-01-18 00:05:47]

Some questions:

a) why do we show the warning before the element is focused?
b) how do we know the element is actually visible and not hidden via some
variety of css?
c) are we going to show the warning when the element does become visible?
d) can you add a test?

https://codereview.chromium.org/2641643002/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/2641643002/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/chrome_autofill_client.cc:258: if
((element_bounds.y() + element_bounds.height()) < 0 ||
nit: element_bounds.bottom()

https://codereview.chromium.org/2641643002/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/chrome_autofill_client.cc:259: (element_bounds.y() +
element_bounds.height()) >= client_area.height()) {
what about the x direction? Seems like this should be something like

if (!gfx::Rect(client_area.size()).Contains(element_bounds))

https://codereview.chromium.org/2641643002/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/chrome_autofill_client.cc:261: // before the relevant
field can scroll into the viewport), so we supress
nit: suppress

[lgarron, 2017-01-18 01:13:05]

Patchset #2 (id:20001) has been deleted

[lgarron, 2017-01-18 01:16:16]

On 2017/01/18 at 00:05:47, estade wrote:
> Some questions:
> 
> a) why do we show the warning before the element is focused?
> b) how do we know the element is actually visible and not hidden via some
variety of css?
> c) are we going to show the warning when the element does become visible?
> d) can you add a test?
> 
>
https://codereview.chromium.org/2641643002/diff/1/chrome/browser/ui/autofill/...
> File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):
> 
>
https://codereview.chromium.org/2641643002/diff/1/chrome/browser/ui/autofill/...
> chrome/browser/ui/autofill/chrome_autofill_client.cc:258: if
((element_bounds.y() + element_bounds.height()) < 0 ||
> nit: element_bounds.bottom()
> 
>
https://codereview.chromium.org/2641643002/diff/1/chrome/browser/ui/autofill/...
> chrome/browser/ui/autofill/chrome_autofill_client.cc:259: (element_bounds.y()
+ element_bounds.height()) >= client_area.height()) {
> what about the x direction? Seems like this should be something like
> 
> if (!gfx::Rect(client_area.size()).Contains(element_bounds))
> 
>
https://codereview.chromium.org/2641643002/diff/1/chrome/browser/ui/autofill/...
> chrome/browser/ui/autofill/chrome_autofill_client.cc:261: // before the
relevant field can scroll into the viewport), so we supress
> nit: suppress

a) Because it's autofilled on page load even if another element is focused (see
the test case, where the credit card field is focused)
b) We don't.
c) No. It always goes away, and we decided it was very confusing to show the
message outside the viewport, and marginally useful to try to show it
inside/later.
d) Yes, but where and how do I write it? chrome_autofill_client doesn't have an
accompanying test, and I'm still terrible at figuring out how to add new kinds
of tests.

[Evan Stade, 2017-01-18 01:33:13]

On 2017/01/18 01:16:16, lgarron wrote:
> On 2017/01/18 at 00:05:47, estade wrote:
> > Some questions:
> > 
> > a) why do we show the warning before the element is focused?
> > b) how do we know the element is actually visible and not hidden via some
> variety of css?
> > c) are we going to show the warning when the element does become visible?
> > d) can you add a test?
> > 
> >
>
https://codereview.chromium.org/2641643002/diff/1/chrome/browser/ui/autofill/...
> > File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/2641643002/diff/1/chrome/browser/ui/autofill/...
> > chrome/browser/ui/autofill/chrome_autofill_client.cc:258: if
> ((element_bounds.y() + element_bounds.height()) < 0 ||
> > nit: element_bounds.bottom()
> > 
> >
>
https://codereview.chromium.org/2641643002/diff/1/chrome/browser/ui/autofill/...
> > chrome/browser/ui/autofill/chrome_autofill_client.cc:259:
(element_bounds.y()
> + element_bounds.height()) >= client_area.height()) {
> > what about the x direction? Seems like this should be something like
> > 
> > if (!gfx::Rect(client_area.size()).Contains(element_bounds))
> > 
> >
>
https://codereview.chromium.org/2641643002/diff/1/chrome/browser/ui/autofill/...
> > chrome/browser/ui/autofill/chrome_autofill_client.cc:261: // before the
> relevant field can scroll into the viewport), so we supress
> > nit: suppress
> 
> a) Because it's autofilled on page load even if another element is focused
(see
> the test case, where the credit card field is focused)

Except it's not actually autofilled in this case, right? The message is just an
advisory to the user, so why show it before the user has expressed interest via
focus?

> b) We don't.
> c) No. It always goes away, and we decided it was very confusing to show the
> message outside the viewport, and marginally useful to try to show it
> inside/later.
> d) Yes, but where and how do I write it? chrome_autofill_client doesn't have
an
> accompanying test, and I'm still terrible at figuring out how to add new kinds
> of tests.

I dunno. I haven't actively worked on autofill in a while. Maybe mathp can help
you.

[lgarron, 2017-04-05 00:59:45]

Publishing leftover comments to get this out of my review queue.

https://codereview.chromium.org/2641643002/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/2641643002/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/chrome_autofill_client.cc:258: if
((element_bounds.y() + element_bounds.height()) < 0 ||
On 2017/01/18 at 00:05:46, Evan Stade wrote:
> nit: element_bounds.bottom()

Ooh, good to know.

https://codereview.chromium.org/2641643002/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/chrome_autofill_client.cc:259: (element_bounds.y() +
element_bounds.height()) >= client_area.height()) {
On 2017/01/18 at 00:05:46, Evan Stade wrote:
> what about the x direction? Seems like this should be something like
> 
> if (!gfx::Rect(client_area.size()).Contains(element_bounds))

Oh, right. I explicitly discussed this with estark@, then promptly forgot to put
it in the code. :-(

Anyhow, your code doesn't work as-is because of Rect/RectF incompatibility, but
that made me realize that it's cleaner to do the calculations using screen
coordinates right after calculating element_bounds_in_screen_space.

https://codereview.chromium.org/2641643002/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/chrome_autofill_client.cc:261: // before the relevant
field can scroll into the viewport), so we supress
On 2017/01/18 at 00:05:46, Evan Stade wrote:
> nit: suppress

Done.