973007 - [B2G] [Messaging] Keyboard appears when accessing the contact list from the compose message screen

Status: RESOLVED FIXED

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

Description

[Chris Kreinbring [:CKreinbring]]

Attachment: Video of keyboard appearance during new message creation

Description:
Tapping the + icon while composing an SMS/MMS message will load the contact list but also cause the keyboard to appear.

Repro Steps:
1) Update Buri to Build ID: 20140214004001
2) Launch the Messaging app.
3) Tap the New Message icon then tap the + icon in the To field.
4) Observe the appearance of the contact list when it loads.

Actual:
The keyboard appears as the contact list loads and remains present.

Expected:
The contact list appears, and the keyboard is not present.

Environmental Variables
Device: Buri 1.3 mozilla RIL
Build ID: 20140214004001
Gecko: https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/e3766683210c
Gaia: 22e065f75193f569a78a8ec827b08e1ed520e1b2
Platform Version: 28.0
Firmware Version: V1.2-device.cfg

Notes:
Repro frequency: 100%
See attached video clip

Comment 1

[Chris Kreinbring [:CKreinbring]]

Also repros on Buri 1.2 mozilla RIL

Build ID: 20140210004002
Gecko: https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/2673f348598c
Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
Platform Version: 26.0
Firmware Version: V1.2-device.cfg

Comment 2

[Rick Waldron [:rwaldron]]

This was addressed in https://bugzilla.mozilla.org/show_bug.cgi?id=920546, which was uplifted to 1.2: https://github.com/mozilla-b2g/gaia/commit/7bb90d2e127792110029bdfb38f7750e62bf8bbe

I'll take a further look to see if I can identify a regression window.

Comment 3

[Julien Wajsberg [:julienw]]

+ it should really be fixed by bug 937487 too.

So while we can certainly workaround this in the SMS app, I'd rather have alive have a look at this first.

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

What I see in 1.4:
Keyboard shows and hides when activity is opening.

BTW, it's really hard to tap the +..... :(

Gary, could you help this one?

Comment 5

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Attachment: pull request: https://github.com/mozilla-b2g/gaia/pull/16390

Hi Rudy,
   Same as bug 963377, please help to review this patch.

Comment 6

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Hi Julienw, Steve and alive,
   Even my patch landed, the keyboard still appears immediately when 'actvity' occured.
   The root cause is Keyboard manager received 'focus' event after 'actvitopen', should we file a follow up bug for this issue?

============Log================
06-12 02:20:18.838: keyboard_manager.js:442 in km_handleEvent: KM handleEvent:activityopening
06-12 02:20:18.838: keyboard_manager.js:648 in km_hideImmediately: KM hideKeyboardImmediately
06-12 02:20:18.848: keyboard_manager.js:264 in km_inputFocusChange: KM inputFocusChange:textarea
06-12 02:20:19.708: keyboard_manager.js:264 in km_inputFocusChange: KM inputFocusChange:blur

Comment 7

[Julien Wajsberg [:julienw]]

I think you should not show the keyboard for a focus that is triggered in an app that is not visible.

I don't know if the issue is in system, keyboard or gecko, but this is the wrong thing and this is what should be fixed :)


(In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #4)
> What I see in 1.4:
> Keyboard shows and hides when activity is opening.
> 

Note the bug has been found in 1.3 and is a regression, so it will likely be 1.3+.

> BTW, it's really hard to tap the +..... :(

bug 946731 ?

Comment 8

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

Comment on attachment 8377426 [details] [review]
pull request: https://github.com/mozilla-b2g/gaia/pull/16390

r+ since this patch is reasonable and should be a safe one.

However, as Alive said, I could not reproduce the issue as the description, with master branch.
what I saw is that the keyboard would show for a short while and then disappear.

Can someone on this bug confirm that this could be reproduced on v1.3 and the patch provided could address that?

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 8377426 [details] [review]
pull request: https://github.com/mozilla-b2g/gaia/pull/16390

I cna have a look.
If someone else wants to check this, feel free and remove the feedback request for me ;)

Comment 10

[Steve Chung [:steveck]]

Hi Gary,
After some discussion, is this still impossible to classify the focused element is visible or not per your investigation? It shouldn't be a difficult task to avoid this from message app, but having a solution from the keyboard manager sounds pleasant :)

