Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2709633003:
Fix: Cursor of Omnibox stays in LTR mode after setting to RTL mode (Closed)
DescriptionCursor of Omnibox stays in LTR mode after setting to RTL mode
Fixed the bug caused by https://codereview.chromium.org/2660593002/.
The newly introduced |cursor_view_| did not consider RTL input mode.
TEST=TextfieldTest.TextCursorPositionInRTLTest and
TextfieldTest.TextCursorPositionInLTRTest
BUG=693437
Review-Url: https://codereview.chromium.org/2709633003
Cr-Commit-Position: refs/heads/master@{#452536}
Committed: https://chromium.googlesource.com/chromium/src/+/947341f55da060dd30bd492ab806049fb23bb109
Patch Set 1 #
Total comments: 4
Patch Set 2 : address comments #
Messages
Total messages: 26 (21 generated)
Description was changed from ========== update cursor location update cursor location temp BUG= ========== to ========== Cursor of Omnibox stays in LTR mode after setting to RTL mode BUG=693437 ==========
The CQ bit was checked by yiyix@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.
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
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.
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Cursor of Omnibox stays in LTR mode after setting to RTL mode BUG=693437 ========== to ========== Cursor of Omnibox stays in LTR mode after setting to RTL mode Fixed the bug caused by https://codereview.chromium.org/2660593002/. The newly introduced |cursor_view_| did not consider RTL input mode. TEST=TextfieldTest.TextCursorPositionInRTLTest and TextfieldTest.TextCursorPositionInLTRTest BUG=693437 ==========
yiyix@chromium.org changed reviewers: + sadrul@chromium.org
The CQ bit was checked by yiyix@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...
@sadrul, could you please review this change?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2709633003/diff/40001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2709633003/diff/40001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:2402: base::i18n::SetICUDefaultLocale("he"); I think you need to restore |locale| at the end of the test. Consider using ScopedRTL from view_unittest.cc https://codereview.chromium.org/2709633003/diff/40001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:2411: // Text cursor stays at same plaec after inserting new charactors in RTL mode. *place
https://codereview.chromium.org/2709633003/diff/40001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2709633003/diff/40001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:2402: base::i18n::SetICUDefaultLocale("he"); On 2017/02/23 03:41:21, sadrul (OOO) wrote: > I think you need to restore |locale| at the end of the test. > > Consider using ScopedRTL from view_unittest.cc Done, i added the restoring part. https://codereview.chromium.org/2709633003/diff/40001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:2411: // Text cursor stays at same plaec after inserting new charactors in RTL mode. On 2017/02/23 03:41:21, sadrul (OOO) wrote: > *place Done.
The CQ bit was checked by yiyix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2709633003/#ps60001 (title: "address comments")
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": 1487867940075870,
"parent_rev": "960b27e7436cf7972cebb76b5d5f38b2497d52ba", "commit_rev":
"947341f55da060dd30bd492ab806049fb23bb109"}
Message was sent while issue was closed.
Description was changed from ========== Cursor of Omnibox stays in LTR mode after setting to RTL mode Fixed the bug caused by https://codereview.chromium.org/2660593002/. The newly introduced |cursor_view_| did not consider RTL input mode. TEST=TextfieldTest.TextCursorPositionInRTLTest and TextfieldTest.TextCursorPositionInLTRTest BUG=693437 ========== to ========== Cursor of Omnibox stays in LTR mode after setting to RTL mode Fixed the bug caused by https://codereview.chromium.org/2660593002/. The newly introduced |cursor_view_| did not consider RTL input mode. TEST=TextfieldTest.TextCursorPositionInRTLTest and TextfieldTest.TextCursorPositionInLTRTest BUG=693437 Review-Url: https://codereview.chromium.org/2709633003 Cr-Commit-Position: refs/heads/master@{#452536} Committed: https://chromium.googlesource.com/chromium/src/+/947341f55da060dd30bd492ab806... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/947341f55da060dd30bd492ab806... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
