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

RESOLVED FIXED in 1.1 QE4 (15jul)

Status

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: leo.bugzilla.gaia, Assigned: leo.bugzilla.gaia)

Tracking

({late-l10n})

unspecified
1.1 QE4 (15jul)
ARM
Gonk (Firefox OS)
late-l10n
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed)

Details

(Whiteboard: [LeoVB+])

Attachments

(3 attachments, 1 obsolete attachment)

177 bytes, text/html
daleharvey
: review+
Details
14.80 KB, image/png
Details
1.32 KB, patch
vingtetun
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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
(Assignee)

Comment 1

6 years ago
Created attachment 776154 [details]
patch
Attachment #776154 - Flags: review?(arthur.chen)
(Assignee)

Updated

6 years ago
blocking-b2g: --- → leo+
Target Milestone: --- → 1.1 QE4 (15jul)
(Assignee)

Updated

6 years ago
OS: Windows 7 → Gonk (Firefox OS)
Hardware: x86 → ARM
Comment on attachment 776154 [details]
patch

Reassign the request to the author of CB.
Attachment #776154 - Flags: review?(arthur.chen) → review?(dale)
Comment on attachment 776154 [details]
patch

Code wise this is good
Attachment #776154 - Flags: review?(dale) → review+
Merged in: https://github.com/mozilla-b2g/gaia/commit/9c75c6100df2f70c30a68518ae9a5399b31537c1
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 5

6 years ago
Please land to v1-train also
status-b2g18: --- → affected
(Assignee)

Comment 6

6 years ago
Created attachment 778430 [details] [diff] [review]
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.
Attachment #778430 - Flags: review?(dale)
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(pdolanjski)
Resolution: FIXED → ---
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)
needinfo for the screenshot requests
Flags: needinfo?(leo.bugzilla.gaia)
(Assignee)

Comment 10

6 years ago
Created attachment 778809 [details]
screenshot_alert_title

Screenshot
STR:
Message - Attach - Camera - video recording until excess the limit size
Flags: needinfo?(leo.bugzilla.gaia)
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
Attachment #778430 - Flags: review?(dale) → review?(anygregor)
Comment on attachment 778430 [details] [diff] [review]
modal_dialog.patch

I am not deciding about blockers for 1.1.
Attachment #778430 - Flags: review?(anygregor) → review?(21)
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);
>   },
>
Attachment #778430 - Flags: review?(21) → review-
(Assignee)

Comment 14

6 years ago
Created attachment 782851 [details] [diff] [review]
modal_dialog_v1.1.patch

I has uploaded the patch again regarding your comments.
Please review again.
Attachment #778430 - Attachment is obsolete: true
Attachment #782851 - Flags: review?(21)
(Assignee)

Updated

6 years ago
Whiteboard: [LeoVB+]
(Assignee)

Updated

5 years ago
Blocks: 890151
Is there a patch available for this bug?
blocking-b2g: leo+ → leo?
Flags: needinfo?(21)
Moving to leo+ as it is Leo VB+
blocking-b2g: leo? → leo+
(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.
Flags: needinfo?(pdolanjski) → needinfo?(wmathanaraj)
(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?
Flags: needinfo?(21)
Vivien,

I meant to ask why the patch hadn't landed on b2g18 yet? Sorry about the confusion.
Flags: needinfo?(21)
(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 :)
Flags: needinfo?(21)
Keywords: checkin-needed
John, can you please assist with the v1-train uplift? :)
Flags: needinfo?(jhford)
Vivien

John is on PTO today/tomorrow. Please help with landing this patch on 18.
Flags: needinfo?(21)
[v1-train 2712456] Merge pull request #10992 from leob2g/Bug_894196_CB_msg_title
status-b2g18-v1.0.1: --- → fixed
Flags: needinfo?(jhford)
Flags: needinfo?(21)
Note that bugs that are to be uplifted should be marked as RESOLVED->FIXED or VERIFIED->FIXED
v1.1.0hd: 27124565251e6e7cbf8c9772cc9eeee0aabd8c48
status-b2g-v1.1hd: --- → fixed
So I guess comment 7 was OK after all...
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
status-b2g18: affected → fixed
status-b2g18-v1.0.1: fixed → ---
Keywords: checkin-needed
Resolution: --- → FIXED
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.
status-b2g18: fixed → affected
status-b2g-v1.1hd: fixed → affected
v1-train: 167cf7974ac2c95eb862da72aa75f24af4437f5c
status-b2g18: affected → fixed
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.
Keywords: late-l10n
v1.1.0hd: 167cf7974ac2c95eb862da72aa75f24af4437f5c
status-b2g-v1.1hd: affected → fixed
Flags: needinfo?(wmathanaraj)

Updated

5 years ago
Assignee: nobody → leo.bugzilla.gaia
You need to log in before you can comment on or make changes to this bug.