Comment 11

[Julien Wajsberg [:julienw]]

Especially this could happen for other apps, not only the Messages app...

Comment 12

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

Comment on attachment 8377426 [details] [review]
pull request: https://github.com/mozilla-b2g/gaia/pull/16390

Just flashed the lastest v1.3 build on buri, but I still can reproduce this issue as the description, even with this patch.

Gary, may I know if you have tried this patch on v1.3?
Thanks.

Comment 13

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

(In reply to Steve Chung [:steveck] from comment #10)
> Hi Gary,
> After some discussion, is this still impossible to classify the focused
> element is visible or not per your investigation? It shouldn't be a
> difficult task to avoid this from message app, but having a solution from
> the keyboard manager sounds pleasant :)

Steve, Julien,

Thanks for the feedback.
However, I don't think we have this "input element visibility" info in keyboard manager now, and it would require gecko change, so I don't think we can make this change in v1.3.

Comment 14

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

As I mentioned on comment 6 same issue on v1.3.
KM can not check 'visible' on current platform, so maybe need to fix in SMS app.

Please ignore my patch and I am not familiar with SMS app so remove assignee first.

Hey Rudy,
Sorry about I did not verify v1.3 first.
===============LOG=======================
06-12 04:27:03.649: keyboard_manager.js:423 in km_handleEvent: KM activitywillopen
06-12 04:27:03.649: keyboard_manager.js:611 in km_hideImmediately: KM hideKeyboardImmediately
06-12 04:27:05.899: keyboard_manager.js:423 in km_handleEvent: KM activitywillopen
06-12 04:27:05.899: keyboard_manager.js:611 in km_hideImmediately: KM hideKeyboardImmediately
06-12 04:27:05.899: keyboard_manager.js:248 in km_inputFocusChange: KM inputs:textarea

Comment 15

[Julien Wajsberg [:julienw]]

It makes me really sad, because I _know_ we fixed this in the past in v1.3.

Bug 934449 fixed it with a specific patch for SMS.
Bug 937487 fixed it in the keyboard manager. But maybe not completely?

Subsequently, I think we reverted bug 934449 at one point.

I'm fine with landing something hacky in 1.3 (I'll file a new bug for this). But let's fix it for real in Gecko, this is happening too much for too much time.

Comment 17

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

Filed bug 974285 to see if we could get gecko support for a proper fix.

Comment 18

[Steve Chung [:steveck]]

Change the name to input manager and message workaround patch would be in bug 973921.

Comment 19

[Ivan Tsay (:ITsay)]

Plus it as the regression needs to be fixed.

Comment 20

[(inactive after 6/18) Alive Kuo [:alive]]

Gary has a patch.

Comment 21

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Hi Alive,
   The patch was out of date.
   This issue was changed to "Only display keyboard when focused element is visible". As Rudy mentioned on comment 13, we need to get more information from gecko to implement in Keyboard manager. I don't think we can do anything in v1.4 now.

Hi Ivan and Rudy,
   Can we remove v1.4+ from this issue since message workaround patch would be in 973921? 

remove assignee.

Comment 22

[Julien Wajsberg [:julienw]]

bug 973921 was closed by Jason.

Comment 23

[Julien Wajsberg [:julienw]]

also, bug 973921 was only about 1.3, not 1.4.

Comment 24

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

Rudy, could take this as #1 in your queue? Thanks.

Comment 25

[Ivan Tsay (:ITsay)]

Rudy/Gary,

Just try the behavior on Buri with the latest v1.4 build. The issue seems a little bit different the one reported in comment 0. When tapping the ;+ for adding receip\], the keyboard will appear and then disappear immediately in stead of being present. My questions are

1. Have we done something in purpose for resolving the issue in this approach?
2. If we want to go for the real solution from gecko side, have you already found gecko developer for the help?
3. We also need to know if the gecko solution is feasible to meet the 1.4 time frame.

Thank you!

Comment 26

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

(In reply to Ivan Tsay (:ITsay) from comment #25)
> Rudy/Gary,
> 
> Just try the behavior on Buri with the latest v1.4 build. The issue seems a
> little bit different the one reported in comment 0. When tapping the ;+ for
> adding receip\], the keyboard will appear and then disappear immediately in
> stead of being present. My questions are
> 
> 1. Have we done something in purpose for resolving the issue in this
> approach?

