Fix unicode bidi mirroring characters are not correctly mirrored in textfield

Unicode bidi mirroring characters are not correctly mirrored in textfield and omnibox (which use CRichEditCtrl) while *inputting*. 

Since bidi mirroring characters are correctly mirrored when rendering the whole text, fix it by re-rendering the whole text every time the text changes.

NOTE: this change will mess up the undo queue. The continuous keystroke wont be treated as one undo event.
Every single keystroke is treated as a undo state.

Using the following as example:
1. paste string "abcd " into find-in-page;
2. type in 'e', 'f', and ' '.
3. paste string "xyz".

When undo, the string sequence will be:
"abcd ef xyz" => "abcd ef " ==> "abcd ef" ==> "abcd e" ==> "abcd " =>"".

Without the change, the string sequence is:
"abcd ef xyz" => "abcd ef " => "abcd " =>"".



BUG=46298
TEST=follow the steps described in the bug.

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

[xji, 2010-06-10 23:55:20]

Some background: I was trying to change textfield and omnibox layout to RTL when
UI is RTL. During previous try out, there were always problems such as keyboard
layout change while changing directionality (which I might found a solution),
bidi mirroring characters are not mirrored. Then, I realized that bidi mirroring
characters are not mirrored not only in RTL context, they are not mirrored in
LTR context either.

Using parenthesis as example, the non-mirroring bug only happens when you type
in Hebrew letters mixed with English parenthesis or English letters mixed with
Hebrew parenthesis. It probably is not a common usage scenario.

This fix messes up the undo queue. The continuous keystroke wont be treated as
one undo event after the change.

I have not figured out any other way on preserving the bidi mirroring, or
managing undo queue manually.

But still, I would like to send this out for your opinion.

[Peter Kasting, 2010-06-11 00:14:40]

There are COM APIs you can use to mess with the Undo state.  See for example
ScopedSuspendUndo() in autocomplete_edit_view_win.cc.  You can probably use some
combination of calls to avoid screwing up the Undo stack with this.

http://codereview.chromium.org/2741007/diff/1/2
File views/controls/textfield/native_textfield_win.cc (right):

http://codereview.chromium.org/2741007/diff/1/2#newcode847
views/controls/textfield/native_textfield_win.cc:847: // Re-rendering the whole
text in text field so that bidi mirroring
I would omit a comment here in favor of a comment in the header.

http://codereview.chromium.org/2741007/diff/1/2#newcode850
views/controls/textfield/native_textfield_win.cc:850: long start = 0, end = 0;
Nit: Use CHARRANGE here (see example above)

http://codereview.chromium.org/2741007/diff/1/2#newcode853
views/controls/textfield/native_textfield_win.cc:853: SetSel(0, -1);
Nit: SelectAll() would be clearer.

http://codereview.chromium.org/2741007/diff/1/3
File views/controls/textfield/native_textfield_win.h (right):

http://codereview.chromium.org/2741007/diff/1/3#newcode1
views/controls/textfield/native_textfield_win.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2741007/diff/1/3#newcode156
views/controls/textfield/native_textfield_win.h:156: void
OnAfterPossibleChange(bool re_rendering_whole_text = true);
Google style guide bans the use of default arguments.

Also "re_rendering_whole_text" isn't a good name for this argument.  How about
"should_redraw_text"?

Also, you should give a more complete explanation of what's going on.  For
example:

When a user types a BiDi mirroring character (e.g. an open parenthesis, which
should be rendered as '(' in an LTR context and ')' in an RTL one), the
textfield does not properly mirror the character when necessary.  However, if we
explicitly set the text in the edit to the entire current string, then things
will be mirrored properly.  When |should_redraw_text| is true, we perform this
transformation any time the text changes.  You should only set this to false if
the text is changing in a way that cannot add, remove, or modify the direction
of any mirroring characters.

[xji, 2010-06-12 00:44:22]

On 2010/06/11 00:14:40, Peter Kasting wrote:
> There are COM APIs you can use to mess with the Undo state.  See for example
> ScopedSuspendUndo() in autocomplete_edit_view_win.cc.  You can probably use
some
> combination of calls to avoid screwing up the Undo stack with this.

