Closed Bug 635857 Opened 9 years ago Closed 9 years ago

nsWindow::ResetInputState() commits duplicated composition string.

Categories

(Core :: Widget: Android, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0
Tracking Status
fennec 2.0+ ---

People

(Reporter: m_kato, Assigned: m_kato)

References

Details

(Keywords: inputmethod, regression)

Attachments

(2 files)

- Step
1. focus Awesome/Address bar 
2. input "あ" (Keep composition state.  Don't commit it) by software keyboard
3. focus address bar again

- Result
In address bar, text becomes "ああ" (last character is selected state).

- Expected result
text is "あ"
I think that this is a regression by bug 588456.
Keywords: regression
bug 588456 isn't fixed yet.  Although NS_TEXT_TEXT/NS_COMPOSITION_END event are synced, restartInput doesn't often destroy composition string.  Because showSoftInput is immediately called.

Also bug 620695's fix is bad.  Root case of bug 620695 is that ResetInputState is often failure.  If ResetInputState is successful, composition string is commited surely.  So text event and composition end event don't work well.  Bug 620695's fix sends NS_TEXT_TEXT/NS_COMPOSITION_END twice.  As event state manager, this behavior doesn't consider!  I cannot understand why this fix is acceptable.

So I have to add fix that ResetInputState is surely successful.
Depends on: 620695, 588456
Assignee: nobody → m_kato
Attached patch fix v1Splinter Review
Duplicate of this bug: 636659
assuming I marked this dupe correctly, this bug blocks fennec
tracking-fennec: --- → 2.0+
This patch fixes bug 636659. Can we get a reviewer for the patch?
(In reply to comment #6)
> This patch fixes bug 636659. Can we get a reviewer for the patch?

Now I am testing using some IMEs for this.  After finished it, I will set review flag.
Duplicate of this bug: 636659
Attachment #515470 - Flags: review?(mwu)
Comment on attachment 515470 [details] [diff] [review]
fix v1

>+            // Don't use IMEStateUpdater for reset.
>+            // Because IME may not work showSoftInput()
>+            // after calling restartInput() immediately.
>+            // So we have to call showSoftInput() delay.

Hmm. So, the IMEStateUpdater is used to call restartInput() and showSoftInput()/hideSoftInputFromWindow() because gecko can focus/blur/focus very quickly and confuse the soft keyboard. Originally, (bug 573800) we *only* delayed/coalesced the showSoftInput/hideSoftInputFromWindow calls. Your patch suggests that we shouldn't delay restartInput(). jchen said that we want delayed resets in https://bugzilla.mozilla.org/show_bug.cgi?id=576065#c20 . I don't know which one is right.

Patch looks good. Requesting feedback from jchen in case he has any thoughts about delaying resetInput() calls.
Attachment #515470 - Flags: review?(mwu)
Attachment #515470 - Flags: review+
Attachment #515470 - Flags: feedback?(jimnchen+bmo)
Whiteboard: [needs-review (jchen)]
I don't have any problem with calling resetInput immediately, but I think somebody from QA should test this with the IMEs they have. I'm mostly worried about breaking some other IME. And now I see calling finishComposingText wasn't very smart :)  Don't know what I was thinking...
(In reply to comment #10)
> I don't have any problem with calling resetInput immediately, but I think
> somebody from QA should test this with the IMEs they have. I'm mostly worried
> about breaking some other IME. And now I see calling finishComposingText wasn't
> very smart :)  Don't know what I was thinking...

At least, current nightly using iWnn or ATOK doesn't work well.  After fixing by my path, it works fine (included Simeji).

Jim, what IME do you test?
(In reply to comment #11)
> 
> At least, current nightly using iWnn or ATOK doesn't work well.  After fixing
> by my path, it works fine (included Simeji).
> 

That's good.

> Jim, what IME do you test?

This is the list that I used: Swype, SwiftKey, HTC Touch Input, Motorola Multi-Touch, Google Pinyin, OpenWnn plus, Simeji. So most of them are English ones.

QA probably has a larger list than I have.
Jim, do you need something from QA here?
(In reply to comment #12)
> (In reply to comment #11)
> > 
> > At least, current nightly using iWnn or ATOK doesn't work well.  After fixing
> > by my path, it works fine (included Simeji).
> > 
> 
> That's good.
> 
> > Jim, what IME do you test?
> 
> This is the list that I used: Swype, SwiftKey, HTC Touch Input, Motorola
> Multi-Touch, Google Pinyin, OpenWnn plus, Simeji. So most of them are English
> ones.

Additional.  I verified this using SwiftKey and Google Pinyin, modified OpenWnn (OpwnWnn+ and OpenWnn Flip input) and Google Korean Input.

About others, I cannot get.
(In reply to comment #12)
> (In reply to comment #11)
> > 
> > At least, current nightly using iWnn or ATOK doesn't work well.  After fixing
> > by my path, it works fine (included Simeji).
> > 
> 
> That's good.
> 
> > Jim, what IME do you test?
> 
> This is the list that I used: Swype, SwiftKey, HTC Touch Input, Motorola
> Multi-Touch, Google Pinyin, OpenWnn plus, Simeji. So most of them are English
> ones.
> 
> QA probably has a larger list than I have.

That's a pretty good list, i think i'd add Google Korean IME.   

Naoki, when this fix lands, can you please verify the fix?   thanks.   We should have litmus tests added as well.
Flags: in-litmus?(nhirata.bugzilla)
Attachment #515470 - Flags: feedback?(jimnchen+bmo)
Keywords: checkin-needed
Whiteboard: [needs-review (jchen)]
landed
http://hg.mozilla.org/mozilla-central/rev/51bdf0bca017
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0
I will reopen this bug because I can still reproduce it using the following
steps: 

1. Go to www.cnn.com
2. Create a bookmark by clicking the white star from the right side panel.
3. After the page is bookmarked, click the yellow star and then Edit.
4. In the resulting dialog edit the Name to be something other than the
existing value (e.g.: "news")
5. Tap in the Tag field.

Actual result:
After step 5, the string typed in the Name field is also entered in the Tag
field.

Expected result:
After step 5, no string should appear in the tag field.

Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre) Gecko/20110307
Firefox/4.0b13pre Fennec /4.0b6pre

Device: Motorola Droid 2 (Android 2.2)

I took these steps to reproduce from the bug 627014, which was a dupe and it is
still reproducing.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #17)
> I will reopen this bug because I can still reproduce it using the following
> steps: 
> 
> 1. Go to www.cnn.com
> 2. Create a bookmark by clicking the white star from the right side panel.
> 3. After the page is bookmarked, click the yellow star and then Edit.
> 4. In the resulting dialog edit the Name to be something other than the
> existing value (e.g.: "news")
> 5. Tap in the Tag field.
> 
> Actual result:
> After step 5, the string typed in the Name field is also entered in the Tag
> field.
> 
> Expected result:
> After step 5, no string should appear in the tag field.
> 
> Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre) Gecko/20110307
> Firefox/4.0b13pre Fennec /4.0b6pre
> 
> Device: Motorola Droid 2 (Android 2.2)

I cannot reproduce with latest my build on My Desire HD and SHARP 003SH.  

What built from URL (hg.mozilla.org/....) of about:buildconfig?
As long as tinderbox, nightly build isn't available yet.  Where do you get android build from?
(In reply to comment #20)
> Built from http://hg.mozilla.org/mozilla-central/rev/1240004b90e6.

This build doesn't have this fix.  Please next nightly (20110308)...
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Attached image Commit Screenshot
This bug no longer occurs in nightly : 20110308 on android, droid 2.

Note:
With simeji, a context menu will appear underneath the soft keyboard in landscape mode.  I am not sure if this is the appropriate behavior or not.
(see screenshot)
Marking this as verified and branching off to bug 639958
Status: RESOLVED → VERIFIED
litmus test case : 50469 created
Flags: in-litmus?(nhirata.bugzilla) → in-litmus+
You need to log in before you can comment on or make changes to this bug.