946068 - The underline of the composition string disappeared and the composition APIs don't clear previous composition strings

Status: RESOLVED FIXED

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

Description

[Luke Chang [:lchang] (inactive)]

When I call "setComposition('A')", I will see a character 'A' in the input field but there is no underline.

Then, if I call "setComposition('BC')", I will see 'ABC' in the input field but I expect that the previous composition string ('A' in this case) will be cleared and it shows 'BC' instead.

You can try this issue by using pinyin or zhuyin IME.

The regression window of gecko.git:
last good build: (12/01) 0e3362fb5625eb6d98c7617b1b3019a2cc553d47
first bad build: (12/02) 668fbfecaf890207a96a19636ba8bca6d60c1823

Comment 1

[Luke Chang [:lchang] (inactive)]

Sorry, please ignore the regression window I mentioned above.

Following is my testing environment:

gaia: ee1bfd4a260e732264f8486cbf62197760de984a
gecko: c5fbc5c2efc86f33591a23b7e54c6ff4f1a472e0

The last good build in my test is 017ed20347cae2cd892d7ef144e7b36cd44f0185 (11/21).

Comment 2

[Luke Chang [:lchang] (inactive)]

I know why I can't narrow down the regression window.

When testing on builds after 11/19, it works for first time I input but will fail if I click the "backspace" to clear the composition strings and input again.

So I think the last good build is b82ef1a8faa5709a9b902b8673a5e8e1415f14a9 (11/19).

Comment 3

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Yuan, Jan,

Any chance one of you know what might cause this regresssion?
Thanks.

Comment 4

[Jan Jongboom [:janjongboom] (inactive)]

Not from the top of my head, but I can take a look on Monday.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Would it be possible this only happens on <div contenteditable>?

Comment 6

[Yuan Xulei [:yxl] (Leave from May 18)]

I looked through the keyboard gecko code and didn't find any clue. We haven't made changes about the composition since late Oct.

Comment 7