Thanks for the quick review and suggestion.

As in omnibox, I used ITextDocument->Undo(tomSuspend..) and Undo(tomResume),
which helped removing SelectAll, ReplaceSelection, and SetSel from the undo
stack.

But still, the undo stack is not completely correct.
Every single keystroke is still treated as a undo state.

Using the following as example:
1. paste string "abcd " into find-in-page;
2. type in 'e', 'f', and ' '.
3. paste string "xyz".

When undo, the string sequence will be:
"abcd ef xyz" => "abcd ef " ==> "abcd ef" ==> "abcd e" ==> "abcd " =>"".

Without the change, the string sequence is:
"abcd ef xyz" => "abcd ef " => "abcd " =>"".

Since the keystroke should be still undoable, it could not be solved by
suspending the keystroke from Undo stack.

Instead, what needed is an opposite of EM_STOPGROUPTYPING.
Looks like ITextDocument::BeginEditCollection() and
ITextDocument::EndEditCollection could do the trick. But they are not
implemented yet (E_NOTIMPL is returned when I tried them).

replacing SelectAll/ReplaceSel with SetTextEx(, ...| ST_KEEPUNDO, 1200) has no
effect on the undo stack.

> 
> http://codereview.chromium.org/2741007/diff/1/2
> File views/controls/textfield/native_textfield_win.cc (right):
> 
> http://codereview.chromium.org/2741007/diff/1/2#newcode847
> views/controls/textfield/native_textfield_win.cc:847: // Re-rendering the
whole
> text in text field so that bidi mirroring
> I would omit a comment here in favor of a comment in the header.

Done.

> 
> http://codereview.chromium.org/2741007/diff/1/2#newcode850
> views/controls/textfield/native_textfield_win.cc:850: long start = 0, end = 0;
> Nit: Use CHARRANGE here (see example above)

Done.

> 
> http://codereview.chromium.org/2741007/diff/1/2#newcode853
> views/controls/textfield/native_textfield_win.cc:853: SetSel(0, -1);
> Nit: SelectAll() would be clearer.

Done.

> 
> http://codereview.chromium.org/2741007/diff/1/3
> File views/controls/textfield/native_textfield_win.h (right):
> 
> http://codereview.chromium.org/2741007/diff/1/3#newcode1
> views/controls/textfield/native_textfield_win.h:1: // Copyright (c) 2009 The
> Chromium Authors. All rights reserved.
> Nit: 2010

Done.
BTW, could the code be committed without changing the copyright year?
This should be automatically checked when committing.

> 
> http://codereview.chromium.org/2741007/diff/1/3#newcode156
> views/controls/textfield/native_textfield_win.h:156: void
> OnAfterPossibleChange(bool re_rendering_whole_text = true);
> Google style guide bans the use of default arguments.

Done.

> 
> Also "re_rendering_whole_text" isn't a good name for this argument.  How about
> "should_redraw_text"?

Done.

> 
> Also, you should give a more complete explanation of what's going on.  For
> example:
> 
> When a user types a BiDi mirroring character (e.g. an open parenthesis, which
> should be rendered as '(' in an LTR context and ')' in an RTL one), the
> textfield does not properly mirror the character when necessary.  However, if
we
> explicitly set the text in the edit to the entire current string, then things
> will be mirrored properly.  When |should_redraw_text| is true, we perform this
> transformation any time the text changes.  You should only set this to false
if
> the text is changing in a way that cannot add, remove, or modify the direction
> of any mirroring characters.

Done.

[jeremy, 2010-06-13 05:57:08]

lgtm

[Peter Kasting, 2010-06-14 19:34:03]

LGTM

http://codereview.chromium.org/2741007/diff/6001/7001
File views/controls/textfield/native_textfield_win.cc (right):

http://codereview.chromium.org/2741007/diff/6001/7001#newcode863
views/controls/textfield/native_textfield_win.cc:863: // Save the original
selection.
Nit: Remove this comment and the one below, they're redundant with the code.

http://codereview.chromium.org/2741007/diff/6001/7002
File views/controls/textfield/native_textfield_win.h (right):

