1429386 - [Leanplum] When the banner is displayed, opened menus freeze on screen

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Oana Horvath [:ohorvath]]

Attachment: leanplum.mp4

Device: Samsung Galaxy Tab 3 (Android 7.0)
Build: Nightly 59.0a1 (2017-01-10);

Steps to reproduce:
1. While leanplum is turned on, go to a wikipedia page to trigger the "reader mode" hint.
2. Open the custom menu. 
3. With both the menu and the banner on screen, tap any menu from the custom menu.
4. Or, tap on the page to dismiss the menu.

Expected result:
The menu links should work. 
The menu should be dismissed.

Actual result:
The custom menu (or other menus opened after) freeze on screen and are ignored when tapped. If there's a link behind the menu tapped, it will open that link.

Notes: see the recording attached.

Comment 1

[Nevin Chen(Not active on Bugzilla)]

Please help advise.

Comment 2

[Wesly Huang (EPM)]

P2 looks good to me, since the menu will be normal after dismiss the banner, per current implementation.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

[triage] Bulk edit: product thinks leanplum is high priority: P1 rank 1.

Comment 4

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Product team would like this fixed in 60, tracked as blocking to ensure it's closely monitored.

Comment 5

[Vlad Baicu]

Attachment: Added missing functionalities that exist on a newer Leanplum SDK version

Hey guys, I noticed a issue regarding updating Leanplum SDK 

https://bugzilla.mozilla.org/show_bug.cgi?id=1438716 . 

In the submitted patch I have added the missing functionalities from the latest version of LP from their github repo relevant to this bug and it fixes the issue with the freezed menu.

https://github.com/Leanplum/Leanplum-Android-SDK/blob/10c8146c4a0afa1a72813194c833a8a9799a1c4b/AndroidSDKCore/src/main/java/com/leanplum/messagetemplates/HTMLTemplate.java

In-app message templates have an option in LP dashboard where you can set if you want to dismiss the banner on tap outside that isn't being taken into consideration by our version of LP SDK.

By upgrading to a newer version of LP that contains the changes from within this patch should resolve the bug.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

> In the submitted patch I have added the missing functionalities from the latest version of LP
> from their github repo relevant to this bug and it fixes the issue with the freezed menu.

Do we need anyone to review analytics SDK upgrades, e.g. for privacy violations? I remember there being a very strict review process around including Adjust and reasons why we couldn't just bump the versions.

Comment 7

[:sdaswani]

Vlad, did you just take the code you needed from the new version of the LP SDK that you need to fix the issues, or did you upgrade the whole SDK? We may as well do the latter, so please update the PR to do so.

Frederik, Dan - can one of you review the new attachment to see if there are any security issues upgrading to the new version of the LP SDK? If this is going to take some time let us know, there is some need to get this into 60 by the end of the week, but I understand my request is late so feel free to give us a time frame needed for security review.

Comment 8

[Vlad Baicu]

Andrew is already working on it https://bugzilla.mozilla.org/show_bug.cgi?id=1438716

Comment 9

[Daniel Veditz [:dveditz]]

Is there an existing pi-request for this? We really need that logged before taking on a security review.

Comment 10

[:sdaswani]

I realized that and will be filing one soon. - sorry!!

Comment 11

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8961855 [details] [diff] [review]
Added missing functionalities that exist on a newer Leanplum SDK version

