894196 - [CB] Disapper the title background alert and need to display channel info

Status: RESOLVED FIXED

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

Description

[Leo]

1. Title : Disapper the title background alert and need to display channel info
2. Precondition : CB turn on
3. Tester's Action : 
[Case 1]
 1) Receive several CB message in homescreen
 2) Check the title and Close 1st CB message(foreground alert)
 3) Check the next CB message.

[Case 2]
Need to display the channel info of CB message

4. Detailed Symptom : 
[Case 1]
 Disapper the title.

[Case 2]
 No dispaly the channel info of CB message

5. Expected : 
[Case 1]
 Display the title in all alert

[Case 2]
 Display the channel info.

6. Reproducibility: Y
   - Frequency Rate : 100%
7.Gaia Master/v1-train : Reproduced
8. Version Info
Gaia : 0d5a9a7577f16b6a72a982148c6f509ee1714ea2
Gecko : 499c1f8ea7ad0cdaa7086214278e2944b8a2ea33

Comment 1

[Leo]

Attachment: patch

Comment 2

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

Comment on attachment 776154 [details]
patch

Reassign the request to the author of CB.

Comment 3

[Dale Harvey (:daleharvey)]

Comment on attachment 776154 [details]
patch

Code wise this is good

Comment 4

[Dale Harvey (:daleharvey)]

Merged in: https://github.com/mozilla-b2g/gaia/commit/9c75c6100df2f70c30a68518ae9a5399b31537c1

Comment 5

[hyuna.cho]

Please land to v1-train also

Comment 6

[Leo]

Attachment: modal_dialog.patch

I found the regression.

When show Modal dialog using alert()
- There is no title

When show Modal dialog using showWithPseudoEvent()
- Set the title

So I update the patch. Please review this.

Comment 7

[Dale Harvey (:daleharvey)]

So I want to needinfo Peter on this as he is the Product manager for the system frontend, which I believe this falls under

We really shouldnt be landing adhoc patches that change the functionality here without getting product approval just because I wrote the original code, this is especially bad since it is being uplifted. We may have requirements from other partners, legal requirements and changes could have unintended consequences.

Leo could you please upload screenshots of both the before and after state of the UX that this (and the previous patch) affects so product can approve this one, in future we should probably figure out a proper workflow to receive these changes.

Apologies I shouldnt have landed the original patch before doing this.

Comment 8

[Dale Harvey (:daleharvey)]

Just to clarify, I am happy doing future reviews on the code for this stuff, but I want to make sure what we are landing is what we actually want and I am not the one who should be making decisions regarding external requirements (and UX)

Comment 9

[Dale Harvey (:daleharvey)]

needinfo for the screenshot requests

Comment 10

[Leo]

Attachment: screenshot_alert_title

Screenshot
STR:
Message - Attach - Camera - video recording until excess the limit size

Comment 11

[Dale Harvey (:daleharvey)]

Comment on attachment 778430 [details] [diff] [review]
modal_dialog.patch

Havent heard back from Peter so forwarding the review to someone in the Gaia System Frontend team. 

Gregor the cell broadcast implementation was added in a fairly ad hoc way, going forward its going to need a PM who understands its implications + partners to be involved in changes. Also the patch has changes to modal_dialogs which could effect stuff I dont know about.

Apologies for the delay Leo

Comment 12

[Gregor Wagner [:gwagner]]

Comment on attachment 778430 [details] [diff] [review]
modal_dialog.patch

I am not deciding about blockers for 1.1.

Comment 13

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

Comment on attachment 778430 [details] [diff] [review]
modal_dialog.patch

