Fix eliding of LTR tab title in RTL UI under non-Windows Views.

Fix eliding of LTR tab title in RTL UI under non-Windows Views.

Use DrawFadeTruncatingString() on non-Windows views platforms too, now
that the implementation is no longer platform specific.

BUG=135058
TEST=Launch chromeos chrome with --lang=he and go to amazon.com.
Check that the tab title is painted LTR and elided (via fade
truncation) on the right end. Now, go to google.com. Verify
that the "Google" string is right-aligned in the tab title.
TBR=sky@chromium.org,xji@chromium.org
R=msw@chromium.org

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

[Alexei Svitkine (slow), 2012-06-28 21:25:13]

Hey Xiaomei and Scott,

This fixes the issue for the case of tab titles, but I am wondering whether we
should be fixing this in a more systematic way instead...

The problem was caused because the code was calling DrawStringInt() without
specifying flags - which ends up just using
Canvas::DefaultCanvasTextAlignment().

Canvas::DefaultCanvasTextAlignment() just sets either LEFT or RIGHT alignment
purely based on base::i18n::IsRTL(), hence in the case of this bug it was
setting right alignment even though the tab title was purely LTR.

I noticed that this issue is not specific to tab titles and that bookmark bar
entries appear to have the same problem.

So I'm wondering whether we should move this logic into canvas.cc instead for
when no flags are specified - instead of using DefaultCanvasTextAlignment()
which doesn't care about the content of the string.

What do you think?

[sky, 2012-06-28 22:18:29]

If you did promote this function are there cases where it could do the wrong
thing? Secondarily is determining this information expensive enough that we
should cache it?

[xji, 2012-06-29 00:59:13]

On 2012/06/28 22:18:29, sky wrote:
> If you did promote this function are there cases where it could do the wrong
> thing? Secondarily is determining this information expensive enough that we
> should cache it?

Is fading and truncation always consistent? which means they are either on left
or right.
And they depends on text directionality only, not the alignment?

[Alexei Svitkine (slow), 2012-06-29 14:46:26]

On Thu, Jun 28, 2012 at 6:18 PM,  <sky@chromium.org> wrote:
> If you did promote this function are there cases where it could do the wrong
> thing?

I can't think of any, but I'm not an i18n expert. xji@, any thoughts?

> Secondarily is determining this information expensive enough that we
> should cache it?

It's O(n) over the text size, so not overly expensive.

However, I think promoting this to the default won't help the
bookmarks bar case, since that code explicitly calls the
DrawStringInt() function with flags that it itself gets from
DefaultCanvasTextAlignment(). So either way, that code would need a
separate fix.

[Alexei Svitkine (slow), 2012-06-29 14:51:33]

> Is fading and truncation always consistent? which means they are either on
> left or right.

