Android Omnibox: Do not force LTR text direction when empty.

Android Omnibox: Do not force LTR text direction when empty.

Fixes a regression introduced in r400359: the "Search or type URL"
placeholder text jumping back and forth from left and right alignment
when in RTL languages.

Also fixes incorrect text direction when restoring a closed tab.

BUG=631422
TEST=On a phone-sized Android device, with language set to
  Hebrew/Arabic, in landscape, open New Tab Page and scroll page up and
  down. Should see the grey placeholder text in Omnibox always
  right-aligned, not jumping from left to right.

Committed: https://crrev.com/1d266aa00775a9e9307c7d342fb6354ba7707012
Cr-Commit-Position: refs/heads/master@{#410510}

[Matt Giuca, 2016-08-05 03:51:34]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 03:51:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Matt Giuca, 2016-08-05 03:53:09]

Description was changed from

==========
Android Omnibox: Do not force LTR text direction when empty.

Fixes the "Search or type URL" placeholder text jumping back and forth
from left and right alignment when in RTL languages.

BUG=631422
TEST=On a phone-sized Android device, with language set to
  Hebrew/Arabic, in landscape, open New Tab Page and scroll page up and
  down. Should see the grey placeholder text in Omnibox always
  right-aligned, not jumping from left to right.
==========

to

==========
Android Omnibox: Do not force LTR text direction when empty.

Fixes a regression introduced in r400359: the "Search or type URL"
placeholder text jumping back and forth from left and right alignment
when in RTL languages.

BUG=631422
TEST=On a phone-sized Android device, with language set to
  Hebrew/Arabic, in landscape, open New Tab Page and scroll page up and
  down. Should see the grey placeholder text in Omnibox always
  right-aligned, not jumping from left to right.
==========

[Matt Giuca, 2016-08-05 03:54:56]

mgiuca@chromium.org changed reviewers:
+ tedchoc@chromium.org

[commit-bot: I haz the power, 2016-08-05 04:43:48]

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

[commit-bot: I haz the power, 2016-08-05 04:43:49]

Dry run: This issue passed the CQ dry run.

[Ted C, 2016-08-05 20:25:05]

https://codereview.chromium.org/2182183008/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java
(right):