>Index: mobile/android/thirdparty/com/leanplum/messagetemplates/HTMLTemplate.java
>IDEA additional info:
>Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
><+>UTF-8
>===================================================================
>--- mobile/android/thirdparty/com/leanplum/messagetemplates/HTMLTemplate.java	(revision 408766:4f1014eb5039bdfdd7a39fb7785d102df1994a6f)
>+++ mobile/android/thirdparty/com/leanplum/messagetemplates/HTMLTemplate.java	(revision 408766+:4f1014eb5039+)
>@@ -22,9 +22,11 @@
> package com.leanplum.messagetemplates;
> 
> import android.app.Activity;
>+import android.graphics.Point;
> import android.support.annotation.NonNull;
> import android.util.Log;
> import android.view.MotionEvent;
>+import android.widget.Toast;
> 
> import com.leanplum.ActionContext;
> import com.leanplum.Leanplum;
>@@ -50,17 +52,32 @@
> 
>   @Override
>   public boolean dispatchTouchEvent(@NonNull MotionEvent ev) {
>-    if (!htmlOptions.isFullScreen()) {
>-      int height = SizeUtil.dpToPx(Leanplum.getContext(), htmlOptions.getHtmlHeight());
>-      int statusBarHeight = SizeUtil.getStatusBarHeight(Leanplum.getContext());
>-      if (htmlOptions.getHtmlAlign().equals(MessageTemplates.Args.HTML_ALIGN_TOP) && ev.getY()
>-          > height + statusBarHeight ||
>-          htmlOptions.getHtmlAlign().equals(MessageTemplates.Args.HTML_ALIGN_BOTTOM) && ev.getY()
>-              < dialogView.getHeight() + statusBarHeight - height) {
>-        activity.dispatchTouchEvent(ev);
>-      }
>-    }
>-    return super.dispatchTouchEvent(ev);
>+      if (!htmlOptions.isFullScreen()) {
>+          Point size = SizeUtil.getDisplaySize(activity);
>+          int dialogWidth = webView.getWidth();
>+          int left = (size.x - dialogWidth) / 2;
>+          int right = (size.x + dialogWidth) / 2;
>+          int height = SizeUtil.dpToPx(Leanplum.getContext(), htmlOptions.getHtmlHeight());
>+          int statusBarHeight = SizeUtil.getStatusBarHeight(Leanplum.getContext());
>+          int htmlYOffset = htmlOptions.getHtmlYOffset(activity);
>+          int top;
>+          int bottom;
>+          if (MessageTemplates.Args.HTML_ALIGN_BOTTOM.equals(htmlOptions.getHtmlAlign())) {
>+              top = size.y - height - statusBarHeight - htmlYOffset;
>+              bottom = size.y - htmlYOffset - statusBarHeight;
>+          } else {
>+              top = htmlYOffset + statusBarHeight;
>+              bottom = height + statusBarHeight + htmlYOffset;
>+          }
>+
>+          if (ev.getY() < top || ev.getY() > bottom || ev.getX() < left || ev.getX() > right) {
>+              if (htmlOptions.isHtmlTabOutsideToClose()) {
>+                  cancel();
>+              }
>+              activity.dispatchTouchEvent(ev);
>+          }
>+      }
>+      return super.dispatchTouchEvent(ev);
>   }
> 
>   public static void register() {
>Index: mobile/android/thirdparty/com/leanplum/messagetemplates/HTMLOptions.java
>IDEA additional info:
>Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
><+>UTF-8
>===================================================================
>--- mobile/android/thirdparty/com/leanplum/messagetemplates/HTMLOptions.java	(revision 408766:4f1014eb5039bdfdd7a39fb7785d102df1994a6f)
>+++ mobile/android/thirdparty/com/leanplum/messagetemplates/HTMLOptions.java	(revision 408766+:4f1014eb5039+)
>@@ -21,21 +21,25 @@
> 
> package com.leanplum.messagetemplates;
> 
>-import android.text.TextUtils;
>-import android.util.Log;
>+
>+        import android.app.Activity;
>+        import android.graphics.Point;
>+        import android.text.TextUtils;
>+        import android.util.Log;
> 
>-import com.leanplum.ActionArgs;
>-import com.leanplum.ActionContext;
>-import com.leanplum.Leanplum;
>+        import com.leanplum.ActionArgs;
>+        import com.leanplum.ActionContext;
>+        import com.leanplum.Leanplum;
>+        import com.leanplum.utils.SizeUtil;
> 
>-import org.json.JSONException;
>+        import org.json.JSONException;
> 
>-import java.io.BufferedReader;
>-import java.io.File;
>-import java.io.IOException;
>-import java.io.InputStream;
>-import java.io.InputStreamReader;
>-import java.util.Map;
>+        import java.io.BufferedReader;
>+        import java.io.File;
>+        import java.io.IOException;
>+        import java.io.InputStream;
>+        import java.io.InputStreamReader;
>+        import java.util.Map;
> 
// The format is kind of strange here:)
> /**
>  * Options used by {@link HTMLTemplate}.
>@@ -52,6 +56,8 @@
>   private ActionContext actionContext;
>   private String htmlAlign;
>   private int htmlHeight;
>+  private Size htmlYOffset;
>+  private boolean htmlTabOutsideToClose;
> 
>   HTMLOptions(ActionContext context) {
>     this.setActionContext(context);
>@@ -63,6 +69,9 @@
>     this.setTrackActionUrl(context.stringNamed(MessageTemplates.Args.TRACK_ACTION_URL));
>     this.setHtmlAlign(context.stringNamed(MessageTemplates.Args.HTML_ALIGN));
>     this.setHtmlHeight(context.numberNamed(MessageTemplates.Args.HTML_HEIGHT).intValue());
>+    this.setHtmlYOffset(context.stringNamed(MessageTemplates.Args.HTML_Y_OFFSET));
>+    this.setHtmlTabOutsideToClose(context.booleanNamed(
>+            MessageTemplates.Args.HTML_TAP_OUTSIDE_TO_CLOSE));
>   }
> 
>   /**
>@@ -143,6 +152,11 @@
>     return map;
>   }
> 
>+  static class Size {
>+    int value;
>+    String type;
>+  }
>+
>   /**
>    * Get HTML template file.
>    *
>@@ -199,6 +213,59 @@
>     this.htmlAlign = htmlAlign;
>   }
> 
>+  //Gets html y offset in pixels.
>+  int getHtmlYOffset(Activity context) {
>+    int yOffset = 0;
>+    if (context == null) {
>+      return yOffset;
>+    }
>+
>+    if (htmlYOffset != null && !TextUtils.isEmpty(htmlYOffset.type)) {
>+      yOffset = htmlYOffset.value;
>+      if ("%".equals(htmlYOffset.type)) {
>+        Point size = SizeUtil.getDisplaySize(context);
>+        yOffset = (size.y - SizeUtil.getStatusBarHeight(context)) * yOffset / 100;
>+      } else {
>+        yOffset = SizeUtil.dpToPx(context, yOffset);
>+      }
>+    }
>+    return yOffset;
>+  }
>+
>+  private void setHtmlYOffset(String htmlYOffset) {
>+    this.htmlYOffset = getSizeValueAndType(htmlYOffset);
>+  }
>+
>+  private Size getSizeValueAndType(String stringValue) {
>+    if (TextUtils.isEmpty(stringValue)) {
>+      return null;
>+    }
>+
>+    Size out = new Size();
>+    if (stringValue.contains("px")) {
>+      String[] sizeValue = stringValue.split("px");
>+      if (sizeValue.length != 0) {
>+        out.value = Integer.parseInt(sizeValue[0]);
>+      }
>+      out.type = "px";
>+    } else if (stringValue.contains("%")) {
>+      String[] sizeValue = stringValue.split("%");
>+      if (sizeValue.length != 0) {
>+        out.value = Integer.parseInt(sizeValue[0]);
>+      }
>+      out.type = "%";
>+    }
>+    return out;
>+  }
>+
>+  boolean isHtmlTabOutsideToClose() {
>+    return htmlTabOutsideToClose;
>+  }
>+
>+  private void setHtmlTabOutsideToClose(boolean htmlTabOutsideToClose) {
>+    this.htmlTabOutsideToClose = htmlTabOutsideToClose;
>+  }
>+
>   ActionContext getActionContext() {
>     return actionContext;
>   }
>Index: mobile/android/thirdparty/com/leanplum/messagetemplates/MessageTemplates.java
>IDEA additional info:
>Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
><+>UTF-8
>===================================================================
>--- mobile/android/thirdparty/com/leanplum/messagetemplates/MessageTemplates.java	(revision 408766:4f1014eb5039bdfdd7a39fb7785d102df1994a6f)
>+++ mobile/android/thirdparty/com/leanplum/messagetemplates/MessageTemplates.java	(revision 408766+:4f1014eb5039+)
>@@ -55,6 +55,8 @@
>     static final String BACKGROUND_COLOR = "Background color";
>     static final String LAYOUT_WIDTH = "Layout.Width";
>     static final String LAYOUT_HEIGHT = "Layout.Height";
>+    static final String HTML_Y_OFFSET = "HTML Y Offset";
>+    static final String HTML_TAP_OUTSIDE_TO_CLOSE = "Tap Outside to Close";
>     static final String HTML_HEIGHT = "HTML Height";
>     static final String HTML_ALIGN = "HTML Align";
>     static final String HTML_ALIGN_TOP = "Top";
>Index: mobile/android/thirdparty/com/leanplum/utils/SizeUtil.java
>IDEA additional info:
>Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
><+>UTF-8
>===================================================================
>--- mobile/android/thirdparty/com/leanplum/utils/SizeUtil.java	(revision 408766:4f1014eb5039bdfdd7a39fb7785d102df1994a6f)
>+++ mobile/android/thirdparty/com/leanplum/utils/SizeUtil.java	(revision 408766+:4f1014eb5039+)
>@@ -23,7 +23,9 @@
> 
> import android.app.Activity;
> import android.content.Context;
>+import android.graphics.Point;
> import android.util.DisplayMetrics;
>+import android.view.Display;
> import android.view.WindowManager;
> 
> /**
>@@ -127,4 +129,23 @@
>     }
>     return result;
>   }
>+
>+  /**
>+   * Gets the size of display in pixels.
>+   *
>+   * @param context Current activity.
>+   * @return A Point object with display size information.
>+   */
>+  public static Point getDisplaySize(Activity context) {
>+    Point size = new Point();
>+    if (context == null) {
>+      return size;
>+    }
>+    try {
>+      Display display = context.getWindowManager().getDefaultDisplay();
>+      display.getSize(size);
>+    } catch (Throwable ignored) {
>+    }
>+    return size;
>+  }
> }

