Investigate Activity Stream context menu behaviour on tablet and landscape phones

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ahunt, Assigned: ahunt)

Tracking

Trunk
Firefox 52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [MobileAS])

Attachments

(4 attachments)

We currently show the same bottomsheet menu, in full-width on tablets. A normal dropdown contextmenu is probably nicer.

On phones in landscape mode, the menu appears full-screen. We could consider making the menu peek height a percentage of screen height instead to show at least part of the Activity Stream panel in the backround? That would also have the advantage of working well across various device sizes (whereas currently the peek and max heights are hardcoded as a dp value).
Assignee: nobody → ahunt
Priority: -- → P2
Whiteboard: [MobileAS]
Depends on: 1300144
On the N9 running android 7/N, the context menu share button is a lighter grey than the remaining buttons. That doesn't happen on an N6P also running 7/N, so this could well be a tablet specific issue.

I haven't tested on any other Android versions yet.
Iteration: --- → 1.8
Priority: P2 → P1
Comment on attachment 8806871 [details]
Bug 1312477 - Pre: increase touch-target size to Android UI guidelines

https://reviewboard.mozilla.org/r/90160/#review90044
Attachment #8806871 - Flags: review?(s.kaspari) → review+
Comment on attachment 8806872 [details]
Bug 1312477 - Pre: refactor AS context menu to allow multiple view implementations

https://reviewboard.mozilla.org/r/90162/#review90048

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/menu/ActivityStreamContextMenu.java:211
(Diff revision 1)
> +    public static void show(Context context,
> +                            View anchor,
> +                            final MenuMode menuMode,
> +                            final String title, @NonNull final String url,
> +                            HomePager.OnUrlOpenListener onUrlOpenListener,
> +                            HomePager.OnUrlOpenInBackgroundListener onUrlOpenInBackgroundListener,
> +                            final int tilesWidth, final int tilesHeight) {
> +        showInternal(context,
> +                anchor,
> +                menuMode,
> +                title, url,
> +                onUrlOpenListener,
> +                onUrlOpenInBackgroundListener,
> +                tilesWidth,
> +                tilesHeight);
> +    }

I'm confused. Why is the delegation from show() to showInternal() needed?
Attachment #8806872 - Flags: review?(s.kaspari) → review+
Comment on attachment 8806873 [details]
Bug 1312477 - Add popupmenu mode to AS context menu for use on tablet

https://reviewboard.mozilla.org/r/90164/#review90050

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/menu/PopupContextMenu.java:24
(Diff revision 1)
> +    final PopupWindow popupWindow;
> +    final NavigationView navigationView;

nit: private?

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/menu/PopupContextMenu.java:76
(Diff revision 1)
> +
> +

nit: Some empty lines
Attachment #8806873 - Flags: review?(s.kaspari) → review+
Comment on attachment 8806874 [details]
Bug 1312477 - Post: remove arbitrary peek and max heights from BottomSheet menu

https://reviewboard.mozilla.org/r/90166/#review90054
Attachment #8806874 - Flags: review?(s.kaspari) → review+
Pushed by ahunt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/594d5fc4d75c
Pre: increase touch-target size to Android UI guidelines r=sebastian
https://hg.mozilla.org/integration/autoland/rev/522f6a80dead
Pre: refactor AS context menu to allow multiple view implementations r=sebastian
https://hg.mozilla.org/integration/autoland/rev/ffdf65db6dba
Add popupmenu mode to AS context menu for use on tablet r=sebastian
https://hg.mozilla.org/integration/autoland/rev/ee67814f9015
Post: remove arbitrary peek and max heights from BottomSheet menu r=sebastian
You need to log in before you can comment on or make changes to this bug.