Vietnamese characters are broken on contenteditable |
||||||||||||
Issue descriptionVersion: 54 OS: Android What steps will reproduce the problem? (1) Go to a webpage with a contenteditable div, e.g. http://www.w3schools.com/TAgs/tryit.asp?filename=tryhtml5_global_contenteditable (2) Use Samsung keyboard or Laban Key keyboard. (3) Type 'a', 'a', ' ', 'a', 'a'. What is the expected result? We should see A (with a hat) and white space, and a small a with a hat. What happens instead? Aaa Originally filed as b/32595569. This reproes on Samsung's email as well because they're using a content editable div. The core reason is that for Vietnamese Blink somehow converts a normal space (\20) into a non-breaking space (\a0). The old ReplicaInputConnection replaced the characters, but the new ThreadedInputConnection was not doing it. I think an immediate solution for M55 here is to add the same replacing logic to ThreadedInputConnection.
,
Nov 10 2016
requesting merge of #1
,
Nov 10 2016
Your change meets the bar and is auto-approved for M55 (branch: 2883)
,
Nov 10 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d744333cb7410f973555d755778b3fa85a2c1c52 commit d744333cb7410f973555d755778b3fa85a2c1c52 Author: Changwan Ryu <changwan@google.com> Date: Thu Nov 10 22:35:30 2016 Fix Vietnamese characters to work correctly on contenteditable Blink seems to convert a normal space (\20) into a non-breaking space (\a0) only when Vietnamese + contenteditable is used. The immediate solution for M55 is to copy the same replacing logic from ReplicaInputConnection to ThreadedInputConnection. FYI, the ReplicaInputConnection logic was there from the beginning: https://chromiumcodereview.appspot.com/10911012/diff/10001/content/ public/android/java/src/org/chromium/content/browser/ContentViewCore.java BUG= 663880 Review-Url: https://codereview.chromium.org/2489233004 Cr-Commit-Position: refs/heads/master@{#431363} (cherry picked from commit a90b47231f92d86db194138b952ad0910ee923eb) Review URL: https://codereview.chromium.org/2497463002 . Cr-Commit-Position: refs/branch-heads/2883@{#527} Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768} [modify] https://crrev.com/d744333cb7410f973555d755778b3fa85a2c1c52/content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java
,
Nov 11 2016
I chatted with aelias@ about this, but ultimately #1 should be replaced by a correct fix in Blink. We need to check if it affects Element.innerText() as well. The major problem was fixed, so let me remove RBS label for now.
,
Nov 11 2016
In case we respin m54, would it be ok to merge #1 as well?
,
Nov 11 2016
This would only be a nice-to-have, and we decided to be more conservative about nice-to-have cherry-picks at the last minute. Respins this late should only have the fix for the P0 issue they were respinned for.
,
Nov 11 2016
Got it, thanks. uncc'ing amineer.
,
Dec 9 2016
assigning to yabinh@ for a possible fix in Blink
,
Dec 13 2016
yosin@, for IME purposes, does it make sense to convert non-breaking space into a normal space? If so, where should we change? Currently, it's confusing keyboard apps so we had to workaround in the browser side. Would it make sense to do this in InputMethodController or TextIterator instead? BTW, I'm also curious if innerText (textContent) should emit non-breaking space or not. (cc'ing kojii@ for this). According to the following page, FireFox emits a non-breaking space while IE emits a normal space for . http://osdir.com/ml/watir-general/2009-12/msg00240.html The current Chrome behavior is to emit a non-breaking space.
,
Jan 20 2017
Some comment in CompositeEditCommand::rebalanceWhitespaceOnTextSubstring(): ''' // FIXME: Because of the problem mentioned at the top of this function, we // must also use nbsps at the start/end of the string because this function // doesn't get all surrounding whitespace, just the whitespace in the // current text node. However, if the next sibling node is a text node // (not empty, see http://crbug.com/632300 ), we should use a plain space. // See http://crbug.com/310149 ''' Blink replaces ns(normal space) with nbsp(non-breaking space) because the space is at the end of the paragraph. If we don't do that, we will not be able to input the space, just like Issue 632300 . Since there is a workaround on the browser side, let's decrease priority of this issue.
,
Jan 20 2017
I see that NBSP workaround was originally added in http://crrev.com/e33c0d56fe69064e4a69 . In that CL, the "problem mentioned at the top of this function" is "// FIXME: Doesn't go into text nodes that contribute adjacent text (siblings, cousins, etc)". So that would be the underlying problem to fix in order to clean this up.
,
Jan 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/34f2476fb5c0fab47e6d0b99d18b99a1b6fce7be commit 34f2476fb5c0fab47e6d0b99d18b99a1b6fce7be Author: aelias <aelias@chromium.org> Date: Wed Jan 25 06:46:12 2017 Avoid reporting space as NBSP in TextInputState. This removes a fix done at the top level Android layer and moves it into TextIterator. Because the text is stored away in intermediate caches (mLastText, mCachedTextInputState), it seems more principled to do it at emission time to avoid potential cache mismatch issues. This change also avoids allocating a new Java String for every update. BUG= 663880 Review-Url: https://codereview.chromium.org/2649923012 Cr-Commit-Position: refs/heads/master@{#445972} [modify] https://crrev.com/34f2476fb5c0fab47e6d0b99d18b99a1b6fce7be/content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java [modify] https://crrev.com/34f2476fb5c0fab47e6d0b99d18b99a1b6fce7be/third_party/WebKit/Source/core/editing/InputMethodController.cpp [modify] https://crrev.com/34f2476fb5c0fab47e6d0b99d18b99a1b6fce7be/third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp [modify] https://crrev.com/34f2476fb5c0fab47e6d0b99d18b99a1b6fce7be/third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp [modify] https://crrev.com/34f2476fb5c0fab47e6d0b99d18b99a1b6fce7be/third_party/WebKit/Source/core/editing/iterators/TextIteratorFlags.h [modify] https://crrev.com/34f2476fb5c0fab47e6d0b99d18b99a1b6fce7be/third_party/WebKit/Source/core/editing/iterators/TextIteratorTextState.cpp [modify] https://crrev.com/34f2476fb5c0fab47e6d0b99d18b99a1b6fce7be/third_party/WebKit/Source/core/editing/iterators/TextIteratorTextState.h
,
Jan 25 2017
|
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by bugdroid1@chromium.org
, Nov 10 2016