Closed
Bug 751513
Opened 13 years ago
Closed 11 years ago
Typing characters in the contenteditable div causes the whole line to be deleted
Categories
(Firefox for Android Graveyard :: Keyboards and IME, defect)
Tracking
(blocking-fennec1.0 -, fennec+)
RESOLVED
WORKSFORME
People
(Reporter: martijn.martijn, Unassigned)
References
(Depends on 1 open bug, )
Details
(Keywords: regression, testcase, Whiteboard: VKB)
Attachments
(3 files, 1 obsolete file)
I'm seeing this on the HTC Desire HD and on the Samsung Galaxy Nexus. I don't see it on the Galaxy SII, but I don't get an IME help there (the suggestion that appear while typing above the vkb, right?).
I don't see the problem occuring on an Auror build of 2012-04-02, so this looks like a regression.
Steps to reproduce:
- With the testcase, focus at the end of the text
- Type 2 characters on the vkb
Expected result:
- 2 characters added
Actual result:
- Whole line deleted, only the 2 typed characters remain
Comment 1•13 years ago
|
||
I was able to repro on Galaxy Nexus, but not Droid Pro with HKB.
Whiteboard: VKB
Comment 2•13 years ago
|
||
This issue doesn't occur on:
Nightly 15.0a1 (2012-04-27)
http://hg.mozilla.org/mozilla-central/rev/450d8cd16316
but it occurs on:
Nightly 15.0a1 (2012-04-28)
http://hg.mozilla.org/mozilla-central/rev/b5c0d6abf69b
Possible range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=450d8cd16316&tochange=b5c0d6abf69b
Keywords: regressionwindow-wanted
Reporter | ||
Comment 3•13 years ago
|
||
Thanks, regression from bug 743468 perhaps?
Comment 4•13 years ago
|
||
mw22, I think you are correct. This looks like a regression from bug 743468.
Comment 5•13 years ago
|
||
Candidate for blocking-fennec1.0. This regression affects contenteditable div, not all text input.
Assignee: nobody → cpeterson
blocking-fennec1.0: --- → ?
Comment 6•13 years ago
|
||
Chris, we'll need you to decide if backing out bug 743468 is right or there is a good fix for this.
blocking-fennec1.0: ? → +
Comment 7•13 years ago
|
||
bug 743468 has been backed out of aurora, but this bug needs to be fixed soon if we want bug 743468 to make Fx14
Comment 8•13 years ago
|
||
Query Gecko selection asynchronously (like XUL Fennec). The Android widget code alerts us BEFORE the selection has changed, so we must ask Gecko for the new selection range AFTER it has been changed.
This change also fixes some problems with "Select All" text not getting the correct selection range.
Attachment #624315 -
Flags: review?(blassey.bugs)
Updated•13 years ago
|
Status: NEW → ASSIGNED
Comment 9•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/43aa1f392da4
Should this have a test?
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
Comment 10•13 years ago
|
||
oops, I mistakenly listed THIS bug number in my commit message for bug 751864. THAT bug has been fixed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•13 years ago
|
status-firefox14:
--- → affected
status-firefox15:
--- → affected
Comment 11•13 years ago
|
||
Comment on attachment 624315 [details] [diff] [review]
bug-751513-query-selection.patch
Review of attachment 624315 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/GeckoInputConnection.java
@@ +502,1 @@
> }
remove the curly braces
@@ +527,5 @@
> + updateSelectionFromGecko(mGeckoSelectionStart, mGeckoSelectionEnd);
> + }}, 200);
> + }
> +
> + private void updateSelectionFromGecko(int start, int end) {
this needs a little more explanation. Are we just hoping that this timer fires after the selection has been changed? I'd rather "just" fire a new event from gecko instead.
@@ +1092,5 @@
> private boolean hasCompositionString() {
> return mCompositionStart != NO_COMPOSITION_STRING;
> }
> +
> + private void queryGeckoSelection() {
which thread does this run on? Should be in a comment
@@ +1094,5 @@
> }
> +
> + private void queryGeckoSelection() {
> + if (mQueryResult == null) {
> + mQueryResult = new SynchronousQueue<String>();
do this in the class constructor
@@ +1107,5 @@
> + Log.e(LOGTAG, "IME: queryGeckoSelection interrupted?!", e);
> + }
> + }
> +
> + public void putQueryResult(String result, int selectionStart, int selectionEnd) {
which thread does this run on? Should be in a comment
Attachment #624315 -
Flags: review?(blassey.bugs) → review-
Comment 12•13 years ago
|
||
The asynchronous queryGeckoSelection was a hack, but it is no longer necessary because I now understand the root cause.
Android is not getting the proper selection change notifications because Gecko is not calling Android widget's OnIMEFocusChange(). This is a regression caused by the fix for bug 612128. Fortunately, Ehsan has a fix in bug 688438.
Comment 13•13 years ago
|
||
I relanded the fix to bug 688438 on the inbound branch. Please let me know if I can help more.
Updated•13 years ago
|
Comment 15•13 years ago
|
||
With Ehsan's fix for bug 688438, the contenteditable div's text is no longer deleted, BUT we are inserting new text in the wrong position. The first new word is correctly appended to the div, but subsequent words are inserted at the beginning of the div.
Keywords: qawanted
Comment 16•13 years ago
|
||
Still investigating. This bug only affects VKB when predictive text is enabled, so the bug is related to text selection around composition strings.
Target Milestone: Firefox 15 → ---
Version: Trunk → Firefox 16
Comment 17•13 years ago
|
||
This bug has something to do with selection offsets being out-of-bounds when contenteditable divs contain plaintext and whitespace immediately before the </div> closing tag. Unfortunately, contenteditable div text is often followed by a newline before the </div> closing tag (including Mozilla's MDN example code).
<div contenteditable=true>TEXT WORKS</div>
<div contenteditable=true>TEXT AND SPACE DOES NOT WORK </div>
<div contenteditable=true>TEXT AND NEWLINE DOES NOT WORK
</div>
<div contenteditable=true><p>ELEMENT WORKS</p></div>
<div contenteditable=true><p>ELEMENT AND SPACE WORKS</p> </div>
<div contenteditable=true><p>ELEMENT AND NEWLINE WORKS</p>
</div>
<div contenteditable=true><p>ELEMENT AND TEXT WORKS</p>TEXT</div>
<div contenteditable=true><p>ELEMENT, SPACE, AND TEXT WORKS</p> TEXT</div>
<div contenteditable=true><p>ELEMENT, TEXT, AND SPACE DOES NOT WORK</p>TEXT </div>
Comment 18•13 years ago
|
||
Do you also get assertions in a debug build when this happens, like bug 757771?
Comment 19•13 years ago
|
||
Ehsan, I don't see the assertions list in bug 757771, but nsContentEventHandler::OnSelectionEvent() logs an error message that nsContentEventHandler::SetRangeFromFlatTextOffset() failed because SetRangeFromFlatTextOffset's computed nativeOffset 32 was less than aNativeOffset 33:
https://hg.mozilla.org/mozilla-central/file/e96e0eaa6d85/content/events/src/nsContentEventHandler.cpp#l453
For comparison, a debug build of Mac Firefox logs these messages when entering Katakana characters using OS X's IME for the same test:
WARNING: NS_ENSURE_SUCCESS(res, res) failed with result 0x80530001: file ../../../../editor/libeditor/html/nsHTMLEditRules.cpp, line 8327
WARNING: NS_ENSURE_SUCCESS(res, res) failed with result 0x80530001: file ../../../../editor/libeditor/html/nsHTMLEditRules.cpp, line 8327
...
Even though Mac IME input seems to behave (more) correctly compared to Android, the problems seem somewhat related because NS_ERROR_DOM_INDEX_SIZE_ERR (0x80530001) is being returned from SetEnd().
Comment 20•13 years ago
|
||
Hmm, OK, then I'm not sure what's going on here. :/
Comment 21•13 years ago
|
||
We should consider bumping this bug from 1.0 to .N+ blocker. I am getting closer to a fix, but the fix would be risky. Some of my other 1.0 blockers look easier to fix and probably affect more users (e.g. bug 755517).
blocking-fennec1.0: + → ?
status-firefox16:
--- → affected
Comment 22•13 years ago
|
||
I'm under the impression that this is only a problem when there is autocomplete suggestions. Can we bandaid this for 14 (and perhaps 15) by turning off autocomplete suggestions when editing a content editable?
Updated•13 years ago
|
tracking-fennec: --- → 15+
blocking-fennec1.0: ? → -
Comment 23•13 years ago
|
||
Android IME has a "no suggestions" hint (InputType.TYPE_TEXT_FLAG_NO_SUGGESTIONS) we can toggle at runtime, but contenteditable would still break for IMEs that must use composition strings (like Swype or Simeji).
Comment 24•13 years ago
|
||
Added simpler test case demonstrating contenteditables with and without trailing whitespace.
nsPlaintextEditor is collapsing the contenteditable's trailing whitespace (in nsWSRunObject), but not notifying Java.
Comment 25•13 years ago
|
||
How should it tell Java?
Comment 26•13 years ago
|
||
I see that the editor is notifying the Android widget (and Java) of the trailing whitespace changes with nsWindow::OnIMETextChange(). The crux is that Gecko trims trailing whitespace _in response_ to Java inserting new text and then the OnIMETextChange() callback reenters the Java code:
1. Java asks Gecko to insert "C" at position 4 of "A B ".
2. Gecko notifies Java that the text is being trimmed to "A B". Unfortunately, this callback reenters Java when Android IME (Google's code, not ours) does not permit text changes!
3. Gecko tries to insert "C" at position 4, but "A B" is now only three characters long.
4. Gecko has "A BC", but Java thinks the text is "A B C".
I think I will need to detect these callbacks and reset the Java IME text to the Gecko text when feasible.
Comment 27•13 years ago
|
||
Can't we just report the text as "A B" from the beginning?
Comment 28•13 years ago
|
||
oops: previous contenteditable.html test was saved as plaintext, not HTML.
Attachment #630668 -
Attachment is obsolete: true
Comment 29•13 years ago
|
||
Ehsan, are you suggesting Gecko's libeditor report "A B" for ALL platforms? Or that Android's widget code could auto-trim whitespace before reporting it to Android's system IME?
Note that Chrome and Safari trim the trailing whitespace BEFORE populating the contenteditable. Desktop Firefox populates the contenteditable with trailing whitespace and later trims it when text is appended.
You can see this using this bug's contenteditable.html test. Just select-all "TRAILING SPACE " or "TRAILING NEWLINE " and paste into another text editor:
https://bug751513.bugzilla.mozilla.org/attachment.cgi?id=630745
Comment 30•13 years ago
|
||
I'm suggesting doing this on all platforms. I believe we discussed it at some point (roc do you remember) but I can't find the bug right now. Basically, our behavior is weird, since such trailing spaces are collapsed for non-editable content, and it doesn't make a lot of sense for us to expose them for editable content.
Note that I'm not suggesting just lying to the IME layer (report "A B" when the thing you're editing is really "A B "). I'm suggesting trimming the trailing space the same way that WebKit does, and then report the real value (which would be "A B") to the IME layer.
Aryeh, what does the spec say about this, if anything?
Bug 681626. Aryeh updated the spec partly based on our discussion.
Comment 32•13 years ago
|
||
If I understand bug 681626 correctly, the conclusion was that trailing whitespace should be trimmed *when* text is deleted or inserted. The trailing whitespace is there and you can position the caret there. That is what I see Gecko doing today.
In contrast, Chrome and Safari trim the whitespace *before* displaying the text. You cannot position or move the caret beyond the contenteditable div's last non-whitespace character.
Depends on: 681626
Comment 33•13 years ago
|
||
(In reply to Ehsan Akhgari [:ehsan] from comment #30)
> Note that I'm not suggesting just lying to the IME layer (report "A B" when
> the thing you're editing is really "A B "). I'm suggesting trimming the
> trailing space the same way that WebKit does, and then report the real value
> (which would be "A B") to the IME layer.
>
> Aryeh, what does the spec say about this, if anything?
Bug 681626 comment 15 ff. contains the relevant info (thanks, roc!). From comment 32, it sounds like Gecko is already doing that.
There's something further we could do: don't allow the cursor to be positioned after insignificant whitespace that's at the end of a line. If the cursor would be placed there, move it back to before the whitespace. Then nothing will try inserting after the to-be-stripped whitespace to begin with.
(In reply to Chris Peterson (:cpeterson) from comment #32)
> In contrast, Chrome and Safari trim the whitespace *before* displaying the
> text. You cannot position or move the caret beyond the contenteditable div's
> last non-whitespace character.
WebKit's selection handling is very weird. IIUC, it's based on the visible text -- their internal selection class is called VisibleSelection. They translate back and forth to DOM locations when the DOM APIs require it. But for instance, if you have something like
<b>foo</b><i>bar</i>
you'll never have the selection at (<b>, 0) or (<b>, 1) or in between the <b> and <i>. Such a selection point can't be represented in WebKit. If you try using addRange() or collapse() or something to get it to go there, it will go to (foo, 0) or (foo, 3) or whatever instead. This results in all kinds of bugs.
The general idea of selection normalization is essential, though, and we don't do enough of it -- WebKit is better in that respect. We should be deciding on a normalization routine that will make the selection points as predictable as possible, so we can't have things like <b>foo{</b><i>b]ar</i> (which looks just like <b>foo</b><i>[b]ar</i> but in Gecko behaves differently). We should then call that routine often, such as before any command or user input. I've been meaning to spec this for a long time.
Comment 34•13 years ago
|
||
OK, so I think that our caret placement should be fixed to make sure that we can't place or move the caret past the trimmed whitespace (which is essentially the fix to bug 681626). Unfortunately I don't have enough time to work on that at the moment (unless Aryeh wants to jump in and help :), but in the mean time, if we stop reporting the trimmed whitespace to the IME layer, that would fix this bug, right?
Comment 35•13 years ago
|
||
(In reply to Ehsan Akhgari [:ehsan] from comment #34)
> but in the mean time, if we stop reporting the trimmed whitespace to the IME
> layer, that would fix this bug, right?
I think so, because Android IME would not try to insert text within or after trailing whitespace that Gecko will trim.
I may be able to implement a less invasive fix just for Android. When Gecko's OnIMETextChange notification reenters Java when Android's IME is busy, I can save the text change and apply it later.
Comment 36•12 years ago
|
||
Fixed by my fix for bug 769520.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Target Milestone: --- → Firefox 16
Comment 37•12 years ago
|
||
Fixed in Aurora 15 because bug 769520 was uplifted to m-a.
Updated•12 years ago
|
Status: RESOLVED → VERIFIED
status-firefox17:
--- → verified
Comment 38•12 years ago
|
||
Reopening because of the backout of bug 769250.
Status: VERIFIED → REOPENED
status-firefox14:
affected → ---
status-firefox15:
verified → ---
status-firefox16:
verified → ---
status-firefox17:
verified → ---
Resolution: FIXED → ---
Target Milestone: Firefox 16 → ---
Comment 39•12 years ago
|
||
Even though bug 769520 was backed out, this testcase still passes with Nightly 18.0a1 2012-09-10).
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 40•12 years ago
|
||
Is it fixed in 15.0.1, 16.0 Beta 3 and Aurora 17?
Comment 41•12 years ago
|
||
Good question, Scoobidiver. I just double-checked the browser versions I tested and discovered that 15.0.1 and Beta 16.0b3 have not been published yet. <:)
This testcase works for me on Nightly 18 and Aurora 17 (which both have my backout), but I will keep this bug open until I can also test 15.0.1 and Beta 16.0b3, too.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 42•12 years ago
|
||
15.0.1 can be downloaded from ftp://ftp.mozilla.org/pub/mobile/candidates/15.0.1-candidates/build2/android/en-US/fennec-15.0.1.en-US.android-arm.apk
I guess if it works in 15.0.1 and 17.0a2, it will work in 16.0b3 that will be built in two days.
Updated•12 years ago
|
tracking-fennec: 15+ → +
Updated•11 years ago
|
Assignee: cpeterson → nobody
Status: REOPENED → NEW
Comment 44•11 years ago
|
||
Device: HTC Desire HD (Android 2.3.5)
Build: Firefox for Android 30.0a1 (2014-02-26)
I cannot reproduce the issue. With the testcase given if I type 2 characters at the end of the text,
the line isn't deleted, and the 2 characters are present after the existing text.
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago → 11 years ago
Keywords: qawanted
Resolution: --- → WORKSFORME
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•