1091483 - Unable to show stk dialog (e.g. user confirmation dialog) when foreground app is Callscreen

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect, P1)

Description

[Edgar Chen [:edgar]]

Found this in emulator (I think the result in real device is the same)

Step:
1. make a MO call.
2. foreground app is Callscreen.
3. send console command `stk pdu d028810301100082028183850c434f4e4649524d4154494f4e8609911032042143651c2c850443414c4c` to generate setupcall proactive command.
5. no user confirmation dialog pop up.

Comment 1

[Edgar Chen [:edgar]]

Hi Fernando, could you help to comment about this behavior? Thank you in advance.

Comment 2

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Manu, can you take a look?
I suspect that this is a z-index issue ...

Thank you

Comment 4

[Manuel Casas Barrado [:mancas]]

Attachment: Patch v2.0m

You're right Fernando,the problem was a z-index issue

Comment 5

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8514839 [details] [review]
Patch v2.0m

I'm not system peer ... so I only can give you the feedback.
f+ with nit

Edgar, can you check if the patch solves the issue in your tests?

Comment 6

[Edgar Chen [:edgar]]

YEs(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #5)
> Comment on attachment 8514839 [details] [review]
> Patch v2.0m
> 
> I'm not system peer ... so I only can give you the feedback.
> f+ with nit
> 
> Edgar, can you check if the patch solves the issue in your tests?

It works good, I can see stk dailog pop up after applying this patch, thank you.

Comment 7

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

Comment on attachment 8514839 [details] [review]
Patch v2.0m

Alive is the better person to review this patch (changing z-index)

Comment 8

[Manuel Casas Barrado [:mancas]]

Attachment: Patch v2.0

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): No regression
[User impact] if declined: The user will be unable to attend the STK message if is calling with the device.
[Testing completed]: No need, just a CSS change
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: No

Comment 9

[Manuel Casas Barrado [:mancas]]

Attachment: Patch v2.1

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): No regression
[User impact] if declined: The user will be unable to attend the STK message if is calling with the device.
[Testing completed]: No need, just a CSS change
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]:No

Comment 10

[Manuel Casas Barrado [:mancas]]

Attachment: Proposed patch for master

Comment 11

[Josh Cheng [:josh]]

Dear Ryan,
Could you help to checkin 2.0M since Kai-Zhen is PTO this week? Thanks!

Comment 12

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/99cd108ce3f9579997b13e43da18f81d18cfe5ab

Comment 13

[Michael Wu [:mwu]]

Backed out for breaking the status bar. Status bar icons disappear when panning down the home screen.

https://github.com/mozilla-b2g/gaia/commit/4684209fbcaf3b3d08fa9dfd0fed9b46163743a4

Comment 14

[bhavana bajaj [:bajaj]]

