Closed Bug 1312477 Opened 7 years ago Closed 6 years ago

Investigate Activity Stream context menu behaviour on tablet and landscape phones

Categories

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

Product:
Firefox for Android Graveyard
Component:
Awesomescreen
Type:
defect

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: ahunt, Assigned: ahunt)

References

Details

(Whiteboard: [MobileAS])

Attachments

(4 files)

Bug 1312477 - Pre: increase touch-target size to Android UI guidelines
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1312477 - Pre: refactor AS context menu to allow multiple view implementations
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1312477 - Add popupmenu mode to AS context menu for use on tablet
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1312477 - Post: remove arbitrary peek and max heights from BottomSheet menu
58 bytes, text/x-review-board-request
sebastian
: review+
Details 
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.