Fix positioning of autofill popup on rtl

Fix positioning of autofill popup on rtl

BUG=454224
Committed: https://crrev.com/a589df30ed070579750bedae3223be025a41785e
Cr-Commit-Position: refs/heads/master@{#331413}

[Evan Stade, 2015-05-22 02:19:02]

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

[Garrett Casto, 2015-05-26 04:30:15]

lgtm

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

https://codereview.chromium.org/1151263002/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:86: //
TODO(estade): use correct text direction.
Shouldn't this just be

base::i18n::GetTextDirectionForLocale(icu::Locale::getDefault().getName());

I'm not sure why this isn't part of the base::i18n API directly. It might be
slightly cleaner to have ControllerCommon take a boolean for is_rtl instead of
the actual text direction. Then you could just use base::i18n::isRTL(). Either
way seems fine to me.

[Evan Stade, 2015-05-26 18:30:47]

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

https://codereview.chromium.org/1151263002/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:86: //
TODO(estade): use correct text direction.
On 2015/05/26 04:30:14, Garrett Casto wrote:
> Shouldn't this just be
> 
> base::i18n::GetTextDirectionForLocale(icu::Locale::getDefault().getName());
> 
> I'm not sure why this isn't part of the base::i18n API directly. It might be
> slightly cleaner to have ControllerCommon take a boolean for is_rtl instead of
> the actual text direction. Then you could just use base::i18n::isRTL(). Either
> way seems fine to me.

I suspect (but am not certain) you want to do the same thing Autofill does,
which is match the text direction of the triggering input, not the Chrome
locale.

[Evan Stade, 2015-05-26 18:30:50]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-05-26 18:31:46]

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

[Garrett Casto, 2015-05-26 18:38:11]

On 2015/05/26 18:30:47, Evan Stade wrote:
>
https://codereview.chromium.org/1151263002/diff/1/chrome/browser/ui/autofill/...
> File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
> (right):
> 
>
https://codereview.chromium.org/1151263002/diff/1/chrome/browser/ui/autofill/...
> chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:86: //
> TODO(estade): use correct text direction.
> On 2015/05/26 04:30:14, Garrett Casto wrote:
> > Shouldn't this just be
> > 
> > base::i18n::GetTextDirectionForLocale(icu::Locale::getDefault().getName());
> > 
> > I'm not sure why this isn't part of the base::i18n API directly. It might be
> > slightly cleaner to have ControllerCommon take a boolean for is_rtl instead
of
> > the actual text direction. Then you could just use base::i18n::isRTL().
Either
> > way seems fine to me.
> 
> I suspect (but am not certain) you want to do the same thing Autofill does,
> which is match the text direction of the triggering input, not the Chrome
> locale.

I had thought not, though I could be wrong. I don't have all the background for
Autofill, but I assumed you used the RTL of the page because the content that
you were Autofilling likely matched the page. I'm actually not sure how you
handle it if the characters of different Autofill elements are not the same RTL.
Regardless, in our case we know that our UI is always of a certain RTL and we
layout with that knowledge. Do you think that the growth of the bubble should be
dependent on the content of the page regardless of the language of the UI? I
would think not, which is why I suggested the change here.

[Evan Stade, 2015-05-26 18:44:25]

On 2015/05/26 18:38:11, Garrett Casto wrote:
> On 2015/05/26 18:30:47, Evan Stade wrote:
> >
>
https://codereview.chromium.org/1151263002/diff/1/chrome/browser/ui/autofill/...
> > File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/1151263002/diff/1/chrome/browser/ui/autofill/...
> > chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:86:
//
> > TODO(estade): use correct text direction.
> > On 2015/05/26 04:30:14, Garrett Casto wrote:
> > > Shouldn't this just be
> > > 
> > >
base::i18n::GetTextDirectionForLocale(icu::Locale::getDefault().getName());
> > > 
> > > I'm not sure why this isn't part of the base::i18n API directly. It might
be
> > > slightly cleaner to have ControllerCommon take a boolean for is_rtl
instead
> of
> > > the actual text direction. Then you could just use base::i18n::isRTL().
> Either
> > > way seems fine to me.
> > 
> > I suspect (but am not certain) you want to do the same thing Autofill does,
> > which is match the text direction of the triggering input, not the Chrome
> > locale.
> 
> I had thought not, though I could be wrong. I don't have all the background
for
> Autofill, but I assumed you used the RTL of the page because the content that
> you were Autofilling likely matched the page. I'm actually not sure how you
> handle it if the characters of different Autofill elements are not the same
RTL.

