Fix incorrect forcing of text directionality in canvas_skia.cc.

Fix incorrect forcing of text directionality in canvas_skia.cc.

There were two issues:

1. RTL wrapping was simply not happening if your UI language locale was not RTL. Which made FORCE_RTL_DIRECTIONALITY not work.
2. RTL wrapping was not being applied in the SizeStringInt() path, which resulted in an inconsistency between the needed width for SizeStringInt() and DrawStringInt(), causing a problem with the label code resulting in truncation.

This CL fixes both issues.

BUG=128073, 105550
TEST=See testcases attached to bug.

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

[Alexei Svitkine (slow), 2012-05-14 21:46:22]

.

[xji, 2012-05-14 22:50:51]

http://codereview.chromium.org/10384168/diff/2003/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

http://codereview.chromium.org/10384168/diff/2003/ui/gfx/canvas_skia.cc#newco...
ui/gfx/canvas_skia.cc:31: base::i18n::WrapStringWithLTRFormatting(text);
This is a bit tricky. In systems without RTL support (such as the he/ar language
package is not installed in Windows Vista), blindly add unicode marker will
render empty squares to displayed.

We had similar problem before when render tooltip.
And what we did last time is that we only add unicode marker when the tooltip's
directionality is not the same as UI's directionality.

I think what we can do here is only WrapStringWithLTRFormatting() when
base::i18n::IsRTL(), assuming the system  has RTL support when UI is RTL or when
text has strong RTL characters in it.

[xji, 2012-05-14 23:09:10]

http://codereview.chromium.org/10384168/diff/2003/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

http://codereview.chromium.org/10384168/diff/2003/ui/gfx/canvas_skia.cc#newco...
ui/gfx/canvas_skia.cc:31: base::i18n::WrapStringWithLTRFormatting(text);
long term wise, I think we should be able to set text's directionality in
RenderText, instead of adding Unicode bidi marks. Uniscribe and Pango should
support that. It is probably pango_context_set_base_dir().

[Alexei Svitkine (slow), 2012-05-15 15:09:04]

> We had similar problem before when render tooltip.
> And what we did last time is that we only add unicode marker when the
tooltip's
> directionality is not the same as UI's directionality.

I see. I've gone and found the bug you're referring to, so that I have a better
understanding of this.

Here's a link to it:

http://crbug.com/17468

And some other related ones:

http://crbug.com/7253
http://crbug.com/5996

> I think what we can do here is only WrapStringWithLTRFormatting() when
> base::i18n::IsRTL(), assuming the system  has RTL support when UI is RTL or
when
> text has strong RTL characters in it.

[Alexei Svitkine (slow), 2012-05-15 15:47:01]

I've updated the CL to have the logic you suggested.

I'm still not sure I'm entirely convinced that this makes sense.

With the updated logic, I have the following questions:

1. Clearly, the "force RTL" case needs to work even if the UI language is not
RTL, else it wouldn't fix the issue in question (with bidi alerts). But wouldn't
this result in boxes being rendered on systems without RTL language support?

Or do we assume that's okay because: a) all callers who set this do it based on
the content of the string and b) if the content of the string is such that we
want to force RTL, then if the system doesn't have RTL language support, it
would already have boxes, so extra boxes wouldn't be any worse? Is that the
correct reasoning here?

2. For the final case in the updated function, right now I simply return false
and don't wrap the text at all. But that case is only reached if the string
contains strong directionality characters and the UI is not RTL. I'm not
convinced that case makes sense.

Do we really need to check IsRTL() before wrapping the string for RTL if we
already know the string contains strong directionality characters? I am thinking
that the same logic as 1. could be applied here: i.e. either a) the system has
RTL support and wrapping won't do any harm or b) the system doesn't have RTL
support, so it will already render boxes since the string already has strong
directionality characters (that we checked for).

So I'm thinking the last if statement could be removed in the function and have
it always end with wrapping and returning true. What do you think?

[xji, 2012-05-15 19:41:53]

On 2012/05/15 15:47:01, Alexei Svitkine wrote:
> I've updated the CL to have the logic you suggested.
> 
> I'm still not sure I'm entirely convinced that this makes sense.
> 
> With the updated logic, I have the following questions:
> 
> 1. Clearly, the "force RTL" case needs to work even if the UI language is not
> RTL, else it wouldn't fix the issue in question (with bidi alerts). But
wouldn't
> this result in boxes being rendered on systems without RTL language support?

Yes. It will render boxes on system without RTL support.

> 
> Or do we assume that's okay because: a) all callers who set this do it based
on
> the content of the string and b) if the content of the string is such that we
> want to force RTL, then if the system doesn't have RTL language support, it
> would already have boxes, so extra boxes wouldn't be any worse? Is that the
> correct reasoning here?

Yes, we assume system has RTL support if users want to display RTL characters.
Currently, FORCE_RTL_DIRECTIONALITY is only set when a JS message starts with
strong RTL characters, in which case, we assume the system has RTL support.

I do not quite understand your a), but I think b) is what we assume.


> 
> 2. For the final case in the updated function, right now I simply return false
> and don't wrap the text at all. But that case is only reached if the string
> contains strong directionality characters and the UI is not RTL. I'm not
> convinced that case makes sense.

