Review Rquest: fix 6125 and 8686 -- cursor positioning in CRichEditCtrl

This CL fixes 
issue 6125 - [edit search engines] after hitting right ctrl+shift, can't change insertion point with the mouse
and 
issue 8686 - RTL: Can't select RTL text from Chrome UI boxes

The problem is cursor positioning and cursor selection in CRichEditCtrl in RTL layout. (cursor positioning is fine even for RTL characters in LTR layout, but cursor positioning is not working even for LTR characters in RTL layout).

The fix is correctly calculation of input boundary in ClipXCoordToVisibleText(), and the fix is mostly part of Nick Carter's un-committed fix in  https://svn.corp.google.com/review/chrome/desc/cb/ncarter/rtl_richedit_fixes@50605

BUG=6125
BUG=8686
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=14158

[xji, 2009-04-20 18:54:36]

Hi Nick,

Since this fix is mostly part of your uncommitted fix for CRichEditCtrl in
TextField and AutocompleteEditViewWin, could you please review it?


I only changed the way to check whether it is in RTL or not to
  PARAFORMAT2 pf2;
  GetParaFormat(pf2);
  bool text_is_ltr =
      ((pf2.wEffects & PFE_RTLPARA) == (10n_util::LEFT_TO_RIGHT);

Instead of yours
  bool text_is_ltr = !l10n_util::StringContainsStrongRTLChars(GetText());

Because I tested that if the layout is LTR, cursor works fine even for RTL
characters. But cursor does not work even for LTR characters if the layout is
RTL. Also, we can always change the layout by pressing ctrl + right/left shift.
So, I think checking the layout is the right way to go.

[ncarter (slow), 2009-04-21 16:08:27]

Looks good.

http://codereview.chromium.org/86003/diff/1002/1003
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/86003/diff/1002/1003#newcode1851
Line 1851: ((pf2.wEffects & PFE_RTLPARA) == l10n_util::LEFT_TO_RIGHT);
I think this makes sense -- is this bit set whenever the user switches his text
input to an RTL language?

http://codereview.chromium.org/86003/diff/1002/1003#newcode1865
Line 1865: 
Is there an extra newline here?

http://codereview.chromium.org/86003/diff/1002/1004
File chrome/views/controls/text_field.cc (right):

http://codereview.chromium.org/86003/diff/1002/1004#newcode882
Line 882: 
I was shocked at the code repetition until I remembered that the copy-and-pasted
code between here and the omnibox code is the norm.

[xji, 2009-04-21 19:38:03]

Hi Nick,

Thanks for the quick review!
Switching input between RTL/LTR language does not re-set the paragraph layout. 
And I removed the extra newline you pointed out.

But I realized that I  made 2 mistakes here, the checking of (pf2.wEffects &
PFE_RTLPARA) == l10n_util::LEFT_TO_RIGHT, should be 
(pf2.wEffects & PFE_RTLPARA) == 0. Also, I mentioned that the cursor positioning
works for RTL character in LTR layout, which is wrong (I incorrectly tested by
placing RTL characters after the LTR characters). The calculation logic you
added should apply to either RTL layout or LTR layout containing RTL characters.

I've uploaded the patch again. Could you please review again? Sorry for the
inconvenience.

Thanks,
Xiaomei

On 2009/04/21 16:08:27, nick wrote:
> Looks good.
> 
> http://codereview.chromium.org/86003/diff/1002/1003
> File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):
> 
> http://codereview.chromium.org/86003/diff/1002/1003#newcode1851
> Line 1851: ((pf2.wEffects & PFE_RTLPARA) == l10n_util::LEFT_TO_RIGHT);
> I think this makes sense -- is this bit set whenever the user switches his
text
> input to an RTL language?
> 
> http://codereview.chromium.org/86003/diff/1002/1003#newcode1865
> Line 1865: 
> Is there an extra newline here?
> 
> http://codereview.chromium.org/86003/diff/1002/1004
> File chrome/views/controls/text_field.cc (right):
> 
> http://codereview.chromium.org/86003/diff/1002/1004#newcode882
> Line 882: 
> I was shocked at the code repetition until I remembered that the
copy-and-pasted
> code between here and the omnibox code is the norm.