http://codereview.chromium.org/2741007/diff/6001/7002#newcode179
views/controls/textfield/native_textfield_win.h:179: // as OnImeComposition.
Is it really true that OnImeComposition can't affect mirroring?  Or is it just
that even if it can, setting the text explicitly would kill the IME composition
and so we can't safely pass "true" there?  Maybe this comment is inaccurate.

[xji, 2010-06-14 20:21:57]

On Mon, Jun 14, 2010 at 12:34 PM, <pkasting@chromium.org> wrote:

> LGTM
>
>
> http://codereview.chromium.org/2741007/diff/6001/7001
>
> File views/controls/textfield/native_textfield_win.cc (right):
>
> http://codereview.chromium.org/2741007/diff/6001/7001#newcode863
> views/controls/textfield/native_textfield_win.cc:863: // Save the
> original selection.
> Nit: Remove this comment and the one below, they're redundant with the
> code.
>
> http://codereview.chromium.org/2741007/diff/6001/7002
>
> File views/controls/textfield/native_textfield_win.h (right):
>
> http://codereview.chromium.org/2741007/diff/6001/7002#newcode179
> views/controls/textfield/native_textfield_win.h:179: // as
> OnImeComposition.
> Is it really true that OnImeComposition can't affect mirroring?  Or is
> it just that even if it can, setting the text explicitly would kill the
> IME composition and so we can't safely pass "true" there?  Maybe this
> comment is inaccurate.


It is the latter: setting the text explicitly  would kill the IME
composition.


>
>
> http://codereview.chromium.org/2741007/show
>

[Peter Kasting, 2010-06-14 20:24:32]

On 2010/06/14 20:21:57, xji wrote:
> It is the latter: setting the text explicitly  would kill the IME
> composition.

OK, I suggest updating the comment to remove the bit at the end, and adding a
comment in OnImeComposition() that notes why we pass false.

[xji, 2010-06-14 22:11:36]

Done.

On 2010/06/14 20:24:32, Peter Kasting wrote:
> On 2010/06/14 20:21:57, xji wrote:
> > It is the latter: setting the text explicitly  would kill the IME
> > composition.
> 
> OK, I suggest updating the comment to remove the bit at the end, and adding a
> comment in OnImeComposition() that notes why we pass false.

[Peter Kasting, 2010-06-14 22:16:44]

LGTM

http://codereview.chromium.org/2741007/diff/13001/14001
File views/controls/textfield/native_textfield_win.cc (right):

http://codereview.chromium.org/2741007/diff/13001/14001#newcode436
views/controls/textfield/native_textfield_win.cc:436: // indicating text in IME
composition mode disappear.
Nit: How about:

// If we allow OnAfterPossibleChange() to redraw the text, it will do this by
setting the edit's text directly, which can cancel the current IME composition
or cause other adverse affects.  So we set |should_redraw_text| to false.

[xji, 2010-06-14 22:32:34]

On Mon, Jun 14, 2010 at 3:16 PM, <pkasting@chromium.org> wrote:

> LGTM
>
>
> http://codereview.chromium.org/2741007/diff/13001/14001
>
> File views/controls/textfield/native_textfield_win.cc (right):
>
> http://codereview.chromium.org/2741007/diff/13001/14001#newcode436
> views/controls/textfield/native_textfield_win.cc:436: // indicating text
> in IME composition mode disappear.
> Nit: How about:
>
> // If we allow OnAfterPossibleChange() to redraw the text, it will do
> this by setting the edit's text directly, which can cancel the current
> IME composition or cause other adverse affects.  So we set
> |should_redraw_text| to false.


comments fine with me.

But more accurately, from the Chinese IME I tested, it actually does not
kill the IME composition. Instead, the underline, which appears under the
text in IME composition and which indicates the IME is in composition phase,
disappears. So, user might think the mode is not composition mode, and the
text is committed. But actually, you can still use left arrow to show other
character candidates, which means it is still in IME composition mode (just
the indicator is gone).

But it causes adverse affects for sure.


>
>
> http://codereview.chromium.org/2741007/show
>