[Po-chiang Chao [:bobchao]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #5)
> Would it be possible this only happens on <div contenteditable>?

It's not -- I have this problem on an app "My name is," and I just checked the source code [1], simply an <textarea>. 

Note that I can't 100% reproduce this issue from different builds -- well, 100% with my own builds, but sometimes it works well with Geeksphone's image [2].

[1] https://marketplace.firefox.com/downloads/file/232617/my-name-is-0.1.zip
[2] http://downloads.geeksphone.com/

Comment 8

[Jason Smith [:jsmith]]

Can we get end user STR or a reduced test case that can be used to reproduce this bug?

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Jason Smith [:jsmith] from comment #8)
> Can we get end user STR or a reduced test case that can be used to reproduce
> this bug?

People would need to build their own phone in order to include Pinyin IME to test this out. I am filing another bug to include Pinyin IME in the eng build.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

STR:

1. Go to Settings > Keyboards > Selected keyboards > Add more keyboards
2. Enable Pinyin (labeled as "拼音输入")
3. Go back to home screen and go to focus on e.me search field
4. Tap N I H A O in sequence, which means "hello" in Mandarin Chinese.
5. Tap "你好" in the selection

Expected:

1. The text field shows "ni hao" with an underline on step 4
2. The text field shows "你好" on step 5

Actual:

1. The text field shows "nnini hni hani hao" with no underline on step 4
2. The text field shows "nnini hni hani hao你好" on step 5

Note:

1. e.me is just one of the examples. Input on UI tests also failed too.
2. Works in Browser pages and external test page: http://jsfiddle.net/timdream/YDGgk/

It's possible that the application is interfering with the input so we need different patches to address different causes. If so please clone this bug.

Blocking: 1.4? since we would like to have 3rd-party keyboard vendors to use this API even if we don't ship our own Asian IMEs.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

With further testing I fail to reproduce this bug on a newer build (Gecko/Gaia around Dec 15).
I am going to ask other dogfooders; if there isn't a problem anymore let's WFM this bug.

It would be nice if the API is protected by tests -- bug 932145 / bug 949961 will be good candidate for this work.

Comment 12

[Po-chiang Chao [:bobchao]]

FYI: On my Keon with nightly image from http://downloads.geeksphone.com/ , the bug's still there except in browser app.

I'm going to pull the latest gaia and try again later... few hours later.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I am setting qawanted, asking for a better reproduce step. Apparently users must do something else before doing what's documented in comment 10 in order to trigger this bug.

Comment 14

[Matthew Vaughan]

As far as I can see, the only additional thing that needs to be done is to set the keyboard to Pinyin (拼音输入).

1. Update Buri to Master M-C (1.4) BuildID: 20131223040210
2. Go to Settings > Keyboards > Selected keyboards > Add more keyboards
2. Enable Pinyin (labeled as "拼音输入")
3. Go back to home screen and go to focus on e.me search field
4. Tap on the text box to bring up the keyboard
5. Switch to the Pinyin keyboard layout via the Notification area
6. Tap N I H A O in sequence, which means "hello" in Mandarin Chinese
7. Tap "你好" in the selection

Using these STR, I am able to reproduce this issue according to the actual results in comment 10. I can get this to reproduce in the Email app, Message app, and the Marketplace app as well.

Environmental Variables:
Device: Buri Master M-C (1.4) MOZ RIL
BuildID: 20131223040210
Gaia: 5c4bb7e9dcdf6f168701508922c232a67cd8ea1f
Gecko: c0f85061c7d3
Version: 29.0a1
Firmware Version: V1.2_US_20131115

Comment 15

[Jason Smith [:jsmith]]

Sounds like we have confirmed str. We also know this regressed on Dec 1st, which makes it a regression in the 1.3 timeframe.

Comment 16

[Ivan Tsay (:ITsay)]

Since we don't ship neither Pinyin nor 3rd-party keyboard support in 1.3, this bug is not necessary to be the blocker for v1.3. nominate this bug to 1.4.

Comment 17

[Jason Smith [:jsmith]]

(In reply to Ivan Tsay (:ITsay) from comment #16)
> Since we don't ship neither Pinyin nor 3rd-party keyboard support in 1.3,
> this bug is not necessary to be the blocker for v1.3. nominate this bug to
> 1.4.

How are you 100% sure this isn't going to happen in 1.3? This is a regression in the 1.3 timeframe, which implies there might be other ways to reproduce this. The Pinyin IME keyboard just so happens to be one way to reproduce this, but it only applies to 1.4+.

Are there other ways this bug could be triggered? This needs more investigation on other ways to reproduce this bug.

Comment 18

[Ivan Tsay (:ITsay)]

Jason,

Agree with you about there may be other ways this bug could be reproduced. At this point, it seems only Pinyin can be used to reproduce this issue. In my opinion, this bug can be renominated to v1.3 if the other ways of reproduction STR are found.

Comment 19

[Yuan Xulei [:yxl] (Leave from May 18)]

This bug makes the last test case of the inputmethod mochitest failed on mozilla-central and I marked the last case as TODO:
http://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/mochitest/test_basic.html?force=1#124

The mochitest could help us to reproduce the bug.

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Jason Smith [:jsmith] from comment #17)
> 
> How are you 100% sure this isn't going to happen in 1.3? This is a
> regression in the 1.3 timeframe, which implies there might be other ways to
> reproduce this. The Pinyin IME keyboard just so happens to be one way to
> reproduce this, but it only applies to 1.4+.
> 
> Are there other ways this bug could be triggered? This needs more
> investigation on other ways to reproduce this bug.

The piece of code in question is the composition APIs. There is no other way to reproduce this bug.

That said, to be on the safe side, I won't object if we want to uplift the fix to 1.3 branch once the bug is fixed.

Comment 21

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

According our tests (Jan and I), now the composition API cannot work for plain input field in various apps, but it could work in browser app - awesome bar.

Kind of strange as it looks like an opposite of Bug 931746.

Kanru, do you happen to know are these 2 bugs related?
Thanks.

Comment 22

[Yuan Xulei [:yxl] (Leave from May 18)]

This bug is caused by Bug 845874. After back out the commit 0471905a267245523ed56212ed0f8768f9a1ca3d (https://git.mozilla.org/releases/gecko.git), the composition API works fine.

I'm not sure why bug 845874 will affect the composition function. I'll look into it tomorrow.

@masayuki, could you shed a light for this?

Comment 23

[Jeff Muizelaar [:jrmuizel]]

FWIW, bug 845874 was backed out of Gecko 28 because of the test failure with non-australis in bug 942250.

Comment 24

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

(In reply to Yuan Xulei [:yxl] from comment #22)
> This bug is caused by Bug 845874. After back out the commit
> 0471905a267245523ed56212ed0f8768f9a1ca3d
> (https://git.mozilla.org/releases/gecko.git), the composition API works fine.
> 
> I'm not sure why bug 845874 will affect the composition function. I'll look
> into it tomorrow.
> 
> @masayuki, could you shed a light for this?

It's odd. I have no idea. It shouldn't be the cause of regression.

Comment 25

[Masashi Honma]

I have checked underlined or not with Japanese keyboard.

o: URL bar of Browser
x: homescreen search bar

Gaia: 34e46e7f16f19623f652dd13bc56019603cc3316
Gecko: a01c2ee1c6a90d7641a7ad8a3fb3464e90620001
Device: KEON

Comment 26

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Hi Xulei, that's the status of this bug?

Comment 27

[Yuan Xulei [:yxl] (Leave from May 18)]

Still have no clue why bug 845874 breaks the composition.
I'm digging into the composition rendering code, but there is quite much code. I need more time to find the cause.

Comment 28

[Yuan Xulei [:yxl] (Leave from May 18)]

I logged composition events dispatched to nsIMEStateManager (http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsIMEStateManager.cpp#540) with the following code:

void
nsIMEStateManager::DispatchCompositionEvent(nsINode* aEventTargetNode,
                                            nsPresContext* aPresContext,
                                            WidgetEvent* aEvent,
                                            nsEventStatus* aStatus,
                                            nsDispatchingCallback* aCallBack)
{
  // Output the message name of the event to log.
  char* message = nullptr;
  switch(aEvent->message) {
    case NS_COMPOSITION_START:
    message = "NS_COMPOSITION_START";
    break;
    case NS_COMPOSITION_UPDATE:
    message = "NS_COMPOSITION_UPDATE";
    break;
    case NS_COMPOSITION_END:
    message = "NS_COMPOSITION_END";
    break;
    case NS_TEXT_TEXT:
    message = "NS_TEXT_TEXT";
    break;
  }
  if (message) {
#if defined(ANDROID)
    __android_log_print(ANDROID_LOG_INFO, "composition", "DispatchCompositionEvent %s\n", message);
#else
 ...
}

Compared the log from b2g-desktop with that from emulator, I found that NS_COMPOSITION_START, NS_COMPOSITION_UPDATE and NS_COMPOSITION_END events were not dispatched on emulator. We only got NS_TEXT_TEXT event. Therefore we failed to start and end the composition.

Comment 29

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: WIP.patch

This is a tentative patch and not fully tested. I'll stop to have a sleep and go on tomorrow :-)

Comment 30

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Fix composition v1 patch

The cause of the bug:

Bug 599550 introduced |seqno| (sequence number) to discard out-of-date event arrives at the parent process (the chrome process) from the child. When we 
synthesize composition event in the parent process with nsDOMWindowUtils::SendCompositionEvent (http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp#1929) and CompositionStringSynthesizer (http://mxr.mozilla.org/mozilla-central/source/dom/base/CompositionStringSynthesizer.cpp#137), the |seqno| isn't initialized and has a random value. If such |seqno| happens to be an out-of-date value, the synthesized event is discarded. That why we randomly fails to get composition work in the parent process.

However in the child process, we don't use |seqno|, so even if it is uninitialized, the composition works.

Solution:

I give the synthesized composition events a special |seqno| value - kLatestSeqno. If that value is detected, the parent process will skip |seqno| checks.

@jchen, since the |seqno| is first introduced by you, could you help to check that the workaround of |seqno| works and has no other side effect?  Thanks :)

Comment 31

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

Comment on attachment 8360187 [details] [diff] [review]
Fix composition v1 patch

Looks a little bit tricky to me, but I'm okay.

And looks like that you are modifying almost all lines using seqno. If so, could you rename it to mSeqno? (Although, I still don't like such name, "seqno", since it's too short. I assume/guess that it's abbreviation of "sequential number".)

Comment 32

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

Comment on attachment 8360187 [details] [diff] [review]
Fix composition v1 patch

Review of attachment 8360187 [details] [diff] [review]:
-----------------------------------------------------------------

It's been a while since I worked on this :), but I think it looks okay. Note that Firefox for Android no longer uses this code, so feel free to change it to suit B2G's needs.

Comment 33

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #31)
> Comment on attachment 8360187 [details] [diff] [review]
> Fix composition v1 patch
> 
> Looks a little bit tricky to me, but I'm okay.
> 
> And looks like that you are modifying almost all lines using seqno. If so,
> could you rename it to mSeqno? (Although, I still don't like such name,
> "seqno", since it's too short. I assume/guess that it's abbreviation of
> "sequential number".)

Thank you, Masayuki:-)

I'll update the patch and rename "seqno" member to "mSeqno".

"seqno" is short for sequence number. Though abbreviation isn't recommended,  we use "seqno in other classes. Maybe we can stay the same with them.

Comment 34

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Jim Chen [:jchen :nchen] from comment #32)
> Comment on attachment 8360187 [details] [diff] [review]
> Fix composition v1 patch
> 
> Review of attachment 8360187 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's been a while since I worked on this :), but I think it looks okay. Note
> that Firefox for Android no longer uses this code, so feel free to change it
> to suit B2G's needs.

Thank you. I didn't expect your reply so quickly. Are you back to China?

Comment 35

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Fix composition v2 patch

Rename "seqno" member to "mSeqno".

https://tbpl.mozilla.org/?tree=Try&rev=a0a5ce101063

Comment 36

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

(In reply to Yuan Xulei [:yxl] from comment #34)
> (In reply to Jim Chen [:jchen :nchen] from comment #32)
> > Comment on attachment 8360187 [details] [diff] [review]
> > Fix composition v1 patch
> > 
> > Review of attachment 8360187 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > It's been a while since I worked on this :), but I think it looks okay. Note
> > that Firefox for Android no longer uses this code, so feel free to change it
> > to suit B2G's needs.
> 
> Thank you. I didn't expect your reply so quickly. Are you back to China?

No, just working late :)  I do wish to visit the China office sometime this year though.

Comment 37

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Jim Chen [:jchen :nchen] from comment #36)
> No, just working late :)  I do wish to visit the China office sometime this
> year though.
We're looking forward to that :-）

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/58663c0918a4

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/58663c0918a4

Comment 40

[Andreas Pehrson [:pehrsons]]

[Blocking Requested - why for this release]:

We have now hit this for 1.4 in bug 1051228. I can't see a motivation for removing the nomination for 1.4 so I'm renominating. See the description in bug 1051228 for details.

Comment 41

[thomas tsai]

It has been in v1.4.
commit c17f701bb8d2ed2c775f31a97121ea0e2b1c70b4
Author: Yuan Xulei <[e-mail]>
Date:   Wed Jan 15 09:41:39 2014 -0500

    Bug 946068 - Fix composition events' synthesizing in chrome process. r=masay

Comment 42

[Wayne Chang [:wchang]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #40)
> [Blocking Requested - why for this release]:
> 
> We have now hit this for 1.4 in bug 1051228. I can't see a motivation for
> removing the nomination for 1.4 so I'm renominating. See the description in
> bug 1051228 for details.

This had landed before 1.4 branched off so it is already on 1.4.