Apply Banner fix for Leanplum SDK

RESOLVED FIXED in Firefox 59

Status

()

P1
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: cnevinchen, Assigned: cnevinchen, NeedInfo)

Tracking

56 Branch
Firefox 59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [FNC][SPT59.1][MVP] )

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
After testing this patch. I'v found that most of the Banner bugs are fixed( click through, cancel not working ...etc)
The last problem is that Photon colored the status bar white. Using Banner will make the status bar black . And after banners disappear it change back to white again. 

I guess there should be some customization at our end.
(Assignee)

Updated

a year ago
Priority: -- → P2
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8922724 - Attachment is obsolete: true
(Assignee)

Comment 6

a year ago
I don't see any change per data we sent to Leanplum.
Most of the change is Leanplum's implementation in Banner, cache inhancement, added utility classes,bug fix ,and PushNotification Channel API upgrade (to API 26) so data review is not needed.

Roughly tested with push notification and in-app diaglog, they are working. But banners needs some tweaking to match our photon theme.

Since underlying API has changed, all related behavior should be verified again. So mutliple phase QA- debug cycle is expected.
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Summary: Update Leanplum SDK for Banner → Apply Banner fix for Leanplum SDK
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

11 months ago
Hi Braynt
Just to let you know that the changes here can't let us:
1. Change the width / height of the banner cause it's a new API (we didn't update entire SDK)
2. Customize the look and feel of the Banner
Flags: needinfo?(bmao)

Comment 11

11 months ago
mozreview-review
Comment on attachment 8928083 [details]
Bug 1390454 - Make Banner dialog status bar transparent.

https://reviewboard.mozilla.org/r/199320/#review204756
Attachment #8928083 - Flags: review?(max) → review+

Comment 12

11 months ago
mozreview-review
Comment on attachment 8897349 [details]
Bug 1390454 - Apply Banner fix for Leanplum SDK.

https://reviewboard.mozilla.org/r/168662/#review204758
Attachment #8897349 - Flags: review?(max) → review+

Updated

11 months ago
Whiteboard: [FNC][SPT59.1][MVP]

Comment 13

11 months ago
Pushed by nechen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8397dc953ce6
Make Banner dialog status bar transparent. r=maliu
https://hg.mozilla.org/integration/autoland/rev/356df0634637
Apply Banner fix for Leanplum SDK. r=maliu
(In reply to Nevin Chen [:nechen] from comment #10)
> Hi Braynt
> Just to let you know that the changes here can't let us:
> 1. Change the width / height of the banner cause it's a new API (we didn't
> update entire SDK)
> 2. Customize the look and feel of the Banner
so that means we can't change the color and the height like the mock do here? https://mozilla.invisionapp.com/share/YHEHZ5CMA#/screens/264324424
Flags: needinfo?(bmao)

Comment 15

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8397dc953ce6
https://hg.mozilla.org/mozilla-central/rev/356df0634637
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59

Updated

10 months ago
Depends on: 1429386
Susheel and Mike, would you mind confirming if the banner implementation made it into Fennec (as far as I know jcollings mentioned it's still something that needs to be implemented).

Product would like to shift this to a P1.
Flags: needinfo?(sdaswani)
Flags: needinfo?(michael.l.comella)
Status: RESOLVED → REOPENED
Priority: P2 → P1
Resolution: FIXED → ---
Jean, can you please send a banner to my Firefox Nightly device and see if it meets the implementation specs you require? Gracias!
Flags: needinfo?(sdaswani) → needinfo?(jcollings)

Comment 18

8 months ago
(In reply to Susheel Daswani from comment #17)
> Jean, can you please send a banner to my Firefox Nightly device and see if
> it meets the implementation specs you require? Gracias!

I just sent you a banner test (will show when you open the app) and a push notification test. Can you please confirm if you see them (maybe take screenshots) and if it works as it should? Thanks!
Flags: needinfo?(jcollings)
Here is what I got. :) https://photos.app.goo.gl/Zy08qECBQNTSnR8u2

How does it look Jean?

FYI this is version 60.0a1.
Flags: needinfo?(jcollings)

Comment 20

8 months ago
(In reply to Susheel Daswani from comment #19)
> Here is what I got. :) https://photos.app.goo.gl/Zy08qECBQNTSnR8u2
> 
> How does it look Jean?
> 
> FYI this is version 60.0a1.

Looks great :) Thanks for testing and the screenshots.
Flags: needinfo?(jcollings)

Updated

8 months ago
Status: REOPENED → RESOLVED
Last Resolved: 11 months ago8 months ago
Resolution: --- → FIXED
Barbara please put this on your master list.
Status: RESOLVED → REOPENED
Flags: needinfo?(michael.l.comella) → needinfo?(bbermes)
Resolution: FIXED → ---
Jean, this needs to be fixed for 60, right? Also, what exactly needs to be fixed?
Flags: needinfo?(jcollings)

Comment 23

8 months ago
Another one that the product team wants uplifted in 60, tracked as blocking.
status-firefox60: --- → affected
tracking-firefox60: --- → blocking

Comment 24

8 months ago
(In reply to Susheel Daswani from comment #22)
> Jean, this needs to be fixed for 60, right? Also, what exactly needs to be
> fixed?

I thought we did not have the ability to change the color or height of the banner but we can (just tested it). However, I noticed that when I hit the banner to a few different deeplinks, nothing happens - the banner disappears but does not take me to where I am supposed to go. I tried it with a generic url like https://mozilla.org and that works so it must be the deeplinks.

Can we QA if these links are right?

For Fx58 and higher:
firefox://default_browser?uid={{User ID}}
firefox://sign_up?uid={{User Id}}
firefox://preferences_search?uid={{User Id}}
firefox://save_as_pdf?uid={{User Id}}
firefox://bookmark_list?uid={{User Id}}
firefox://history_list?uid={{User Id}}
firefox://preferences?uid={{User Id}}
firefox://preferences_privacy?uid={{User Id}}
firefox://preferences_notifications?uid={{User Id}}
firefox://preferences_accessibility?uid={{User Id}}
firefox://preferences_accessibility?uid={{User Id}}
Flags: needinfo?(jcollings)

Comment 25

8 months ago
I reviewed the deeplinks and they are now working if you input capitalized "ID"

This bug is resolved/fixed.
Status: REOPENED → RESOLVED
Last Resolved: 8 months ago8 months ago
Resolution: --- → FIXED
Very confused as to the status here as this was reopened several times without explanation, but from comment 25 I understand that there's no work to do here for 60 so clearing corresponding tracking flags.
status-firefox60: affected → ---
tracking-firefox60: blocking → ---
(In reply to Julien Cristau [:jcristau] from comment #26)
> Very confused as to the status here as this was reopened several times
> without explanation, but from comment 25 I understand that there's no work
> to do here for 60 so clearing corresponding tracking flags.

Sorry Julien, there was a miscommunication about how to make this work, so it was re-opened mistakenly. It's all good, thanks for removing those flags.
You need to log in before you can comment on or make changes to this bug.