|
|
Created:
3 years, 7 months ago by Yusuke Sato Modified:
3 years, 7 months ago CC:
chromium-reviews, nona+watch_chromium.org, James Su, shuchen+watch_chromium.org, yusukes+watch_chromium.org, tfarina Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrevent 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
Patch Set 1 #Patch Set 2 : add more tests #
Total comments: 5
Patch Set 3 : address a comment from shuchen@ #
Total comments: 2
Messages
Total messages: 38 (26 generated)
The CQ bit was checked by yusukes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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). ==========
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). ==========
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). ==========
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). ==========
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). ==========
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by yusukes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yusukes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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). ==========
Patchset #2 (id:20001) has been deleted
yusukes@chromium.org changed reviewers: + oshima@chromium.org, shuchen@chromium.org
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.
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.
The CQ bit was checked by yusukes@chromium.org to run a CQ dry run
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.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
oshima@: please do an owner review. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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?
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?
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?
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.
lgtm this CL so the better fix would be to fix RenderText to keep both cursor position and selection separately?
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.
The CQ bit was checked by yusukes@chromium.org
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@")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495559968393520, "parent_rev": "a5ac6b4b9fc2170c8edad07d6a7666674da7f9ab", "commit_rev": "e635c3e1ed9ed9a0838beba6893b996a63c3e6a1"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e635c3e1ed9ed9a0838beba6893b... |