Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1889053003:
Fix InputConnection.deleteSurroundingText() (Closed)
|
Created:
4 years, 8 months ago by yabinh Modified:
4 years, 2 months ago CC:
alexmos, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dcheng, dglazkov+blink, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-blink_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix InputConnection.deleteSurroundingText()
According to the Android API documentation, InputConnection.deleteSurroundingText()
should delete text before and after the current cursor position, excluding the
selection, NOT including the selection. The old implementation reused
ExtendSelectionAndDelete which has different semantics and was originally introduced
for ChromeOS; this adds a new method and IPC which behaves according
to Android requirements.
BUG=451027
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/90f1be18144ff4106ad203d45915b116938bf13a
Cr-Commit-Position: refs/heads/master@{#425244}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Move deleteSurroundingText to WebLocalFrame #Patch Set 3 : Deal with multi-code-text, and add more tests. #
Total comments: 6
Patch Set 4 : Changed the type of some parameters, and added some comments. #
Total comments: 2
Patch Set 5 : Changed the function for multi-code-text, and added some tests. #
Total comments: 2
Patch Set 6 : Changed the type of some parameters. #Patch Set 7 : Use ICU to handle multi-code text. #Patch Set 8 : Try again. #
Total comments: 1
Patch Set 9 : handle multi-code text with BackspaceStateMachine::previousPositionOf #
Total comments: 2
Patch Set 10 : add some comment #
Total comments: 13
Patch Set 11 : For Test. To show that deleteSurroundingText will trigger 2 selectionchange events. #Patch Set 12 : For test. To show that deleteSurroundingText doesn't trigger select or change event. #Patch Set 13 : Apply the same logic to |after| text. Add a test. #Patch Set 14 : Use ReplaceSelectionCommand to delete #
Total comments: 2
Patch Set 15 : deleteContents doesn’t work well for multiple nodes. #
Total comments: 2
Patch Set 16 : Same to patch set 13, except adding a test for multiple nodes. #
Total comments: 6
Patch Set 17 : Change some comments. #
Total comments: 5
Patch Set 18 : change for changwan@'s review #
Total comments: 1
Patch Set 19 : Use a different method to handle "\n" node #
Total comments: 15
Patch Set 20 : Address yosin@'s review #Patch Set 21 #Patch Set 22 #
Total comments: 6
Patch Set 23 : Address yosin@'s review #
Total comments: 8
Patch Set 24 : Address yosin@'s review #
Total comments: 2
Patch Set 25 : Address clamy@'s review #
Total comments: 3
Patch Set 26 : change int to size_t #
Total comments: 1
Patch Set 27 #Patch Set 28 : change back to patch set #25 #Messages
Total messages: 140 (58 generated)
Description was changed from ========== InputConnection.deleteSurroundingText() can't work correctly. It should delete text before and after the current cursor position, excluding the selection, NOT including the selection. BUG=451027 ========== to ========== InputConnection.deleteSurroundingText() can't work correctly. It should delete text before and after the current cursor position, excluding the selection, NOT including the selection. BUG=451027 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== InputConnection.deleteSurroundingText() can't work correctly. It should delete text before and after the current cursor position, excluding the selection, NOT including the selection. BUG=451027 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== InputConnection.deleteSurroundingText() can't work correctly. It should delete text before and after the current cursor position, excluding the selection, NOT including the selection. BUG=451027 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
yabinh@chromium.org changed reviewers: + aelias@chromium.org, changwan@chromium.org
This issue is a following of issue 1877243002. As changwan@ said, we delete the text 2 times and change the selection 3 times in this function. It might not be atomic. To verify this, I registered onclick() and onselect() event in the URL, and ran some tests, like ImeTest#testDeleteSurroundingTextWithRangeSelection, ImeTest#testDeleteSurroundingTextWithCursorSelection. But the events were not triggered at all. It's weird, because at least onselect() event should be triggered after setSelection(). By the way, these 2 events can be triggered when I use content_shell to open a local webpage on the phone. In all, even if I change the text in onselect() and onchange(), the code can still work because the events are not triggered at all. @changwan, I uploaded a new patch, can you take another look?
On 2016/04/15 06:52:27, yabinh wrote: > This issue is a following of issue 1877243002. > > As changwan@ said, we delete the text 2 times and change the selection 3 times > in this function. It might not be atomic. > > To verify this, I registered onclick() and onselect() event in the URL, and ran > some tests, like ImeTest#testDeleteSurroundingTextWithRangeSelection, > ImeTest#testDeleteSurroundingTextWithCursorSelection. But the events were not > triggered at all. It's weird, because at least onselect() event should be > triggered after setSelection(). > > By the way, these 2 events can be triggered when I use content_shell to open a > local webpage on the phone. > > In all, even if I change the text in onselect() and onchange(), the code can > still work because the events are not triggered at all. > > @changwan, I uploaded a new patch, can you take another look? I still think we should be able to verify the number of times onselect / onchange gets triggered. Please refer to the following CL which listens to key and composition events: https://codereview.chromium.org/1840993002/
Looks like there's already a primitive for atomically running a series of edit commands, see CompositeEditCommand.h. It prevents prematurely firing events to JS with a ScopedEventQueue.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/1889053003/diff/1/third_party/WebKit/public/w... File third_party/WebKit/public/web/WebFrame.h (right): https://codereview.chromium.org/1889053003/diff/1/third_party/WebKit/public/w... third_party/WebKit/public/web/WebFrame.h:462: virtual void deleteSurroundingText(int before, int after) = 0; This doesn't look relevant to remote frames: can we add this to WebLocalFrame instead?
https://codereview.chromium.org/1889053003/diff/1/third_party/WebKit/public/w... File third_party/WebKit/public/web/WebFrame.h (right): https://codereview.chromium.org/1889053003/diff/1/third_party/WebKit/public/w... third_party/WebKit/public/web/WebFrame.h:462: virtual void deleteSurroundingText(int before, int after) = 0; On 2016/04/16 at 03:37:07, dcheng wrote: > This doesn't look relevant to remote frames: can we add this to WebLocalFrame instead? Why do you say that? Remote frames can have textboxes just like local ones I thought. And this is just another IME method similar to the ones right above it, so if you have a concern it's more general than this patch.
https://codereview.chromium.org/1889053003/diff/1/third_party/WebKit/public/w... File third_party/WebKit/public/web/WebFrame.h (right): https://codereview.chromium.org/1889053003/diff/1/third_party/WebKit/public/w... third_party/WebKit/public/web/WebFrame.h:462: virtual void deleteSurroundingText(int before, int after) = 0; On 2016/04/16 at 03:43:29, aelias wrote: > On 2016/04/16 at 03:37:07, dcheng wrote: > > This doesn't look relevant to remote frames: can we add this to WebLocalFrame instead? > > Why do you say that? Remote frames can have textboxes just like local ones I thought. And this is just another IME method similar to the ones right above it, so if you have a concern it's more general than this patch. In that case, the request should have been routed to the renderer hosting that frame. Consider the followup setup: ┌───────────── PROCESS 1 ─────────────┐ │┌───────────────────────────────────┐│ ││ a.com (local) ││ ││ ││ ││ ┌───────────────────┐ ││ ││ │ b.com (remote) │ ││ ││ │ │ ││ ││ └───────────────────┘ ││ │└───────────────────────────────────┘│ └─────────────────────────────────────┘ ┌───────────── PROCESS 2 ─────────────┐ │┌───────────────────────────────────┐│ ││ a.com (remote) ││ ││ ││ ││ ┌───────────────────┐ ││ ││ │ b.com (local) │ ││ ││ │ │ ││ ││ └───────────────────┘ ││ │└───────────────────────────────────┘│ └─────────────────────────────────────┘ If the b.com frame has focus but the IME request is routed to [PROCESS 1], there's nothing we can do with it: the DOM, etc is all in [PROCESS 2]. In order for this to work correctly, the IME routing should have been talking to [PROCESS 2] to begin with. This is reflected in the WebRemoteFrame methods for IME: they're all stubbed out with NOTREACHED(). The only reason still on WebFrame is because we didn't use to have the split between WebLocalFrame and WebRemoteFrame: most new methods should only be on WebLocalFrame. We have a longstanding cleanup task to move APIs that don't really belong on WebFrame to WebLocalFrame, so eventually that will fix this inconsistency as well.
OK, makes sense, but I don't think we should just move this one method piecemeal to WebLocalFrame while the other IME methods remain here. Yabin, could you prepare a patch (maybe separate from this one if too large) to move all of the composition-related methods to WebLocalFrame?
No problem. By the way, I've verified that this patch doesn't trigger any onselect or onchange event.
I moved deleteSurroundingText to WebLocalFrame in patch set 2. I'll move other IME methods to WebLocalFrame in the next patch. @changwan, can you take another look?
On 2016/04/19 02:42:19, yabinh wrote: > I moved deleteSurroundingText to WebLocalFrame in patch set 2. I'll move other > IME methods to WebLocalFrame in the next patch. > @changwan, can you take another look? Aren't Chrome OS and Android the only platforms that use extendSelectionAndDelete()? (See https://developer.chrome.com/extensions/input_ime#method-deleteSurroundingText for more details.) If that is the case, I'm curious if Chrome OS can consider changing its method signature such that we can probably replace extendSelectionAndDelete(). The API set seems strikingly similar to that of Android, anyways. Otherwise, we may need to have a matching set of tests for render view and WebView: https://code.google.com/p/chromium/codesearch#search/&q=extendselectionanddel... aelias@, what do you think?
There are 27 functions related to IME in WebFrame, from insertText() to setCaretVisible(). Some of them are easy to move to WebLocalFrame, like insertText(); some are not, like this one: virtual bool executeCommand(const WebString&, const WebNode& = WebNode()) This function is called in many places, like RenderViewImpl#OnExecuteEditCommand: webview()->focusedFrame()->executeCommand(...); webview()->focusedFrame() returns a pointer to WebFrame, so if I simply move executeCommand to WebLocalFrame, it won't work. A workaround is to call webview()->focusedFrame()->toWebLocalFrame to return a pointer to WebLocalFrame. But there are a lot of places like this. That means a lot of files need to be modified. And this's only for executeCommand. Among the 27 functions, there are other functions should be handled like this. aelias@, dcheng@, what do you think?
On 2016/04/19 at 07:55:46, yabinh wrote: > There are 27 functions related to IME in WebFrame, from insertText() to setCaretVisible(). Some of them are easy to move to WebLocalFrame, like insertText(); some are not, like this one: > > virtual bool executeCommand(const WebString&, const WebNode& = WebNode()) > > This function is called in many places, like RenderViewImpl#OnExecuteEditCommand: > > webview()->focusedFrame()->executeCommand(...); > > webview()->focusedFrame() returns a pointer to WebFrame, so if I simply move executeCommand to WebLocalFrame, it won't work. A workaround is to call webview()->focusedFrame()->toWebLocalFrame to return a pointer to WebLocalFrame. But there are a lot of places like this. That means a lot of files need to be modified. > And this's only for executeCommand. Among the 27 functions, there are other functions should be handled like this. > > aelias@, dcheng@, what do you think? It's probably easier to move the low-hanging fruit first. I've been asking new interfaces that don't make sense on WebFrame to be added to WebLocalFrame exclusively to lessen the amount of migrating we'll want to do down the road. This does mean some interfaces are split up, but I think that's the lesser evil here. For the specific case of OnExecuteEditCommand... it looks like it always asks on the focused frame? In the long-term, I think what we want is for this to be always routed to the 'right' renderer to begin with. Then we can change WebView::focusedFrame() to match what we've already done for FocusController::focusedFrame() (which only returns a LocalFrame*), and this problem becomes much simpler.
I've filed https://crbug.com/604645 to track the focused frame issue.
I see. I'll work on that issue.
For Patch Set 3: I modified InputMethodController#deleteSurroundingText to handle multi-code-text, and added a test for that. I also added some tests in render_view_browsertest.cpp and WebViewTest.cpp. changwan@, could you take another look?
https://codereview.chromium.org/1889053003/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1889053003/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2163: void RenderFrameHostImpl::DeleteSurroundingText(size_t before, size_t after) { This is somewhat inconsistent. How about making the parameters int? https://codereview.chromium.org/1889053003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:492: if (after > 0) { shouldn't we apply the same logic to afterText? do we need more tests?
lgtm aelias@, could you take a look at this change? https://codereview.chromium.org/1889053003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:492: if (after > 0) { On 2016/04/21 01:07:06, Changwan Ryu wrote: > shouldn't we apply the same logic to afterText? do we need more tests? talked to yabinh@ offline. It's already covered by the test.
https://codereview.chromium.org/1889053003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:492: if (after > 0) { We don't need that. Some multi-code-texts take 2 positions, like "\xF0\x9F\x8F\x86"(U+1F3C6 == "trophy"). We can select the whole multi-code-text successfully if we only select the left half of it, but sometimes we can't select it successfully if we only select the right half of it, so we need to select more. That's why we need to apply a while-loop to the beforeText and don't need to apply it to the afterText.
On 2016/04/21 01:24:16, yabinh wrote: > https://codereview.chromium.org/1889053003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): > > https://codereview.chromium.org/1889053003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/InputMethodController.cpp:492: if (after > > 0) { > We don't need that. > > Some multi-code-texts take 2 positions, like "\xF0\x9F\x8F\x86"(U+1F3C6 == > "trophy"). We can select the whole multi-code-text successfully if we only > select the left half of it, but sometimes we can't select it successfully if we > only select the right half of it, so we need to select more. That's why we need > to apply a while-loop to the beforeText and don't need to apply it to the > afterText. Talked to yabinh@. It seems that the difference is in selection behavior - it always tries to select the 'end' of 32-bit characters when you try to select half of multi-code characters. We probably need to comment it in the new implementation.
https://codereview.chromium.org/1889053003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:492: if (after > 0) { On 2016/04/21 at 01:24:16, yabinh wrote: > We don't need that. > > Some multi-code-texts take 2 positions, like "\xF0\x9F\x8F\x86"(U+1F3C6 == "trophy"). We can select the whole multi-code-text successfully if we only select the left half of it, but sometimes we can't select it successfully if we only select the right half of it, so we need to select more. That's why we need to apply a while-loop to the beforeText and don't need to apply it to the afterText. OK, but can we call a generic method to snap to the beginning instead of walking through the codepoints ourselves? It's extremely likely that such a method already exists somewhere in the codebase. I'd look for such a method in the ICU headers, for example. https://codereview.chromium.org/1889053003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:467: int selectionStart = static_cast<int>(selectionOffsets.start()); Why static_cast, is there a warning/error if you just used size_t here? If so, can you fix the warning/error by changing more things to size_t? Most things here cannot be negative, right?
changwan@, I uploaded a new patch. Please take another look. aelias@, As for the ICU headers, could you speak in detail? I think "third_party/icu/source/common/unicode/uchar.h" could be the ICU header you were talking about, but I didn't find any useful functions there. By the way, I think the while-loop might be OK because it's not complicated. What do you think?
https://codereview.chromium.org/1889053003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:467: int selectionStart = static_cast<int>(selectionOffsets.start()); Since we use "std::min(selectionStart, before)", and PlainTextRange#PlainTextRange(int, int) takes int as its papameter, it might be better if "selectionStart" is int in the first place.
https://codereview.chromium.org/1889053003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:500: } while (frame().selection().start().computeOffsetInContainerNode() + before > frame().selection().end().computeOffsetInContainerNode() && before <= static_cast<int>(selectionOffsets.start())); 1) while condition is too long, and the latter condition is true throughout the loop. 2) too many variables. 3) comment belongs to inside the if statement. I just tried to simplify it as follows: if (before > 0) { before = std::min(selectionStart, before); // Select the text to be deleted before selectionStart, // and adjust 'before' in case of multi-code text. for (; before <= selectionStart; ++before) { if (!setSelectionOffsets(PlainTextRange(selectionStart - before, selectionStart))) return; if (frame().selection.start().computeOffsetInContainerNode() + before <= frame().selection().end().computeOffsetInContainerNode()) break; } CHECK_LE(before, selectionStart); TypingCommand::deleteSelection(*frame().document()); selectionStart = selectionStart - before; selectionEnd = selectionEnd - before; } WDYT?
aelias@, I talked with changwan@, and now I think size_t is better than int. I'll change it in the next patch. https://codereview.chromium.org/1889053003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:500: } while (frame().selection().start().computeOffsetInContainerNode() + before > frame().selection().end().computeOffsetInContainerNode() && before <= static_cast<int>(selectionOffsets.start())); You are right. It'll be more simple.
https://codereview.chromium.org/1889053003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:492: if (after > 0) { I suggest looking in http://icu-project.org/apiref/icu4c/utf16_8h.html, would U16_BACK_1 be equivalent?
https://codereview.chromium.org/1889053003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:493: U8_SET_CP_START(text, 0, adjustedStart); This is incorrect if the given text is UTF16 string. It's quite confusing that WTFString allows both UTF8 and UTF16. (Hence the name...?) Could you try using EditingUtilities::previousPositionOfAlgorithm with PositionMoveType::BackwardDeletion? Then it will check the surrogate for you in BackspaceStateMachine::feedPrecedingCodeUnit().
changwan@, aelias@, I uploaded a new patch. Could you take another look?
my lgtm is still valid w/ nits https://codereview.chromium.org/1889053003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:486: if (before != 0) { before > 0u https://codereview.chromium.org/1889053003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:503: if (after != 0) { after > 0u Comment still missing: why do you need to adjust 'before' and not 'after'?
Description was changed from ========== InputConnection.deleteSurroundingText() can't work correctly. It should delete text before and after the current cursor position, excluding the selection, NOT including the selection. BUG=451027 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix InputConnection.deleteSurroundingText() According to the documented API, InputConnection.deleteSurroundingText() should delete text before and after the current cursor position, excluding the selection, NOT including the selection. BUG=451027 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Fix InputConnection.deleteSurroundingText() According to the documented API, InputConnection.deleteSurroundingText() should delete text before and after the current cursor position, excluding the selection, NOT including the selection. BUG=451027 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix InputConnection.deleteSurroundingText() According to the Android API documentation, InputConnection.deleteSurroundingText() should delete text before and after the current cursor position, excluding the selection, NOT including the selection. BUG=451027 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Fix InputConnection.deleteSurroundingText() According to the Android API documentation, InputConnection.deleteSurroundingText() should delete text before and after the current cursor position, excluding the selection, NOT including the selection. BUG=451027 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix InputConnection.deleteSurroundingText() According to the Android API documentation, InputConnection.deleteSurroundingText() should delete text before and after the current cursor position, excluding the selection, NOT including the selection. The old implementation reused ExtendSelectionAndDelete which has different semantics and was originally introduced for ChromeOS; this adds a new method and IPC which behaves according to Android requirements. BUG=451027 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
OK. Comment has been added to patch set 10.
aelias@chromium.org changed reviewers: + yosin@chromium.org
Non-core/ parts lgtm, adding yosin@ for core/editing/ OWNERS. https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:487: before = std::min(selectionStart, before); What is this line for? I don't understand it. https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:494: Position basePosition(frame().selection().start().anchorNode(), selectionStart - before + 1); Why + 1? https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:500: TypingCommand::deleteSelection(*frame().document()); Hmm, I still don't like the two deletion calls too much because it may fire duplicate events to JS. I had in mind that this could be done using a CompositeEditCommand although looking at that file, it's not too obvious to me how to do so. yosin@, any thoughts on if this is the best way?
https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:487: before = std::min(selectionStart, before); It's for boundary checking. In case that "before" is too large to exceed the left boundary of the text. https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:494: Position basePosition(frame().selection().start().anchorNode(), selectionStart - before + 1); Because previousPositionOf(basePosition, PositionMoveType::GraphemeCluster) is used to move to left by 1 user-visible character. For example, if the text is "hello", previousPositionOf is equivalent to minus 1. https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:500: TypingCommand::deleteSelection(*frame().document()); I've verified that this patch doesn't trigger any onselect or onchange event. Do I need to check other events?
yosin@chromium.org changed reviewers: + nona@chromium.org
+nona@ as emoji expert. It seems we need to handle grapheme cluster in DeleteSurroundingText(). At least we have assertion to ensure |before| and |after| are at grapheme boundary. https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:475: void InputMethodController::deleteSurroundingText(size_t before, size_t after) Can you guarantee |before| is at end of grapheme and |after| is at start of grapheme? Note: I assume |before| and |after| is code unit (16-bit). https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:500: TypingCommand::deleteSelection(*frame().document()); On 2016/04/26 at 07:29:32, aelias wrote: > Hmm, I still don't like the two deletion calls too much because it may fire duplicate events to JS. I had in mind that this could be done using a CompositeEditCommand although looking at that file, it's not too obvious to me how to do so. yosin@, any thoughts on if this is the best way? Better way is using Range::delteContent(). It dispatches only DOM mutation events. You could write: frame().firstRange()->deleteContent(). https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:355: // "trophy" + "trophy". Are below tests handle grapheme clusters? +nona@ for emoji expert.
On 2016/04/26 08:18:54, Yosi_UTC9 wrote: > +nona@ as emoji expert. > It seems we need to handle grapheme cluster in DeleteSurroundingText(). At least > we have assertion to ensure |before| and |after| are at grapheme boundary. Sorry what means "handle grapheme cluster"? According to the InputConnection.deleteSurroundingText API spec, it says the unit of |before| and |after| are in Java chars(so UTF-16 code untis). http://developer.android.com/intl/ja/reference/android/view/inputmethod/Input..., int) Thus, the correct behavior is deleting in UTF-16 code units even if IME specifies to delete in the middle of grapheme cluster. If the IME forwards forward delete key events for character deletes, it should delete in grapheme cluster. (backspace almost works with grapheme cluster but it works a bit different.) > > https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): > > https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/InputMethodController.cpp:475: void > InputMethodController::deleteSurroundingText(size_t before, size_t after) > Can you guarantee |before| is at end of grapheme and |after| is at start of > grapheme? > Note: I assume |before| and |after| is code unit (16-bit). > > https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/InputMethodController.cpp:500: > TypingCommand::deleteSelection(*frame().document()); > On 2016/04/26 at 07:29:32, aelias wrote: > > Hmm, I still don't like the two deletion calls too much because it may fire > duplicate events to JS. I had in mind that this could be done using a > CompositeEditCommand although looking at that file, it's not too obvious to me > how to do so. yosin@, any thoughts on if this is the best way? > > Better way is using Range::delteContent(). It dispatches only DOM mutation > events. > You could write: > frame().firstRange()->deleteContent(). > > https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp > (right): > > https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:355: // > "trophy" + "trophy". > Are below tests handle grapheme clusters? > +nona@ for emoji expert.
https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:355: // "trophy" + "trophy". On 2016/04/26 08:18:54, Yosi_UTC9 wrote: > Are below tests handle grapheme clusters? > +nona@ for emoji expert. Looks like this is the case that the IME specifies invalid surrogate pair boundary. Looks like following four test cases are reasonable to me. API spec says IME author should care about surrogate pair handling but it doesn't mention about the behavior of Editor in that case. http://developer.android.com/intl/ja/reference/android/view/inputmethod/Input..., int) Thus, it is make sense to remove the unpaired surrogate pair. FWIW, it may be good if we can add a test case that removing unpaired leading and trailing surrogate pairs, like deleteSurroundingText(1, 1) for "\uD83C \uDFC6 | \uD83C \uDFC6" goes empty string. // "trophy" + "trophy". input->setValue(String::fromUTF8("\xF0\x9F\x8F\x86\xF0\x9F\x8F\x86 foo")); controller().setEditableSelectionOffsets(PlainTextRange(2, 2)); EXPECT_STREQ("\xF0\x9F\x8F\x86\xF0\x9F\x8F\x86 foo", input->value().utf8().data()); controller().deleteSurroundingText(1, 1); EXPECT_STREQ(" foo", input->value().utf8().data()); For the grapheme cluster, I think as far as IME deletes a text by InputConnection.deleteSurroundinText, IME should care about the grapheme boundary since InputConnection.deleteSurroundingText only accepts in UTF16 code unit and API spec doesn't say anything about grapheme clusters. On the other hand, the backspace or forward delete keyevent should be cared by editor.
https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:500: TypingCommand::deleteSelection(*frame().document()); On 2016/04/26 at 08:05:16, yabinh wrote: > I've verified that this patch doesn't trigger any onselect or onchange event. Do I need to check other events? How did you check these events? Did you check with event loop? "selectionchange" event should be dispatched whenever selection is changed. But, it is dispatched in event loop since it is async event. https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:507: if (!setSelectionOffsets(PlainTextRange(static_cast<int>(selectionEnd), static_cast<int>(selectionEnd + after)))) We may want to |selectionEnd + after| is at end of grapheme. Could you call nextPositionOf or previousPositionOf with |PositionMoveType::GraphemeCluster| like adjusting position for |before|?
> How did you check these events? Did you check with event loop? I checked it with js, similiar to this patch: https://codereview.chromium.org/1840993002/ > We may want to |selectionEnd + after| is at end of grapheme. Could you call > nextPositionOf or previousPositionOf with |PositionMoveType::GraphemeCluster| > like adjusting position for |before|? No problem.
On 2016/04/27 at 02:30:33, yabinh wrote: > > How did you check these events? Did you check with event loop? > > I checked it with js, similiar to this patch: > https://codereview.chromium.org/1840993002/ > Could you attach your JS file for reference, it should be removed at commit?
https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:487: before = std::min(selectionStart, before); On 2016/04/26 at 08:05:15, yabinh wrote: > It's for boundary checking. In case that "before" is too large to exceed the left boundary of the text. When |selectionStart|=0 and |before|=2, |before| goes to |0|.
In this patch, deleteSurroundingText doesn't trigger select or change event, but it can trigger selectionchange event. Patch Set 11 and 12 are used to verify that. aelias@ changewan@ yosin@ In Patch Set 13, nextPositionOf is used to adjust the |after| position. yosin@ I also added a test in this patch. nona@
> When |selectionStart|=0 and |before|=2, |before| goes to |0|. You are right. It should goes to 0, otherwise it will exceed the left boundary, because there is no text on the left when |selectionStart|=0.
not lgtm https://codereview.chromium.org/1889053003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:492: before = std::min(selectionStart, before); This should be checked before the if statement. When selectionStart == 0, this doesn't need to run. https://codereview.chromium.org/1889053003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:526: ReplaceSelectionCommand::create(*frame().document(), fragment, ReplaceSelectionCommand::SelectReplacement)->apply(); You shouldn't try to replace the selection with new text here. Imagine we have <div contenteditable><b>a</b><i>b</i><b>c</b></div>, and "b" is selected. Then replacing selection may also remove the style. What is the reason Range::deleteContents() does not update HTMLElement's value? Is there any event you need to dispatch?
deleteContents doesn’t work well for multiple nodes. Please see the comments in the code. In InputMethodController::extendSelectionAndDelete(), we use TypingCommand::deleteSelection() to delete. It can work well for multiple nodes. However, as we all know, if we use similar functions in deleteSurroundingText, it will fire duplicate JS events. Maybe we will have to use functions similar to TypingCommand::deleteSelection() if there is no better choice. https://codereview.chromium.org/1889053003/diff/280001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/1889053003/diff/280001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:180: deleteContents can work for multiple nodes if there is no ‘\n’ before <div> label, i.e. <div id="contenteditable_event" contenteditable><b>ab</b>cd<i>ef<b>gh</b></i></div> https://codereview.chromium.org/1889053003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/1889053003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:454: deleteContents doesn’t work well for multiple nodes. It omits the ‘\n’ before <div> label.
On 2016/05/23 10:25:00, yabinh wrote: > deleteContents doesn’t work well for multiple nodes. Please see the comments in > the code. > > In InputMethodController::extendSelectionAndDelete(), we use > TypingCommand::deleteSelection() to delete. It can work well for multiple nodes. > However, as we all know, if we use similar functions in deleteSurroundingText, > it will fire duplicate JS events. > > Maybe we will have to use functions similar to TypingCommand::deleteSelection() > if there is no better choice. > > https://codereview.chromium.org/1889053003/diff/280001/content/public/android... > File > content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java > (right): > > https://codereview.chromium.org/1889053003/diff/280001/content/public/android... > content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:180: > > deleteContents can work for multiple nodes if there is no ‘\n’ before <div> > label, i.e. > > <div id="contenteditable_event" > contenteditable><b>ab</b>cd<i>ef<b>gh</b></i></div> > > https://codereview.chromium.org/1889053003/diff/280001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp > (right): > > https://codereview.chromium.org/1889053003/diff/280001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:454: > deleteContents doesn’t work well for multiple nodes. It omits the ‘\n’ before > <div> label. Talked to yabinh@ offline. Now I prefer the approach in patch #13 because deleting emitted characters is much more important than suppressing unnecessary selection change events. However, all the complicated deletion logic seems contained inside DeleteSelectionCommand, so ultimately we might need to add DeleteRangeCommand and have DeleteSelectionCommand wrap around DeleteRangeCommand. I personally don't mind doing it in a separate CL.
changwan@, can you take a look at the latest patch? It's similar to patch set 13. As we discussed before, it works fine for multiple nodes, though it fires duplicate JS events. https://codereview.chromium.org/1889053003/diff/300001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/1889053003/diff/300001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1254: // TODO(yabinh): It should only fire 1 input and 2 selectionchange events. It will fire duplicate JS events. https://codereview.chromium.org/1889053003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/1889053003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:455: It works for multiple nodes.
lgtm https://codereview.chromium.org/1889053003/diff/300001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/1889053003/diff/300001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1254: // TODO(yabinh): It should only fire 1 input and 2 selectionchange events. nit: 1 selectionchange? https://codereview.chromium.org/1889053003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:551: void InputMethodController::deleteSurroundingText(size_t before, size_t after) Could you add a TODO to reduce the number of selectionchanged events?
https://codereview.chromium.org/1889053003/diff/300001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/1889053003/diff/300001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1254: // TODO(yabinh): It should only fire 1 input and 2 selectionchange events. On 2016/06/20 10:03:24, Changwan Ryu wrote: > nit: 1 selectionchange? You are right. https://codereview.chromium.org/1889053003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:551: void InputMethodController::deleteSurroundingText(size_t before, size_t after) On 2016/06/20 10:03:24, Changwan Ryu wrote: > Could you add a TODO to reduce the number of selectionchanged events? Acknowledged.
https://codereview.chromium.org/1889053003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:571: Position position(frame().selection().start().anchorNode(), selectionStart - before + 1); On a second look, I'm concerned about this line. Is the position always valid for the offset when anchorNode != rootEditableElement? Could you add some more test cases around such a case?
https://codereview.chromium.org/1889053003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:571: Position position(frame().selection().start().anchorNode(), selectionStart - before + 1); On 2016/07/05 15:47:36, Changwan Ryu wrote: > On a second look, I'm concerned about this line. Is the position always valid > for the offset when anchorNode != rootEditableElement? Could you add some more > test cases around such a case? I think we've already had that case in DeleteSurroundingTextForMultipleNodes. Let * stand for |position|, and # stand for |adjustedPosition|, then: hello#\n*world Notice that for |position|, its anchorNode is the first child node; for |adjustedPosition|, its anchorNode is the third child node. Their parent node is rootEditableElement. https://codereview.chromium.org/1889053003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/1889053003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:443: { There are 3 child nodes: "hello", "\n", "world". https://codereview.chromium.org/1889053003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:455: Cursor is before the third child node.
https://codereview.chromium.org/1889053003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:571: Position position(frame().selection().start().anchorNode(), selectionStart - before + 1); On 2016/07/06 02:15:02, yabinh wrote: > On 2016/07/05 15:47:36, Changwan Ryu wrote: > > On a second look, I'm concerned about this line. Is the position always valid > > for the offset when anchorNode != rootEditableElement? Could you add some more > > test cases around such a case? > > I think we've already had that case in DeleteSurroundingTextForMultipleNodes. > > Let * stand for |position|, and # stand for |adjustedPosition|, then: > hello#\n*world > Notice that for |position|, its anchorNode is the first child node; for > |adjustedPosition|, its anchorNode is the third child node. Their parent node is > rootEditableElement. Hmm... I was more worried about the case where before > 1 and that selection's anchor node is different from deletion start position's anchor node. Adjusted position is irrelevant in my comment.
https://codereview.chromium.org/1889053003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/1889053003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:441: Add test for multiple nodes, especially for line break. BTW, the previous patch can't pass this test.
The CQ bit was checked by yabinh@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
changwan@, can you take another look?
https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:591: int diff = adjustedPosition.computeOffsetInContainerNode() - position.computeOffsetInContainerNode(); what should happen if adjustedPosition and position have different anchor nodes? Also, I'm a bit worried that we're not using PositionMoveType::BackwardDeletion. Keyboard apps may expect that code point will be deleted, and may do something weird if the whole grapheme cluster gets deleted. What happens when you're at the end of a + telugu combining character and try to delete one value? https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:615: int diff = adjustedPosition.computeOffsetInContainerNode() - position.computeOffsetInContainerNode(); same comment applies here
https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:591: int diff = adjustedPosition.computeOffsetInContainerNode() - position.computeOffsetInContainerNode(); On 2016/07/20 08:37:42, Changwan Ryu wrote: > what should happen if adjustedPosition and position have different anchor nodes? > There is no such case. I'll add assertion here. > Also, I'm a bit worried that we're not using PositionMoveType::BackwardDeletion. > Keyboard apps may expect that code point will be deleted, and may do something > weird if the whole grapheme cluster gets deleted. What happens when you're at > the end of a + telugu combining character and try to delete one value? There are 2 problems about "\xE0\xB0\x83"(telugu combining character): 1. BackwardDeletion is only available for prevPositionOf(). This will cause incompatibility. For example: "a\xE0\xB0\x83\xE0\xB0\x83*", deleteSurroundingText(2, 0), * stands for cursor. That means we want to delete [1, 3]. |position.computeOffsetInContainerNode()|==1. If we call nextPosition = nextPositionOf(position, PositionMoveType::GraphemeCluster), we will get |nextPosition.computeOffsetInContainerNode()|==3. Then call adjustedPosition = previousPositionOf(nextPosition, PositionMoveType::BackwardDeletion), we will get |adjustedPosition.computeOffsetInContainerNode()|==2. That means we are trying to delete [2, 3]. It's not correct. 2. TypingCommand::deleteSelection() adjusts selection for grapheme boundary ( like in crbug.com/620690 ), so we can't delete it successfully. For example: "a\xE0\xB0\x83*", deleteSurroundingText(1, 0), * stands for cursor. That means we want to delete [1, 2]. Even if we can select [1, 2], we will not delete it. Actually, we will delete nothing, because TypingCommand::deleteSelection() adjusts selection to [2, 2]
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
changwan@chromium.org changed reviewers: + yukawa@chromium.org
yukawa@, is it ok to delete by graphme clusters, not by code points for deleteSurroundingText()? I'm afraid that keyboard apps might be confused. What do you think?
On 2016/07/21 02:33:09, Changwan Ryu wrote: > yukawa@, is it ok to delete by graphme clusters, not by code points for > deleteSurroundingText()? I'm afraid that > keyboard apps might be confused. What do you think? No, it's not OK. The length always needs to be handled in Java chars. https://developer.android.com/reference/android/view/inputmethod/InputConnect..., int) | The lengths are supplied in Java chars, not in code points or in glyphs. We do understand that currently there is no way for IMEs to ask the app to delete surrounding characters in grapheme clusters, we cannot change the semantics of existing APIs.
On 2016/07/21 20:48:15, yukawa wrote: > On 2016/07/21 02:33:09, Changwan Ryu wrote: > > yukawa@, is it ok to delete by graphme clusters, not by code points for > > deleteSurroundingText()? I'm afraid that > > keyboard apps might be confused. What do you think? > > No, it's not OK. The length always needs to be handled in Java chars. > > https://developer.android.com/reference/android/view/inputmethod/InputConnect..., > int) > | The lengths are supplied in Java chars, not in code points or in glyphs. > > We do understand that currently there is no way for IMEs to ask the app to > delete surrounding characters in grapheme clusters, we cannot change the > semantics of existing APIs. Thanks for the quick response. It is perfectly clear that the lengths are in Java chars. deleteSurroundingText also takes surrogate pairs into consideration, so the whole code point will be deleted if it was at the boundary. The question here is whether it should be ok to delete a grapheme cluster (> codepoint) when it's at the boundary, which was indirectly suggested in comment #44. If that is not the right way to go, then we may need to refactor lots of code, so we wanted your answer earliest possible. Thank you.
On 2016/07/22 00:11:48, Changwan Ryu wrote: > The question here is whether it should be ok to delete a grapheme cluster (> > codepoint) when it's at the boundary, > which was indirectly suggested in comment #44. Unfortunately, the answer is no. Even if the result text could be an invalid Java Char sequence, the API needs to delete only the requested number of Java chars. This is how the built-in EditText behaves, and unfortunately we cannot change the existing semantics anymore. Just FYI, in Android N I added a new API that takes code points. Per-grapheme text deletion is still on my TODO list though. https://developer.android.com/reference/android/view/inputmethod/InputConnect..., int) in_reply_to: 5741129492332544 send_mail: 1 subject: Fix InputConnection.deleteSurroundingText()
On 2016/07/22 02:29:29, yukawa wrote: > On 2016/07/22 00:11:48, Changwan Ryu wrote: > > The question here is whether it should be ok to delete a grapheme cluster (> > > codepoint) when it's at the boundary, > > which was indirectly suggested in comment #44. > > Unfortunately, the answer is no. Even if the result text could be an invalid > Java Char sequence, the API needs to delete only the requested number of Java > chars. This is how the built-in EditText behaves, and unfortunately we cannot > change the existing semantics anymore. > > Just FYI, in Android N I added a new API that takes code points. Per-grapheme > text deletion is still on my TODO list though. > https://developer.android.com/reference/android/view/inputmethod/InputConnect..., > int) > in_reply_to: 5741129492332544 > send_mail: 1 > subject: Fix InputConnection.deleteSurroundingText() Thanks for the quick confirmation. yabinh@, we shouldn't try to compromise with grapheme approach.
On 2016/07/22 02:44:43, Changwan Ryu wrote: > On 2016/07/22 02:29:29, yukawa wrote: > > On 2016/07/22 00:11:48, Changwan Ryu wrote: > > > The question here is whether it should be ok to delete a grapheme cluster (> > > > codepoint) when it's at the boundary, > > > which was indirectly suggested in comment #44. > > > > Unfortunately, the answer is no. Even if the result text could be an invalid > > Java Char sequence, the API needs to delete only the requested number of Java > > chars. This is how the built-in EditText behaves, and unfortunately we cannot > > change the existing semantics anymore. > > > > Just FYI, in Android N I added a new API that takes code points. Per-grapheme > > text deletion is still on my TODO list though. > > > https://developer.android.com/reference/android/view/inputmethod/InputConnect..., > > int) > > in_reply_to: 5741129492332544 > > send_mail: 1 > > subject: Fix InputConnection.deleteSurroundingText() > > Thanks for the quick confirmation. yabinh@, we shouldn't try to compromise with > grapheme approach. I'm ok with leaving it as a TODO as the change would require a lot of code work.
https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:583: // For multi-code text, we can't select it successfully if we only What do you mean "multi-code text"? Do you mean a grapheme cluster contains more than one code point? https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:586: const EphemeralRange range = PlainTextRange(0, start).createRange(*rootEditableElement); s/const EphemeralRange/const EphemeralRange&/ https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:613: Position position = validRange.endPosition(); nit: s/Position/const Position&/ https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:614: Position adjustedPosition = nextPositionOf(previousPositionOf(position, PositionMoveType::GraphemeCluster), PositionMoveType::GraphemeCluster); nit: s/Position/const Position&/ https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:615: int diff = adjustedPosition.computeOffsetInContainerNode() - position.computeOffsetInContainerNode(); We should handle in case of position.computeContainerNode() != adjustedPosition().computedContainerNode() https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.h (right): https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.h:81: void deleteSurroundingText(size_t before, size_t after); Since we use |int| for offset in other member functions, let's use |int| instead of |size_t|.
The CQ bit was checked by yabinh@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: Your CL was about to rely on recently removed CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
Description was changed from ========== Fix InputConnection.deleteSurroundingText() According to the Android API documentation, InputConnection.deleteSurroundingText() should delete text before and after the current cursor position, excluding the selection, NOT including the selection. The old implementation reused ExtendSelectionAndDelete which has different semantics and was originally introduced for ChromeOS; this adds a new method and IPC which behaves according to Android requirements. BUG=451027 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix InputConnection.deleteSurroundingText() According to the Android API documentation, InputConnection.deleteSurroundingText() should delete text before and after the current cursor position, excluding the selection, NOT including the selection. The old implementation reused ExtendSelectionAndDelete which has different semantics and was originally introduced for ChromeOS; this adds a new method and IPC which behaves according to Android requirements. BUG=451027 ==========
The CQ bit was checked by yabinh@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...
https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:583: // For multi-code text, we can't select it successfully if we only On 2016/10/04 09:59:54, Yosi_UTC9 wrote: > What do you mean "multi-code text"? Do you mean a grapheme cluster contains more > than one code point? Yes. I'll add some comment in the next patch. https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:586: const EphemeralRange range = PlainTextRange(0, start).createRange(*rootEditableElement); On 2016/10/04 09:59:54, Yosi_UTC9 wrote: > s/const EphemeralRange/const EphemeralRange&/ Done. https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:613: Position position = validRange.endPosition(); On 2016/10/04 09:59:54, Yosi_UTC9 wrote: > nit: s/Position/const Position&/ Done. https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:614: Position adjustedPosition = nextPositionOf(previousPositionOf(position, PositionMoveType::GraphemeCluster), PositionMoveType::GraphemeCluster); On 2016/10/04 09:59:54, Yosi_UTC9 wrote: > nit: s/Position/const Position&/ Done. https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:615: int diff = adjustedPosition.computeOffsetInContainerNode() - position.computeOffsetInContainerNode(); On 2016/10/04 09:59:54, Yosi_UTC9 wrote: > We should handle in case of position.computeContainerNode() != > adjustedPosition().computedContainerNode() It seems that we couldn't find such a case. As we discussed offline, I'll just add assertion in the next patch. https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.h (right): https://codereview.chromium.org/1889053003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.h:81: void deleteSurroundingText(size_t before, size_t after); On 2016/10/04 09:59:54, Yosi_UTC9 wrote: > Since we use |int| for offset in other member functions, let's use |int| instead > of |size_t|. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix InputConnection.deleteSurroundingText() According to the Android API documentation, InputConnection.deleteSurroundingText() should delete text before and after the current cursor position, excluding the selection, NOT including the selection. The old implementation reused ExtendSelectionAndDelete which has different semantics and was originally introduced for ChromeOS; this adds a new method and IPC which behaves according to Android requirements. BUG=451027 ========== to ========== Fix InputConnection.deleteSurroundingText() According to the Android API documentation, InputConnection.deleteSurroundingText() should delete text before and after the current cursor position, excluding the selection, NOT including the selection. The old implementation reused ExtendSelectionAndDelete which has different semantics and was originally introduced for ChromeOS; this adds a new method and IPC which behaves according to Android requirements. BUG=451027 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by yabinh@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 yabinh@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...
https://codereview.chromium.org/1889053003/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:716: const Position& adjustedPosition = previousPositionOf( I saw this pattern in another CL. Can we have functions to compute diff for both previous/next? https://codereview.chromium.org/1889053003/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:724: start = start + diff; Let's introduce new variable, e.g. |adjustedStart|. https://codereview.chromium.org/1889053003/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:754: end = end + diff; Let's introduce new variable, e.g. |adjustedEnd|.
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 yabinh@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...
https://codereview.chromium.org/1889053003/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:716: const Position& adjustedPosition = previousPositionOf( On 2016/10/12 08:34:17, Yosi_UTC9 wrote: > I saw this pattern in another CL. Can we have functions to compute diff for both > previous/next? Done. https://codereview.chromium.org/1889053003/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:724: start = start + diff; On 2016/10/12 08:34:17, Yosi_UTC9 wrote: > Let's introduce new variable, e.g. |adjustedStart|. Done. https://codereview.chromium.org/1889053003/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:754: end = end + diff; On 2016/10/12 08:34:17, Yosi_UTC9 wrote: > Let's introduce new variable, e.g. |adjustedEnd|. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm for third_party/Webkit/Source/core/editing https://codereview.chromium.org/1889053003/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:875: PlainTextRange selectionOffsets(getSelectionOffsets()); s/PlainTextRange/const PlainTextRange/ https://codereview.chromium.org/1889053003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:878: Element* rootEditableElement = frame().selection().rootEditableElement(); s/Element*/Element* const/ https://codereview.chromium.org/1889053003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:912: PlainTextRange range(0, selectionEnd + after); s/PlainTextRange/const PlainTextRange/ https://codereview.chromium.org/1889053003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:926: nit: Remove an extra blank line
The CQ bit was checked by yabinh@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...
yabinh@chromium.org changed reviewers: + clamy@chromium.org
Thanks~ clamy@, Please review changes in content/browser/frame_host/, content/common/, content/test/, Thanks~ https://codereview.chromium.org/1889053003/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1889053003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:875: PlainTextRange selectionOffsets(getSelectionOffsets()); On 2016/10/13 03:40:10, Yosi_UTC9 wrote: > s/PlainTextRange/const PlainTextRange/ Done. https://codereview.chromium.org/1889053003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:878: Element* rootEditableElement = frame().selection().rootEditableElement(); On 2016/10/13 03:40:10, Yosi_UTC9 wrote: > s/Element*/Element* const/ Done. https://codereview.chromium.org/1889053003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:912: PlainTextRange range(0, selectionEnd + after); On 2016/10/13 03:40:10, Yosi_UTC9 wrote: > s/PlainTextRange/const PlainTextRange/ Done. https://codereview.chromium.org/1889053003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:926: On 2016/10/13 03:40:10, Yosi_UTC9 wrote: > nit: Remove an extra blank line Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! content/ looks mostly good, just a question about the integers types, since in places we have size_t and other places int. https://codereview.chromium.org/1889053003/diff/460001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1889053003/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:461: void DeleteSurroundingText(int before, int after); Change the ints into a size_t to match the function above? It would also make more sense to use an unsigned type for a position.
The CQ bit was checked by yabinh@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...
Thanks~ https://codereview.chromium.org/1889053003/diff/460001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1889053003/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:461: void DeleteSurroundingText(int before, int after); On 2016/10/13 12:26:00, clamy wrote: > Change the ints into a size_t to match the function above? It would also make > more sense to use an unsigned type for a position. Done.
Thanks! One more comment. https://codereview.chromium.org/1889053003/diff/480001/content/common/input_m... File content/common/input_messages.h (right): https://codereview.chromium.org/1889053003/diff/480001/content/common/input_m... content/common/input_messages.h:151: int /* before */, I think we should also convert this to size_t if they are instantiated from size_t (and the same in RenderFrameImpl).
The CQ bit was checked by yabinh@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...
https://codereview.chromium.org/1889053003/diff/480001/content/common/input_m... File content/common/input_messages.h (right): https://codereview.chromium.org/1889053003/diff/480001/content/common/input_m... content/common/input_messages.h:151: int /* before */, On 2016/10/13 13:10:07, clamy wrote: > I think we should also convert this to size_t if they are instantiated from > size_t (and the same in RenderFrameImpl). Done.
Thanks! Lgtm with a nit. https://codereview.chromium.org/1889053003/diff/500001/content/test/test_rend... File content/test/test_render_frame.h (right): https://codereview.chromium.org/1889053003/diff/500001/content/test/test_rend... content/test/test_render_frame.h:41: void DeleteSurroundingText(int before, int after); nit: this too should be changed to size_t.
The CQ bit was checked by yabinh@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...
https://codereview.chromium.org/1889053003/diff/480001/content/common/input_m... File content/common/input_messages.h (right): https://codereview.chromium.org/1889053003/diff/480001/content/common/input_m... content/common/input_messages.h:151: int /* before */, On 2016/10/13 13:39:37, yabinh wrote: > On 2016/10/13 13:10:07, clamy wrote: > > I think we should also convert this to size_t if they are instantiated from > > size_t (and the same in RenderFrameImpl). > > Done. Got trybot compile failure in patch set #26: content/common/input_messages.h:150:1: error: [chromium-ipc] IPC tuple references banned type 'size_t' Does that mean we shouldn't use size_t here? If so, should we keep using size_t in render_frame_impl and test_render_frame?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/10/13 13:58:57, yabinh wrote: > https://codereview.chromium.org/1889053003/diff/480001/content/common/input_m... > File content/common/input_messages.h (right): > > https://codereview.chromium.org/1889053003/diff/480001/content/common/input_m... > content/common/input_messages.h:151: int /* before */, > On 2016/10/13 13:39:37, yabinh wrote: > > On 2016/10/13 13:10:07, clamy wrote: > > > I think we should also convert this to size_t if they are instantiated from > > > size_t (and the same in RenderFrameImpl). > > > > Done. > > Got trybot compile failure in patch set #26: > > content/common/input_messages.h:150:1: error: [chromium-ipc] IPC tuple > references banned type 'size_t' > > Does that mean we shouldn't use size_t here? If so, should we keep using size_t > in render_frame_impl and test_render_frame? If it can't be put in IPCs then yes it should probably be changed to an int (and the rest of them as well). I would just like us to be consistent and avoid implicit int/size_t conversions.
The CQ bit was checked by yabinh@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...
On 2016/10/13 14:40:35, clamy wrote: > On 2016/10/13 13:58:57, yabinh wrote: > > > https://codereview.chromium.org/1889053003/diff/480001/content/common/input_m... > > File content/common/input_messages.h (right): > > > > > https://codereview.chromium.org/1889053003/diff/480001/content/common/input_m... > > content/common/input_messages.h:151: int /* before */, > > On 2016/10/13 13:39:37, yabinh wrote: > > > On 2016/10/13 13:10:07, clamy wrote: > > > > I think we should also convert this to size_t if they are instantiated > from > > > > size_t (and the same in RenderFrameImpl). > > > > > > Done. > > > > Got trybot compile failure in patch set #26: > > > > content/common/input_messages.h:150:1: error: [chromium-ipc] IPC tuple > > references banned type 'size_t' > > > > Does that mean we shouldn't use size_t here? If so, should we keep using > size_t > > in render_frame_impl and test_render_frame? > > If it can't be put in IPCs then yes it should probably be changed to an int (and > the rest of them as well). I would just like us to be consistent and avoid > implicit int/size_t conversions. Got it. Then we'll only keep the size_t in render_frame_host_impl. :)
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 yabinh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, changwan@chromium.org, yosin@chromium.org, clamy@chromium.org Link to the patchset: https://codereview.chromium.org/1889053003/#ps510019 (title: "change back to patch set #25")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dcheng@, can you take a look at content/common/input_messages.h? It seems that I need l-g-t-m from SECURITY_OWNERS. Thanks~
ipc lgtm
The CQ bit was checked by yabinh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #28 (id:510019)
Message was sent while issue was closed.
Description was changed from ========== Fix InputConnection.deleteSurroundingText() According to the Android API documentation, InputConnection.deleteSurroundingText() should delete text before and after the current cursor position, excluding the selection, NOT including the selection. The old implementation reused ExtendSelectionAndDelete which has different semantics and was originally introduced for ChromeOS; this adds a new method and IPC which behaves according to Android requirements. BUG=451027 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix InputConnection.deleteSurroundingText() According to the Android API documentation, InputConnection.deleteSurroundingText() should delete text before and after the current cursor position, excluding the selection, NOT including the selection. The old implementation reused ExtendSelectionAndDelete which has different semantics and was originally introduced for ChromeOS; this adds a new method and IPC which behaves according to Android requirements. BUG=451027 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/90f1be18144ff4106ad203d45915b116938bf13a Cr-Commit-Position: refs/heads/master@{#425244} ==========
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/90f1be18144ff4106ad203d45915b116938bf13a Cr-Commit-Position: refs/heads/master@{#425244} |
