1117185 - [b2g-desktop] [gaia] sms settings back button responding but not working

Status: RESOLVED WONTFIX

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

Description

[Jamie Charlton (:Jamie_)]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20141215134203

Steps to reproduce:

WebIDE 2.2 
build identifier 20150102010205


1.) open sms
2.) enter sms settings
3.) try to select the back button


Actual results:

as you select it, it reacts like its being pressed but it does nothing. The only way to get back to the main sms menu is to close it via task manager and re enter sms


Expected results:

selected the back button, responed with noramal actions and the take you back to the main sms page

Comment 1

[Dietrich Ayala (:dietrich)]

Confirmed in 2.2 simulator.

Comment 2

[Dietrich Ayala (:dietrich)]

In 2.1 simulator, it just opens Settings app at main screen. So not sure if a regression or just new and different ways of being broken.

Comment 3

[Jamie Charlton (:Jamie_)]

It was happening in 2.2 in a lot of the buttons till a patch was made, later found also in 2.1 in a lot of the settings and back buttons. was uplifted to 2.1 from 2.2. seems to be happening a little different though this was the bug Bug 1112414

Comment 4

[Parul Mathur [:pragmatic] - out of office - please ni? [:bajaj] instead]

:jryans, would you like to comment?

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I am unlikely to have an opinion on these issues with specific Gaia apps.  Checking with someone on the team making the app seems like a much better approach.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 27354

Arthur, I don't really understand this code and "just made it work".
But see comment 0, messaging activity is broken on b2g desktop.
`_currentNavigation` is null when we click on back button,
so that, _shallCloseActivity(panelId) returns false and we don't dismiss the activity from here:
https://github.com/ochameau/gaia/blob/settings-back/apps/settings/js/modules/settings_service.js#L315

Please consider addressing this issue yourself if this patch is incorrect.

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8548273 [details] [review]
Pull request 27354

EJ, could you help review the patch? Thanks!

Comment 8

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

hey Alexandre,

is it possible to reproduce this bug in latest build ? I have tried several times to reproduce this but with no luck. Based on you guys' comments above, it seems this would only happen in simulator or (?)

I want to know more about the syndrome if possible. 

Thanks :)

Comment 9

[Alexandre Poirot [:ochameau]]

It is not simulator specific, it happens anywhere on desktop, so you can reproduce that using b2g-desktop.

Comment 10

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

How about the real device ? I can't make this happen on my flame, so is this b2g-desktop specific problem ?? 

More details would be helpful, thanks !!

Comment 11

[Jamie Charlton (:Jamie_)]

I have an odd thought on this one, as you go to enter the settings for sms its not the same as entering other things. when you go to you go through what ill call a "prompt screen" and the back button runs the same commands as for ones that don’t have a "prompt screen" so the process may be different and require its own unique set up for this action so its doesn’t know what to do.

Comment 12

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

This doesn't seem to occur on device.  It looks like a simulator/desktop bug.

Comment 13

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

I noticed that in desktop client, you can't even go into message panel from Settings app.

So, is this bug valid in such scenario ??

Comment 14

[Alexandre Poirot [:ochameau]]

Yes, that doesn't happen on device, but that doesn't necessarily mean it is b2g-desktop fault.
In most cases there is something wrong on gaia. Applications should do their best to also work on desktop, even if some APIs are missing!

The only detail I can give is please look at step to reproduce in comment 0, you can easily reproduce it by using b2g desktop. See comment 6, where I explain what is wrong in the codebase.

Comment 15

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

I know what you mean here, but I think for b2g-desktop, there must be some "basic functionalities" support to make sure our application can work well as expected. Otherwise, in our codebase, there would be lots of 
messy codes like this :

```
if (a && a.b && a.b.c) {
  // do something
}
else {
  // early return
}
```

and I don't think this would be healthy for our codebase. As a temporary solution, I think your patch is okay and did workaround the problem. If possible, can you help put some comments above the code you changed ?

Comments can be something like this :

```
// Because on b2g desktop client, we can't get needed SIM slot instance, so `SIMSlotManager.get` would break the code sequence to make us unable to register `_currentNavigation` successfully. So, we should handle this special case here to make webActivity can exit successfully.
```

THanks !!

Comment 16

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8548273 [details] [review]
Pull request 27354

r+ iff the comments are noted on our codebase.

Thanks !! :)

Comment 17

[Alexandre Poirot [:ochameau]]

(In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #15)
> I know what you mean here, but I think for b2g-desktop, there must be some
> "basic functionalities" support to make sure our application can work well
> as expected. Otherwise, in our codebase, there would be lots of 
> messy codes like this :
> 
> ```
> if (a && a.b && a.b.c) {
>   // do something
> }
> else {
>   // early return
> }
> ```
> 
> and I don't think this would be healthy for our codebase. As a temporary
> solution, I think your patch is okay and did workaround the problem. If
> possible, can you help put some comments above the code you changed ?
> 
> Comments can be something like this :
> 
> ```
> // Because on b2g desktop client, we can't get needed SIM slot instance, so
> `SIMSlotManager.get` would break the code sequence to make us unable to
> register `_currentNavigation` successfully. So, we should handle this
> special case here to make webActivity can exit successfully.
> ```

I can put this comment but it is hard to understand believe we are fixing some issue related to SIM API from some Panel/UI related code :/
Can't we fix that where SIMSlotManager.get fails??
We should gracefully degrade on desktop if the API is not available, that's something Settings app is already doing...

I don't know much about settings, but is it really failing because of this??
https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/panels/messaging/panel.js#L113

Comment 18

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Alexandre Poirot [:ochameau] from comment #17)
> I can put this comment but it is hard to understand believe we are fixing
> some issue related to SIM API from some Panel/UI related code :/

I don't want to do so too either XD. For me this is a hack to be honest.

> Can't we fix that where SIMSlotManager.get fails??
> We should gracefully degrade on desktop if the API is not available, that's
> something Settings app is already doing...
> 

Yes you are right, but I am thinking that if one day in the future there is another panel using webActivity and breaks the calling sequence like this, we are still unable to make the panel clickable to exit. So, in order to protect this, your original patch is totally acceptable. 

No matter how panel breaks, we should always make Settings app able to exit webActivity.

> I don't know much about settings, but is it really failing because of this??
> https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/panels/
> messaging/panel.js#L113

Yes, I did check the code and noticed that `SIMSlotManager.get(cardIndex)` would return `null` here and in this way, we can't make the calling chain work as expected. So for this panel, we can do something more like this :

```
var simSlot = SIMSlotManager.get(cardIndex);
if (simSlot) {
  simSlot.getSmsc(function(result) {
    // do something ...
  });
}
```

In this way, I think we can gracefully degrade on desktop. What do you think ?

Comment 19

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

@Alexandre, 

I think this is what we can do here. First, your original patch is needed, but we can also add something additional to protect simSlot did exist like what I said in previous comment.

And I forgot one thing, can you help to update our tests for this case ? I'll give any advices if needed !

THanks :)