1518002 - Evented textarea jumping typing

Reporter

Description

•

5 years ago

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

Seen in version Firefox 65 on Android 6.0 (thanks Samsung). Attached video shows what happens in firefox with "bad" textarea, simple textarea, and with the "bad" textarea in samsung internet. Possibly related (but old) bugs: https://bugzilla.mozilla.org/show_bug.cgi?id=693266 https://bugzilla.mozilla.org/show_bug.cgi?id=1237286

1. Go to  a site that has a textarea that has event listeners on it (suspected cause)
2. Start typing
3. Try to change position and type again


Actual results:

Jumbled writing: text is written backwards, carat jumps first position if try to reposition and continue typing


Expected results:

Writing should work as normal (ltr, not jumping back and forth) as per a normal textarea / what happens in samsung internet

Jack

Reporter

Comment 1

•

5 years ago

Screen capture video of bug: https://www.youtube.com/watch?v=8iZi7lGLwCY

miralobontiu

Comment 2

•

5 years ago

Hi Jack,
I was not able to reproduce the issue. Tested on Samsung Galaxy Tab S3 (Android 8), on Firefox Beta 65.0b8 and Nightly 66.0a1 (2019-01-07).
Could you tell us the device you found the issue and the URL?
Thanks!

Flags: needinfo?(firefox)

miralobontiu

Updated

•

5 years ago

Component: General → Text Selection

Jack

Reporter

Comment 3

•

5 years ago

