Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1847583003:
Fix setComposingText when newCursorPosition != 1 (Closed)
|
Created:
4 years, 8 months ago by yabinh Modified:
4 years, 7 months ago CC:
blink-reviews, chromium-reviews, darin-cc_chromium.org, jam, loading-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAfter fixing, it behaves like this:
Case1: When newCursorPosition < 1, the cursor will move to the left of the composing text. Specially, when cursor position exceeds the left boundary, it will move to the left boundary.
Case2: When newCursorPosition > 1, the cursor will move to the right of the composing text. Specially, when cursor position exceeds the right boundary, it will move to the right boundary.
BUG=570920
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/25dec933e90def173fe9682a6d2b80d46283be52
Cr-Commit-Position: refs/heads/master@{#393198}
Patch Set 1 : Fix setComposingText when newCursorPosition < 1 #
Total comments: 13
Patch Set 2 : Fix setComposingText when newCursorPosition > 1 #Patch Set 3 : Adding C++ unit test #
Total comments: 12
Patch Set 4 : Modified some C++ unit tests #
Total comments: 10
Patch Set 5 : Add some C++ unit tests. #
Total comments: 12
Patch Set 6 : Without changing PlainTextRange #
Total comments: 3
Patch Set 7 : For multiple nodes. #
Total comments: 3
Patch Set 8 : Use a different method to handle exceeding right boundary. #Patch Set 9 : Avoid using innerText() #
Total comments: 5
Patch Set 10 : Store Position objects as local variables... #
Messages
Total messages: 58 (21 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== When cursor position exceeds the left boundary, it will move the left boundary. Howvever, when newCursorPosition > 1, it will behave as newCursorPosition = 1. I'll fix that later. BUG=570920 ========== to ========== When cursor position exceeds the left boundary, it will move the left boundary. Howevver, when newCursorPosition > 1, it will behave as newCursorPosition = 1. I'll fix that later. BUG=570920 ==========
yabinh@chromium.org changed reviewers: + aelias@chromium.org, changwan@chromium.org
On 2016/03/31 13:22:56, yabinh wrote: > mailto:yabinh@chromium.org changed reviewers: > + mailto:aelias@chromium.org, mailto:changwan@chromium.org I didn't change "base/feature_list.cc". I tried using "gclient sync" on master branch, and using "git rebase master on my working branch, but I still got "base/feature_list.cc" uploaded.
Description was changed from ========== When cursor position exceeds the left boundary, it will move the left boundary. Howevver, when newCursorPosition > 1, it will behave as newCursorPosition = 1. I'll fix that later. BUG=570920 ========== to ========== When cursor position exceeds the left boundary, it will move the left boundary. However, when newCursorPosition > 1, it will behave as newCursorPosition = 1. I'll fix that later. BUG=570920 ==========
Description was changed from ========== When cursor position exceeds the left boundary, it will move the left boundary. However, when newCursorPosition > 1, it will behave as newCursorPosition = 1. I'll fix that later. BUG=570920 ========== to ========== When cursor position exceeds the left boundary, it will move to the left boundary. However, when newCursorPosition > 1, it will behave as newCursorPosition = 1. I'll fix that later. BUG=570920 ==========
Description was changed from ========== When cursor position exceeds the left boundary, it will move to the left boundary. However, when newCursorPosition > 1, it will behave as newCursorPosition = 1. I'll fix that later. BUG=570920 ========== to ========== After fixing, it behaves like this: When cursor position exceeds the left boundary, it will move to the left boundary. However, when newCursorPosition > 1, it will behave as newCursorPosition = 1. I'll fix that later. BUG=570920 ==========
Description was changed from ========== After fixing, it behaves like this: When cursor position exceeds the left boundary, it will move to the left boundary. However, when newCursorPosition > 1, it will behave as newCursorPosition = 1. I'll fix that later. BUG=570920 ========== to ========== After fixing, it behaves like this: It can work well when newCursorPosition < 1. When cursor position exceeds the left boundary, it will move to the left boundary. However, when newCursorPosition > 1, it will behave as newCursorPosition = 1. This is not correct. I'll fix it later. BUG=570920 ==========
https://codereview.chromium.org/1847583003/diff/40001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/1847583003/diff/40001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:109: public void testSetComposingTextWhennewCursorPositionsChanges() throws Throwable { WhenNewCursorPositionChanges --> This sounds as if the value actually changes dynamically. How about testSetComposingTextForDifferentNewCursorPositions? Or testSetComposingText_NewCursorPosition https://codereview.chromium.org/1847583003/diff/40001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:111: // BaseInputConnection. 1. s/there are bugs/there is a bug 2. casing error: s/setcomposingText/setComposingText 3. It sounds as if setComposingText didn't work at all. https://codereview.chromium.org/1847583003/diff/40001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:113: // after composing text this doesn't deliver enough context. what is after composing text? https://codereview.chromium.org/1847583003/diff/40001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:117: finishComposingText(); probably you don't need to finish composing text every time, do you? https://codereview.chromium.org/1847583003/diff/40001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:127: // to left boundary could you make it more clear, e.g. The new position will be on the left of composing text when a negative value is given. https://codereview.chromium.org/1847583003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1847583003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:322: int start = std::min(std::max((int)baseOffset + selectionStart, 0), (int)extentOffset); baseOffset >= 0 so you just need to make sure selectionStart >= 0. I believe that should happen in some other place.
Description was changed from ========== After fixing, it behaves like this: It can work well when newCursorPosition < 1. When cursor position exceeds the left boundary, it will move to the left boundary. However, when newCursorPosition > 1, it will behave as newCursorPosition = 1. This is not correct. I'll fix it later. BUG=570920 ========== to ========== After fixing, it behaves like this: Case1: When newCursorPosition < 1, the cursor will move to the left of the composing text. Specially, when cursor position exceeds the left boundary, it will move to the left boundary. Case2: When newCursorPosition > 1, the cursor will move to the right of the composing text. Specially, when cursor position exceeds the right boundary, it will move to the right boundary. BUG=570920 ==========
https://codereview.chromium.org/1847583003/diff/40001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/1847583003/diff/40001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:109: public void testSetComposingTextWhennewCursorPositionsChanges() throws Throwable { On 2016/04/01 00:37:26, Changwan Ryu wrote: > WhenNewCursorPositionChanges > > --> This sounds as if the value actually changes dynamically. How about > testSetComposingTextForDifferentNewCursorPositions? Or > > testSetComposingText_NewCursorPosition Acknowledged. https://codereview.chromium.org/1847583003/diff/40001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:111: // BaseInputConnection. On 2016/04/01 00:37:26, Changwan Ryu wrote: > 1. s/there are bugs/there is a bug > 2. casing error: s/setcomposingText/setComposingText > 3. It sounds as if setComposingText didn't work at all. Acknowledged. https://codereview.chromium.org/1847583003/diff/40001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:113: // after composing text On 2016/04/01 00:37:26, Changwan Ryu wrote: > this doesn't deliver enough context. what is after composing text? Acknowledged. https://codereview.chromium.org/1847583003/diff/40001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:117: finishComposingText(); On 2016/04/01 00:37:26, Changwan Ryu wrote: > probably you don't need to finish composing text every time, do you? Acknowledged. https://codereview.chromium.org/1847583003/diff/40001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:127: // to left boundary On 2016/04/01 00:37:26, Changwan Ryu wrote: > could you make it more clear, e.g. The new position will be on the left of > composing text when a negative value is given. Acknowledged. https://codereview.chromium.org/1847583003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1847583003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:322: int start = std::min(std::max((int)baseOffset + selectionStart, 0), (int)extentOffset); On 2016/04/01 00:37:26, Changwan Ryu wrote: > baseOffset >= 0 so you just need to make sure selectionStart >= 0. I believe > that should happen in some other place. selectionStart can be negative, because it is the relative position of the cursor. If newCursorPosition < 1, selectionStart = newCursorPosition; If newCursorPosition >= 1, selectionStart = newCursorPosition + text.length() - 1.
Patchset #2 (id:60001) has been deleted
https://codereview.chromium.org/1847583003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1847583003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:322: int start = std::min(std::max((int)baseOffset + selectionStart, 0), (int)extentOffset); On 2016/04/01 04:14:17, yabinh wrote: > On 2016/04/01 00:37:26, Changwan Ryu wrote: > > baseOffset >= 0 so you just need to make sure selectionStart >= 0. I believe > > that should happen in some other place. > > selectionStart can be negative, because it is the relative position of the > cursor. > If newCursorPosition < 1, selectionStart = newCursorPosition; If > newCursorPosition >= 1, selectionStart = newCursorPosition + text.length() - 1. I see. Thanks for clarification. By the way, we prefer using static casting to c-style casting. So it should be static_cast<int>(baseOffset). Also you'll need unit tests around the changed behavior in Blink. I was a bit worried that there would be a slight chance of overflow converting from unsigned to int, but there are so many other places which does it incorrectly. I'll punt unsigned-int conversion issues to aelias@.
Hi Changwan@, I added C++ unit test. Could you take another look?
https://codereview.chromium.org/1847583003/diff/100001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/1847583003/diff/100001/base/feature_list.cc#n... base/feature_list.cc:39: : initialized_(false), initialized_from_command_line_(false) {} please remove unnecessary changes from this patch https://codereview.chromium.org/1847583003/diff/100001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/1847583003/diff/100001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:117: finishComposingText(); could you remove all the instances of finishComposingText? maybe except at the end.. https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:323: int start = std::max(static_cast<int>(baseOffset) + selectionStart, 0); Why do you remove the logic around extentOffset? https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:331: const EphemeralRange selectedRange = PlainTextRange(start, end).createRange(*rootEditableElement); Why did you remove RefPtrWillberawPtr? https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:64: // Start is on the left boundary, and End is on the right boundary The below code seems to belong to PlainTextRange tests. https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/PlainTextRange.cpp (right): https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRange.cpp:151: if (!startRangeFound) { Could you leave some comment on how startRangeFound is related to the right boundary exceeding case? Also, is there any genuine case where we want to return empty ephemeral range? e.g. there was no text information in this node.
https://codereview.chromium.org/1847583003/diff/100001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/1847583003/diff/100001/base/feature_list.cc#n... base/feature_list.cc:39: : initialized_(false), initialized_from_command_line_(false) {} On 2016/04/04 06:23:13, Changwan Ryu wrote: > please remove unnecessary changes from this patch Acknowledged. https://codereview.chromium.org/1847583003/diff/100001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/1847583003/diff/100001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:117: finishComposingText(); On 2016/04/04 06:23:13, Changwan Ryu wrote: > could you remove all the instances of finishComposingText? maybe except at the > end.. I need the cursor stay in the middle of text before calling setComposingText(). So I need to call finishComposingText(). https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:323: int start = std::max(static_cast<int>(baseOffset) + selectionStart, 0); On 2016/04/04 06:23:13, Changwan Ryu wrote: > Why do you remove the logic around extentOffset? extendoffset = length(text before composing region) + length(text). Since there might be text on the right of the composing region, if I don't remove it, the cursor can't move to right further. I.E., setComposingText(text,newCursorPosition) will behave the same as setComposingText(text,1) for any newCursorPosition>1. https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:331: const EphemeralRange selectedRange = PlainTextRange(start, end).createRange(*rootEditableElement); On 2016/04/04 06:23:13, Changwan Ryu wrote: > Why did you remove RefPtrWillberawPtr? RefPtrWillberawPtr is only used to create selectedRange. Since another method is used to create selectedRange, RefPtrWillberawPtr is not used any more. https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:64: // Start is on the left boundary, and End is on the right boundary On 2016/04/04 06:23:13, Changwan Ryu wrote: > The below code seems to belong to PlainTextRange tests. OK. I'll create a new file to put the test. https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/PlainTextRange.cpp (right): https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRange.cpp:151: if (!startRangeFound) { On 2016/04/04 06:23:13, Changwan Ryu wrote: > Could you leave some comment on how startRangeFound is related to the right > boundary exceeding case? OK. > Also, is there any genuine case where we want to return empty ephemeral range? > e.g. there was no text information in this node. startRangeFound = start() >= docTextPosition && start() <= docTextPosition + len. The former ensure new cursor doesn't exceed the left boundary, and the latter ensures the right boundary. In the old implementation, it will return empty ephemeral range if and only if new cursor exceeds the boundary.
On 2016/04/04 08:31:51, yabinh wrote: > https://codereview.chromium.org/1847583003/diff/100001/base/feature_list.cc > File base/feature_list.cc (right): > > https://codereview.chromium.org/1847583003/diff/100001/base/feature_list.cc#n... > base/feature_list.cc:39: : initialized_(false), > initialized_from_command_line_(false) {} > On 2016/04/04 06:23:13, Changwan Ryu wrote: > > please remove unnecessary changes from this patch > > Acknowledged. > > https://codereview.chromium.org/1847583003/diff/100001/content/public/android... > File > content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java > (right): > > https://codereview.chromium.org/1847583003/diff/100001/content/public/android... > content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:117: > finishComposingText(); > On 2016/04/04 06:23:13, Changwan Ryu wrote: > > could you remove all the instances of finishComposingText? maybe except at the > > end.. > > I need the cursor stay in the middle of text before calling setComposingText(). > So I need to call finishComposingText(). > > https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): > > https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/InputMethodController.cpp:323: int start > = std::max(static_cast<int>(baseOffset) + selectionStart, 0); > On 2016/04/04 06:23:13, Changwan Ryu wrote: > > Why do you remove the logic around extentOffset? > extendoffset = length(text before composing region) + length(text). > > Since there might be text on the right of the composing region, if I don't > remove it, the cursor can't move to right further. I.E., > setComposingText(text,newCursorPosition) will behave the same as > setComposingText(text,1) for any newCursorPosition>1. > > https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/InputMethodController.cpp:331: const > EphemeralRange selectedRange = PlainTextRange(start, > end).createRange(*rootEditableElement); > On 2016/04/04 06:23:13, Changwan Ryu wrote: > > Why did you remove RefPtrWillberawPtr? > > RefPtrWillberawPtr is only used to create selectedRange. Since another method is > used to create selectedRange, RefPtrWillberawPtr is not used any more. > > https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp > (right): > > https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:64: // > Start is on the left boundary, and End is on the right boundary > On 2016/04/04 06:23:13, Changwan Ryu wrote: > > The below code seems to belong to PlainTextRange tests. > > OK. I'll create a new file to put the test. > > https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/PlainTextRange.cpp (right): > > https://codereview.chromium.org/1847583003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/PlainTextRange.cpp:151: if > (!startRangeFound) { > On 2016/04/04 06:23:13, Changwan Ryu wrote: > > Could you leave some comment on how startRangeFound is related to the right > > boundary exceeding case? > OK. > > Also, is there any genuine case where we want to return empty ephemeral range? > > e.g. there was no text information in this node. > > startRangeFound = start() >= docTextPosition && start() <= docTextPosition + > len. > The former ensure new cursor doesn't exceed the left boundary, and the latter > ensures the right boundary. > > In the old implementation, it will return empty ephemeral range if and only if > new cursor exceeds the boundary. You mean if and only if start position exceeds the right-hand boundary. Makes sense to me now. Please comment accordingly.
Patchset #4 (id:120001) has been deleted
changwan@, I uploaded a new patch. Please take another look.
Patchset #4 (id:140001) has been deleted
https://codereview.chromium.org/1847583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/1847583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:180: EXPECT_EQ(6, range->endOffset()); I think you need more tests, e.g. (-1, 8), (-4, -2), (3, 8) https://codereview.chromium.org/1847583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/PlainTextRange.cpp (right): https://codereview.chromium.org/1847583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRange.cpp:157: // in Android, behaves in that way. how about renaming startRangeFound as startOutOfBound and not adding any comment here. https://codereview.chromium.org/1847583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/PlainTextRangeTest.cpp (right): https://codereview.chromium.org/1847583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRangeTest.cpp:49: TEST_F(PlainTextRangeTest, CreateDifferentRanges) CreateRange? https://codereview.chromium.org/1847583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRangeTest.cpp:94: TEST_F(PlainTextRangeTest, CreateDifferentRangesForSelection) CreateRangeForSelection should be enough? Could you also try to write tests for contenteditable? https://codereview.chromium.org/1847583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRangeTest.cpp:104: // Start and End are on the right boundary no need to capitalize E and you need to add a period at the end of a full sentence.
https://codereview.chromium.org/1847583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/1847583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:180: EXPECT_EQ(6, range->endOffset()); On 2016/04/05 07:50:37, Changwan Ryu wrote: > I think you need more tests, e.g. (-1, 8), (-4, -2), (3, 8) Acknowledged. https://codereview.chromium.org/1847583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/PlainTextRange.cpp (right): https://codereview.chromium.org/1847583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRange.cpp:157: // in Android, behaves in that way. On 2016/04/05 07:50:37, Changwan Ryu wrote: > how about renaming startRangeFound as startOutOfBound and not adding any comment > here. Acknowledged. https://codereview.chromium.org/1847583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/PlainTextRangeTest.cpp (right): https://codereview.chromium.org/1847583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRangeTest.cpp:49: TEST_F(PlainTextRangeTest, CreateDifferentRanges) On 2016/04/05 07:50:37, Changwan Ryu wrote: > CreateRange? Acknowledged. https://codereview.chromium.org/1847583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRangeTest.cpp:94: TEST_F(PlainTextRangeTest, CreateDifferentRangesForSelection) On 2016/04/05 07:50:37, Changwan Ryu wrote: > CreateRangeForSelection should be enough? > > Could you also try to write tests for contenteditable? Acknowledged. https://codereview.chromium.org/1847583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRangeTest.cpp:104: // Start and End are on the right boundary On 2016/04/05 07:50:37, Changwan Ryu wrote: > no need to capitalize E and you need to add a period at the end of a full > sentence. Acknowledged.
@changwan, I uploaded a new patch. Could you take another look?
There are still test failures. Please fix them first. https://codereview.chromium.org/1847583003/diff/180001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/1847583003/diff/180001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:124: // Cursor is on the left boundary for the completeness of tests, could you also add test cases for when the new cursor is between left boundary and composing texts? https://codereview.chromium.org/1847583003/diff/180001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:135: // Cursor is on the right boundary ditto https://codereview.chromium.org/1847583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/PlainTextRange.cpp (right): https://codereview.chromium.org/1847583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRange.cpp:122: // resultStart should be assigned the first time foundStart is true. I don't feel that this comment is needed here. https://codereview.chromium.org/1847583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRange.cpp:123: if (foundStart && startOutOfBound) { no need to check startOutOfBound? https://codereview.chromium.org/1847583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/PlainTextRangeTest.cpp (right): https://codereview.chromium.org/1847583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRangeTest.cpp:59: // start and end are on the right boundary. what happens when start / end are within boundary? https://codereview.chromium.org/1847583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRangeTest.cpp:96: // Since the cursor never exceeds the left boundary, our test should focus on right boundary cases. this line is not needed https://codereview.chromium.org/1847583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRangeTest.cpp:139: TEST_F(PlainTextRangeTest, CreateRangeForEditableWithEmptyNode) s/Editable/ContentEditable/ here and other places https://codereview.chromium.org/1847583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRangeTest.cpp:141: // Since the cursor never exceeds the left boundary, our test should focus on right boundary cases. This line is probably not needed. https://codereview.chromium.org/1847583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRangeTest.cpp:198: TEST_F(PlainTextRangeTest, CreateRangeForEditableWithLineBreakInTheMiddle) could you also check the cases where start and end are within boundaries? https://codereview.chromium.org/1847583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRangeTest.cpp:200: // Since the cursor never exceeds the left boundary, our test should focus on right boundary cases. this line is not needed
https://codereview.chromium.org/1847583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/PlainTextRange.cpp (right): https://codereview.chromium.org/1847583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRange.cpp:123: if (foundStart && startOutOfBound) { On 2016/04/07 11:46:19, Changwan Ryu wrote: > no need to check startOutOfBound? In "foundStart = start() >= docTextPosition && start() <= docTextPosition + len", there is a "=". It means that foundStart can be true for 2 times in the loop. But resultStart is assigned correctly only in the first time. For example, if there are 3 nodes:"hello","\n","world!", and we want to move the cursor to 5, like: hello* world! where * stands for cursor. foundstart will be true both in the first node(because start()==0, docTextPosition==0, len==5, and 0 <= 0 <=5) and in the second node(because start()==5, docTextPosition==5, len==1, and 5 <= 5 <=6). If resultStart is assigned in the second time, the cursor will be like this: hello *world! which is not correct.
https://codereview.chromium.org/1847583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/PlainTextRange.cpp (right): https://codereview.chromium.org/1847583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/PlainTextRange.cpp:123: if (foundStart && startOutOfBound) { I was wrong about it. I thought the selected range was wrong, and I should change it. But it turned out that although the selected range was wrong, it was changed in SelectionEditor::setSelectedRange. So there is no need to change it in advance. And also, it shouldn’t be changed twice, because it will cause some problems for certain boundary cases. Anyway, no need to change “if(foundStart)”.
Description was changed from ========== After fixing, it behaves like this: Case1: When newCursorPosition < 1, the cursor will move to the left of the composing text. Specially, when cursor position exceeds the left boundary, it will move to the left boundary. Case2: When newCursorPosition > 1, the cursor will move to the right of the composing text. Specially, when cursor position exceeds the right boundary, it will move to the right boundary. BUG=570920 ========== to ========== After fixing, it behaves like this: Case1: When newCursorPosition < 1, the cursor will move to the left of the composing text. Specially, when cursor position exceeds the left boundary, it will move to the left boundary. Case2: When newCursorPosition > 1, the cursor will move to the right of the composing text. Specially, when cursor position exceeds the right boundary, it will move to the right boundary. BUG=570920 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Patchset #6 (id:200001) has been deleted
changwan@, could you take another look?
https://codereview.chromium.org/1847583003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1847583003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:341: int textEnd = textRange.endPosition().computeOffsetInContainerNode(); How about simply the following? int textEnd = Position::lastOffsetForEditing(baseNode);
https://codereview.chromium.org/1847583003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1847583003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:341: int textEnd = textRange.endPosition().computeOffsetInContainerNode(); Do you mean: int textEnd = EditingStrategy::lastOffsetForEditing(baseNode); It can't work correctly for multiple nodes, because textEnd is the length of text in the current node, not the length of the whole text.
https://codereview.chromium.org/1847583003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1847583003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:341: int textEnd = textRange.endPosition().computeOffsetInContainerNode(); On 2016/05/02 09:48:10, yabinh wrote: > > Do you mean: > int textEnd = EditingStrategy::lastOffsetForEditing(baseNode); > > It can't work correctly for multiple nodes, because textEnd is the length of > text in the current node, not the length of the whole text. Ok, I couldn't find a better solution here. But calling parentElement() doesn't seem right. Could you add the following test cases? 1) baseNode is two levels below the contenteditable, such that baseNode->parentElement() can fail. 2) Adding selectionStart ends up in a node other than the currently selected/composited node (such that lastOffsetForEditing() can fail)
changwan@, can you take another look?
lgtm w/ nits aelias@, could you take a look? https://codereview.chromium.org/1847583003/diff/240001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/1847583003/diff/240001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:159: if (!usingReplicaInputConnection()) { could you use @CommandLineFlags.Add({}) instead? https://codereview.chromium.org/1847583003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1847583003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:344: const EphemeralRange startRange = PlainTextRange(0, start).createRange(*rootEditableElement); You can create only range to handle both: const EphemeralRange range = PlainTextRange(start, end);
https://codereview.chromium.org/1847583003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1847583003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:344: const EphemeralRange startRange = PlainTextRange(0, start).createRange(*rootEditableElement); If both number1 and number2 exceed the right boundary, PlainTextRange::createRange(number1, number2) will return a default value, which is [0,0]. So if we want to get the right boundary, we should make sure number1 is within range at least. That's why we need to call it 2 times.
On 2016/05/09 13:02:50, yabinh wrote: > https://codereview.chromium.org/1847583003/diff/240001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): > > https://codereview.chromium.org/1847583003/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/InputMethodController.cpp:344: const > EphemeralRange startRange = PlainTextRange(0, > start).createRange(*rootEditableElement); > If both number1 and number2 exceed the right boundary, > PlainTextRange::createRange(number1, number2) will return a default value, which > is [0,0]. So if we want to get the right boundary, we should make sure number1 > is within range at least. That's why we need to call it 2 times. Hmm... I thought that you were fixing PlainTextRange behavior at some point. Any reason you didn't pursue that path? In any case, I think you'll need to add some comments or TODO here.
> Hmm... I thought that you were fixing PlainTextRange behavior at some point. Any > reason you didn't pursue that path? > In any case, I think you'll need to add some comments or TODO here. I think we'd better not change the behavior of Range::create(), since it's also reasonable to return [0,0] in that case, and some other code may rely on this behavior. Besides, if Range::create() is changed, some weired trybot failures will occur. In the latest patch(patch set 8), a different method is used to deal with exceeding right boundary, so we can avoid that problem.
changwan@, aelias@, can you take a look at the latest patch?
https://codereview.chromium.org/1847583003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1847583003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:349: const EphemeralRange startRange = PlainTextRange(0, start).createRange(*rootEditableElement); These ranges are odd, wouldn't it have the same effect to just create two Position objects instead?
https://codereview.chromium.org/1847583003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1847583003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:349: const EphemeralRange startRange = PlainTextRange(0, start).createRange(*rootEditableElement); On 2016/05/10 20:44:11, aelias wrote: > These ranges are odd, wouldn't it have the same effect to just create two > Position objects instead? That's right. But in order to create Position objects, we need to know |anchorNode| and |offset|. And PlainTextRange::createRange() can do that.
aelias@chromium.org changed reviewers: + yosin@chromium.org
OK, lgtm modulo minor comment below, adding yosin@ for core/editing OWNERS. https://codereview.chromium.org/1847583003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1847583003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:349: const EphemeralRange startRange = PlainTextRange(0, start).createRange(*rootEditableElement); On 2016/05/11 at 00:28:38, yabinh wrote: > On 2016/05/10 20:44:11, aelias wrote: > > These ranges are odd, wouldn't it have the same effect to just create two > > Position objects instead? > > That's right. But in order to create Position objects, we need to know |anchorNode| and |offset|. And PlainTextRange::createRange() can do that. OK, it looks like the algorithm in createRange() isn't split off anywhere else, I guess it's OK then. Please store Position objects as local variables instead of the EphemeralRange to make it clearer that the ranges aren't really of interest.
Waiting for using Position's here. https://codereview.chromium.org/1847583003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1847583003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:349: const EphemeralRange startRange = PlainTextRange(0, start).createRange(*rootEditableElement); nit: s/const EphemeralRange/const EphemeralRange&/ to avoid redundant copy https://codereview.chromium.org/1847583003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:350: const EphemeralRange endRange = PlainTextRange(0, end).createRange(*rootEditableElement); nit: s/const EphemeralRange/const EphemeralRange&/ to avoid redundant copy
yosin@, can you take another look?
On 2016/05/12 at 01:44:26, yabinh wrote: > yosin@, can you take another look? lgtm Thanks!
The CQ bit was checked by yabinh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from changwan@chromium.org, aelias@chromium.org Link to the patchset: https://codereview.chromium.org/1847583003/#ps300001 (title: "Store Position objects as local variables...")
The CQ bit was unchecked by yabinh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1847583003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1847583003/300001
The CQ bit was checked by yabinh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1847583003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1847583003/300001
The CQ bit was checked by yabinh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1847583003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1847583003/300001
Message was sent while issue was closed.
Committed patchset #10 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== After fixing, it behaves like this: Case1: When newCursorPosition < 1, the cursor will move to the left of the composing text. Specially, when cursor position exceeds the left boundary, it will move to the left boundary. Case2: When newCursorPosition > 1, the cursor will move to the right of the composing text. Specially, when cursor position exceeds the right boundary, it will move to the right boundary. BUG=570920 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== After fixing, it behaves like this: Case1: When newCursorPosition < 1, the cursor will move to the left of the composing text. Specially, when cursor position exceeds the left boundary, it will move to the left boundary. Case2: When newCursorPosition > 1, the cursor will move to the right of the composing text. Specially, when cursor position exceeds the right boundary, it will move to the right boundary. BUG=570920 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/25dec933e90def173fe9682a6d2b80d46283be52 Cr-Commit-Position: refs/heads/master@{#393198} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/25dec933e90def173fe9682a6d2b80d46283be52 Cr-Commit-Position: refs/heads/master@{#393198} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
