932370 - Problems when entering a linebreak in the default Rich Text Editor of forums running XenForo software

Status: RESOLVED INACTIVE

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[lmmozillabug]

A line break is created, but the cursor immediately returns to the space prior to the line break. From this point forward text is inputted as if in "insert mode", with all text appearing to the right of the cursor which no longer moves. "Insert mode" persists until page is refreshed or user switches to simplified text editor.

Using the Google Keyboard, keyboard suggestions are immediately entered following any keystroke. Though this suggestion behavior does not occur when using SwiftKey, the switch to "insert mode" following the pressing of the return key is consistent across keyboards.

Tested on Firefox v25, v26, and v28. All versions exhibit identical behavior. Problem does not occur in Chrome or Opera apps.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Thanks for the report. Can you provide a test URL?

Comment 2

[lmmozillabug]

XenForo has a forum on their website, but you'll need to create an account to access the reply box:

http://xenforo.com/community/forums/general-xenforo-discussion-and-feedback.45/

Comment 3

[lmmozillabug]

Problem remains in versions 26.01, 27, 28.0a2, and 29.0a1.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Which device and Android version are you using?

Comment 5

[lmmozillabug]

I am using a Sprint HTC One, running stock Android 4.3 (the latest available official release for my model).

Comment 6

[lmmozillabug]

I just confirmed that the bug also occurs on an LG G Pad 8.3 Google Play Edition running Android 4.4.2.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Have you used the Android "adb" tool before? It'll be very helpful if you can provide the logcat output using this Nightly below, while you're trying to enter a line break. Thanks!

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-be395f8ddacd/try-android/fennec-29.0a1.en-US.android-arm.apk

Comment 8

[lmmozillabug]

Attachment: Demonstration of XenForo Rich Text Editor linebreak bug

I started logcat output on my HTC One, tapped the text box to open the SwiftKey keyboard, and then input the following text one character at a time:

>test
>testtest
>test test
>test
>
>test
>testtest
>test test
>test

The resultant text in the text box was this:

>test test test test testtesttteststestet
>testtteststestetestt testtesttteststestet
>testtteststestetestttesttteststestet
>testtteststestet
>
>testtteststestet

When using the provided Nightly, I am seeing what appears to be identical behavior when using the SwiftKey keyboard as when using the Google Keyboard. Under Firefox v26, Swiftkey behavior differs and the following text results from the test input above:

>testtset
>tset tset
>tsettset
>tset
>
>tset
>tset tset
>tsettset

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Thank you very much! I'll look at the logcat and hopefully identify what's happening.

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Does this Nightly improve things? http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0f24721f46dd/try-android/fennec-29.0a1.en-US.android-arm.apk

Please attach the logcat as well. Thanks!

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Properly measure offsets for deleted BR nodes (v1)

One problem is nsTextStateManager::ContentRemoved() doesn't work for deleted BR nodes. We measure the text length of the deleted BR node by calling GetFlatTextOffsetOfRange with an offset of 1, but that does not work. This patch adds a behavior to GetFlatTextOffsetOfRange: when the specified offset is -1, the function will measure to the end of the node, and will work for BR nodes.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8383946 [details] [diff] [review]
Properly measure offsets for deleted BR nodes (v1)

I don't understand what is the cause actually. When this bug occurs, what are the aContainer, aChild and the childOffset value before a call of the second GetFlatTextOffsetOfRange()?

Anyway, I don't like the approach using "-1" for it because it may run the special case with a caller side's bug (e.g., passing 2 - 3).

And I'm not familiar with mutation observer's behavior, though. Does this work fine with remove text node or element in HTML editor?

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

This happens with a document like "<div>Test<br></div>". When the <br> node is deleted, nsTextStateManager::ContentRemoved is called with these arguments,

aDocument = doc
aContainer = div
aChild = br
aIndexInContainer = 1 (index 0 is text node)
aPreviousSibling = <text>

Now we want to notify widget of text change with arguments, start = 4, oldEnd = 5, newEnd = 4.

To get start and newEnd = 4, we call GetFlatTextOffsetOfRange with node = aContainer (div) and offset = aIndexInContainer (1).

To get oldEnd = 5, we need to find the length of the deleted aChild (br node):

* With the old code, we call GetFlatTextOffsetOfRange with node = aChild (br) and offset = 1. However, this does not work because <br> does not have a child with offset = 1, and the call fails.

* With the new code, we call GetFlatTextOffsetOfRange with node = aChild (br) and offset = -1. This works because we define offset = -1 as "end of the node", so the returned offset is the length of the node.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

So, it sounds like that SetEnd(node, node->Length() + 1) always fails.

If so, in GetFlatTextOffsetOfRange(), isn't following code enough to fix this bug? Other callers might have same bug.

   if (aNode->Length() >= aNodeOffset) {
     prev->SetEnd(startDOMNode, aNodeOffset);
     iter->Init(prev);
   } else if (aNode != static_cast<nsINode*>(aRootContent)) {
     prev->SetEndAfter(startDOMNode);
     iter->Init(prev);
   } else {
     iter->Init(aRootContent);
   }

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Properly measure offsets for deleted BR nodes (v2)

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #14)
> So, it sounds like that SetEnd(node, node->Length() + 1) always fails.
> 
> If so, in GetFlatTextOffsetOfRange(), isn't following code enough to fix
> this bug? Other callers might have same bug.

Yup, that works too.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI: the file name is now dom/events/ContentEventHandler.cpp. Please change the file name in the diff file before landing onto m-i.

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

Thanks for the reminder!

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4f51e54c14d

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

Fix for bustage,

https://hg.mozilla.org/integration/mozilla-inbound/rev/f98f596ad056

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c4f51e54c14d
https://hg.mozilla.org/mozilla-central/rev/f98f596ad056

Comment 20

[lmmozillabug]

I apologize for the late response. This bug is still present in versions 28.0, 29.0a2, and 31.0a1. The last APK link above is dead; would you like logcat output from the Nightly 31.0a1, or should I test with a different build?

Comment 21

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to lmmozillabug from comment #20)
> I apologize for the late response. This bug is still present in versions
> 28.0, 29.0a2, and 31.0a1. The last APK link above is dead; would you like
> logcat output from the Nightly 31.0a1, or should I test with a different
> build?

No worries! Can you try this build and post the logs?

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6233fe5e3a76/try-android/fennec-31.0a1.en-US.android-arm.apk

Comment 22

[lmmozillabug]

Attachment: Demonstration of bug under Nightly 31.0a1

Here is the requested log. I used the same test:

Expected output:
>test
>testtest
>test test
>test
>
>test
>testtest
>test test
>test

Actual output:
>test testtesttteststestet testtesttteststestet
>testtteststestetestt testtteststestetestttesttteststestet
>testtteststestetestttesttteststestet
>testtteststestet
>
>testtteststestet