1417361 - [Activity Stream] - Tablet menu is not opened correctly

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Activity Stream, defect, P1)

Description

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

Device: 
 - Huawei MediaPad M2 (Android 5.1.1);
 - Samsung Galaxy Tab S3 (Android 7.0);

Builds:
 - Nightly (2017-11-14);
 - Beta (58.0b3);
 - Release (57.0);

Steps to reproduce:
 1. Open Fennec;
 2. In Top Sites tap on the 3 dot menu for Pocket stories or Highlights.

Expected result:
 The menu is opened in the 3 dot menu area.

Actual result:
 The menu is opened in the top left corner.

Notes:
 This is reproducible for Nightly and Beta, but not for Release.

Video: https://goo.gl/CkNNVS

Comment 1

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

We changed the view anchor in order to prevent a crash when the RecyclerView item view was no longer visible. Perhaps the solution here is to pass in the primary view to use as an anchor and a back-up that is guaranteed to be in the view hierarchy.

Comment 2

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

Attachment: Bug 1417361: Use anchor for context menu on tablet.

This uses the same anchors as the old code so should be low risk.

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

Comment 3

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

Attachment: Bug 1417361: Rename OnCardLongClickListener.onClick -> onLongClick.

This should make the code slightly simpler to follow.

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

Comment 4

[Chenxia Liu [:liuche]]

Comment on attachment 8932677 [details]
Bug 1417361: Use anchor for context menu on tablet.

https://reviewboard.mozilla.org/r/203740/#review209570

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/menu/PopupContextMenu.java:31
(Diff revision 1)
>      private final PopupWindow popupWindow;
>      private final NavigationView navigationView;
>  
> -    private final View anchor;
> +    private final View contextMenuAnchor;
>  
> -    public PopupContextMenu(final View anchor,
> +    public PopupContextMenu(final View contextMenuAnchor,

I guess we still need both because we show a snackbar for bookmarking.

Comment 5

[Chenxia Liu [:liuche]]

Comment on attachment 8932678 [details]
Bug 1417361: Rename OnCardLongClickListener.onClick -> onLongClick.

https://reviewboard.mozilla.org/r/203742/#review209572

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/508c22743a6d
Use anchor for context menu on tablet. r=liuche
https://hg.mozilla.org/integration/autoland/rev/5189909a58bb
Rename OnCardLongClickListener.onClick -> onLongClick. r=liuche

Comment 7

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/508c22743a6d
https://hg.mozilla.org/mozilla-central/rev/5189909a58bb

Comment 8

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

Verified on the latest Nightly build using a Samsung Galaxy Tab S3 (Android 7.0). The issue is no longer reproducible, marking this as verified for 59.

Comment 9

[Julien Cristau [:jcristau]]

Do you want to request uplift to 58?

Comment 10

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

Comment on attachment 8932677 [details]
Bug 1417361: Use anchor for context menu on tablet.

Approval Request Comment
[Feature/Bug causing the regression]: fix for bug 1410221
[User impact if declined]: Users on tablet will see the Activity Stream context menu menu appearing in the wrong place

[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes, comment 8
[Needs manual test from QE? If yes, steps to reproduce]: Already done.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low to medium.
[Why is the change risky/not risky?]: It's a simple change – we pass an additional View instance through several layers of abstraction (the same one we passed before bug 1410221, which caused this one so it's unlikely to cause problems as we also kept the fix for bug 1410221) and anchor our context menu on it. This is slightly tricky just because of all the layers of abstraction. Normally, this would be 100% covered by type checking but we're passing two View parameters so it's possible we swapped them. However, the code is verified by QA and my self as working so it's unlikely we've done so.

[String changes made/needed]: None

Comment 11

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

Comment on attachment 8932678 [details]
Bug 1417361: Rename OnCardLongClickListener.onClick -> onLongClick.

Approval Request Comment: see comment 10. This patch isn't necessary but for simplicity, I figure we should uplift both. This is a simple rename, done by the IDE, so shouldn't have any problems.

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8932677 [details]
Bug 1417361: Use anchor for context menu on tablet.

Fix an incorrect tablet menu in Activity Stream and was verified. Beta58+.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b3759baf9588
https://hg.mozilla.org/releases/mozilla-beta/rev/f92dde39c94c