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

RESOLVED FIXED in Firefox OS v2.0M

Status

Firefox OS
Gaia::System
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: edgar, Assigned: mancas)

Tracking

(Blocks: 1 bug)

unspecified
2.1 S9 (21Nov)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.1+, b2g-v2.0 wontfix, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
(Reporter)

Updated

3 years ago
See Also: → bug 1089440
(Reporter)

Comment 1

3 years ago
Hi Fernando, could you help to comment about this behavior? Thank you in advance.
Flags: needinfo?(frsela)

Updated

3 years ago
blocking-b2g: --- → 2.0M?

Updated

3 years ago
Blocks: 1054172
Manu, can you take a look?
I suspect that this is a z-index issue ...

Thank you
Flags: needinfo?(frsela) → needinfo?(b.mcb)
(Assignee)

Updated

3 years ago
Assignee: nobody → b.mcb
Flags: needinfo?(b.mcb)

Updated

3 years ago
Duplicate of this bug: 1089440

Updated

3 years ago
blocking-b2g: 2.0M? → 2.0M+
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → affected
(Assignee)

Comment 4

3 years ago
Created attachment 8514839 [details] [review]
Patch v2.0m

You're right Fernando,the problem was a z-index issue
Attachment #8514839 - Flags: review?(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?
Flags: needinfo?(echen)
Attachment #8514839 - Flags: review?(timdream)
Attachment #8514839 - Flags: review?(frsela)
Attachment #8514839 - Flags: feedback+
(Reporter)

Comment 6

3 years ago
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.
Flags: needinfo?(echen)
Comment on attachment 8514839 [details] [review]
Patch v2.0m

Alive is the better person to review this patch (changing z-index)
Attachment #8514839 - Flags: review?(timdream) → review?(alive)
Attachment #8514839 - Flags: review?(alive) → review+

Updated

3 years ago
Blocks: 1080337
(Assignee)

Comment 8

3 years ago
Created attachment 8516524 [details] [review]
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
Attachment #8516524 - Flags: approval-gaia-v2.0?
(Assignee)

Comment 9

3 years ago
Created attachment 8516525 [details] [review]
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
Attachment #8516525 - Flags: approval-gaia-v2.1?
(Assignee)

Comment 10

3 years ago
Created attachment 8516526 [details] [review]
Proposed patch for master
(Reporter)

Updated

3 years ago
See Also: → bug 1090769

Comment 11

3 years ago
Dear Ryan,
Could you help to checkin 2.0M since Kai-Zhen is PTO this week? Thanks!
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/commit/99cd108ce3f9579997b13e43da18f81d18cfe5ab
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S8 (7Nov)

Comment 13

