859224 - [SMS] Back key in thread view is not working when we receive an SMS during call

Status: VERIFIED FIXED

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

Description

[sasikala]

1. Title : Back key in thread view is not working when we receive an SMS during call
2. Precondition : Messageing app should be working.
3. Tester's Action : During call - Receive new sms - Open an sms - select back key
4. Detailed Symptom (ENG.): Not able to perform any operation with back key.
5. Expected : Should be able to move to previous screen using back key.
6.Reproducibility: Y
1)Frequency Rate : 100%
7.Gaia Revision: 8e7262e3d98ea9574d7f79c1e890ad80cfa40c27

Comment 1

[Fernando Campo (:fcampo)]

Most definitely related with bug 852387, but need to check some stuff to mark it as dup or not

Comment 2

[Wayne Chang [:wchang]]

Triage 4/10 - Leo+ as a UI failure.

Steve, can you check this?

Comment 3

[Borja Salguero [:borjasalguero]]

It's a regression. I've marked as duplicated of 860187 due to it's for the same reason, and I will apply the patch there.

Comment 4

[Leo]

Able to reproduce the issue 

Please check the Reproducing steps:
1.Receive and accept a call
2.Receive an sms during call
3.Open the sms,click on the back button.
4.Back button is not working.

The duplicated issue is different from this one.So please change the status of this issue

Comment 5

[Julien Wajsberg [:julienw]]

I uplifted Bug 860187 quite late yesterday, maybe the build you're using does not have the fix yet ?

Comment 6

[Leo]

Attachment: Proposed patch

We have checked in the Gaia Version:9944dd8671c305a0be62d9cef918e022e5184ac0

This issue is different from the issue marked as duplicate.
I am attaching the patch for this issue.Please check and add your comments.

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 737914 [details] [diff] [review]
Proposed patch

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

::: apps/sms/js/thread_ui.js
@@ +202,5 @@
>        }
>      };
>  
> +    //This defines the screen height during active call
> +      var activeCallHeight = 390;

such "magic" values are not good.

I'd prefer that we check that our height is lower than a specific threshold, like lower than two third of the total height.

Comment 8

[Julien Wajsberg [:julienw]]

reopning, this is indeed another issue.

Comment 9

[Julien Wajsberg [:julienw]]

qawanted to know if that happens on 1.0.1 too.

Comment 10

[Leo]

Attachment: Proposed patch with comments from mozilla

Please check the patch with changes.
Added and tested the patch with the comments.

Thanks

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 737947 [details] [diff] [review]
Proposed patch with comments from mozilla

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

