Sync Promo: Fix up focus.

Sync Promo: Fix up focus.

R=sail@chromium.org,estade@chromium.org
BUG=98928
TEST=Focus starts on user name.  When you press shift tab you land on "Learn
more" link.  When you press enter on that link and press tab again, you're still
on the same link (that now reads "Hide").


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113123

[sail, 2011-11-09 01:54:39]

http://codereview.chromium.org/8502042/diff/1/chrome/browser/resources/sync_p...
File chrome/browser/resources/sync_promo.js (right):

http://codereview.chromium.org/8502042/diff/1/chrome/browser/resources/sync_p...
chrome/browser/resources/sync_promo.js:149: $('gaia-email').focus();
I don't think this is the correct fix.

sync_setup_handler.js:showGaiaLogin_: already attempts to put focus on the
correct field.

The correct fix would be to fix that so that it works for the promo.

[Dan Beam, 2011-11-09 07:11:58]

On 2011/11/09 01:54:39, sail wrote:
>
http://codereview.chromium.org/8502042/diff/1/chrome/browser/resources/sync_p...
> File chrome/browser/resources/sync_promo.js (right):
> 
>
http://codereview.chromium.org/8502042/diff/1/chrome/browser/resources/sync_p...
> chrome/browser/resources/sync_promo.js:149: $('gaia-email').focus();
> I don't think this is the correct fix.
> 
> sync_setup_handler.js:showGaiaLogin_: already attempts to put focus on the
> correct field.
> 
> The correct fix would be to fix that so that it works for the promo.

I was doing this but it was a bit hacky as I had to send the right flags to
avoid most of the logic in the method anyway, so I just used .loginSetFocus_()
instead.  Let me know if there's an issue with this.

[sail, 2011-11-09 15:18:43]

> I was doing this but it was a bit hacky as I had to send the right flags to
> avoid most of the logic in the method anyway, so I just used .loginSetFocus_()
> instead.  Let me know if there's an issue with this.

I think refactoring sync_setup_handler.js would be better.

Here's one example of why it's better:
   - open the sync promo
   - make sure the text fields don't have focus
   - open a new tab
   - click Wrench Menu -> Sign in to Chrome

This uses showGaiaLogin_() to set focus to the email address in the sync promo.

It doesn't make sense to add another function to do the same thing.

[Dan Beam, 2011-11-09 19:47:20]

On 2011/11/09 15:18:43, sail wrote:
> > I was doing this but it was a bit hacky as I had to send the right flags to
> > avoid most of the logic in the method anyway, so I just used
.loginSetFocus_()
> > instead.  Let me know if there's an issue with this.
> 
> I think refactoring sync_setup_handler.js would be better.

And higher risk, no?  I mean, not a lot, but a little bit.

> Here's one example of why it's better:
>    - open the sync promo
>    - make sure the text fields don't have focus
>    - open a new tab
>    - click Wrench Menu -> Sign in to Chrome
> 
> This uses showGaiaLogin_() to set focus to the email address in the sync
promo.

