810304 - [Keyboard] After clicking outside keyboard when it's shown, events are not managed properly (i.e. SMS App when clicking 'Send')

Status: RESOLVED DUPLICATE of bug 821414

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

Comment 1

[Josh Carpenter [:jcarpenter]]

Bumping to UX-P2 ("should fix for release"). 

Sergi, can you confirm whether this is still a problem, and if so, decide whether to nominate for blocking-basecamp. If you do nominate, please also provide a rationale for the triagers.

Comment 2

[steve]

This is still a problem, it has not been resolved. I nominate for blocking basecamp.

Rationale: Sending a text is a fundamental requirement for any phone, something our feature phone users ( our target audience) will be used to doing a lot. Having to press 'Send' twice will lead instantly to texts not being sent, confusion and frustration. I personally think it should stay as a P1. I'd like to think its an easy fix.

Comment 3

[Borja Salguero [:borjasalguero]]

This was due to Keyboard. There was a layer in 'system' called 'keyboard-overlay' which managed this type of events, but If Im not wrong this layer was removed some days ago. Could you try again and if error is there, could you explain the scenario?

David, we should rename as 'After clicking outside keyboard when it's shown, events are not managed properly', due to if this error is there, is an error from 'System', and should be reproducible in all apps with a form.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Can someone please provide a clear STR? Not sure I'm following.

Comment 5

[Borja Salguero [:borjasalguero]]

If keyboard it's shown, there was a layer called 'keyboard-overlay' which managed all 'click' actions on it, and was hiding the keyboard. However, this mechanism was wrong due to clicking some button in the screen while the keyboard was shown (for example click 'send' in SMS, or 'Done' in Contacts) had no effect (keyboard was hidden, but the action 'click' was not handle properly).

Currently this layer is not in System anymore, so we have to test first of all if this problem persist, and if it is there, check why it's happening.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

So the STR would be:

1) Open SMS app
2) Create new text message
3) Click 'Send' while the keyboard is opened

Expected: Message is sent.
Actual: Need to click the 'Send' button again.

(right?)

With latest Gaia and latest Gecko 18 this works for me on Unagi.

Comment 7

[Borja Salguero [:borjasalguero]]

[:ttaubert]  For me it's working as well, due to 'keyboard-overlay' was removed some days ago. David, could you try?

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

This is actually very easy to reproduce and this is annoying.

Comment 9

[Borja Salguero [:borjasalguero]]

[:vingtetun] Could you define the steps for reproducing it? Thanks!

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

This seems to be easily reproducible in the contacts app. I was trying to edit a contact and had to double-press the 'Add Comment' or 'Update' buttons.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) from comment #8)
> This is actually very easy to reproduce and this is annoying.

Sure!

Steps to reproduce:
 - Launch the sms app and click on the '+' to create a new message
 - Type something in the text message
 - Put the finger on the 'send' button but don't release it.

Actual result:
 - The keyboard disappear even if you have not remove the finger from it.

Expected result:
 - The keyboard stays.

Comment 13

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

An application workaround will be have the Sms app listen to mousedown instead of click on it's Send button, and switch the focus back to the input when send is invoked.

I agree we should fix this though.

Comment 14

[Borja Salguero [:borjasalguero]]