I don't know what you mean exactly by "the same RTL" but Autofill already
right-justifies the suggestions shown for an RTL <input>. This is independent of
the directionality of the suggestion text itself (be that RTL, LTR, or
bidirectional). This change only affects the positioning of the bubble, its
contents won't change.

> Regardless, in our case we know that our UI is always of a certain RTL and we
> layout with that knowledge. Do you think that the growth of the bubble should
be
> dependent on the content of the page regardless of the language of the UI? I
> would think not, which is why I suggested the change here.

I dunno, I guess you should consult someone more expert about
i18n/bidirectionality. I do think it makes sense for the bubble's position (not
size) to depend on content locale, even if the inside of the bubble depends on
Chrome's locale. But that's purely based on intuition.

[Garrett Casto, 2015-05-26 18:50:04]

On 2015/05/26 18:44:25, Evan Stade wrote:
> On 2015/05/26 18:38:11, Garrett Casto wrote:
> > On 2015/05/26 18:30:47, Evan Stade wrote:
> > >
> >
>
https://codereview.chromium.org/1151263002/diff/1/chrome/browser/ui/autofill/...
> > > File
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1151263002/diff/1/chrome/browser/ui/autofill/...
> > >
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:86:
> //
> > > TODO(estade): use correct text direction.
> > > On 2015/05/26 04:30:14, Garrett Casto wrote:
> > > > Shouldn't this just be
> > > > 
> > > >
> base::i18n::GetTextDirectionForLocale(icu::Locale::getDefault().getName());
> > > > 
> > > > I'm not sure why this isn't part of the base::i18n API directly. It
might
> be
> > > > slightly cleaner to have ControllerCommon take a boolean for is_rtl
> instead
> > of
> > > > the actual text direction. Then you could just use base::i18n::isRTL().
> > Either
> > > > way seems fine to me.
> > > 
> > > I suspect (but am not certain) you want to do the same thing Autofill
does,
> > > which is match the text direction of the triggering input, not the Chrome
> > > locale.
> > 
> > I had thought not, though I could be wrong. I don't have all the background
> for
> > Autofill, but I assumed you used the RTL of the page because the content
that
> > you were Autofilling likely matched the page. I'm actually not sure how you
> > handle it if the characters of different Autofill elements are not the same
> RTL.
> 
> I don't know what you mean exactly by "the same RTL" but Autofill already
> right-justifies the suggestions shown for an RTL <input>. This is independent
of
> the directionality of the suggestion text itself (be that RTL, LTR, or
> bidirectional). This change only affects the positioning of the bubble, its
> contents won't change.
> 

Right, that's not what this current CL changes, I was just curious.

> > Regardless, in our case we know that our UI is always of a certain RTL and
we
> > layout with that knowledge. Do you think that the growth of the bubble
should
> be
> > dependent on the content of the page regardless of the language of the UI? I
> > would think not, which is why I suggested the change here.
> 
> I dunno, I guess you should consult someone more expert about
> i18n/bidirectionality. I do think it makes sense for the bubble's position
(not
> size) to depend on content locale, even if the inside of the bubble depends on
> Chrome's locale. But that's purely based on intuition.

Not what I had though, which probably means I should check with someone who
knows more. I'll ask around.

[commit-bot: I haz the power, 2015-05-26 20:00:16]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-05-26 20:01:51]

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