>diff --git a/apps/system/js/modal_dialog.js b/apps/system/js/modal_dialog.js
>index 2604d74..d27f03a 100644
>--- a/apps/system/js/modal_dialog.js
>+++ b/apps/system/js/modal_dialog.js
>@@ -206,7 +206,7 @@ var ModalDialog = {
>       case 'alert':
>         elements.alertMessage.innerHTML = message;
>         elements.alert.classList.add('visible');
>-        this.setTitle('alert', title);
>+        this.setTitle('alert', '');

Instead of removing the |title| value maybe what you want is to define a value for title before the switch.
For example:

 // Display a title for System Modal Dialogs.
 var title = (this.currentOrigin == 'system' && title) ? title : '';

And goes with that.

>         elements.alertOk.textContent = evt.yesText ? evt.yesText : _('ok');
>         elements.alert.focus();
>         break;
>@@ -239,6 +239,10 @@ var ModalDialog = {
>         break;
>     }
> 
>+    // display the title in case of 'system' alert using showWithPseudoEvent() instaed of alert()
>+    if(this.currentOrigin =='system')
>+        this.setTitle('alert', title);
>+
>     this.setHeight(window.innerHeight - StatusBar.height);
>   },
>

Comment 14

[Leo]

Attachment: modal_dialog_v1.1.patch

I has uploaded the patch again regarding your comments.
Please review again.

Comment 15

[Preeti Raghunath(:Preeti)]

Is there a patch available for this bug?

Comment 16

[Preeti Raghunath(:Preeti)]

Moving to leo+ as it is Leo VB+

Comment 17

[Peter Dolanjski [:pdol]]

(In reply to Dale Harvey (:daleharvey) from comment #7)
> We really shouldnt be landing adhoc patches that change the functionality
> here without getting product approval just because I wrote the original
> code, this is especially bad since it is being uplifted. We may have
> requirements from other partners, legal requirements and changes could have
> unintended consequences.

Thanks Dale.  Sorry for the delay in response here.  
I'm adding Wilfred as he actually did the CB requirements and has a better view than I on this.

Comment 18

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

(In reply to Preeti Raghunath(:Preeti) from comment #15)
> Is there a patch available for this bug?

Sorry Preeti I'm not sure to understand the question?

Comment 19

[Preeti Raghunath(:Preeti)]

Vivien,

I meant to ask why the patch hadn't landed on b2g18 yet? Sorry about the confusion.

Comment 20

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

(In reply to Preeti Raghunath(:Preeti) from comment #19)
> Vivien,
> 
> I meant to ask why the patch hadn't landed on b2g18 yet? Sorry about the
> confusion.

Likely because it needs to have checkin-needed in the keywords of the bug :)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

John, can you please assist with the v1-train uplift? :)

Comment 22

[Preeti Raghunath(:Preeti)]

Vivien

John is on PTO today/tomorrow. Please help with landing this patch on 18.

Comment 23

[John Ford [:jhford] CET/CEST Berlin Time]

[v1-train 2712456] Merge pull request #10992 from leob2g/Bug_894196_CB_msg_title

Comment 24

[John Ford [:jhford] CET/CEST Berlin Time]

Note that bugs that are to be uplifted should be marked as RESOLVED->FIXED or VERIFIED->FIXED

Comment 25

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

v1.1.0hd: 27124565251e6e7cbf8c9772cc9eeee0aabd8c48

Comment 26

[Ryan VanderMeulen [:RyanVM]]

So I guess comment 7 was OK after all...

Comment 27

[John Ford [:jhford] CET/CEST Berlin Time]

This was backed out with 4764cbcbdd39b68fc5c184b0eb40e9f43f0dde55 to see if it was the cause for breaking b2g18.  While the backout hasn't been pushed to v1.1.0hd train, it will be when the next merge happens.

Comment 28

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: 167cf7974ac2c95eb862da72aa75f24af4437f5c

Comment 29

[Axel Hecht [:Pike]]

The landing on 1.1, https://github.com/mozilla-b2g/gaia/commit/167cf7974ac2c95eb862da72aa75f24af4437f5c, introduced a new string.

What are the expectations on that string?

Given that we'll switch all kinds of repos on Monday, this is even harder to fix.

Comment 30

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

v1.1.0hd: 167cf7974ac2c95eb862da72aa75f24af4437f5c