Closed Bug 1429386 Opened 2 years ago Closed 2 years ago

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

Categories

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

Firefox 59
ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 61
Tracking Status
fennec + ---
firefox59 --- wontfix
firefox60 blocking verified
firefox61 --- verified

People

(Reporter: oana.horvath, Assigned: vlad.baicu)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Leanplum] [60])

Attachments

(3 files, 2 obsolete files)

Attached video 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.
Blocks: 1351571
Blocks: 1390454
No longer blocks: 1351571
Please help advise.
tracking-fennec: ? → +
Flags: needinfo?(wehuang)
Priority: -- → P2
P2 looks good to me, since the menu will be normal after dismiss the banner, per current implementation.
Flags: needinfo?(wehuang)
[triage] Bulk edit: product thinks leanplum is high priority: P1 rank 1.
Rank: 1
Priority: P2 → P1
Whiteboard: [Leanplum]
Whiteboard: [Leanplum] → [Leanplum] [60]
Product team would like this fixed in 60, tracked as blocking to ensure it's closely monitored.
Assignee: nobody → vlad.baicu
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.
Attachment #8961855 - Flags: review?(michael.l.comella)
> 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.
Flags: needinfo?(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.
Flags: needinfo?(vlad.baicu)
Flags: needinfo?(sdaswani)
Flags: needinfo?(fbraun)
Flags: needinfo?(dveditz)
Andrew is already working on it https://bugzilla.mozilla.org/show_bug.cgi?id=1438716
Flags: needinfo?(vlad.baicu)
Is there an existing pi-request for this? We really need that logged before taking on a security review.
Flags: needinfo?(dveditz)
I realized that and will be filing one soon. - sorry!!
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;
>+  }
> }
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.
Flags: needinfo?(vlad.baicu)
Flags: needinfo?(sdaswani)
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.
Attachment #8961855 - Attachment is obsolete: true
Attachment #8961855 - Flags: review?(michael.l.comella)
Flags: needinfo?(vlad.baicu)
Attachment #8963704 - Flags: review?(cnevinchen)
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.
Attachment #8963704 - Flags: review?(cnevinchen) → review+
Thanks Nevin for the review, once Vlad gets it in mozreview, I will r+ it.
Flags: needinfo?(sdaswani)
(In reply to :sdaswani from comment #7)
> Frederik, Dan - can one of you review the new attachment ...

Bug 1450908.
Flags: needinfo?(fbraun)
Attachment #8965363 - Flags: review?(sdaswani) → review?(cnevinchen)
Comment on attachment 8965363 [details]
Bug 1429386  Added missing functionalities from a newer LP SDK version

https://reviewboard.mozilla.org/r/234082/#review240410
Attachment #8965363 - Flags: review?(cnevinchen) → review+
Bogdan or Sorina, can one of you confirm this fix please?
Flags: needinfo?(sorina.florean)
Flags: needinfo?(bogdan.surd)
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]:
Attachment #8965363 - Flags: approval-mozilla-beta?
Attachment #8963704 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4f4e8888611f
Added missing functionalities from a newer LP SDK version r=nechen
Vlad, NI'ing you too to see if anyone else besides Bogdan or Sorina should QA this.
Flags: needinfo?(vlad.baicu)
https://hg.mozilla.org/mozilla-central/rev/4f4e8888611f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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.
Attachment #8965363 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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!
Attached video 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.
Flags: needinfo?(bogdan.surd)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Vlad, looks like this wasn't fixed?
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?
Flags: needinfo?(sorina.florean) → needinfo?(sdaswani)
Flags: needinfo?(vlad.baicu)
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.
Flags: needinfo?(sdaswani)
And thanks for talking this over guys!
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.