3 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 1095765
Depends on: 1095657
(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

Updated

3 years ago
Flags: needinfo?(b.mcb)
Comment on attachment 8516525 [details] [review]
Patch v2.1

Given the breakage on master, way too risky to take on 2.1.
Attachment #8516525 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1-
(Assignee)

Comment 16

3 years ago
Created attachment 8520445 [details] [review]
Proposed patch for master

The STK messages screen should be displayed at the same level as an attention window?
Attachment #8516526 - Attachment is obsolete: true
Flags: needinfo?(b.mcb)
Attachment #8520445 - Flags: review?(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.
Attachment #8520445 - Flags: review?(alive)
(Assignee)

Comment 18

3 years ago
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
Flags: needinfo?(alive)
This might work.
Flags: needinfo?(alive)
(Assignee)

Updated

3 years ago
Attachment #8520445 - Flags: review?(alive)

Comment 20

3 years ago
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!
Flags: needinfo?(b.mcb)

Updated

3 years ago
Blocks: 1080481
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1090769
Attachment #8520445 - Flags: review?(alive) → review+
(Assignee)

Comment 22

3 years ago
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
Flags: needinfo?(b.mcb)
Attachment #8514839 - Flags: review?(alive)
Attachment #8514839 - Flags: review+
Attachment #8514839 - Flags: feedback+
Comment on attachment 8514839 [details] [review]
Patch v2.0m

Please take care of the possible regression
Attachment #8514839 - Flags: review?(alive) → review+
(Assignee)

Comment 24

3 years ago
Checkin-needed for master patch
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/7890288e0f0d6a67b6b4c8945201f613b82f3325
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-b2g-v2.2: affected → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: 2.1 S8 (7Nov) → 2.1 S9 (21Nov)
v2.0m: https://github.com/mozilla-b2g/gaia/commit/c8b198c0417bffe71b3b325c3bbd3bad2ab0f9c0
status-b2g-v2.0M: affected → fixed
Blocks: 1100201
(Assignee)

Updated

3 years ago
Depends on: 1100250
(Assignee)

Updated

3 years ago
Attachment #8516524 - Flags: review?(alive)
(Assignee)

Comment 27

3 years ago
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
Attachment #8516525 - Flags: approval-gaia-v2.1- → review?(alive)
(Assignee)

Comment 28

3 years ago
> See Bug 956902

See Bug 1100250
Attachment #8516524 - Flags: review?(alive) → review+
Attachment #8516525 - Flags: review?(alive) → review+
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.
Attachment #8516524 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0-
Depends on: 1103843

Updated

3 years ago
Blocks: 1107999

Updated

3 years ago
Priority: -- → P1

Updated

3 years ago
No longer blocks: 1107999
Dear Edgar,
Could you please provide the method of send console command in step3?
Flags: needinfo?(echen)
(Reporter)

Comment 31

3 years ago
(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
Flags: needinfo?(echen)
Kevin, do we need to fix this for 2.1?
Flags: needinfo?(khu)

Updated

3 years ago
status-b2g-v2.1S: --- → affected
status-b2g-master: --- → fixed

Comment 33

3 years ago
Hi Shawn,
Could you advise whether we should fix this for 2.1/2.1S?
Thanks!
Flags: needinfo?(sku)

Comment 34

3 years ago
[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.
blocking-b2g: 2.0M+ → 2.1?
Flags: needinfo?(sku)

Comment 35

3 years ago
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:
Attachment #8516525 - Flags: approval-mozilla-b2g37?

Updated

3 years ago
blocking-b2g: 2.1? → 2.1+

Updated

3 years ago
Attachment #8516525 - Flags: approval-mozilla-b2g37?
Attachment #8516525 - Flags: approval-gaia-v2.2+
Attachment #8516525 - Flags: approval-gaia-v2.1+
Needs rebasing for v2.1.
status-b2g-v2.0: affected → wontfix
status-b2g-master: fixed → ---
Flags: needinfo?(b.mcb)
v2.1: https://github.com/mozilla-b2g/gaia/commit/b4da4ab43db44201bc632a5a9d80e42a8b97f057
status-b2g-v2.1: affected → fixed
Flags: needinfo?(b.mcb)

Comment 38

3 years ago
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
Flags: needinfo?(sku)
Flags: needinfo?(b.mcb)

Comment 39

3 years ago
Chens, Please help check comment 38 to see how to avoid such case.
Flags: needinfo?(sku) → needinfo?(chens)
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.
Flags: needinfo?(arvin.zhang)

Comment 41

3 years ago
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.
Flags: needinfo?(arvin.zhang)
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?
Flags: needinfo?(chens) → needinfo?(arvin.zhang)

Comment 43

3 years ago
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.
Flags: needinfo?(arvin.zhang) → needinfo?(chens)

Updated

3 years ago
status-b2g-v2.1S: affected → fixed
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
Flags: needinfo?(chens)

Comment 45

3 years ago
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.

Updated

3 years ago
Duplicate of this bug: 1125021
(Assignee)

Comment 47

3 years ago
Clearing my ni
Flags: needinfo?(b.mcb)

Comment 48

3 years ago
clear ni because it's in 2.1 now.
Flags: needinfo?(khu)
You need to log in before you can comment on or make changes to this bug.