How is this an example of why it's better?  showGaiaLogin_() is called every
time the login is shown so to reset its state while still on the page (i.e.
joebob@gmail.com tries to login, doesn't succeed, hides overlay,
marysue@google.com clicks "Sign in to Chrome" and all the fields get reset). 
This isn't useful to us on the sync promo, as you can't hide the overlay (so
there's no reason to reset) and we're only calling this once on DOMContentLoaded
(not on every auth attempt).

> It doesn't make sense to add another function to do the same thing.

I didn't add another function, I just re-used the same function that's called at
the end of showGaiaLogin_() (http://goo.gl/e71qr).  That's what "so I just used
.loginSetFocus_()" meant, sorry if that was unclear.  The rest of the method
will be skipped and effectively only this will be called, but it really doesn't
matter to me if you want to run more code, DOM queries, and modify the existing
behavior/look (which calling this originally did for me).

Let me know if I'm still missing something.

[sail, 2011-11-09 20:00:15]

> And higher risk, no?  I mean, not a lot, but a little bit.

This is a M17 bug. I'd rather have a correct fix for this.

My problem with your change is this. Currently, showGaiaLogin_() is called it
attempts to change focus to the email field.

Your CL doesn't answer why showGaiaLogin_() fails. We should understand this
before we add more code.

[Dan Beam, 2011-11-09 22:20:07]

> My problem with your change is this. Currently, showGaiaLogin_() is called it
> attempts to change focus to the email field.
>
> Your CL doesn't answer why showGaiaLogin_() fails. We should understand this
> before we add more code.

I didn't know it was already being called - I could've sworn I set a breakpoint
there and it wasn't being called.  Sorry.  I think this might already be working
as expected, I do think it'd be worthwhile to modify the tabIndex of the learn
more link to make it more natural (or move the bytestream order, or add it in
the markup).  We'll see if Anna can still reproduce.

[Dan Beam, 2011-11-11 03:28:17]

NOTE: tabindex="1" doesn't affect the sync setup overlay or the overall tab
order as that part of the markup is [hidden] in this case (which means they're
skipped while tabbing).  Leaving these attributes in the markup all the time
should never affect tab order in settings as long as they're hidden.

[Dan Beam, 2011-11-12 02:33:49]

ping

[sail, 2011-11-12 20:12:24]

LGTM
Might want to also get this reviewed by someone more familiar with HTML.

[Dan Beam, 2011-11-14 08:43:19]

On 2011/11/12 20:12:24, sail wrote:
> LGTM
> Might want to also get this reviewed by someone more familiar with HTML.

estade is familiar with HTML, he'll be able to point out any problems with this
(as will testing it).

[Evan Stade, 2011-11-14 19:16:12]

what happened to reordering the nodes so that tab order is naturally correct

[Dan Beam, 2011-11-14 19:46:09]

On 2011/11/14 19:16:12, Evan Stade wrote:
> what happened to reordering the nodes so that tab order is naturally correct

It was a bitch and required changing a lot of style.  I can still do, I just
don't see a reason not to do this.  You think there's a risk of this leaking
onto a page while not [hidden]?

[Dan Beam, 2011-11-14 20:15:58]

On 2011/11/14 19:46:09, Dan Beam wrote:
> On 2011/11/14 19:16:12, Evan Stade wrote:
> > what happened to reordering the nodes so that tab order is naturally correct
> 
> It was a bitch and required changing a lot of style.  I can still do, I just
> don't see a reason not to do this.  You think there's a risk of this leaking
> onto a page while not [hidden]?

Talked with estade offline, will re-arrange byte order anyways to Do The Right
Thing ™.

[Dan Beam, 2011-12-03 02:04:59]

btw, I grepped for sync_promo_minimize.png in chrome/browser and didn't get any
results - are we even using it anywhere?

[sail, 2011-12-03 02:07:59]

On 2011/12/03 02:04:59, Dan Beam wrote:
> btw, I grepped for sync_promo_minimize.png in chrome/browser and didn't get
any
> results - are we even using it anywhere?

We were using that in ntp3. It's safe to delete that now.

[Evan Stade, 2011-12-05 02:23:37]

http://codereview.chromium.org/8502042/diff/19001/chrome/browser/resources/sy...
File chrome/browser/resources/sync_promo/sync_promo.css (right):

http://codereview.chromium.org/8502042/diff/19001/chrome/browser/resources/sy...
chrome/browser/resources/sync_promo/sync_promo.css:34: /* Clearfix wrapper
around the promo information and login. */
i don't remember what this is and it's not described anywhere (comment in code,
description in cl, etc.)

[Dan Beam, 2011-12-05 16:45:40]

http://codereview.chromium.org/8502042/diff/19001/chrome/browser/resources/sy...
File chrome/browser/resources/sync_promo/sync_promo.css (right):

http://codereview.chromium.org/8502042/diff/19001/chrome/browser/resources/sy...
chrome/browser/resources/sync_promo/sync_promo.css:34: /* Clearfix wrapper
around the promo information and login. */
On 2011/12/05 02:23:37, Evan Stade wrote:
> i don't remember what this is and it's not described anywhere (comment in
code,
> description in cl, etc.)

In standard browsers if you float: [ left | right ]; an element it removes that
element's height from the render tree's calculations (i.e. there's no
particularly easy way to make something with variable height assume the correct
amount of space on the page).  Adding an :after {} rule with a block level
element returns variable height to that node (which we need in the case of the
login box, as it can have an error, no error, or recapthca) and essentially
fixes this problem and doesn't require any more markup / logic.

http://www.positioniseverything.net/easyclearing.html

I thought this was a pretty widespread terminology, but I guess I should leave a
better description in the comment?

[Evan Stade, 2011-12-05 20:43:46]

On 2011/12/05 16:45:40, Dan Beam wrote:
>
http://codereview.chromium.org/8502042/diff/19001/chrome/browser/resources/sy...
> File chrome/browser/resources/sync_promo/sync_promo.css (right):
> 
>
http://codereview.chromium.org/8502042/diff/19001/chrome/browser/resources/sy...
> chrome/browser/resources/sync_promo/sync_promo.css:34: /* Clearfix wrapper
> around the promo information and login. */
> On 2011/12/05 02:23:37, Evan Stade wrote:
> > i don't remember what this is and it's not described anywhere (comment in
> code,
> > description in cl, etc.)
> 
> In standard browsers if you float: [ left | right ]; an element it removes
that
> element's height from the render tree's calculations (i.e. there's no
> particularly easy way to make something with variable height assume the
correct
> amount of space on the page).  Adding an :after {} rule with a block level
> element returns variable height to that node (which we need in the case of the
> login box, as it can have an error, no error, or recapthca) and essentially
> fixes this problem and doesn't require any more markup / logic.
> 
> http://www.positioniseverything.net/easyclearing.html
> 
> I thought this was a pretty widespread terminology, but I guess I should leave
a
> better description in the comment?

yes. lgtm with that comment

[sail, 2011-12-05 22:03:25]

lgtm

[Dan Beam, 2011-12-05 23:34:51]

> lgtm with that comment

Just removed the clear fix instead, didn't end up needing it any more (markup
change + clear: both; on nextSibling ended up being fine).

[Evan Stade, 2011-12-06 00:19:14]

great, lgtm

[commit-bot: I haz the power, 2011-12-06 00:20:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/8502042/24002

[commit-bot: I haz the power, 2011-12-06 06:18:36]

Change committed as 113123