That is true currently (after http://crrev.com/144791). Truncating is
the same as alignment (in the sense that if the text is too long, it
will truncate on the opposite side of where its aligned too - so
left-aligned text will truncate at the right end.)

> And they depends on text directionality only, not the alignment?

Truncation and alignment are synonymous when the text is too long to
fit its destination. So the answer to this question is "no" in
general.

Users of the API can also pass any alignment they want, which may or
may not correspond to the text direction or the UI direction. In the
case of this CL (as it currently stands), the issue was with the
default alignment value, was purely based on the UI direction, which
is incorrect for tabs with LTR titles under RTL UIs.

-Alexei

[xji, 2012-06-29 18:11:18]

On 2012/06/29 14:51:33, Alexei Svitkine wrote:
> > Is fading and truncation always consistent? which means they are either on
> > left or right.
> 
> That is true currently (after http://crrev.com/144791). Truncating is
> the same as alignment (in the sense that if the text is too long, it
> will truncate on the opposite side of where its aligned too - so
> left-aligned text will truncate at the right end.)
> 
> > And they depends on text directionality only, not the alignment?
> 
> Truncation and alignment are synonymous when the text is too long to
> fit its destination. So the answer to this question is "no" in
> general.
> 
> Users of the API can also pass any alignment they want, which may or
> may not correspond to the text direction or the UI direction. In the
> case of this CL (as it currently stands), the issue was with the
> default alignment value, was purely based on the UI direction, which
> is incorrect for tabs with LTR titles under RTL UIs.
> 
> -Alexei

I do not quite understand why truncation/fading is related to alignment. 
when truncation/fading, we want truncate logically trailing part.
If so, truncation should corresponding to text directionality, not alignment.
Is fading following the same philosophy?

But I did just checked Window's canary build on a LTR tab title on RTL UI. The
title is aligned left, which I think it might be wrong (I need to confirm. I
vaguelly remember in RTL UI, we want things align right, even for text in
left-to-right directionality). In current situation (ltr text showed as left
aligned in RTL UI), alignment does match text directionality. Then, truncation
looks like using alignment.

As you said, users can pass any kinds of alignment.
For example, for a long LTR text, 
is the truncation always on right side no matter what kind of alignment it is?
how about fading, is it always on right side too no matter what kinds of
alignment (left/right/center)?

>> If you did promote this function are there cases where it could do the wrong
>> thing?

>I can't think of any, but I'm not an i18n expert. xji@, any thoughts?

I did not think of any.

[xji, 2012-06-29 18:11:24]

http://codereview.chromium.org/10701024/diff/1/chrome/browser/ui/views/tabs/b...
File chrome/browser/ui/views/tabs/base_tab.cc (right):

http://codereview.chromium.org/10701024/diff/1/chrome/browser/ui/views/tabs/b...
chrome/browser/ui/views/tabs/base_tab.cc:555: }
Since RenderText has both directionality and horizotal aligment information, can
we not pass information here and do fading/truncation inside RenderText by
querying its directionality and alignment information?

[Alexei Svitkine (slow), 2012-06-29 18:33:24]

On Fri, Jun 29, 2012 at 2:11 PM,  <xji@chromium.org> wrote:
> On 2012/06/29 14:51:33, Alexei Svitkine wrote:
>>
>> > Is fading and truncation always consistent? which means they are either
>> > on
>> > left or right.
>
>
>> That is true currently (after http://crrev.com/144791). Truncating is
>> the same as alignment (in the sense that if the text is too long, it
>> will truncate on the opposite side of where its aligned too - so
>> left-aligned text will truncate at the right end.)
>
>
>> > And they depends on text directionality only, not the alignment?
>
>
>> Truncation and alignment are synonymous when the text is too long to
>> fit its destination. So the answer to this question is "no" in
>> general.
>
>
>> Users of the API can also pass any alignment they want, which may or
>> may not correspond to the text direction or the UI direction. In the
>> case of this CL (as it currently stands), the issue was with the
>> default alignment value, was purely based on the UI direction, which
>> is incorrect for tabs with LTR titles under RTL UIs.
>
>
>> -Alexei
>
>
> I do not quite understand why truncation/fading is related to alignment.
> when truncation/fading, we want truncate logically trailing part.
> If so, truncation should corresponding to text directionality, not
> alignment.
> Is fading following the same philosophy?

Truncation is related to alignment because the side that is truncated
(assuming overflow) is the opposite side of where alignment is. Fading
is applied to the truncated side.

>
> But I did just checked Window's canary build on a LTR tab title on RTL UI.
> The title is aligned left, which I think it might be wrong (I need to confirm.
I
> vaguelly remember in RTL UI, we want things align right, even for text in
> left-to-right directionality).

I thought per previous discussion we specifically wanted LTR text to
be left-aligned. Otherwise (in the case of truncation), you're seeing
the end of the (e.g.) English sentence, which is not really useful to
someone trying to read an English sentence, no?

> As you said, users can pass any kinds of alignment.
> For example, for a long LTR text,
> is the truncation always on right side no matter what kind of alignment it
> is?

Truncation is directly related to alignment. For example, assume the
following two cases:

1. aaaaaaaa|

2. |aaaaaaa

In the first case, which is right-aligned, the truncation happens on
the left if the text is unable to fit into its destination bounds.
In the second case, which is left-aligned, the truncation happens on
the right if the text is unable to fit into its destination bounds.

For example, if the input is "alignment", this may look like:

1. nment|
2. |alignm


I think you are confusing truncation (which happens automatically if
the text doesn't fit) with eliding (i.e. using "..."), which is a
different concept.

If eliding is requested, then the above example would look like:

1. align...|
2. |align...

-Alexei

[xji, 2012-06-29 19:15:52]

> >
> > But I did just checked Window's canary build on a LTR tab title on RTL UI.
> > The title is aligned left, which I think it might be wrong (I need to
confirm.
> I
> > vaguelly remember in RTL UI, we want things align right, even for text in
> > left-to-right directionality).
> 
> I thought per previous discussion we specifically wanted LTR text to
> be left-aligned. Otherwise (in the case of truncation), you're seeing
> the end of the (e.g.) English sentence, which is not really useful to
> someone trying to read an English sentence, no?

Sorry that I do not remember previous discussion.
If I said that we want LTR text to be left-aligned in RTL UI, I think I was
wrong.
(We can check with our native speakers on their point of view.) 

For consistency in tab titles and with other parts of the UI, such as
history/download pages (looks like we center ltr title in new tab page),
in RTL UI, I think LTR text should be right aligned, but in ltr directionality.
when truncation, it truncates the trailing text, for example, if the text is
"alignment", it shows as "align" (same as in LTR UI).

> 
> > As you said, users can pass any kinds of alignment.
> > For example, for a long LTR text,
> > is the truncation always on right side no matter what kind of alignment it
> > is?
> 
> Truncation is directly related to alignment. For example, assume the
> following two cases:
> 
> 1. aaaaaaaa|
> 
> 2. |aaaaaaa
> 
> In the first case, which is right-aligned, the truncation happens on
> the left if the text is unable to fit into its destination bounds.
> In the second case, which is left-aligned, the truncation happens on
> the right if the text is unable to fit into its destination bounds.
> 
> For example, if the input is "alignment", this may look like:
> 
> 1. nment|
> 2. |alignm

But why we want truncate "alignment" to "nment"?

> 
> 
> I think you are confusing truncation (which happens automatically if
> the text doesn't fit) with eliding (i.e. using "..."), which is a
> different concept.
> 
> If eliding is requested, then the above example would look like:
> 
> 1. align...|
> 2. |align...

I probably confused these two.
When eliding is used? and when truncation is used?

I just think truncation should truncate logical ending part, which is related to
text directionality, disregard alignment.

> 
> -Alexei

[Alexei Svitkine (slow), 2012-06-29 20:01:04]

> If I said that we want LTR text to be left-aligned in RTL UI, I think I was
> wrong.
> (We can check with our native speakers on their point of view.) 

I see your point now and I can confirm you're right - that's what we do on
Windows. I was previously only thinking in the context of truncation - but
you're right that in the case where the text is not truncated, it should be
aligned according to UI directionality.

So I think this fix is actually not correct, then.

We probably want to instead use DrawFadeTruncatingString() here, which is what
Windows does. DrawFadeTruncatingString() was previously limited only to
canvas_win.cc but is now also implemented in canvas_skia.cc too, so we can make
it available cross-platform.

I'll take a look at this.

-Alexei

[Alexei Svitkine (slow), 2012-06-29 20:34:38]

I've updated the CL and TEST= instructions.

[xji, 2012-06-29 21:07:53]

On 2012/06/29 20:34:38, Alexei Svitkine wrote:
> I've updated the CL and TEST= instructions.

the code change looks good to me.

But sorry that I still have a couple of questions:
1. I tried in windows canary 22.0.1190.0, in RTL UI, LTR title is left-aligned
in tab title.
This code seems not touch anything about alignment. Why 'Google' in "google.com"
is right-aligned in tab title in chromeos? Are they going through different
pathes?

2. does DrawFadeTruncatingString() call RenderText::ApplyFadeEffects()?
If so, following code might not work (at least for Linux, assuming alignment is
based on UI directionality):

if (horizontal_alignment() == ALIGN_RIGHT)
  std::swap(fade_left, fade_right);

[Alexei Svitkine (slow), 2012-06-29 21:15:20]

On 2012/06/29 21:07:53, xji wrote:
> On 2012/06/29 20:34:38, Alexei Svitkine wrote:
> > I've updated the CL and TEST= instructions.
> 
> the code change looks good to me.
> 
> But sorry that I still have a couple of questions:
> 1. I tried in windows canary 22.0.1190.0, in RTL UI, LTR title is left-aligned
> in tab title.
> This code seems not touch anything about alignment. Why 'Google' in
"google.com"
> is right-aligned in tab title in chromeos? Are they going through different
> pathes?

Yes, that was the case before since Windows was using DrawFadeTruncatingString()
and ChromeOS was using DrawStringInt(). Note that neither passed the alignment.

The logic within the implementations of those functions was different in terms
of what default alignment to use. In particular, DrawFadeTruncatingString()
would determine alignment based on the UI directionality and content of the
string, whereas DrawStringInt() would only use the UI directionality.

> 
> 2. does DrawFadeTruncatingString() call RenderText::ApplyFadeEffects()?
> If so, following code might not work (at least for Linux, assuming alignment
is
> based on UI directionality):
> 
> if (horizontal_alignment() == ALIGN_RIGHT)
>   std::swap(fade_left, fade_right);

It does call it, but it has logic to specify alignment based on RTL-ness and
fading specified. In practice, it appears to work correctly per my TEST=
instructions line.

[xji, 2012-06-29 23:51:18]

On 2012/06/29 21:15:20, Alexei Svitkine wrote:
> On 2012/06/29 21:07:53, xji wrote:
> > On 2012/06/29 20:34:38, Alexei Svitkine wrote:
> > > I've updated the CL and TEST= instructions.
> > 
> > the code change looks good to me.
> > 
> > But sorry that I still have a couple of questions:
> > 1. I tried in windows canary 22.0.1190.0, in RTL UI, LTR title is
left-aligned
> > in tab title.
> > This code seems not touch anything about alignment. Why 'Google' in
> "google.com"
> > is right-aligned in tab title in chromeos? Are they going through different
> > pathes?
> 
> Yes, that was the case before since Windows was using
DrawFadeTruncatingString()
> and ChromeOS was using DrawStringInt(). Note that neither passed the
alignment.
> 
> The logic within the implementations of those functions was different in terms
> of what default alignment to use. In particular, DrawFadeTruncatingString()
> would determine alignment based on the UI directionality and content of the
> string, whereas DrawStringInt() would only use the UI directionality.

Sorry for the confusion. My question is:
In  windows canary 22.0.1190.0, in RTL UI, LTR title is *left-aligned*
in tab title. The tab title is drawn through DrawFadeTruncatingString().

*Now*, you changed the Linux tab title draw to the same function
(DrawFadeTruncatingString()), if this function draws LTR title in RTL UI in
Windows as left aligned, why it draws LTR title in RTL UI in Linux in the right
alignment?

Maybe that is related to some logic "to specify alignment based on RTL-ness and
fading specified" you mentioned below?

> 
> > 
> > 2. does DrawFadeTruncatingString() call RenderText::ApplyFadeEffects()?
> > If so, following code might not work (at least for Linux, assuming alignment
> is
> > based on UI directionality):
> > 
> > if (horizontal_alignment() == ALIGN_RIGHT)
> >   std::swap(fade_left, fade_right);
> 
> It does call it, but it has logic to specify alignment based on RTL-ness and
> fading specified. In practice, it appears to work correctly per my TEST=
> instructions line.

[Alexei Svitkine (slow), 2012-07-03 19:18:01]

On 2012/06/29 23:51:18, xji wrote:
> On 2012/06/29 21:15:20, Alexei Svitkine wrote:
> > On 2012/06/29 21:07:53, xji wrote:
> > > On 2012/06/29 20:34:38, Alexei Svitkine wrote:
> > > > I've updated the CL and TEST= instructions.
> > > 
> > > the code change looks good to me.
> > > 
> > > But sorry that I still have a couple of questions:
> > > 1. I tried in windows canary 22.0.1190.0, in RTL UI, LTR title is
> left-aligned
> > > in tab title.
> > > This code seems not touch anything about alignment. Why 'Google' in
> > "google.com"
> > > is right-aligned in tab title in chromeos? Are they going through
different
> > > pathes?
> > 
> > Yes, that was the case before since Windows was using
> DrawFadeTruncatingString()
> > and ChromeOS was using DrawStringInt(). Note that neither passed the
> alignment.
> > 
> > The logic within the implementations of those functions was different in
terms
> > of what default alignment to use. In particular, DrawFadeTruncatingString()
> > would determine alignment based on the UI directionality and content of the
> > string, whereas DrawStringInt() would only use the UI directionality.
> 
> Sorry for the confusion. My question is:
> In  windows canary 22.0.1190.0, in RTL UI, LTR title is *left-aligned*
> in tab title. The tab title is drawn through DrawFadeTruncatingString().

You're right, that's a real bug, which I have a fix for here:

http://codereview.chromium.org/10703075/

It happens because DrawFadeTruncatingString() was short-circuiting to
DrawStringInt() without specifying an alignment, and DrawStringInt() wasn't
using DefaultCanvasTextAlignment() in that case.

[Alexei Svitkine (slow), 2012-07-03 21:11:37]

xji@ is already OO.

Swapping reviewers to see if I can get an LGTM before the American holidays. ;)

msw: PTAL

[msw, 2012-07-03 21:15:18]

LGTM

[commit-bot: I haz the power, 2012-07-03 21:23:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10701024/8001

[commit-bot: I haz the power, 2012-07-03 22:35:55]

Try job failure for 10701024-8001 (retry) (retry) (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-07-04 03:23:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10701024/8001

[commit-bot: I haz the power, 2012-07-04 04:40:40]

Change committed as 145432