(In reply to Manuel Casas Barrado [:mancas] from comment #9)
> Created attachment 8516525 [details] [review]
> Patch v2.1
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #): No regression
Is this bug not a regression in 2.0 ? If not, I do not think we would land it at 2.0/2.1 at this point. It also seems to have fallout post master landing which makes me further nervous on considering this unless this is a ship blocker for 2.0/2.1
> [User impact] if declined: The user will be unable to attend the STK message
> if is calling with the device.
> [Testing completed]: No need, just a CSS change
> [Risk to taking this patch] (and alternatives if risky): Low
> [String changes made]:No

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 8516525 [details] [review]
Patch v2.1

Given the breakage on master, way too risky to take on 2.1.

Comment 16

[Manuel Casas Barrado [:mancas]]

Attachment: Proposed patch for master

The STK messages screen should be displayed at the same level as an attention window?

Comment 17

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

Comment on attachment 8520445 [details] [review]
Proposed patch for master

I don't think put at same level fix the problem. Please give it a higher one but at the same time explain why the master patch broke.

I have the same problem: is this a regression? If not then it's a feature request.

Comment 18

[Manuel Casas Barrado [:mancas]]

AFAIK this is not a regression. The problem with the previous patch was that the zindex file has been modified and it seems my changes has been mixed with the new ones.

I've added a new level with higher priority to the STK messages. Please take a look. Sorry for the inconveniences.

Thanks

Comment 19

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

This might work.

Comment 20

[Josh Cheng [:josh]]

Hi Kai-Zhen,
Can you help to land the patch on 2.0M? Thanks!(In reply to Manuel Casas Barrado [:mancas] from comment #18)
> AFAIK this is not a regression. The problem with the previous patch was that
> the zindex file has been modified and it seems my changes has been mixed
> with the new ones.
> 
> I've added a new level with higher priority to the STK messages. Please take
> a look. Sorry for the inconveniences.
> 
> Thanks
Hi Manuel,
If the new patch work for master. Could you please also provide new patch for 2.0M? This is requested by partner. Thank you!

Comment 22

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8514839 [details] [review]
Patch v2.0m

Alive, could you review the patch for version 2.0m when you get a chance?

We can't make the same approach as we did in the main path because the z index.css file, is slightly different in this branch

Thanks

Comment 23

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

Comment on attachment 8514839 [details] [review]
Patch v2.0m

Please take care of the possible regression

Comment 24

[Manuel Casas Barrado [:mancas]]

Checkin-needed for master patch

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/7890288e0f0d6a67b6b4c8945201f613b82f3325

Comment 26

[Kai-Zhen Li [:seinlin][:kli]]

v2.0m: https://github.com/mozilla-b2g/gaia/commit/c8b198c0417bffe71b3b325c3bbd3bad2ab0f9c0

Comment 27

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8516525 [details] [review]
Patch v2.1

Hey Alive, I've fixed the issue in branches 2.0 and 2.1. Could you review it?

Besides, I've noticed that we miss the case in 2.0m and master, when the screen is locked and a STK message with an input is received. In that case, the keyboard is not visible. I've opened a follow-up to solve it instead of make a back-out of the patches.

See Bug 956902

Thanks

Comment 28

[Manuel Casas Barrado [:mancas]]

> See Bug 956902

See Bug 1100250

Comment 29

[bhavana bajaj [:bajaj]]

Comment on attachment 8516524 [details] [review]
Patch v2.0

Does not look like this is a new 2.0 regression and a show stopper for 2.0 and given where we are in the cycle its too late to land there.

Comment 30

[Shine(Leave from Mozilla)]

Dear Edgar,
Could you please provide the method of send console command in step3?

Comment 31

[Edgar Chen [:edgar]]

(In reply to Shine from comment #30)
> Dear Edgar,
> Could you please provide the method of send console command in step3?

You need to connect to emulator with telnet and the default port is `5554`.

$ telnet localhost 5554

then 

$ stk pdu d028810301100082028183850c434f4e4649524d4154494f4e8609911032042143651c2c850443414c4c

Comment 32

[Gregor Wagner [:gwagner]]

Kevin, do we need to fix this for 2.1?

Comment 33

[Josh Cheng [:josh]]

Hi Shawn,
Could you advise whether we should fix this for 2.1/2.1S?
Thanks!

Comment 34

[shawn ku [:sku]]

[Blocking Requested - why for this release]:

We should take this on 2.1/2.1s, even 2.2.
Nominate this as 2.1? since STK information will be blocked w/o this patch.

Comment 35

[shawn ku [:sku]]

Comment on attachment 8516525 [details] [review]
Patch v2.1

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: STK information will be blocked if hit this case w/o the patch. Some important STK action cannot be taken.
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for v2.1.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/b4da4ab43db44201bc632a5a9d80e42a8b97f057

Comment 38

[helloarvin]

Dear all,

 It seems that the patch might give rise to a new issue on the scenario --- "receive an MTC when a stk dialog shown on screen". The incoming call screen will remain in back of STK interface and user fail to pick call unless he clicked to close the stk dialog first. Maybe we should ensure the callscreen can get the highest priority to show on the top.
 Please find more details on https://bugzilla.mozilla.org/show_bug.cgi?id=1125021

 Please help check my above concern.
 Thanks a lot.

Arvin

Comment 39

[shawn ku [:sku]]

Chens, Please help check comment 38 to see how to avoid such case.

Comment 40

[Sherman Chen [:chens] (inactive)]

Hi Arvin, 

Could you elaborate more and what's the connection to bug 1125021? I found bug 1125021 is reported before this patch landed on 2.1, how can this patch regressed before it was landed?

Keeping my ni and will follow up tomorrow.

Comment 41

[helloarvin]

Hi Chens,

I'm sorry that I didn't explain enough.

1) Bug 1125021 should be the same issue as the bug 1091483 and later we can marked those two as duplicated;
2) Per comment1 of bug 1125021, I tried to adjust the z-index of icc-view but this would cause a serious problem: the callscreen will display on the behind of icc-view when the new incoming call arrived.

Hence, I put my doubt on comment 38 to query that if "the callscreen will remain in back of STK interface when a stk dialog shown on screen" or not after the patch merged. And if so, how can we handle the priority conflict between icc-view and callscreen better?

Thank you.

Comment 42

[Sherman Chen [:chens] (inactive)]

I think the question should be what's the priority between stk dialog and call screen, if stk dialog has higher priority then it should be on top of call screen and vice versa, you just can't have both.

So we need to know which use case you need, is stk dialog more important or call screen more important?

Comment 43

[helloarvin]

Hmmm, I agreed with you that we can't have it both ways for the priority conflict between icc-view and callscreen. But user may always feel unreasonable on the other scenario --- "the callscreen will remain in back of STK interface when a stk dialog shown on screen" or conversely.

Ideally, the callscreen and stk dialog can share the equal z-index priority value and latecomers become the first. But only system support on the same z-index priority pages can achieve this approach. Could you please help confirm FFOS support 'latecomers become the first'?

Besides, we might check if the callscreen is shown on the top or not before to display stk dialog. And if so we can close the callscreen temporarily just like the effect pressed home button.

Please help check and feel free to call me if you have any problem.

Comment 44

[Sherman Chen [:chens] (inactive)]

Great idea, but unfortunately there's no such 'late-comer become first' rule. What works in web is called stacking context[1], it will follow dom hierarchy and z-index and there's no way to change it.

IMHO you should really think of which one is more important instead of trying to take them all. The second suggestion is a feature request, and I believe you guys are in performance and stabilization test phase, requesting a new feature here will definitely introduce more risk and instability.

[1] https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Understanding_z_index/The_stacking_context

Comment 45

[helloarvin]

Since we cannot have the both under the scenario, I would prefer to give the higher priority to stk dialog because we'd have to make response to the stk dialog during active call under some special cases.
And meanwhile, user can click to close the stk dialog and then answer the incoming call. Although this is  somewhat inconvenient for user, it usually indeed belongs to corner case.

So let's keep the current state of higher priority of stk dialog.

Thanks for chens' professional explanation.

Comment 47

[Manuel Casas Barrado [:mancas]]

Clearing my ni

Comment 48

[Kai-Chih Hu [:khu]]

clear ni because it's in 2.1 now.