https://codereview.chromium.org/2182183008/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java:449: if
(focused || length() == 0) {
If you aren't on the NTP (i.e. there is a URL), you focus the omnibox and then
clear the text.  Then you defocus the omnibox w/o hitting enter, do we hit this
check before we revert the text back to the URL?

I'm wondering if we also need to hook into text changed to see if the text
changes and set it to LTR.

Or what happens if you're on the NTP and we set this as INHERIT on defocus, then
you click on a most visited item.  Wouldn't it still be in inherit until you
cycle the omnibox focus?

[Matt Giuca, 2016-08-08 05:24:05]

Description was changed from

==========
Android Omnibox: Do not force LTR text direction when empty.

Fixes a regression introduced in r400359: the "Search or type URL"
placeholder text jumping back and forth from left and right alignment
when in RTL languages.

BUG=631422
TEST=On a phone-sized Android device, with language set to
  Hebrew/Arabic, in landscape, open New Tab Page and scroll page up and
  down. Should see the grey placeholder text in Omnibox always
  right-aligned, not jumping from left to right.
==========

to

==========
Android Omnibox: Do not force LTR text direction when empty.

Fixes a regression introduced in r400359: the "Search or type URL"
placeholder text jumping back and forth from left and right alignment
when in RTL languages.

Also fixes incorrect text direction when restoring a closed tab.

BUG=631422
TEST=On a phone-sized Android device, with language set to
  Hebrew/Arabic, in landscape, open New Tab Page and scroll page up and
  down. Should see the grey placeholder text in Omnibox always
  right-aligned, not jumping from left to right.
==========

[Matt Giuca, 2016-08-08 05:24:18]

https://codereview.chromium.org/2182183008/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java
(right):

https://codereview.chromium.org/2182183008/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java:449: if
(focused || length() == 0) {
On 2016/08/05 20:25:05, Ted C wrote:
> If you aren't on the NTP (i.e. there is a URL), you focus the omnibox and then
> clear the text.  Then you defocus the omnibox w/o hitting enter, do we hit
this
> check before we revert the text back to the URL?
> 
> I'm wondering if we also need to hook into text changed to see if the text
> changes and set it to LTR.
> 
> Or what happens if you're on the NTP and we set this as INHERIT on defocus,
then
> you click on a most visited item.  Wouldn't it still be in inherit until you
> cycle the omnibox focus?

Right, good thinking. Basically those two issues arise because the text can
change without focus changing. I just found another one which is when you launch
Chrome and it restores tabs, the tab's URL bar is never focused so this was not
being called.

I moved it into a separate method and called it from setText. Now the alignment
is set correctly in all of these cases. Thanks for the catch!

[Matt Giuca, 2016-08-08 05:24:36]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 05:24:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 06:03:15]

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

[commit-bot: I haz the power, 2016-08-08 06:03:16]

Dry run: This issue passed the CQ dry run.

[Ted C, 2016-08-08 17:35:25]

On 2016/08/08 05:24:18, Matt Giuca wrote:
>
https://codereview.chromium.org/2182183008/diff/1/chrome/android/java/src/org...
> File chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java
> (right):
> 
>
https://codereview.chromium.org/2182183008/diff/1/chrome/android/java/src/org...
> chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java:449:
if
> (focused || length() == 0) {
> On 2016/08/05 20:25:05, Ted C wrote:
> > If you aren't on the NTP (i.e. there is a URL), you focus the omnibox and
then
> > clear the text.  Then you defocus the omnibox w/o hitting enter, do we hit
> this
> > check before we revert the text back to the URL?
> > 
> > I'm wondering if we also need to hook into text changed to see if the text
> > changes and set it to LTR.
> > 
> > Or what happens if you're on the NTP and we set this as INHERIT on defocus,
> then
> > you click on a most visited item.  Wouldn't it still be in inherit until you
> > cycle the omnibox focus?
> 
> Right, good thinking. Basically those two issues arise because the text can
> change without focus changing. I just found another one which is when you
launch
> Chrome and it restores tabs, the tab's URL bar is never focused so this was
not
> being called.
> 
> I moved it into a separate method and called it from setText. Now the
alignment
> is set correctly in all of these cases. Thanks for the catch!

lgtm

[Matt Giuca, 2016-08-09 00:10:37]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2016-08-09 00:11:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 00:16:10]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-09 00:20:01]

Description was changed from

==========
Android Omnibox: Do not force LTR text direction when empty.

Fixes a regression introduced in r400359: the "Search or type URL"
placeholder text jumping back and forth from left and right alignment
when in RTL languages.

Also fixes incorrect text direction when restoring a closed tab.

BUG=631422
TEST=On a phone-sized Android device, with language set to
  Hebrew/Arabic, in landscape, open New Tab Page and scroll page up and
  down. Should see the grey placeholder text in Omnibox always
  right-aligned, not jumping from left to right.
==========

to

==========
Android Omnibox: Do not force LTR text direction when empty.

Fixes a regression introduced in r400359: the "Search or type URL"
placeholder text jumping back and forth from left and right alignment
when in RTL languages.

Also fixes incorrect text direction when restoring a closed tab.

BUG=631422
TEST=On a phone-sized Android device, with language set to
  Hebrew/Arabic, in landscape, open New Tab Page and scroll page up and
  down. Should see the grey placeholder text in Omnibox always
  right-aligned, not jumping from left to right.

Committed: https://crrev.com/1d266aa00775a9e9307c7d342fb6354ba7707012
Cr-Commit-Position: refs/heads/master@{#410510}
==========

[commit-bot: I haz the power, 2016-08-09 00:20:02]

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