Hello. It is a Samsung Galaxy S5 Active. The textarea was on the issue editor form in gitlab.com (eg https://gitlab.com/bytesnz/ls-ignore/issues/new, but you need to log in to gitlab). I have seen it happen on a few other web sites, but can't remember which ones.

Flags: needinfo?(firefox)

miralobontiu

Comment 4

•

5 years ago

Tested with Samsung Galaxy S6 (Android 7) on URL provided by you (made an account), but I could not reproduce the issue.
Do you have some add-ons installed? Could you try to test to see if you can reproduce with a clean profile?
Thank you!

miralobontiu

Updated

•

5 years ago

Flags: needinfo?(firefox)

Jack

Reporter

Comment 5

•

5 years ago

I tried it again on a fresh install of Firefox Beta on another phone (Samsung Galaxy S4 mini). It turns out I missed a step. Steps I used to reproduce:

Goto website like https://gitlab.com/bytesnz/ls-ignore/issues/new
Enter multiple lines of text into textare
Click the Preview button above the textarea
Click the Write button above the textarea to go back to the editor and textarea
Focus somewhere in the textarea and start typing.
Cursor starts jumping after a small pause

I noticed there is flashes of a scrollbar in/near the textarea, so I wonder if that has something to do with it. I tested it again in Samsung Internet and in Chrome and both did not show the issue.

Apologies for missing steps. Hopefully that will work for you.

Flags: needinfo?(firefox)

miralobontiu

Comment 6

•

5 years ago

Hi Jack,
I was able to reproduce the issue, following the steps from Comment 5.
Tested with Samsung Galaxy S6 (Android 7) on the latest Nightly 66.0a1 (2019-01-09).
I will confirm it.
Thank you!

Status: UNCONFIRMED → NEW

status-firefox64: --- → unaffected

status-firefox65: --- → affected

status-firefox66: --- → affected

Ever confirmed: true

OS: Unspecified → Android

Hardware: Unspecified → ARM

miralobontiu

Comment 7

•

5 years ago

Attached file logcat — Details

Please see attached logcat.

miralobontiu

Comment 8

•

5 years ago

Ran a regression with Samsung Galaxy S7 Edge (Android 8.0):
Last good revision: 5cdef82c0fb01e17ab451c546f4bff1b90a1f7af
First bad revision: bda6c79cf03c921d16e4944fc200a424b4af78b0

Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5cdef82c0fb01e17ab451c546f4bff1b90a1f7af&tochange=bda6c79cf03c921d16e4944fc200a424b4af78b0

Keywords: regression

Jared Wein [:jaws] (please needinfo? me)

Updated

•

5 years ago

Blocks: 1465702

Flags: needinfo?(masayuki)

Liz Henry (:lizzard) (relman/hg->git project)

Updated

•

5 years ago

Priority: -- → P2

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

Comment 9

•

5 years ago

Sounds like a bug of Android widget's IME handling code, but according to comment 8, this is a regression of bug 1465702... Odd. Why cannot we reproduce similar bug on desktop??

Anyway, bug 1465702 was clean-up of editor so that the changed points are too many. So, without minimized testcase which can reproduce this bug on desktop, it's really difficult to find the cause...

Flags: needinfo?(masayuki)

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

Comment 10

•

5 years ago

When I test this on Zenfone 5 (Android 8.0.0) with ATOK or GBoard, typing something after switching from Preview to Write, all text in <textarea> is removed.

Ccing Makoto-san, he might have some ideas...

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

Comment 11

•

5 years ago

The "Write" and "Preview" button handlers are here:

function(t) {
  var i = e(this);
  if (!n.allowAction(i))
    return n.stopEverything(t);
  var o = i.attr("name"),
    r = o ? {
      name: o,
      value: i.val()
    } : null,
    a = i.closest("form");
  0 === a.length && (a = e("#" + i.attr("form"))),
                     a.data("ujs:submit-button", r),
                     a.data("ujs:formnovalidate-button",
                     i.attr("formnovalidate")),
                     a.data("ujs:submit-button-formaction",
                     i.attr("formaction")),
                    a.data("ujs:submit-button-formmethod", i.attr("formmethod"))
}

and either one of the followings:

a() (document).on('markdown-preview:show', function (e, t) {
  t && (i = t.find('textarea.markdown-area'),
        d = i.height(),
        t.find('.js-md-write-button').parent().removeClass('active'),
        t.find('.js-md-preview-button').parent().addClass('active'),
        t.find('.md-write-holder').hide(),
        t.find('.md-preview-holder').show(),
        f.removeClass('active'),
        o.showPreview(t))
}),
a() (document).on('markdown-preview:hide', function (e, t) {
  t && (i = null, d && t.find('textarea.markdown-area').height(d),
        t.find('.js-md-write-button').parent().addClass('active'),
        t.find('.js-md-preview-button').parent().removeClass('active'),
        t.find('.md-write-holder').show(),
        t.find('textarea.markdown-area').focus(),
        t.find('.md-preview-holder').hide(),
        f.addClass('active'),
        o.hideReferencedCommands(t))
}),

Makoto Kato [:m_kato]

Assignee

Updated

•

5 years ago

Component: Text Selection → Keyboards and IME

Whiteboard: [geckoview]

Makoto Kato [:m_kato]

Assignee

Comment 12

•

5 years ago

I guess that DoReplaceText cannot handle this situation well.

01-15 16:56:16.995 24949 24965 D GeckoViewModule: dispatch GeckoView:ZoomToInput, data=null
01-15 16:56:16.996 24949 24965 D GeckoViewContent: onEvent: event=GeckoView:ZoomToInput, data=null
01-15 16:56:17.021 24949 24965 D GeckoViewModule: dispatch GeckoView:ZoomToInput, data=null
01-15 16:56:17.022 24949 24965 D GeckoViewContent: onEvent: event=GeckoView:ZoomToInput, data=null
01-15 16:56:17.027 24949 24965 D GeckoViewContent[C]: receiveMessage: GeckoView: ZoomToInput
01-15 16:56:17.029 24949 24965 D GeckoViewContent[C]: receiveMessage: GeckoView: ZoomToInput
01-15 16:56:17.117 24949 24965 I Gecko : nsWindow[0xc42b5380]::Resize [0.000000 48.000000 720.000000 526.000000] (repaint 0)
01-15 16:56:17.117 24949 24965 I Gecko : nsWindow: 0xc42b5380 OnSizeChanged [720 526]
01-15 16:56:17.134 24949 24949 D GeckoViewActivity: onFirstComposite
01-15 16:56:27.329 24949 25150 D GeckoInputConnection: > beginBatchEdit()
01-15 16:56:27.329 24949 25150 D GeckoInputConnection: < beginBatchEdit: true
01-15 16:56:27.331 24949 25150 D GeckoInputConnection: > setComposingText("G", 1)
01-15 16:56:27.332 24949 25150 D GeckoEditable: getSpanStart(android.view.inputmethod.ComposingText@832f942) = -1
01-15 16:56:27.332 24949 25150 D GeckoEditable: getSpanEnd(android.view.inputmethod.ComposingText@832f942) = -1
01-15 16:56:27.332 24949 25150 D GeckoEditable: getSpanStart(android.text.Selection$START@d2d1cd) = 20
01-15 16:56:27.332 24949 25150 D GeckoEditable: getSpanStart(android.text.Selection$END@73dfa82) = 20
01-15 16:56:27.333 24949 25150 D GeckoEditable: length() = 20
01-15 16:56:27.333 24949 25150 D GeckoEditable: getSpanStart(android.text.Selection$START@d2d1cd) = 20
01-15 16:56:27.333 24949 25150 D GeckoEditable: getSpanStart(android.text.Selection$END@73dfa82) = 20
01-15 16:56:27.334 24949 25150 D GeckoEditable: offer: Action(TYPE_REPLACE_TEXT)
01-15 16:56:27.334 24949 25150 D GeckoEditable: icSendComposition("G", 0, 1)
01-15 16:56:27.334 24949 25150 D GeckoEditable: range = 0-1
01-15 16:56:27.334 24949 25150 D GeckoEditable: selection = 1-1
01-15 16:56:27.335 24949 25150 D GeckoEditable: found 1 spans @ 0-1
01-15 16:56:27.335 24949 25150 D GeckoEditable: added 4 : 1 : 0 : 0
01-15 16:56:27.335 24949 24965 I GeckoEditableSupport: IME: IME_REPLACE_TEXT: text="G"
01-15 16:56:27.335 24949 25150 D GeckoEditable: replace(20, 20, "G") = GeckoEditable
01-15 16:56:27.335 24949 25150 D GeckoInputConnection: < setComposingText: true
01-15 16:56:27.336 24949 25150 D GeckoInputConnection: > endBatchEdit()
01-15 16:56:27.336 24949 25150 D GeckoInputConnection: < endBatchEdit: true
01-15 16:56:27.357 24949 24965 I GeckoEditableSupport: IME: NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED
01-15 16:56:27.455 24949 24965 I Gecko : [Parent 24949, Main Thread] WARNING: '!CanHandleEditAction()', file /home/makoto/mozilla-central/editor/libeditor/TextEditRules.cpp, line 287
01-15 16:56:27.455 24949 24965 I Gecko : [Parent 24949, Main Thread] WARNING: Failed to get value: 'NS_SUCCEEDED(rv)', file /home/makoto/mozilla-central/dom/html/nsTextEditorState.cpp, line 2106
01-15 16:56:27.458 24949 24965 I Gecko : [Parent 24949, Main Thread] WARNING: '!aSelection.RangeCount()', file /home/makoto/mozilla-central/editor/libeditor/EditorBase.cpp, line 3556
01-15 16:56:27.459 24949 24965 I Gecko : [Parent 24949, Main Thread] WARNING: '!selectionStartPoint.IsSet()', file /home/makoto/mozilla-central/editor/libeditor/TextEditRules.cpp, line 529
01-15 16:56:27.459 24949 24965 I Gecko : [Parent 24949, Main Thread] WARNING: Failed to selection to after the text node in TextEditor: 'NS_SUCCEEDED(rv)', file /home/makoto/mozilla-central/editor/libeditor/TextEditRules.cpp, line 280
01-15 16:56:27.459 24949 24965 I Gecko : [Parent 24949, Main Thread] WARNING: '!firstRange', file /home/makoto/mozilla-central/editor/libeditor/TextEditRules.cpp, line 809
01-15 16:56:27.459 24949 24965 I Gecko : [Parent 24949, Main Thread] WARNING: 'NS_FAILED(rv)', file /home/makoto/mozilla-central/editor/libeditor/TextEditor.cpp, line 1023
01-15 16:56:27.459 24949 24965 I Gecko : [Parent 24949, Main Thread] WARNING: 'NS_FAILED(rv)', file /home/makoto/mozilla-central/editor/libeditor/TextEditor.cpp, line 1225
01-15 16:56:27.459 24949 24965 I Gecko : [Parent 24949, Main Thread] WARNING: Failed to replace selection with new string: 'NS_SUCCEEDED(rv)', file /home/makoto/mozilla-central/editor/libeditor/TextEditor.cpp, line 1204
01-15 16:56:27.459 24949 24965 I Gecko : [Parent 24949, Main Thread] WARNING: '!aSelection.RangeCount()', file /home/makoto/mozilla-central/editor/libeditor/EditorBase.cpp, line 3556
01-15 16:56:27.459 24949 24965 I Gecko : [Parent 24949, Main Thread] WARNING: '!selectionStartPoint.IsSet()', file /home/makoto/mozilla-central/editor/libeditor/TextEditRules.cpp, line 529
01-15 16:56:27.459 24949 24965 I Gecko : [Parent 24949, Main Thread] WARNING: Failed to selection to after the text node in TextEditor: 'NS_SUCCEEDED(rv)', file /home/makoto/mozilla-central/editor/libeditor/TextEditRules.cpp, line 280
01-15 16:56:27.459 24949 24965 I Gecko : [Parent 24949, Main Thread] WARNING: '!aDocument', file /home/makoto/mozilla-central/editor/libeditor/EditorBase.cpp, line 1947
01-15 16:56:27.459 24949 24965 I Gecko : [Parent 24949, Main Thread] WARNING: NS_ENSURE_TRUE(mShell) failed: file /home/makoto/mozilla-central/layout/generic/nsFrameSelection.cpp, line 1208
01-15 16:56:27.459 24949 24965 I Gecko : [Parent 24949, Main Thread] WARNING: '!firstRange', file /home/makoto/mozilla-central/editor/libeditor/EditorBase.cpp, line 4342
01-15 16:56:27.503 24949 24965 D GeckoEditable: reply: Action(TYPE_REPLACE_TEXT)
01-15 16:56:27.503 24949 25150 D GeckoInputConnection: > onTextChange()
01-15 16:56:27.503 24949 25150 D GeckoInputConnection: < onTextChange

Makoto Kato [:m_kato]

Assignee

Comment 13

•

5 years ago

nsTextEditorState seems to use destroyed editor. So InsertTextAsAction will be failure. nsTextEditorState should reinitialize editor or create editor.

Assignee: nobody → m_kato

Makoto Kato [:m_kato]

Assignee

Comment 14

•

5 years ago

Why is nsTextEditorState::UnbindFrame called on Android only?

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

Comment 15

•

5 years ago

Even if I use PC-site mode, I can reproduce it. I don't know how to do it, but I sometimes see such web apps (i.e., even in PC mode, only Firefox for Android is broken).

Makoto Kato [:m_kato]

Assignee

Comment 16

•

5 years ago

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(sick) from comment #15)

Even if I use PC-site mode, I can reproduce it. I don't know how to do it, but I sometimes see such web apps (i.e., even in PC mode, only Firefox for Android is broken).

input event handler of <textarea> element calls element.scrollTop(). When it causes reflow, editor is destroyed by UnbindFromFrame.

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

Comment 17

•

5 years ago

Do you mean that InsertTextAsAction() is called by an input event listener while the editor is temporarily destroyed?

Makoto Kato [:m_kato]

Assignee

Comment 18

•

5 years ago

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(sick) from comment #17)

Do you mean that InsertTextAsAction() is called by an input event listener while the editor is temporarily destroyed?

No, editor is destroyed and initialized again by scrollTop due to reflow. InsertTextAsAction will be called since nsTextEditorState has to set current value via SetText action. So since SetText is failed, editor text is nothing.

Why is SetText failed? Because this reflow occurs during edit action. So selection is cached by bug 1465702, but TextEditor::Init has new selection controller. So selection is invalid for newer root node. So I think we have to update selection cache in Init.

(But I cannot create test case... hmm)

Makoto Kato [:m_kato]

Assignee

Comment 19

•

5 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af69f9de460161a4c3ab1028475223083ab35c83

Makoto Kato [:m_kato]

Assignee

Comment 20

•

5 years ago

Attached file Bug 1518002 - Update selection cache when initializing editor. r=masayuki — Details

GitLab's comment calls scrollTop on input event handler. The scollTop may cause
reflow. When causing reflow, editor is destroyed and initialized again. Then
nsTextEditorState will set current value to editor. But this is failure.

By bug 1465702, selection is cached in edit action. When initializing editor,
selection controller is updated, so selection into cache becomes invalid. It
means that all selection methods will return error since document is different.

So we should update selection cache when initializing editor.

Also, I cannot create test case for this situation since we have to cause reflow
in input and/or composition event. Do you have any idea?

Pulsebot

Comment 21

•

5 years ago

Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/16ad98f608fe
Update selection cache when initializing editor. r=masayuki

Ryan VanderMeulen [:RyanVM]

Updated

•

5 years ago

status-firefox-esr60: --- → unaffected

tracking-firefox65: --- → +

tracking-firefox66: --- → +

Oana Pop-Rus

Comment 22

•

5 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/16ad98f608fe

Status: NEW → RESOLVED

Closed: 5 years ago

status-firefox66: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 66

Makoto Kato [:m_kato]

Assignee

Comment 23

•

5 years ago

Comment on attachment 9037477 [details]
Bug 1518002 - Update selection cache when initializing editor. r=masayuki

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1465702

User impact if declined: When writing a comment in gitlab.com using Firefox/Android, your cannot input a character in caret position after viewing preview.

we can reproduce this easy on Android, but if content script causes reflow in input or composition event, this occurs even if desktop version.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: 1. Goto website like https://gitlab.com/bytesnz/ls-ignore/issues/new
2. Enter multiple lines of text into textare
3. Click the Preview button above the textarea
4. Click the Write button above the textarea to go back to the editor and textarea
5. Focus somewhere in the textarea and start typing.

Result
Cursor starts jumping after a small pause

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Since Editor uses invalid selection (window.selection) object into editor's cache, we use valid selection instead.

String changes made/needed: no

Attachment #9037477 - Flags: approval-mozilla-beta?

Ryan VanderMeulen [:RyanVM]

Updated

•

5 years ago

Flags: qe-verify+

Ryan VanderMeulen [:RyanVM]

Comment 24

•

5 years ago

Comment on attachment 9037477 [details]
Bug 1518002 - Update selection cache when initializing editor. r=masayuki

[Triage Comment]
Fixes a painful-sounding regression causing character input to be broken on some sites, especially on Android. Approved for 65.0b13/65.0rc1.

Attachment #9037477 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Ryan VanderMeulen [:RyanVM]

Comment 25

•

5 years ago

bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/ff8f2153265d

status-firefox65: affected → fixed

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

Comment 26

•

5 years ago

Off topic though, according to the investigation of Mokoto-san, we are recreating all of nsFrameSelection instance, Selection instances and nsRange instances every reflow which causes reframing an editor. This must be really bad for the reflow performance because of the slowness of allocation and creating nsRange instances. So, perhaps, when editor becomes not necessary to keep those objects, nsTextEditorState should keep them for a while. Then, it should release them when editor won't be recreated. Otherwise, they should be used in the new editor. Note that those objects are hidden from web apps. So, we can reuse them safely.

miralobontiu

Comment 27

•

5 years ago

Verified as fixed on the latest Nightly 66.0a1 (2019-01-21) and on Beta 65.0b13.
Tested with HTC 10 (Android 8.0) and Samsung Galaxy S7 Edge (Android 8.0).

Status: RESOLVED → VERIFIED

status-firefox65: fixed → verified

status-firefox66: fixed → verified

miralobontiu

Updated

•

5 years ago

Flags: qe-verify+

BMO Automation

Updated

•

3 years ago

Product: Firefox for Android → Firefox for Android Graveyard

Tyson Smith [:tsmith]

Updated

•

2 years ago

Regressions: 1753508

logcat 5 years ago miralobontiu 8.52 MB, text/plain		Details
Bug 1518002 - Update selection cache when initializing editor. r=masayuki 5 years ago Makoto Kato [:m_kato] 47 bytes, text/x-phabricator-request	RyanVM : approval-mozilla-beta+	Details \| Review