Closed Bug 972210 Opened 6 years ago Closed 5 years ago

[Pinyin][Zhuyin] Tapping "X" button on the text field to clear it does not clean out candidates, and tapping these candidates has no effect

Categories

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(b2g-v2.1 verified)

VERIFIED FIXED
2.1 S3 (29aug)
Tracking Status
b2g-v2.1 --- verified

People

(Reporter: osk4095, Assigned: timdream)

References

Details

(Whiteboard: [p=1])

Attachments

(6 files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131206152142

Steps to reproduce:

It has been implanted in below field/ screen and works as below.
- Searching field on the top of the Home screen
- Create New Contact/ Edit Contact screen

  1. Input characters in a text field
   (Undefined character by "inputcontext.setComposition")
  2. Tap CLEAR Button
  3. Input characters in a text field but they are not displayed


Actual results:

At step No.2 above, any event such as “inputcontextchange” is not sent and strings hold by IME are not cleared.
At step No.3 above, input strings are not displayed in the text field.

We have confirmed same behavior with built-in Pinyin keyboard.


Expected results:

We believe that an event should be sent at Step No.2.
Additionally, if you define characters at Step No.1, "inputcontextchange" is sent at Step No.2.
Attached image 2014-02-10-07-03-01.png
Attached image 2014-02-10-07-03-02.png
blocking-b2g: --- → 1.3?
OS: All → Gonk (Firefox OS)
Hardware: All → ARM
Can you include a video here? That will help clarify the bug a bit better - I'm trying to get a clear picture of severity here.
Flags: needinfo?(osk4095)
Leaving qawanted also to answer comment 3
Keywords: qawanted
Duplicate of this bug: 972197
No response to the needinfo request, so I'm clearing the blocking flag.
blocking-b2g: 1.3? → ---
Flags: needinfo?(osk4095)
Keywords: qawanted
jsmith:

Looks like a video was attached which you wanted.

# And I think that waiting only 3 days for a needinfo is too short...

Although, I don't familiar with Gaia's IME request handling, looks like CompositionManager.onCompositionEnd() does nothing. Anyway, "inputcontextchange" sounds odd to me...
Flags: needinfo?(jsmith)
Someone from Tim's team could probably give input here.
Component: Gaia::Contacts → Gaia::Keyboard
Flags: needinfo?(jsmith) → needinfo?(timdream)
This is a valid bug. masayuki, thanks for bring this our attention (and sorry for the delayed reply -- I was OOO yesterday). 

This bug shall remain in the Keyboard component because the following STRs behaves as expected:

STR1:
1. Go to Contacts -> New Contacts
2. Type something with English layout with word suggestion on.
3. Tap the clear button
Expected & Actual: 
A. The text field are cleared.
B. The word suggestions are cleared out.

STR2:
1. Go to Contacts -> New Contacts
2. Type something with Pinyin (e.g. t a i b e i)
3. Move the caret

Expected & Actual: 
A. Composition is forced cancelled and the composing alphabets remains on the text field.
B. The candidates are cleared out.

With this two STRs behaves correctly I can fairly certain this is simply an issue with the Pinyin IME. It has to respond to the clear action but it didn't.

Note that Zhuyin has the same bug here.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(timdream)
Summary: Regarding "CLEAR Button" used in text field. → [Pinyin][Zhuyin] Tapping "X" button on the text field to clear it does not clean out candidates, and tapping these candidates has no effect
I am putting this on my queue without figuring out when I can work on it -- feel free to steal and investigate.
Assignee: nobody → timdream
Perhaps, I find the cause.

The "X" button is in shadow DOM and it's not focusable because at mousedown event handler, the event is always prevented default. And its click event handler sets the value attribute of the input event as empty string.

Then, input event is NOT fired on the input element because it's explicit case for the setter. This behavior is same as IE and Google Chrome. So, we cannot change the editor behavior for compatibility with other browsers. But this causes forms.js never catching any events for setting value of input element. So, I think that nsEditor should notify observers of changing its content by setting value attribute or firing custom event.

I'll try to check this next week if Tim doesn't have much time for this now.
Well, that doesn't explain why Latin IME will be able to clean up suggestions when [X] is hit, that's why I classified this as a keyboard app bug, not an API one.

I can try to see if Latin IME clean up the suggestions because of the selectionchange event from the API, if so I will try to see if it's possible to fix it during the weekend. I am OOO next week.
OK, after console.log() and console.trace() inside Keyboard app, I realized this is indeed an API bug.

The first STR, for Latin IME triggers "inputcontextchange" from InputMethod API, and cause the keyboard layout to reload, along with the IMEngine. I don't understand why tapping the [X] counts as a "inputcontextchange" but that works anyway.

For the second STR, InputMethod API triggers nothing. The only different I can see it that when the [X] is tapped is the compositing.

Xulei, do you know the answer to the two questions above?
Component: Gaia::Keyboard → DOM
Flags: needinfo?(xyuan)
Product: Firefox OS → Core
Hum, I think I just rediscovered what comment 0 said ... sorry about that, I should read bugs more thoroughly.
I wonder if web applications set InputElement.value from compositionupdate or input event handler, does it work well with IME on Firefox OS?

ns*Editor notifies IME of requesting commit or cancel via nsIWidget::NotifyIME(). But Gonk widget always ignore it. I think that this is the root cause of this and similar bugs. See bug 917323.
Oh... forms.js implements nsIEditorObserver. Then, setting value attribute causes a call of EditAction() method.
http://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#319

Could be the _editing true in this case??
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #14)
> OK, after console.log() and console.trace() inside Keyboard app, I realized
> this is indeed an API bug.
> 
> The first STR, for Latin IME triggers "inputcontextchange" from InputMethod
> API, and cause the keyboard layout to reload, along with the IMEngine. I
> don't understand why tapping the [X] counts as a "inputcontextchange" but
> that works anyway.
This is legacy code from mozKeyboard. In this case, "selectionchange" and 
"surroundingtextchange" should be fired. see bug 860546 for details.
> 
> For the second STR, InputMethod API triggers nothing. The only different I
> can see it that when the [X] is tapped is the compositing.
It is a bug, whose cause is mentioned in Comment 8 by masayuki. We have a fix for it (Bug 1026997), but not landed yet.
Flags: needinfo?(xyuan)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #17)
> Oh... forms.js implements nsIEditorObserver. Then, setting value attribute
> causes a call of EditAction() method.
> http://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#319
> 
> Could be the _editing true in this case??

It should not. _editing == true means the text change is triggered by IME API calling 
from the keyboard app.
(In reply to Yuan Xulei [:yxl] from comment #18)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #14)
> > For the second STR, InputMethod API triggers nothing. The only different I
> > can see it that when the [X] is tapped is the compositing.
> It is a bug, whose cause is mentioned in Comment 8 by masayuki. We have a
> fix for it (Bug 1026997), but not landed yet.

Oh, sounds like it's a cause of this bug. If selection change event is fired on IME, it's okay for the vendors.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #20)
> Oh, sounds like it's a cause of this bug. If selection change event is fired
> on IME, it's okay for the vendors.

Does this mean the resolution to this bug is

1. Wait for bug 1026997 to land
2. Have the Pinyin/Zhuyin IME respond to selectionchange event in this bug.

If so we should mark the dependency for (1) and I can work on (2).
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #21)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #20)
> > Oh, sounds like it's a cause of this bug. If selection change event is fired
> > on IME, it's okay for the vendors.
> 
> Does this mean the resolution to this bug is
> 
> 1. Wait for bug 1026997 to land
> 2. Have the Pinyin/Zhuyin IME respond to selectionchange event in this bug.
> 
> If so we should mark the dependency for (1) and I can work on (2).

At least for Japanese IME, (1) is enough solution. The vendor confirmed that the patch helps their product.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #22)
> At least for Japanese IME, (1) is enough solution. The vendor confirmed that
> the patch helps their product.

Good. But we still need to fix the open source'd Chinese IMEs in our Gaia source tree :)
Depends on: 1026997
Component: DOM → Gaia::Keyboard
Product: Core → Firefox OS
WIP https://github.com/timdream/gaia/tree/keyboard-inputcontext-events

If bug 1026997 is blocked for too long I might land the first commit in another bug first.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #24)
> WIP https://github.com/timdream/gaia/tree/keyboard-inputcontext-events
> 
> If bug 1026997 is blocked for too long I might land the first commit in
> another bug first.

We've not get reply from janjongboom in the bug yet :-(

He was set needinfo himself, therefore, I cannot use it for pinging him.
Rudy wrote a patch in bug 1026997. Just waiting before try opens again.
Let me see if I could finish the patch today.
Status: NEW → ASSIGNED
Whiteboard: [p=1]
Target Milestone: --- → 2.1 S3 (29aug)
Attachment #8474362 - Flags: review?(rlu)
Attachment #8474362 - Flags: feedback?(lchang)
Attachment #8474362 - Flags: feedback?(janjongboom)
Attachment #8474362 - Flags: feedback?(dflanagan)
Comment on attachment 8474362 [details] [review]
mozilla-b2g:master PR#22970

Sorry that I have to clear the review flag first,
 1. I found an issue when you type really fast with pinyin input method, that the word candidate and the composed text would get cleared.
    I think this is because we did several actions that would cause selectionchange and once we receive a selectionchange event for one of those actions to set the flag back to false, and then we got another "expected" selectionchange, this issue would occur.

 2. Should we update the latin_test.js as well?

Thanks.
Attachment #8474362 - Flags: review?(rlu)
(In reply to Rudy Lu [:rudyl] from comment #29)
>  1. I found an issue when you type really fast with pinyin input method,
> that the word candidate and the composed text would get cleared.
>     I think this is because we did several actions that would cause
> selectionchange and once we receive a selectionchange event for one of those
> actions to set the flag back to false, and then we got another "expected"
> selectionchange, this issue would occur.

Nice catch!

>  2. Should we update the latin_test.js as well?

OK.
Comment on attachment 8474362 [details] [review]
mozilla-b2g:master PR#22970

Test fixed and comment addressed.
Attachment #8474362 - Flags: review?(rlu)
Comment on attachment 8474362 [details] [review]
mozilla-b2g:master PR#22970

I only took a quick look at the latin IME changes, but they seem good to me.
Attachment #8474362 - Flags: feedback?(dflanagan) → feedback+
Comment on attachment 8474362 [details] [review]
mozilla-b2g:master PR#22970

I've simply tested the pinyin and zhuyin IME and they work fine. Thanks.
Attachment #8474362 - Flags: feedback?(lchang) → feedback+
Comment on attachment 8474362 [details] [review]
mozilla-b2g:master PR#22970

For jspinyin and jszhuyin, I am afraid a single boolean variable might be enough to handle this case.
With this patch, sometimes I would see this issue would not be resolved, i.e. would not clear the word suggestion when pressing the [x] icon.

Do you think we could use a counter like this?
https://github.com/mozilla-b2g/gaia/blob/494dc6f6aa589b2dd676f93a63220187b58abeb4/apps/keyboard/js/imes/latin/latin.js#L366

Sorry that did not make this clear in the previous comment.
Comment on attachment 8474362 [details] [review]
mozilla-b2g:master PR#22970

I'll clear the review flag, so that I'll get notified when flagged again.
Attachment #8474362 - Flags: review?(rlu)
OK. The more I see it the more I think we can never get this right in IMEngine or even in Keyboard app along. Say if the user taps backspace on the empty input, the promise will resolve but it will not trigger selectionchange, my boolean flag and your counter will always fail in this case, and there is no way of telling that.

I recommend we instead fix this in MozKeyboard.js by setting a flag in the detail object of the event, denote that the selectionchange/surroundingtextchange was due to our own action. If you agree I would probably use bug 1054839 to patch the API (if that bug is still valid.) 

As Jan is the last person who touches this bit (bug 1026997), I would like to know if you have better idea.
Flags: needinfo?(rlu)
Flags: needinfo?(janjongboom)
Yeah, this sounds like a more elegant way to resolve this kind of issues.
So, I'm find with this.

Thank you very much.
Flags: needinfo?(rlu)
Comment on attachment 8474362 [details] [review]
mozilla-b2g:master PR#22970

Let's revisit the new patch when bug 1054839 lands.
Attachment #8474362 - Flags: feedback?(janjongboom)
Attachment #8474362 - Flags: feedback+
Comment on attachment 8474362 [details] [review]
mozilla-b2g:master PR#22970

The patch has been updated with what would work with bug 1054839.
Attachment #8474362 - Flags: review?(rlu)
Attachment #8474362 - Flags: feedback?(janjongboom)
Attachment #8474362 - Flags: feedback?(dflanagan)
Flags: needinfo?(janjongboom)
Jan, David, I should have needinfo both of you on bug 1054839 too but I decided against spamming you with flags. It might be worthwhile checking both patches. Thanks!
Comment on attachment 8474362 [details] [review]
mozilla-b2g:master PR#22970

f+ after discussion on IRC
Attachment #8474362 - Flags: feedback?(janjongboom) → feedback+
Comment on attachment 8474362 [details] [review]
mozilla-b2g:master PR#22970

Looks good to me as well.
Thanks for this patch.
Attachment #8474362 - Flags: review?(rlu) → review+
master: https://github.com/mozilla-b2g/gaia/commit/be07b9666b48ecc56c185e709e46fd9575e98ec1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
This issue has been failed verified on Flame 2.1.
[Issue Steps]:
1.Searching field on the top of the Home screen.
2.Create New Contact/ Edit Contact screen.
3.Input characters in a text field.
4.Tap X Button.
5.Input characters in a text field.

[Actual results]:
At step 2, strings hold by IME are not deleted.

See attachment: v2.1_1402.MP4 and logcat_1402.txt
Reproducing rate: 3/8

Flame 2.1 build:
Gaia-Rev        db2e84860f5a7cc334464618c6ea9e92ff82e9dd
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/211eae88f119
Build-ID        20141126001202
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141126.033519
FW-Date         Wed Nov 26 03:35:30 EST 2014
Bootloader      L1TC00011880
Flags: needinfo?(hlu)
(In reply to Shally from comment #44)
> [Actual results]:
> At step 2, strings hold by IME are not deleted.

Sorry to update:
[Actual results]:
At step 4, strings hold by IME are not deleted.
Hi Mike,
    Per talk, please help handle v2.1 bug verified by Marigold. Thank you.
Flags: needinfo?(hlu) → needinfo?(mlien)
Blocks: 1105678
Verified again with v2.1, candidates already be cleaned
But there might have a side effect as bug 1105678

Gaia-Rev        5372b675e018b6aac97d95ff5db8d4bd16addb9b
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f34377ae402b
Build-ID        20141126161202
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141126.193631
FW-Date         Wed Nov 26 19:36:42 EST 2014
Bootloader      L1TC10011880
Flags: needinfo?(mlien)
Status: RESOLVED → VERIFIED
Comment on attachment 8474362 [details] [review]
mozilla-b2g:master PR#22970

clearing old feedback request
Attachment #8474362 - Flags: feedback?(dflanagan)
You need to log in before you can comment on or make changes to this bug.