Prevent the IME candidate window from flickering on Chrome OS

Prevent the IME candidate window from flickering on Chrome OS

On Chrome OS, IME candidate window is anchored to either bottom-left
of the whole composition text (when its WindowPosition settings is
"composition") or bottom-left of the cursor (when it's "cursor".)
https://developer.chrome.com/extensions/input_ime#type-WindowPosition

The former setting doesn't have any issues regarding IME candidate
window positioning, but the latter (anchor to "cursor") does. The
cursor position may change every single time when you press the
convert key because for views::TextField, or more precisely for its
gfx::RenderText, the cursor position is defined as the bottom-right
of the selected range (if any) but the width of the selected range
may change on each conversion. For example, when pressing the
conversion key, a character in the selected range may change from a
full-width Hiragana Japanese character to a half-width Katakana
character.

The CL swaps |start| and |end| of the selected range on Chrome OS
so that the cursor position becomes bottom *left* of the range which
never changes during conversion. Note that Blink does the same
(i.e. placing the cursor at the beginning of the selection range)
to prevent the window flicker.

This CL is for Chrome OS only and doesn't change other platforms'
behavior. This does not affect Chrome OS IMEs that use "composition"
WindowPosition.

Demo:
 Omnibox without this fix: https://goo.gl/WVpjhE
 Omnibox with this fix: https://goo.gl/FH1fs9

BUG=311206
TEST=Follow the repro steps in crbug.com/311206 to confirm Omnibox
 no longer has the issue. Do the same with the find bar (Ctrl+F).

Review-Url: https://codereview.chromium.org/2886313002
Cr-Commit-Position: refs/heads/master@{#473984}
Committed: https://chromium.googlesource.com/chromium/src/+/e635c3e1ed9ed9a0838beba6893b996a63c3e6a1

[Yusuke Sato, 2017-05-17 22:29:03]

The CQ bit was checked by yusukes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 22:30:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yusuke Sato, 2017-05-17 23:19:45]

Description was changed from

==========
wip2

BUG=311206
TEST=wip
==========

to

==========
Prevent the IME candidate window from flickering on Chrome OS

On Chrome OS, IME candidate window is anchored to either bottom-left
of the whole composition text (when its WindowPosition settings is
"composition") or bottom-left of the cursor (when it's "cursor".)

The former setting doesn't have any issues regarding IME candidate
window positioning, but the latter does. The cursor position may
change every time when you press the convert key because for
views::TextField, or more precisely its gfx::RenderText, the cursor
position is defined as the bottom-right of the selected range (if
any) and the length of the selected range may change on each
conversion.

The CL swaps |start| and |end| of the selected range on Chrome OS
so that the cursor position becomes bottom-left of the range which
doesn't change during conversion. Note that Blink does the same
(i.e. placing the cursor at the beginning of the selection range)
to prevent the window flicker.

Omnibox without this fix: https://goo.gl/WVpjhE
Omnibox with this fix: https://goo.gl/FH1fs9

BUG=311206
TEST=Follow the repro steps in crbug.com/311206 to confirm Omnibox
 no longer has the issue. Do the same with the find bar (Ctrl+F).
==========

[Yusuke Sato, 2017-05-17 23:20:34]

Description was changed from

==========
Prevent the IME candidate window from flickering on Chrome OS

On Chrome OS, IME candidate window is anchored to either bottom-left
of the whole composition text (when its WindowPosition settings is
"composition") or bottom-left of the cursor (when it's "cursor".)

The former setting doesn't have any issues regarding IME candidate
window positioning, but the latter does. The cursor position may
change every time when you press the convert key because for
views::TextField, or more precisely its gfx::RenderText, the cursor
position is defined as the bottom-right of the selected range (if
any) and the length of the selected range may change on each
conversion.

The CL swaps |start| and |end| of the selected range on Chrome OS
so that the cursor position becomes bottom-left of the range which
doesn't change during conversion. Note that Blink does the same
(i.e. placing the cursor at the beginning of the selection range)
to prevent the window flicker.

Omnibox without this fix: https://goo.gl/WVpjhE
Omnibox with this fix: https://goo.gl/FH1fs9

BUG=311206
TEST=Follow the repro steps in crbug.com/311206 to confirm Omnibox
 no longer has the issue. Do the same with the find bar (Ctrl+F).
==========

to

==========
Prevent the IME candidate window from flickering on Chrome OS

On Chrome OS, IME candidate window is anchored to either bottom-left
of the whole composition text (when its WindowPosition settings is
"composition") or bottom-left of the cursor (when it's "cursor".)
https://developer.chrome.com/extensions/input_ime#type-WindowPosition

The former setting doesn't have any issues regarding IME candidate
window positioning, but the latter does. The cursor position may
change every time when you press the convert key because for
views::TextField, or more precisely its gfx::RenderText, the cursor
position is defined as the bottom-right of the selected range (if
any) and the length of the selected range may change on each
conversion.

The CL swaps |start| and |end| of the selected range on Chrome OS
so that the cursor position becomes bottom-left of the range which
doesn't change during conversion. Note that Blink does the same
(i.e. placing the cursor at the beginning of the selection range)
to prevent the window flicker.

Omnibox without this fix: https://goo.gl/WVpjhE
Omnibox with this fix: https://goo.gl/FH1fs9

BUG=311206
TEST=Follow the repro steps in crbug.com/311206 to confirm Omnibox
 no longer has the issue. Do the same with the find bar (Ctrl+F).
==========

[Yusuke Sato, 2017-05-17 23:24:16]

Description was changed from

==========
Prevent the IME candidate window from flickering on Chrome OS

On Chrome OS, IME candidate window is anchored to either bottom-left
of the whole composition text (when its WindowPosition settings is
"composition") or bottom-left of the cursor (when it's "cursor".)
https://developer.chrome.com/extensions/input_ime#type-WindowPosition

The former setting doesn't have any issues regarding IME candidate
window positioning, but the latter does. The cursor position may
change every time when you press the convert key because for
views::TextField, or more precisely its gfx::RenderText, the cursor
position is defined as the bottom-right of the selected range (if
any) and the length of the selected range may change on each
conversion.

The CL swaps |start| and |end| of the selected range on Chrome OS
so that the cursor position becomes bottom-left of the range which
doesn't change during conversion. Note that Blink does the same
(i.e. placing the cursor at the beginning of the selection range)
to prevent the window flicker.

Omnibox without this fix: https://goo.gl/WVpjhE
Omnibox with this fix: https://goo.gl/FH1fs9

BUG=311206
TEST=Follow the repro steps in crbug.com/311206 to confirm Omnibox
 no longer has the issue. Do the same with the find bar (Ctrl+F).
==========

to

==========
Prevent the IME candidate window from flickering on Chrome OS

On Chrome OS, IME candidate window is anchored to either bottom-left
of the whole composition text (when its WindowPosition settings is
"composition") or bottom-left of the cursor (when it's "cursor".)
https://developer.chrome.com/extensions/input_ime#type-WindowPosition

The former setting doesn't have any issues regarding IME candidate
window positioning, but the latter does. The cursor position may
change every single time when you press the convert key because for
views::TextField, or more precisely for its gfx::RenderText, the
cursor position is defined as the bottom-right of the selected range
(if any) but the width of the selected range may change on each
conversion. For example, when pressing the conversion key, a
character in the selected range may change from a full-width Hiragana
Japanese character to a half-width Katakana character.

The CL swaps |start| and |end| of the selected range on Chrome OS
so that the cursor position becomes bottom *left* of the range which
doesn't change during conversion. Note that Blink does the same
(i.e. placing the cursor at the beginning of the selection range)
to prevent the window flicker.

This CL is for Chrome OS only and doesn't change other platforms'
behavior.

Demo:
 Omnibox without this fix: https://goo.gl/WVpjhE
 Omnibox with this fix: https://goo.gl/FH1fs9

BUG=311206
TEST=Follow the repro steps in crbug.com/311206 to confirm Omnibox
 no longer has the issue. Do the same with the find bar (Ctrl+F).
==========

[Yusuke Sato, 2017-05-17 23:30:31]

Description was changed from

==========
Prevent the IME candidate window from flickering on Chrome OS

On Chrome OS, IME candidate window is anchored to either bottom-left
of the whole composition text (when its WindowPosition settings is
"composition") or bottom-left of the cursor (when it's "cursor".)
https://developer.chrome.com/extensions/input_ime#type-WindowPosition

The former setting doesn't have any issues regarding IME candidate
window positioning, but the latter does. The cursor position may
change every single time when you press the convert key because for
views::TextField, or more precisely for its gfx::RenderText, the
cursor position is defined as the bottom-right of the selected range
(if any) but the width of the selected range may change on each
conversion. For example, when pressing the conversion key, a
character in the selected range may change from a full-width Hiragana
Japanese character to a half-width Katakana character.

The CL swaps |start| and |end| of the selected range on Chrome OS
so that the cursor position becomes bottom *left* of the range which
doesn't change during conversion. Note that Blink does the same
(i.e. placing the cursor at the beginning of the selection range)
to prevent the window flicker.

This CL is for Chrome OS only and doesn't change other platforms'
behavior.

Demo:
 Omnibox without this fix: https://goo.gl/WVpjhE
 Omnibox with this fix: https://goo.gl/FH1fs9

BUG=311206
TEST=Follow the repro steps in crbug.com/311206 to confirm Omnibox
 no longer has the issue. Do the same with the find bar (Ctrl+F).
==========

to

==========
Prevent the IME candidate window from flickering on Chrome OS

On Chrome OS, IME candidate window is anchored to either bottom-left
of the whole composition text (when its WindowPosition settings is
"composition") or bottom-left of the cursor (when it's "cursor".)
https://developer.chrome.com/extensions/input_ime#type-WindowPosition

The former setting doesn't have any issues regarding IME candidate
window positioning, but the latter does. The cursor position may
change every single time when you press the convert key because for
views::TextField, or more precisely for its gfx::RenderText, the
cursor position is defined as the bottom-right of the selected range
(if any) but the width of the selected range may change on each
conversion. For example, when pressing the conversion key, a
character in the selected range may change from a full-width Hiragana
Japanese character to a half-width Katakana character.

The CL swaps |start| and |end| of the selected range on Chrome OS
so that the cursor position becomes bottom *left* of the range which
doesn't change during conversion. Note that Blink does the same
(i.e. placing the cursor at the beginning of the selection range)
to prevent the window flicker.

This CL is for Chrome OS only and doesn't change other platforms'
behavior. This does not affect Chrome OS IMEs that use "composition"
WindowPosition.

Demo:
 Omnibox without this fix: https://goo.gl/WVpjhE
 Omnibox with this fix: https://goo.gl/FH1fs9

BUG=311206
TEST=Follow the repro steps in crbug.com/311206 to confirm Omnibox
 no longer has the issue. Do the same with the find bar (Ctrl+F).
==========

[Yusuke Sato, 2017-05-17 23:58:03]

Description was changed from

==========
Prevent the IME candidate window from flickering on Chrome OS

On Chrome OS, IME candidate window is anchored to either bottom-left
of the whole composition text (when its WindowPosition settings is
"composition") or bottom-left of the cursor (when it's "cursor".)
https://developer.chrome.com/extensions/input_ime#type-WindowPosition

The former setting doesn't have any issues regarding IME candidate
window positioning, but the latter does. The cursor position may
change every single time when you press the convert key because for
views::TextField, or more precisely for its gfx::RenderText, the
cursor position is defined as the bottom-right of the selected range
(if any) but the width of the selected range may change on each
conversion. For example, when pressing the conversion key, a
character in the selected range may change from a full-width Hiragana
Japanese character to a half-width Katakana character.

The CL swaps |start| and |end| of the selected range on Chrome OS
so that the cursor position becomes bottom *left* of the range which
doesn't change during conversion. Note that Blink does the same
(i.e. placing the cursor at the beginning of the selection range)
to prevent the window flicker.

This CL is for Chrome OS only and doesn't change other platforms'
behavior. This does not affect Chrome OS IMEs that use "composition"
WindowPosition.

Demo:
 Omnibox without this fix: https://goo.gl/WVpjhE
 Omnibox with this fix: https://goo.gl/FH1fs9

BUG=311206
TEST=Follow the repro steps in crbug.com/311206 to confirm Omnibox
 no longer has the issue. Do the same with the find bar (Ctrl+F).
==========

to

==========
(WIP, writing/updating views_unittests)

Prevent the IME candidate window from flickering on Chrome OS

On Chrome OS, IME candidate window is anchored to either bottom-left
of the whole composition text (when its WindowPosition settings is
"composition") or bottom-left of the cursor (when it's "cursor".)
https://developer.chrome.com/extensions/input_ime#type-WindowPosition

The former setting doesn't have any issues regarding IME candidate
window positioning, but the latter does. The cursor position may
change every single time when you press the convert key because for
views::TextField, or more precisely for its gfx::RenderText, the
cursor position is defined as the bottom-right of the selected range
(if any) but the width of the selected range may change on each
conversion. For example, when pressing the conversion key, a
character in the selected range may change from a full-width Hiragana
Japanese character to a half-width Katakana character.

The CL swaps |start| and |end| of the selected range on Chrome OS
so that the cursor position becomes bottom *left* of the range which
doesn't change during conversion. Note that Blink does the same
(i.e. placing the cursor at the beginning of the selection range)
to prevent the window flicker.

This CL is for Chrome OS only and doesn't change other platforms'
behavior. This does not affect Chrome OS IMEs that use "composition"
WindowPosition.

Demo:
 Omnibox without this fix: https://goo.gl/WVpjhE
 Omnibox with this fix: https://goo.gl/FH1fs9

BUG=311206
TEST=Follow the repro steps in crbug.com/311206 to confirm Omnibox
 no longer has the issue. Do the same with the find bar (Ctrl+F).
==========

[commit-bot: I haz the power, 2017-05-18 01:23:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 01:23:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yusuke Sato, 2017-05-18 03:28:07]

The CQ bit was checked by yusukes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 03:29:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yusuke Sato, 2017-05-18 05:26:25]

The CQ bit was checked by yusukes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 05:27:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 06:46:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 06:46:37]

Dry run: This issue passed the CQ dry run.

[Yusuke Sato, 2017-05-18 17:13:39]

Description was changed from

==========
(WIP, writing/updating views_unittests)

Prevent the IME candidate window from flickering on Chrome OS

On Chrome OS, IME candidate window is anchored to either bottom-left
of the whole composition text (when its WindowPosition settings is
"composition") or bottom-left of the cursor (when it's "cursor".)
https://developer.chrome.com/extensions/input_ime#type-WindowPosition

The former setting doesn't have any issues regarding IME candidate
window positioning, but the latter does. The cursor position may
change every single time when you press the convert key because for
views::TextField, or more precisely for its gfx::RenderText, the
cursor position is defined as the bottom-right of the selected range
(if any) but the width of the selected range may change on each
conversion. For example, when pressing the conversion key, a
character in the selected range may change from a full-width Hiragana
Japanese character to a half-width Katakana character.

The CL swaps |start| and |end| of the selected range on Chrome OS
so that the cursor position becomes bottom *left* of the range which
doesn't change during conversion. Note that Blink does the same
(i.e. placing the cursor at the beginning of the selection range)
to prevent the window flicker.

This CL is for Chrome OS only and doesn't change other platforms'
behavior. This does not affect Chrome OS IMEs that use "composition"
WindowPosition.

Demo:
 Omnibox without this fix: https://goo.gl/WVpjhE
 Omnibox with this fix: https://goo.gl/FH1fs9

BUG=311206
TEST=Follow the repro steps in crbug.com/311206 to confirm Omnibox
 no longer has the issue. Do the same with the find bar (Ctrl+F).
==========

to

==========
Prevent the IME candidate window from flickering on Chrome OS

On Chrome OS, IME candidate window is anchored to either bottom-left
of the whole composition text (when its WindowPosition settings is
"composition") or bottom-left of the cursor (when it's "cursor".)
https://developer.chrome.com/extensions/input_ime#type-WindowPosition

The former setting doesn't have any issues regarding IME candidate
window positioning, but the latter (anchor to "cursor") does. The
cursor position may change every single time when you press the
convert key because for views::TextField, or more precisely for its
gfx::RenderText, the cursor position is defined as the bottom-right
of the selected range (if any) but the width of the selected range
may change on each conversion. For example, when pressing the
conversion key, a character in the selected range may change from a
full-width Hiragana Japanese character to a half-width Katakana
character.

The CL swaps |start| and |end| of the selected range on Chrome OS
so that the cursor position becomes bottom *left* of the range which
never changes during conversion. Note that Blink does the same
(i.e. placing the cursor at the beginning of the selection range)
to prevent the window flicker.

This CL is for Chrome OS only and doesn't change other platforms'
behavior. This does not affect Chrome OS IMEs that use "composition"
WindowPosition.

Demo:
 Omnibox without this fix: https://goo.gl/WVpjhE
 Omnibox with this fix: https://goo.gl/FH1fs9

BUG=311206
TEST=Follow the repro steps in crbug.com/311206 to confirm Omnibox
 no longer has the issue. Do the same with the find bar (Ctrl+F).
==========

[Yusuke Sato, 2017-05-18 17:13:54]

Patchset #2 (id:20001) has been deleted

[Yusuke Sato, 2017-05-18 17:14:30]

yusukes@chromium.org changed reviewers:
+ oshima@chromium.org, shuchen@chromium.org

[Yusuke Sato, 2017-05-18 17:14:32]

oshima@, shuchen@, could you have a look?

https://codereview.chromium.org/2886313002/diff/40001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield_model_unittest.cc (left):

https://codereview.chromium.org/2886313002/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model_unittest.cc:913:
EXPECT_EQ(gfx::Range(5, 7), model.render_text()->selection());
Removed as this line looked just redundant.

https://codereview.chromium.org/2886313002/diff/40001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield_model_unittest.cc (right):

https://codereview.chromium.org/2886313002/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model_unittest.cc:896:
composition.selection = gfx::Range(1, 3);
Changed to (1, 3) to make sure L898 has the precedence.

https://codereview.chromium.org/2886313002/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model_unittest.cc:938:
composition.selection = gfx::Range(0, 1);
Added a case where composition.selection exists but a (bold) underline doesn't.

[Shu Chen, 2017-05-21 03:54:30]

lgtm

https://codereview.chromium.org/2886313002/diff/40001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield_model_unittest.cc (right):

https://codereview.chromium.org/2886313002/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model_unittest.cc:938:
composition.selection = gfx::Range(0, 1);
On 2017/05/18 17:14:31, Yusuke Sato wrote:
> Added a case where composition.selection exists but a (bold) underline
doesn't.

nit: pls put this as a code comment.

[Yusuke Sato, 2017-05-22 05:48:46]

The CQ bit was checked by yusukes@chromium.org to run a CQ dry run

[Yusuke Sato, 2017-05-22 05:49:00]

https://codereview.chromium.org/2886313002/diff/40001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield_model_unittest.cc (right):

https://codereview.chromium.org/2886313002/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model_unittest.cc:938:
composition.selection = gfx::Range(0, 1);
On 2017/05/21 03:54:30, Shu Chen wrote:
> On 2017/05/18 17:14:31, Yusuke Sato wrote:
> > Added a case where composition.selection exists but a (bold) underline
> doesn't.
> 
> nit: pls put this as a code comment.

Done.

[commit-bot: I haz the power, 2017-05-22 05:49:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yusuke Sato, 2017-05-22 05:49:32]

oshima@: please do an owner review. Thanks.

[commit-bot: I haz the power, 2017-05-22 06:57:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 06:57:02]

Dry run: This issue passed the CQ dry run.

[oshima, 2017-05-22 09:55:21]

https://codereview.chromium.org/2886313002/diff/60001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield_model.cc (right):

https://codereview.chromium.org/2886313002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model.cc:299:
render_text->SelectRange(gfx::Range(cursor + start, cursor + end));
what's the reason why we don't change the UI side to align the window to left
instead?

[Yusuke Sato, 2017-05-22 16:55:02]

https://codereview.chromium.org/2886313002/diff/60001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield_model.cc (right):

https://codereview.chromium.org/2886313002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model.cc:299:
render_text->SelectRange(gfx::Range(cursor + start, cursor + end));
On 2017/05/22 09:55:21, oshima wrote:
> what's the reason why we don't change the UI side to align the window to left
> instead?

That's another option, but strictly speaking, if we do that, the new behavior
won't match chrome.input.ime API document at
https://developer.chrome.com/extensions/input_ime#type-WindowPosition which
explicitly says that the window position follows the cursor, not the selection
range. We could add yet another WindowPosition type like "selection" and let the
Japanese IME migrate to the new mode, but this seems overkill given that the
Blink side (RWHVA) already implements the existing "cursor" mode very well.

So instead of changing the UI behavior, this CL tried to improve Textfield as
good as RWHVA regarding the "cursor" mode support. WDYT?

[oshima, 2017-05-22 18:04:26]

On 2017/05/22 16:55:02, Yusuke Sato wrote:
>
https://codereview.chromium.org/2886313002/diff/60001/ui/views/controls/textf...
> File ui/views/controls/textfield/textfield_model.cc (right):
> 
>
https://codereview.chromium.org/2886313002/diff/60001/ui/views/controls/textf...
> ui/views/controls/textfield/textfield_model.cc:299:
> render_text->SelectRange(gfx::Range(cursor + start, cursor + end));
> On 2017/05/22 09:55:21, oshima wrote:
> > what's the reason why we don't change the UI side to align the window to
left
> > instead?
> 
> That's another option, but strictly speaking, if we do that, the new behavior
> won't match chrome.input.ime API document at
> https://developer.chrome.com/extensions/input_ime#type-WindowPosition which
> explicitly says that the window position follows the cursor, not the selection
> range. We could add yet another WindowPosition type like "selection" and let
the
> Japanese IME migrate to the new mode, but this seems overkill given that the
> Blink side (RWHVA) already implements the existing "cursor" mode very well.
> 
> So instead of changing the UI behavior, this CL tried to improve Textfield as
> good as RWHVA regarding the "cursor" mode support. WDYT?

Two q:
1) Why can't (or shouldn't) textfield use "selection" ?
2) Won't the app that sets "cursor" have the same problem on cros?

[Yusuke Sato, 2017-05-22 19:57:41]

Answers inlined:

On 2017/05/22 18:04:26, oshima wrote:
> On 2017/05/22 16:55:02, Yusuke Sato wrote:
> >
>
https://codereview.chromium.org/2886313002/diff/60001/ui/views/controls/textf...
> > File ui/views/controls/textfield/textfield_model.cc (right):
> > 
> >
>
https://codereview.chromium.org/2886313002/diff/60001/ui/views/controls/textf...
> > ui/views/controls/textfield/textfield_model.cc:299:
> > render_text->SelectRange(gfx::Range(cursor + start, cursor + end));
> > On 2017/05/22 09:55:21, oshima wrote:
> > > what's the reason why we don't change the UI side to align the window to
> left
> > > instead?
> > 
> > That's another option, but strictly speaking, if we do that, the new
behavior
> > won't match chrome.input.ime API document at
> > https://developer.chrome.com/extensions/input_ime#type-WindowPosition which
> > explicitly says that the window position follows the cursor, not the
selection
> > range. We could add yet another WindowPosition type like "selection" and let
> the
> > Japanese IME migrate to the new mode, but this seems overkill given that the
> > Blink side (RWHVA) already implements the existing "cursor" mode very well.
> > 
> > So instead of changing the UI behavior, this CL tried to improve Textfield
as
> > good as RWHVA regarding the "cursor" mode support. WDYT?
> 
> Two q:
> 1) Why can't (or shouldn't) textfield use "selection" ?

This is because WindowPosition is a per-IME setting. It's not per UI element
that implements ui::TextInputClient.

Or, if you're saying that we should instead change TextField::GetCaretBounds()
from

 gfx::Rect Textfield::GetCaretBounds() const {
   gfx::Rect rect = GetRenderText()->GetUpdatedCursorBounds();
   ConvertRectToScreen(this, &rect);
   return rect;
 }

to something like

 gfx::Rect Textfield::GetCaretBounds() const {
   if (selection_exists)
     return left_end_of_the_selected_range;
   gfx::Rect rect = GetRenderText()->GetUpdatedCursorBounds();
   ConvertRectToScreen(this, &rect);
   return rect;
 }

to fix the window flicker, I think that would also work.

> 2) Won't the app that sets "cursor" have the same problem on cros?

ARC apps (actually their ui::TextInputClient which is
components/arc/ime/arc_ime_service.cc) don't have the window flickering issue.
Just in case, I've tested it with both ARC-M and ARC-N with the Japanese IME
whose WindowPosition setting is "cursor" and didn't see the issue. Regarding
candidate window positioning, components/arc/ime/arc_ime_service.cc behaves more
like RWHVA.

[oshima, 2017-05-23 15:34:09]

lgtm this CL

so the better fix would be to fix RenderText to keep both cursor position and
selection separately?

[Yusuke Sato, 2017-05-23 17:19:20]

On 2017/05/23 15:34:09, oshima (OOO May 22-24) wrote:
> lgtm this CL
> 
> so the better fix would be to fix RenderText to keep both cursor position and
> selection separately?

Yeah, that allows us to remove the std::swap code. The model will likely have to
have more code to handle cursor position separately, though.

[Yusuke Sato, 2017-05-23 17:19:28]

The CQ bit was checked by yusukes@chromium.org

[Yusuke Sato, 2017-05-23 17:19:28]

The patchset sent to the CQ was uploaded after l-g-t-m from shuchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2886313002/#ps60001
(title: "address a comment from shuchen@")

[commit-bot: I haz the power, 2017-05-23 17:19:54]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 18:18:49]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495559968393520,
"parent_rev": "a5ac6b4b9fc2170c8edad07d6a7666674da7f9ab", "commit_rev":
"e635c3e1ed9ed9a0838beba6893b996a63c3e6a1"}

[commit-bot: I haz the power, 2017-05-23 18:19:01]

Description was changed from

==========
Prevent the IME candidate window from flickering on Chrome OS

On Chrome OS, IME candidate window is anchored to either bottom-left
of the whole composition text (when its WindowPosition settings is
"composition") or bottom-left of the cursor (when it's "cursor".)
https://developer.chrome.com/extensions/input_ime#type-WindowPosition

The former setting doesn't have any issues regarding IME candidate
window positioning, but the latter (anchor to "cursor") does. The
cursor position may change every single time when you press the
convert key because for views::TextField, or more precisely for its
gfx::RenderText, the cursor position is defined as the bottom-right
of the selected range (if any) but the width of the selected range
may change on each conversion. For example, when pressing the
conversion key, a character in the selected range may change from a
full-width Hiragana Japanese character to a half-width Katakana
character.

The CL swaps |start| and |end| of the selected range on Chrome OS
so that the cursor position becomes bottom *left* of the range which
never changes during conversion. Note that Blink does the same
(i.e. placing the cursor at the beginning of the selection range)
to prevent the window flicker.

This CL is for Chrome OS only and doesn't change other platforms'
behavior. This does not affect Chrome OS IMEs that use "composition"
WindowPosition.

Demo:
 Omnibox without this fix: https://goo.gl/WVpjhE
 Omnibox with this fix: https://goo.gl/FH1fs9

BUG=311206
TEST=Follow the repro steps in crbug.com/311206 to confirm Omnibox
 no longer has the issue. Do the same with the find bar (Ctrl+F).
==========

to

==========
Prevent the IME candidate window from flickering on Chrome OS

On Chrome OS, IME candidate window is anchored to either bottom-left
of the whole composition text (when its WindowPosition settings is
"composition") or bottom-left of the cursor (when it's "cursor".)
https://developer.chrome.com/extensions/input_ime#type-WindowPosition

The former setting doesn't have any issues regarding IME candidate
window positioning, but the latter (anchor to "cursor") does. The
cursor position may change every single time when you press the
convert key because for views::TextField, or more precisely for its
gfx::RenderText, the cursor position is defined as the bottom-right
of the selected range (if any) but the width of the selected range
may change on each conversion. For example, when pressing the
conversion key, a character in the selected range may change from a
full-width Hiragana Japanese character to a half-width Katakana
character.

The CL swaps |start| and |end| of the selected range on Chrome OS
so that the cursor position becomes bottom *left* of the range which
never changes during conversion. Note that Blink does the same
(i.e. placing the cursor at the beginning of the selection range)
to prevent the window flicker.

This CL is for Chrome OS only and doesn't change other platforms'
behavior. This does not affect Chrome OS IMEs that use "composition"
WindowPosition.

Demo:
 Omnibox without this fix: https://goo.gl/WVpjhE
 Omnibox with this fix: https://goo.gl/FH1fs9

BUG=311206
TEST=Follow the repro steps in crbug.com/311206 to confirm Omnibox
 no longer has the issue. Do the same with the find bar (Ctrl+F).

Review-Url: https://codereview.chromium.org/2886313002
Cr-Commit-Position: refs/heads/master@{#473984}
Committed:
https://chromium.googlesource.com/chromium/src/+/e635c3e1ed9ed9a0838beba6893b...
==========

[commit-bot: I haz the power, 2017-05-23 18:19:02]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/e635c3e1ed9ed9a0838beba6893b...