1376856 - Focus is lost in chatzilla after typing in the input box and hitting enter/return

Status: RESOLVED WORKSFORME

(Core :: DOM: Editor, defect, P3)

Description

[:Gijs (he/him)]

STR:

1. open chatzilla ( installed from https://addons.mozilla.org/en-US/firefox/addon/16 )
2. type /nick foopy
3. hit enter/return

ER:
cursor remains in the input box and blinks, focus remains in the input box

AR:
cursor disappears, typing doesn't do anything, but input box still has focus highlight (on OS X, at least)

mozregression shows this regressed with bug 1370806. This reproduces on OS X, Windows and Linux. Could you take a look? Not sure if this is XUL-specific or if this change in behaviour is web-observable (which would not be so good).

Comment 1

[:Gijs (he/him)]

Requesting tracking in case this affects the web. If it doesn't, it probably doesn't need to track, but my understanding is that XUL inputs are thin wrappers around html ones, so my default assumption is that the same issue is web-observable unless there's evidence otherwise.

Comment 2

[Makoto Kato [:m_kato]]

Caret seems to be after bonus node?

Comment 3

[:Gijs (he/him)]

(In reply to Makoto Kato [:m_kato] from comment #2)
> Caret seems to be after bonus node?

I'm not sure what this means and if it was a question for me - if so, please request needinfo. :-)

Comment 4

[Makoto Kato [:m_kato]]

(In reply to :Gijs from comment #3)
> (In reply to Makoto Kato [:m_kato] from comment #2)
> > Caret seems to be after bonus node?
> 
> I'm not sure what this means and if it was a question for me - if so, please
> request needinfo. :-)

Sorry for comment.  That is root cause of this issue.  This issue is kind of bug 1368544 and mozInlineSpellChecker::SpellCheckAfterEditorChange will return error...

Comment 5

[Makoto Kato [:m_kato]]

No longer occurs on 56 after landing bug 1375910.  For 55, I might have to back out bug 1370806 from 55 tree.

Comment 6

[Makoto Kato [:m_kato]]

Attachment: Bug 1376856 - Backed out bug 1370806 part 2 from Firefox 55.

When setting to empty on input.value, previous text node is removed.  Then, since we don't set selection to root node, SpellCheckAfterEditorChange might return error.  So this issue occur.

Although this issue has been fixed by bug 1375910, this is risky for beta.  So I think that we should back out bug 1370806 part 2 from beta.

Also, unit test will create and add to m-c after landing this...

Review commit: https://reviewboard.mozilla.org/r/154168/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154168/

Comment 7

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

Comment on attachment 8883241 [details]
Bug 1376856 - Backed out bug 1370806 part 2 from Firefox 55.

https://reviewboard.mozilla.org/r/154168/#review159324

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 8883241 [details]
Bug 1376856 - Backed out bug 1370806 part 2 from Firefox 55.

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1370806

[User impact if declined]:
When using chatzilla that is old extension, focus might be lost by [enter] key.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
please if possible.  comment #0 has reproduce step.

[List of other uplifts needed for the feature/fix]:
Nothing

[Is the change risky?]:
Low.  This is backed out only.

[Why is the change risky/not risky?]:
Back to old behavior.

[String changes made/needed]:
No.

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 8883241 [details]
Bug 1376856 - Backed out bug 1370806 part 2 from Firefox 55.

backout to fix a regression in beta55

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9856974e03e2

Comment 11

[(no longer active)]

I think this patch can actually land in m-c too, since this code right now is just doing unnecessary work...  Selection::Collapse() already calls Clear() which does RemoveAllRanges().

Comment 12

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

Comment 13

[Makoto Kato [:m_kato]]

This is workitem for test since we have already fixed on all channel.  Down to P3.

Comment 14

[Makoto Kato [:m_kato]]

Original issue is fixed, so mark as WFM