I would rather avoiding workarounds with it, because you are gonna find the same in all apps... :(

Scenario 2:
- Open Browser
- Type something to search in browse-bar
- Put the finger on the 'browse' button but don't release it.

Actual result:
 - The keyboard disappear even if you have not remove the finger from it.

Expected result:
 - The keyboard stays.

Also other annoying issue in form (Scenario 3):
- Open Contacts App > New
- Fill name field.
- Click in 'Last Name' Field
- Keyboard dissapears/appears during the whole form :/

Probably we should update the logic for hiding the keyboard in System, because it's so important in order to have the best User Experience as possible.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Vivien Nicolas (:vingtetun) from comment #12)
> Steps to reproduce:
>  - Launch the sms app and click on the '+' to create a new message
>  - Type something in the text message
>  - Put the finger on the 'send' button but don't release it.

That problem is very different from the actual problem described here, no? I think this bug is about sometimes having to press buttons twice like described in comment #6.

Comment 16

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 6979 - fix keyboard on send and receive

Here is a patch to fix obvious breakages living in sms app:
The keyboard is hidden when you click on send.
And when you receive a message while writing a message, you have the sms app being resize to the full height so that messages and the input are displayed behind the keyboard.

Having said that fixing both of these issues isn't obvious. Especially the 2nd issue, as the faulty code lives in window manager. That's why I'm asking alive's review here. But feel free to forward the review.

The problem with the window manager was that while receiving the open-app event on sms reception, we were calling `setAppSize`. And the concrete issue was that this method wasn't taking keyboard size into account.
So I added some code is keyboard manager to expose that and used it in setAppSize.

The other part of this patch lives in sms app and is quite simple and just do what Tim suggested in comment 13.

Comment 17

[Borja Salguero [:borjasalguero]]

Only for clarifying. Is this bug fixing all the scenarios in https://bugzilla.mozilla.org/show_bug.cgi?id=810304#c14 ? Or is it only a workaround for SMS App? Thanks!

Comment 18

[Alexandre Poirot [:ochameau]]

(In reply to Borja Salguero [:borjasalguero] from comment #17)
> Only for clarifying. Is this bug fixing all the scenarios in
> https://bugzilla.mozilla.org/show_bug.cgi?id=810304#c14 ? Or is it only a
> workaround for SMS App? Thanks!

It only fixes sms.

Comment 19

[Borja Salguero [:borjasalguero]]

Ok. For clarifying this bug is NOT related with SMS, it's a Keyboard issue. As you can see, we modified the name some days ago because it's a bug that it's easily reproducible though all the apps, so the fix here should be in 'Keyboard', avoiding 'workarounds' for a specific app, due to if we follow this way we need a 'workaround' in every app of the system :S. Could you create a patch for the 'keyboard' only? Thanks!

Comment 20

[Borja Salguero [:borjasalguero]]

I've R- due to the explanation before. It's not a bug in SMS, it's a Keyboard bug (so there is no need to modify SMS App ;) ). That's why I've added the R-. Once the patch will be only for 'Keyboard' feel free to remove this R-. Thanks!

Comment 21

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

Comment on attachment 691413 [details]
Pull request 6979 - fix keyboard on send and receive

Cancel review.
I think there's a more clear way to resolve this. I don't know why we need to open the same app again when it is already at foreground. Please investigate more as you could. Avoid opening the same app. 
Request again when you are ready. Thank you. Talk/Comment to me if you have new idea about what I raise.

Comment 22

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

Tim told me that why sms need to be opened when we got messsge. The sms app itself relies on system message which got by system app. System app then needs to open sms app 'in background' and set its size because cards view would get wrong screeshot if we don't do so. So the correct key to the case is just early return after hash setting.

I agree that write every evt.preventDefault() in every app is stupid so we should file another bug for general solution. I think this is gecko work. Please retitle this bug again.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Before you spend too much time on an app-specific fix here, please take a look at bug 766066.

Comment 24

[Alexandre Poirot [:ochameau]]

If we follow borja vision of this bug, this bug is already fixed for sms.

So I'm unassigning from this bug and opened two other bugs and splitted the pull request in two:
 - bug 821334: keyboard hide on sms reception or attention screen hide (window manager issue)
 - bug 821414: keyboard hide on send button tap (sms app issue)

Comment 25

[Borja Salguero [:borjasalguero]]

[:ochameu] I did not say that it's fixed for SMS, I told you that this bug is easily reproducible in all apps in the platform (not only in SMS), so probably the bug would be a 'System' or 'Keyboard' bug. That's why we need to identify where are the bugs or the bug and try to fix it without standalone workarounds in every app.

From my point of view bug 821414 should be a 'duplicated' of this one or, if there is more than one bugs related with Keyboard, we should create a [META] bug in order to track all issues there (and 821334 should be part of this META bug)

On the other hand (despite Im gonna review the code for checking if it is a bug in SMS), bug 821414 sounds like a 'workaround' for getting the expected result, but probably the problem is not there and, as [:alive] and [:ttaubert] told you, there are more issues behind this weird behaviour of 'Send' Button, or other examples though all our apps.

Comment 26

[Josh Carpenter [:jcarpenter]]

Might be related to bug #806540?

Comment 27

[:Margaret Leibovic]

(In reply to Josh Carpenter [:jcarpenter] from comment #26)
> Might be related to bug #806540?

Bug 806540 just landed and changed a lot of our event handling code for the keyboard, but the changes in that patch should just affect events on the keyboard itself, so I don't know if it would impact touching outside of the keyboard.

However, I would encourage anyone investigating this bug to make sure you update to include that change!

Comment 28

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Sergi can you give a clearer description of the bug you see? A lof of assumptions has been made about this bug and I wonder what was your initial issue :)

Comment 29

[Dylan Oliver [:doliver]]

I don't think this one qualifies for smoketest priority anymore given that bug 821414 has landed and that addressed the original bug that was reported here.

Comment 30

[:Margaret Leibovic]

From comment 14, it sounds like this bug is about the fact that the keyboard hides when the app content gets focus (focus changes on mousedown, not mouseup, see bug 766066 comment 72).

Looking at the keyboard code, this is intentional, and I'm not sure what kind of fix would would make to avoid this. The fix for the SMS app was considered a workaround, but maybe it should be up to apps to call .preventDefault() themselves if they don't want the keyboard to go away.

However, testing on Fennec to compare, focus doesn't actually change when you hold your finger on content outside of the active input. Instead, we enter a panning state. So maybe the real issue here is that we shouldn't be firing mousedown and changing focus if the user is holding a finger on the screen instead of just making a single tap.

I'm cc'ing cpeterson because he would know more about this. It sounds related to bug 766066 and friends.

Comment 31

[Dietrich Ayala (:dietrich)]

Will this be fixed by bug 823619, or need further work?

Comment 32

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We would want to back out bug 821334.

Comment 33

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Correction: none of those workarounds landed.  So this works out of the box.

Comment 34

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Note: bug 823619 isn't going to "fix" the issue of focus changing, because that's not a bug.  Apps that don't want focus to change when their content is focused should preventDefault().  That continues to work flawlessly with bug 823619.

Bug 823619 will fix the wonky state with the button where sometimes it wouldn't send a text when almost-but-not-quite tapped, and then dismiss the keyboard.  It will also fix the bug where panning content dismisses the keyboard.  Both of these are really more general though (bug 819595).  But I'm happy to claim this as another win from 823619 and let anyone who's unhappy with the fix reopen, and be more specific.

Comment 35

[Kevin Grandon :kgrandon]

The originally reported issue of this bug is that SMS has focus issues when sending. Bug #821414 addresses this and has been landed.

There may be additional work for keyboard focus, but I agree that we should use bug #819595/#823619 for tracking.

Marking this one as duplicate.