[ncarter (slow), 2009-04-21 22:23:35]

http://codereview.chromium.org/86003/diff/2002/2003
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/86003/diff/2002/2003#newcode1855
Line 1855: ltr_text_in_ltr_layout = false;
I think you should have {}'s here, since the if() is multi-line.  The logic
looks better now -- sorry for missing that in the first review.  If PFE_RTLPARA
isn't set when the user changes the input language, how does it ever get set (is
it just alt-shift)?  I am fine with this logic but want to understand which
cases users are likely to fall into.

http://codereview.chromium.org/86003/diff/2002/2003#newcode1889
Line 1889: left_bound;
We need {}'s here too.

http://codereview.chromium.org/86003/diff/2002/2003#newcode1891
Line 1891: return x;
How hard would it be to test this code in a unit test?

[xji, 2009-04-21 22:46:25]

Hi Nick,

I've added "{}" in both cases in both files.

PFE_RTLPARA could be set in the following scenario:
1. the UI is RTL UI.
2. in a text field (for example, the name or keyword area in "edit search
engines" dialog)
3. hit right ctrl + right shift, to set the box to RTL mode.

Then, the PFE_RTLPARA is set.
Also, the IME input changes to RTL IME, which matches what you observed that
changing paragraph directionality changes IME input when you made other parts of
the changes in omnibox and textfield.

But such change is not a round way change. I mean changing IME method to RTL 
does not change the paragraph's layout directionality. Which I think makes
sense.

I am not sure how to unit test the code. Let me think about it.

Thanks,
Xiaomei

On 2009/04/21 22:23:35, nick wrote:
> http://codereview.chromium.org/86003/diff/2002/2003
> File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):
> 
> http://codereview.chromium.org/86003/diff/2002/2003#newcode1855
> Line 1855: ltr_text_in_ltr_layout = false;
> I think you should have {}'s here, since the if() is multi-line.  The logic
> looks better now -- sorry for missing that in the first review.  If
PFE_RTLPARA
> isn't set when the user changes the input language, how does it ever get set
(is
> it just alt-shift)?  I am fine with this logic but want to understand which
> cases users are likely to fall into.
> 
> http://codereview.chromium.org/86003/diff/2002/2003#newcode1889
> Line 1889: left_bound;
> We need {}'s here too.
> 
> http://codereview.chromium.org/86003/diff/2002/2003#newcode1891
> Line 1891: return x;
> How hard would it be to test this code in a unit test?

[ncarter (slow), 2009-04-21 22:50:31]

LGTM.  If you think unit tests are feasible, I'll happily look at those diffs
too.

[idana, 2009-04-21 23:00:22]

Thanks for making this change, Xiaomei.

I remember that there was a DCHECK I encountered when I tried apply this patch
back in the day (this is why we decided not to submit it then because it was too
close to our Beta launch).

IIRC - the DCHECK occurred when I selected text in the Omnibox and tried to drag
it (yeah, I know it sounds strange that I even tried that...). Can you please
run a debug build and verify that you don't see the same problem?

[xji, 2009-04-21 23:22:08]

I am using a debug build and tried to drag a selected text (RTL, LTR, or bidi
text) to a text area or to randomly empty space, no DCHECK encountered in
English UI and Hebrew UI.

Maybe because this is only one part of the change, and it does not change the
layout of omnibox and textfield at all.

Thanks,
Xiaomei

On 2009/04/21 23:00:22, idana wrote:
> Thanks for making this change, Xiaomei.
> 
> I remember that there was a DCHECK I encountered when I tried apply this patch
> back in the day (this is why we decided not to submit it then because it was
too
> close to our Beta launch).
> 
> IIRC - the DCHECK occurred when I selected text in the Omnibox and tried to
drag
> it (yeah, I know it sounds strange that I even tried that...). Can you please
> run a debug build and verify that you don't see the same problem?

[idana, 2009-04-21 23:32:24]

LGTM

Thanks for verifying.