Re: Fix incorrect forcing of text directionality in canvas_skia.cc. (issue 10384168)

[asvi...@chromium.org]

http://codereview.chromium.org/10384168/

[x...@chromium.org]

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#newcode31
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.

http://codereview.chromium.org/10384168/

[x...@chromium.org]

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#newcode31
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().

http://codereview.chromium.org/10384168/

[asvi...@chromium.org]

> 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

[asvi...@chromium.org]

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?

http://codereview.chromium.org/10384168/

[x...@chromium.org]

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.

http://codereview.chromium.org/10384168/

[x...@chromium.org]

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#newcode42
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#newcode53
ui/gfx/canvas_skia.cc:53: return false;
I'd prefer the old 'if'/'else' structure.

http://codereview.chromium.org/10384168/

[asvi...@chromium.org]

Thanks for the explanation.

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

http://codereview.chromium.org/10384168/

[asvi...@chromium.org]

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.)

http://codereview.chromium.org/10384168/

[x...@chromium.org]

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#newcode29
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#newcode50
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.)

http://codereview.chromium.org/10384168/

[asvi...@chromium.org]

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#newcode29
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#newcode50
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.

http://codereview.chromium.org/10384168/

[commi...@chromium.org]

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


https://chromiumcodereview.appspot.com/10384168/

[commi...@chromium.org]

Change committed as 137535

https://chromiumcodereview.appspot.com/10384168/