::: apps/sms/js/thread_ui.js
@@ +203,4 @@
>      };
>  
>      // We're waiting for the keyboard to disappear before animating back
> +    if (this.view.offsetHeight < ( 2 * MessageManager.fullHeight)/3 ) {

I talked with someone from Gaia team and with Sharaf, and now we think this calculation (that I suggested, sorry for that) is not reliable enough.

:etienne said that a keyboard is at least 150px. Also, I'd like to see this in an external function. So what about :

isKeyboardDisplayed: function thui_isKeyboardDisplayed() {
  // minimal keyboard height is 150px
  return (this.container.offsetHeight < ThreadListUI.fullHeight - 150);
}

and then :

if (this.isKeyboardDisplayed()) {
...
}

(note: my code here works on current gaia master, that's where your patch should work too)

Comment 12

[Julien Wajsberg [:julienw]]

Paul, could you please check if this happens on 1.0.1 too ?

This bug is not so serious but I can see a use case where the user wants to find something in his sms to answer someone that is on the call.

Comment 13

[Angela Hubenya]

The back key does not work on v1.0.1  

Unagi: 20130416070200
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/6bac24e14538
Gaia: c883af5ecd0998f78d9aaa4c2337c842e1dbb5a0

Comment 14

[Julien Wajsberg [:julienw]]

Asking tef? based on comment 13.

Comment 15

[Julien Wajsberg [:julienw]]

(note: it was leo+ and I changed it to leo? by error in comment 12)

Comment 16

[Leo]

Attachment: Proposed patch with new calculation

Thanks for the comments.
Updated the patch with new suggestion.
Tested the scenarios on gaia master code.
Please review the patch once and provide your comments.

Thanks,

Comment 17

[Dietrich Ayala (:dietrich)]

After heated discussion, triage says to block on this.

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 738350 [details] [diff] [review]
Proposed patch with new calculation

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

r=me
works fine, thanks

please either do a pull request or generate your patch with "git format-patch -U8 master" so that we have the necessary commit information.

Also, we'll need a 1.0.1 specific patch, as some variable names were changed. Would you please create this patch too, and ask me for review on it ? thanks

Comment 19

[Leo]

Hi Julien,

I will do this and send you for review.

Thanks

Comment 20

[Leo]

Attachment: Proposed specific Patch for V1_0_1

Hi Julienw,

I have tested and attached the patch for 1.0.1.
Please review it and provide your comments.
I will do a pull request for attachment 738350 [details] [diff] [review].

Thanks

Comment 21

[Julien Wajsberg [:julienw]]

Comment on attachment 738424 [details] [diff] [review]
Proposed specific Patch for V1_0_1

I think you don't use a current v1.0.1 branch. The sms.js file does not exist anymore since the 25th March.

please provide a patch generated with format-patch or a pull request (or both) against the current v1.0.1 branch.

Comment 22

[Leo]

Attachment: Pointer to pull request https://github.com/mozilla-b2g/gaia/pull/9236

Please review the pull request and provide your comments

Thanks,

Comment 23

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

(In reply to leo.bugzilla.gaia from comment #22)
> Created attachment 738456 [details]
> Pointer to pull request https://github.com/mozilla-b2g/gaia/pull/9236
> 
> Please review the pull request and provide your comments
> 
> Thanks,

I would like to make sure we won't enter again into this kind of bug. Can you add a timeout guard? So for example if the resize event has not happened after 400ms it would be nice to force the app to go back.

Comment 24

[Leo]

Hi julienw,

I will check the v1.0.1 branch and provide the patch to you.
Sorry for wrong attachment(patch).

Thanks,
Leo

Comment 25

[Julien Wajsberg [:julienw]]

I agree with Vivien's proposition here.

Could you please do this last change ?

thanks !

Comment 26

[Leo]

Hi Julien,

I will check it and upload the patch with Vivien's proposition.

Thanks,

Comment 27

[Leo]

@Julienw
Hi,

Added the timeout gaurd as per the suggestion.
Tested on master code and squash is done.
Please check it once and provide comments.

Thanks

Comment 28

[Julien Wajsberg [:julienw]]

Comment on attachment 738456 [details]
Pointer to pull request https://github.com/mozilla-b2g/gaia/pull/9236

r=me thanks

I'll push it to our repository.

Comment 29

[Julien Wajsberg [:julienw]]

master: d45a008dc23fbb89a1276e0afa73cf157804a06a

please provide a patch for the 1.0.1 branch :)

Comment 30

[Leo]

Attachment: Patch for v1.0.1 code

Hi Julien,

Please check the patch for v1.0.1 code.
Please review and provide comments.

Thanks,
Leo

Comment 31

[James Lal [:lightsofapollo]]

landed in v1.0.1: f13d0f7d94e7fcefcaf1c0e84911aafe88a7ea71.


Which patch needs to be uplifted to v1 train?

Comment 32

[Julien Wajsberg [:julienw]]

James: the master commit d45a008dc23fbb89a1276e0afa73cf157804a06a

It should be cherry picked without a problem.

Comment 33

[James Lal [:lightsofapollo]]

=( does not apply to v1-train anymore.

Comment 34

[Julien Wajsberg [:julienw]]

v1-train: 09fbd40a706548a062f5a1304784bac6c4c26641

Comment 35

[Julien Wajsberg [:julienw]]

Comment on attachment 739515 [details] [diff] [review]
Patch for v1.0.1 code

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

so it landed without a review, but it looked good nevertheless.

Comment 36

[pallavi]

Issue is not reproducing. During the phone call opening the received message and clicking on the back key takes you back to the respective page, thus it works.

Verified on Inari Build ID: 20130429070204
Kernel Date: Feb 21
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/45aa5ba0ed53
Gaia: cf2d4136f0ebc66039637fdbeb72ed184dfbc0f2

Comment 37

[Angela Hubenya]

The user is able to select the back key in v1 as well. Verifying as Fixed.

Unagi 
Build ID: 20130501070205
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/765d296cff66
Gaia: e420d71c9528786621f176fb4ce67d291e0a530e