No, not sure why v1.4 behaves differently from v1.3, could be caused by window management refinement.

> 2. If we want to go for the real solution from gecko side, have you already
> found gecko developer for the help?

The gecko bug is bug 974285, but we still don't have a consensus on what to do.

> 3. We also need to know if the gecko solution is feasible to meet the 1.4
> time frame.

From past experience, I would suggest we estimate the feasibility after we have the gecko solution first.

> 
> Thank you!

Re-nominate this as v1.4, for 
 1. The issue we saw on v1.4 is not the same as comment 0, and is much better (show keyboard for a short time), though not perfect.

 2. The total solution here would need both Gaia + Gecko changes, and in the limited v1.4 time frame, we would suggest we postpone this to the next release.

Comment 27

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

Remove regression keyword as this bug is for v1.4 and right now the behavior is different from v1.3.

Julien,

Could you help take a look at comment 26 and this one?
And please re-nominate if you don't agree on this.

Thanks.

Comment 28

[Julien Wajsberg [:julienw]]

Jason said on bug 973921 that we don't want to block on something that was present in previous releases. So I'm fine with fixing this for real in v1.5.

Comment 29

[Ivan Tsay (:ITsay)]

Move it to v1.5 based on the comment 26 and 28

Comment 30

[Jack Liu]

Dears,

It's bad user experience and nearly 100% reproduce rate on Sora 1.3, why not fix it in 1.3?

Comment 31

[Jason Smith [:jsmith]]