Comment 12

[Nevin Chen(Not active on Bugzilla)]

Oops. Sorry, I left too many un-related code in my previous comment. It's my first time not using mozReview 

Hi Vlad
Can you use mozReview? I think it's easier for reviewers to add comments.


I think the patch here only grab the code from Leanplum's SDK that fixes the UI issue.
Since it's all UI code, no message is displayed, no overlay is added,  no data is sent out. I think it's a low sec-risk (in my two cents). But the import format must be fixed before it can be landed.

Comment 13

[Vlad Baicu]

Attachment: Bug 1429386 - Added missing functionalities from a newer LP SDK version

Thank you for the review Nevin, I fixed the imports from HTMLOptions.java.

Currently we are still sorting out an issue regarding mercurial access for our LDAP accounts in order to configure MozReview, we will use it once that gets sorted out.

Comment 14

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8963704 [details] [diff] [review]
Bug 1429386 - Added missing functionalities from a newer LP SDK version

Review of attachment 8963704 [details] [diff] [review]:
-----------------------------------------------------------------

We want to align the code in Leanplum SDK as much as possible (if they are risky, we'll modify them)
We normally don't test/change their implementation/ coding style either.
So as long as this patch is aligned with their SDK, it looks good to me.

Comment 15

[:sdaswani]

Thanks Nevin for the review, once Vlad gets it in mozreview, I will r+ it.

Comment 16

[Frederik Braun [:freddy]]

(In reply to :sdaswani from comment #7)
> Frederik, Dan - can one of you review the new attachment ...

Bug 1450908.

Comment 17

[Vlad Baicu]

Attachment: Bug 1429386 Added missing functionalities from a newer LP SDK version

Review commit: https://reviewboard.mozilla.org/r/234082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234082/

Comment 18

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8965363 [details]
Bug 1429386  Added missing functionalities from a newer LP SDK version

https://reviewboard.mozilla.org/r/234082/#review240410

Comment 19

[:sdaswani]

Bogdan or Sorina, can one of you confirm this fix please?

Comment 20

[:sdaswani]

Comment on attachment 8965363 [details]
Bug 1429386  Added missing functionalities from a newer LP SDK version

Approval Request Comment
[Feature/Bug causing the regression]: Bug in older version of LP SDK.
[User impact if declined]: Users won't be able to interact with Firefox when LP banner is shown.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Not yet, but I have NI'ed Softvision to take a look.
[Needs manual test from QE? If yes, steps to reproduce]: Fire a leanplum banner and ensure you can interact with FF.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No, as it's just taking code from a newer version of the LP SDK.
[Why is the change risky/not risky?]:
[String changes made/needed]:

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f4e8888611f
Added missing functionalities from a newer LP SDK version r=nechen

Comment 22

[:sdaswani]

Vlad, NI'ing you too to see if anyone else besides Bogdan or Sorina should QA this.

Comment 23

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/4f4e8888611f

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8965363 [details]
Bug 1429386  Added missing functionalities from a newer LP SDK version

Needed in 60 for product reasons. Approved for 60.0b11.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/64c9c9119d10

Comment 26

[:sdaswani]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #24)
> Comment on attachment 8965363 [details]
> Bug 1429386  Added missing functionalities from a newer LP SDK version
> 
> Needed in 60 for product reasons. Approved for 60.0b11.

Thanks!

Comment 27

[Bogdan Surd, QA [:bsurd, NI]]

Attachment: banner.mp4

Device:
 - Samsung Galaxy S8 (Android 8.0)

 The settings menu still seems to remain on screen in the latest Beta (60.0b11). After closing the banner I could interact with the settings menu.

Note:
 Could not get LeanPlum to activate on the latest Nightly build in order to verify if its the same issue there as well.
 Please see attached video.

Comment 28

[:sdaswani]

Vlad, looks like this wasn't fixed?

Comment 29

[Bogdan Surd, QA [:bsurd, NI]]

Device:
 - Nexus 5 (Android 6.0)

Hello,

After talking to Petru today and doing some more investigation into this issue the problem seems to be when upgrading from a .multi to a .multi build. 

If using an .en-US build, enabling LeanPlum there and updating to a .multi build everything seems to work find, for both this and the issue in Bug 1445799.

So there seems to be a different issue here when updating from a certain build to another. I will do some more investigating into this tomorrow and file a new bug for that together with all of my findings.

Nightly builds seem to suffer from the same problem as Beta for this issue only. The attribute from Bug 1445799 is displayed.

@Susheel Is is safe to mark this as resolved and log this issue as a different bug? Or should we leave this and Bug 145799 open until the issue is solved?

Comment 30

[:sdaswani]

Yes, it's safe to do that and log a different bug with your new findings. For the new bug I also want to understand how probable that would occur for users in the wild.

Comment 31

[:sdaswani]

And thanks for talking this over guys!

Comment 32

[Bogdan Surd, QA [:bsurd, NI]]

As per Comment 30 marking this as verified. Will file a new bug for the other issue after I do some more testing with different build types.