1312477 - Investigate Activity Stream context menu behaviour on tablet and landscape phones

Status: RESOLVED FIXED

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

Description

[Andrzej Hunt :ahunt]

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).

Comment 1

[Andrzej Hunt :ahunt]

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.

Comment 2

[Andrzej Hunt :ahunt]

Attachment: Bug 1312477 - Pre: increase touch-target size to Android UI guidelines

48dp is the minimum touch target size as suggested in the Android UI guidelines:
https://developer.android.com/design/patterns/accessibility.html

A similar recommendation is made in:
https://material.google.com/layout/metrics-keylines.html#metrics-keylines-touch-target-size

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

Comment 3

[Andrzej Hunt :ahunt]

Attachment: Bug 1312477 - Pre: refactor AS context menu to allow multiple view implementations

This is in preparation for having a popup menu on tablets.

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

Comment 4

[Andrzej Hunt :ahunt]

Attachment: Bug 1312477 - Add popupmenu mode to AS context menu for use on tablet

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

Comment 5

[Andrzej Hunt :ahunt]

Attachment: Bug 1312477 - Post: remove arbitrary peek and max heights from BottomSheet menu

Setting the max height is completely arbitrary, and largely unnecessary - I think it's better
to let the bottom automatically handle this until we know more about how the bottom sheet
is perceived by real users.

The peek height is similarly arbitrary - we'll probably want to discuss a good default height
algorithm with UX, but setting an arbitrary hardcoded value seems wrong, especially since
it doesn't behave well in landscape mode. BottomSheetDialog already does an acceptable
job of finding a default peek height.

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

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8806871 [details]
Bug 1312477 - Pre: increase touch-target size to Android UI guidelines

https://reviewboard.mozilla.org/r/90160/#review90044

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

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?

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8806874 [details]
Bug 1312477 - Post: remove arbitrary peek and max heights from BottomSheet menu

https://reviewboard.mozilla.org/r/90166/#review90054

Comment 10

[Andrzej Hunt :ahunt]

Comment on attachment 8806871 [details]
Bug 1312477 - Pre: increase touch-target size to Android UI guidelines

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90160/diff/1-2/

Comment 11

[Andrzej Hunt :ahunt]

Comment on attachment 8806872 [details]
Bug 1312477 - Pre: refactor AS context menu to allow multiple view implementations

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90162/diff/1-2/

Comment 12

[Andrzej Hunt :ahunt]

Comment on attachment 8806873 [details]
Bug 1312477 - Add popupmenu mode to AS context menu for use on tablet

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90164/diff/1-2/

Comment 13

[Andrzej Hunt :ahunt]

Comment on attachment 8806874 [details]
Bug 1312477 - Post: remove arbitrary peek and max heights from BottomSheet menu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90166/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/594d5fc4d75c
https://hg.mozilla.org/mozilla-central/rev/522f6a80dead
https://hg.mozilla.org/mozilla-central/rev/ffdf65db6dba
https://hg.mozilla.org/mozilla-central/rev/ee67814f9015