(In reply to Jack Liu from comment #30)
> Dears,
> 
> It's bad user experience and nearly 100% reproduce rate on Sora 1.3, why not
> fix it in 1.3?

Yeah - let me get clarification here. Does this happen on 1.1?

Comment 32

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #31)
> (In reply to Jack Liu from comment #30)
> > Dears,
> > 
> > It's bad user experience and nearly 100% reproduce rate on Sora 1.3, why not
> > fix it in 1.3?
> 
> Yeah - let me get clarification here. Does this happen on 1.1?

Oh I see what happened here - this bug got morphed. That shouldn't have happened in the first place.

I'm reverting the title here as this changes the context of what the original reporter intended, which is in violation of Bugzilla's etiquette guidelines (https://bugzilla.mozilla.org/page.cgi?id=etiquette.html).

Please focus the issue on the original problem here on the basis of fixing the problem in the least risky form possible. If you want a different solution here for long term, then please open up a new bug for this. But don't morph this bug please.

Comment 33

[Jack Liu]

Hi Jason,

It's ok on buri 1.1 and also ok on Sora 1.3 with buildID 20140105004001 and Geekspone 1.3 with buildID 20140123113317.

Comment 34

[Julien Wajsberg [:julienw]]

Jason, please note that I originally filed bug 973921 to fix this for v1.3 with a SMS-specific patch, that's why the bug was morphed to something that would also fix this bug, but in a more general way.

You closed Bug 973921 as a dupe (please see Bug 973921 comment 1 for your own rationale).

The regressing bug is probably Bug 957714 but while we can do a SMS-specific patch, there is a underlying bug in the Input Management. That's why there was originally 2 bugs, where you forcibly closed one.

So, what should we do now? I'm upset by this way of doing.

Comment 35

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #34)
> Jason, please note that I originally filed bug 973921 to fix this for v1.3
> with a SMS-specific patch, that's why the bug was morphed to something that
> would also fix this bug, but in a more general way.

We should not morph bugs. That's going to do nothing but cause confusion to everyone who reads the bug.

> 
> You closed Bug 973921 as a dupe (please see Bug 973921 comment 1 for your
> own rationale).

That was confusion from the fact that this bug was morphed.

> 
> The regressing bug is probably Bug 957714 but while we can do a SMS-specific
> patch, there is a underlying bug in the Input Management. That's why there
> was originally 2 bugs, where you forcibly closed one.

If the real fix is too risky for 1.3, then that should be opened as a separate bug. This bug shouldn't be morphed - this should stay focused on the blocking factor of the bug to what the reporter originally filed the bug about.

> 
> So, what should we do now? I'm upset by this way of doing.

Focus on the original problem of what regressed here to resolve the blocking factor of this bug. But do not morph bugs - that causes nothing but confusion to everyone who reads the bug. Please open a separate bug for the real fix that can't be done in the 1.3 timeframe due to risk and focus this bug on the blocking factor.

Comment 36

[Preeti Raghunath(:Preeti)]

Does this repro on 1.3 Buri? What's the impact to the user? Gecko changes per comment 26 this late is definitely a risk so need to see impact to user.

Comment 37

[Jason Smith [:jsmith]]

(In reply to Preeti Raghunath(:Preeti) from comment #36)
> Does this repro on 1.3 Buri? What's the impact to the user? Gecko changes
> per comment 26 this late is definitely a risk so need to see impact to user.

Yes - this is device independent. Impact is the fact that the keyboard is coming up in a UI that's not intended to show up, so you'll get a smaller portion of the UI showing up that the keyboard doesn't cover.

The gecko changes aren't necessary to resolve this bug - there's a huge set of confusion that happened here due to bug morphing. We only need to focus on resolving the blocking factor on the bug, which should only focus on providing the messaging workaround, not the other patches originally worked on in the bug.

Comment 38

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

Unassign myself if this bug would be kept to describe the message app part.
Let me know if you need any help.

Thanks.

Comment 39

[Julien Wajsberg [:julienw]]

Because Jason is always right and teaches us how to use Bugzilla, Rudy, can you please file a new other bug to fix the core Input Management issue and make it depend on bug 974285?

(In reply to Jason Smith [:jsmith] from comment #35)
> (In reply to Julien Wajsberg [:julienw] from comment #34)
> > Jason, please note that I originally filed bug 973921 to fix this for v1.3
> > with a SMS-specific patch, that's why the bug was morphed to something that
> > would also fix this bug, but in a more general way.
> 
> We should not morph bugs. That's going to do nothing but cause confusion to
> everyone who reads the bug.
> 
> > 
> > You closed Bug 973921 as a dupe (please see Bug 973921 comment 1 for your
> > own rationale).
> 
> That was confusion from the fact that this bug was morphed.

The bug was not morphed. I thought the fix in Bug 937487 regressed, which is actually an Input Management bug. So I made this bug an Input Management bug. That does not mean we can't fix it in another way, which is why I created another bug, that you closed.

Now we can do it the other way around, I don't really care. I'll take it if this bug is made a blocker.

Comment 40

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

Filed bug 979191 to track the underlying input management issue.

Julien, Jason,

Thanks for the advice.

Comment 41

[Preeti Raghunath(:Preeti)]

David,

Please help reassign

Comment 42

[Steve Chung [:steveck]]

Attachment: Bug-973007.patch

Hi Julien, the reason that cause the keyboard still visiable is because the recipient view receives a click event after entering the activity(picker activity is triggered by icon 'mousedown', and there is no use to set focusable to false here because click event will still set it back to focusable later). The first simplest and harmless workaround in my mind is just stop the icon click event propagate to receipient view, but any better solution is welcome.

Comment 43

[Julien Wajsberg [:julienw]]

Thanks for the patch Steve, I'll have a look asap this morning!

Comment 44

[Julien Wajsberg [:julienw]]

Comment on attachment 8385905 [details] [diff] [review]
Bug-973007.patch

This is definitely the right fix, good idea!

Maybe in the comment you can say that the "Recipients.View" constructor attach a "click" event on the "messages-to-field" element.

And just add a unit test for this.

Comment 45

[Steve Chung [:steveck]]

Attachment: Link to github

Here is the github patch with more comments and unit test.

Comment 46

[Julien Wajsberg [:julienw]]

Comment on attachment 8387432 [details] [review]
Link to github

r=me with the comments addressed

Comment 47

[Steve Chung [:steveck]]

Landed in master: e91db4ca0ff894cc92ea1880683e306cf0b86bc5

Comment 48

[Ryan VanderMeulen (PTO, back 6-April)]

Please request approval-gaia-v1.3 on this patch for 1.3 uplift.

Comment 49

[Julien Wajsberg [:julienw]]

Comment on attachment 8387432 [details] [review]
Link to github

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): -
[User impact] if declined: wrong UX
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none

Comment 50

[Ryan VanderMeulen (PTO, back 6-April)]

v1.3: 8d235a284173ea6d441c8d1ce1cc164b6b91423a