In our Windows system, we want the UI strings to be displayed as left-to-right
directionality when UI itself is LTR, vice versa.

So, for English Chrome, even if the string is "abc DEF" (assume DEF is Hebrew),
which contains strong RTL characters, we still want it to be displayed as LTR as
"abcFED". 

Same string in Hebrew Chrome, we want the string display as RTL as "FEDabc".
So, the last case is correct.

Currently, the FORCE directionality cases are only used for JS message display.
The other cases are for Chrome's UI strings.
(that is the reason I feel the old 'if'/'else' structure is more symmetric and
readable.)

> 
> Do we really need to check IsRTL() before wrapping the string for RTL if we
> already know the string contains strong directionality characters? 

Yes, we do as what explained above.

> I am thinking
> that the same logic as 1. could be applied here: i.e. either a) the system has
> RTL support and wrapping won't do any harm or b) the system doesn't have RTL
> support, so it will already render boxes since the string already has strong
> directionality characters (that we checked for).
> 
> So I'm thinking the last if statement could be removed in the function and
have
> it always end with wrapping and returning true. What do you think?

that would yield wrong result for UI string in LTR locale.

[xji, 2012-05-15 19:42:07]

http://codereview.chromium.org/10384168/diff/10003/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

http://codereview.chromium.org/10384168/diff/10003/ui/gfx/canvas_skia.cc#newc...
ui/gfx/canvas_skia.cc:42: base::i18n::WrapStringWithLTRFormatting(text);
I think in our current Uniscribe implementation of RenderText, text is assumed
to be in left-to-right directionality, no matter the UI is a RTL UI or LTR UI.
In which case, we do not need to add bidi mark to force directionality here.

But we probably want to change the RenderText Win behavior to set base
directionality as RTL if UI is RTL, and base directionality as LTR if UI is LTL,
in which case, we will need to wrap the text only when IsRTL() is true.

Either case, I feel better to wrap the string when IsRTL() is true to reduce
empty squares situation.

Or ultimately, to reset RenderText's base directionalty based on the each text
itself.

BTW, while testing the JS alert display in RTL UI, I think there might be
another regression. The message got truncated. It might be a regression after
r124736. If you run the test file, are the messages got truncated with your CL
(which force correct directionality)?

http://codereview.chromium.org/10384168/diff/10003/ui/gfx/canvas_skia.cc#newc...
ui/gfx/canvas_skia.cc:53: return false;
I'd prefer the old 'if'/'else' structure.

[Alexei Svitkine (slow), 2012-05-15 19:44:33]

Thanks for the explanation.

I'll update comments in the code to explain this.

[Alexei Svitkine (slow), 2012-05-16 17:52:01]

I've updated the CL to fix the issues and it now works correctly with the test
case you had me try (I've also uploaded that test onto the bug tracker).

Please take another look.

(I still kept the early returns instead of the else/if pattern because I felt it
makes it easier to put comments explaining each block.)

[xji, 2012-05-16 19:53:12]

lgtm.

http://codereview.chromium.org/10384168/diff/15001/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

http://codereview.chromium.org/10384168/diff/15001/ui/gfx/canvas_skia.cc#newc...
ui/gfx/canvas_skia.cc:29: // default RenderText directionality is already LTR.
I think this is true (for now). But please verify it. For test file alert_1.html
attached in the bug, the 2rd line should be rendered as left-to-right in RTL
locale too (chrome --lang=he), it should be like "abc שנב".

http://codereview.chromium.org/10384168/diff/15001/ui/gfx/canvas_skia.cc#newc...
ui/gfx/canvas_skia.cc:50: // still be rendered RTL.)
nit: I feel the comment is confusing.
In this default case, the string should be displayed as LTR. We do not add any
marks because RenderText's default dir is LTR. (If we later change the
RenderText's default dir is UI's dir, we will have to
WrapStringWithLTRFormatting() when IsRTL() is true.)

[Alexei Svitkine (slow), 2012-05-16 20:11:17]

Thanks!

http://codereview.chromium.org/10384168/diff/15001/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

http://codereview.chromium.org/10384168/diff/15001/ui/gfx/canvas_skia.cc#newc...
ui/gfx/canvas_skia.cc:29: // default RenderText directionality is already LTR.
On 2012/05/16 19:53:12, xji wrote:
> I think this is true (for now). But please verify it. For test file
alert_1.html
> attached in the bug, the 2rd line should be rendered as left-to-right in RTL
> locale too (chrome --lang=he), it should be like "abc שנב".

I have verified that is indeed the case.

http://codereview.chromium.org/10384168/diff/15001/ui/gfx/canvas_skia.cc#newc...
ui/gfx/canvas_skia.cc:50: // still be rendered RTL.)
On 2012/05/16 19:53:12, xji wrote:
> nit: I feel the comment is confusing.
> In this default case, the string should be displayed as LTR. We do not add any
> marks because RenderText's default dir is LTR. (If we later change the
> RenderText's default dir is UI's dir, we will have to
> WrapStringWithLTRFormatting() when IsRTL() is true.)

Reworded the comment to be clearer.

[commit-bot: I haz the power, 2012-05-16 20:11:31]

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

[commit-bot: I haz the power, 2012-05-16 22:14:07]

Change committed as 137535