Closed Bug 1300144 Opened 8 years ago Closed 8 years ago

Implement menu for activity stream cards

Categories

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

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect

Tracking

(firefox52 fixed)

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

People

(Reporter: sebastian, Assigned: ahunt)

References

(
URL
)

Details

(Whiteboard: [MobileAS])

Attachments

(6 files)

Bug 1300144 - Increase touch area for menu button
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1300144 - Import Activity Stream (VectorDrawable) menu icons
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1300144 - Implement Activity Stream "context" bottomsheet menu
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1300144 - Pre: pass mUrlOpenInBackgroundListener into HighlightsItem
58 bytes, text/x-review-board-request
sebastian
: review+
Details
menu_snapshort.png
66.32 KB, image/png
Details
DO NOT LAND: Bug 1300144 - Remove "--non-constant-id" from aapt invocations
58 bytes, text/x-review-board-request
Details 
Bryan is working on the mocks currently.
Assignee: nobody → ahunt
Priority: P2 → P1
Status: NEW → ASSIGNED
I was going to try and use the ContextMenu for this (with setHeaderView for the content at the top), but that results in old-style menus on older devices, which isn't ideal. However that's also what we currently use for other HomePager context menus (e.g. history/bookmarks/etc).

I'm wondering if we want to just hardcode the "menu" items in the same layout as the "header" and show that inside a CardView, instead of using menu inflation. (Or we could repurpose GeckoMenu to be more flexible and use it here?)

Note: for the main browser menu we do our own menu layouting in GeckoMenu, which results in consistent styling across Android versions. That also means we have an inconsistency between main app menu (always material'y), and context menus (whatever the user's Android version provides) - I'm guessing we want modern menus everywhere, and the old context menus are an oversight.
Priority: P1 → P2
I've also been wondering whether it would be better to put this in a BottomSheet. Google docs seems to use that for their "context" menu when viewing a list of documents on phones, although on tablets they seem to use a PopupMenu (with no header - although the header probably becomes redundant on tablets?).

https://material.google.com/components/bottom-sheets.html#bottom-sheets-persistent-bottom-sheets

Bottom sheets requires design support library >= 23.2 (which we could upgrade to without and SDK update, but it's still a bunch of work).
Priority: P2 → P1
(In reply to Andrzej Hunt :ahunt from comment #2)
> I've also been wondering whether it would be better to put this in a
> BottomSheet. Google docs seems to use that for their "context" menu when
> viewing a list of documents on phones, although on tablets they seem to use
> a PopupMenu (with no header - although the header probably becomes redundant
> on tablets?).

Did you already talk to Bryan about this idea?

I also left a comment on the trello card about some of my concerns before I left. Not sure if you already discussed this:
https://trello.com/c/gr23yaD4/89-i-want-to-be-able-to-dismiss-items-so-that-i-can-easily-remove-things-i-m-not-interested-in-seeing
Status: ASSIGNED → NEW
Priority: P1 → P2
Priority: P2 → P1
Comment on attachment 8791426 [details]
Bug 1300144 - Increase touch area for menu button

https://reviewboard.mozilla.org/r/78830/#review82860

Based on my Googling, this looks good.  If it works for you (make sure to test early devices!) it works for me.
Attachment #8791426 - Flags: review?(nalexander) → review+
(In reply to Nick Alexander :nalexander from comment #14)
> Comment on attachment 8791426 [details]
> Bug 1300144 - Pre: enable VectorDrawable support
> 
> https://reviewboard.mozilla.org/r/78830/#review82860
> 
> Based on my Googling, this looks good.  If it works for you (make sure to
> test early devices!) it works for me.

Yup, I've actually been testing mainly on 4.4 devices. The aapt flag was needed for |mach build|'s, gradle apparently adds that flag automatically when the vector flag is added.
Comment on attachment 8791426 [details]
Bug 1300144 - Increase touch area for menu button

https://reviewboard.mozilla.org/r/78830/#review83518
Attachment #8791426 - Flags: review?(s.kaspari) → review+
Comment on attachment 8791427 [details]
Bug 1300144 - Import Activity Stream (VectorDrawable) menu icons

https://reviewboard.mozilla.org/r/78832/#review83520

I'm not really happy about changing all the icons in one place and keeping the "old" icons everywhere else. I'd prefer either changing all icons or none - to have a consistent set of icons everywhere. Maybe we should discuss this antlam and bbell?
Attachment #8791427 - Flags: review?(s.kaspari)
Priority: P1 → P2
Depends on: 1309821
I want to land the first, reviewed patch separately in bug 1309821. I'm going to need vector drawable support for a different bug too. :)
Priority: P2 → P1
Blocks: 1311555
Blocks: 1311561
Attached image menu_snapshort.png — 
Here's a screenshot of the current state of the menu.

For landing I still need to address comment #17 (patch 3), and get the design verified.
Comment on attachment 8791428 [details]
Bug 1300144 - Implement Activity Stream "context" bottomsheet menu

https://reviewboard.mozilla.org/r/78834/#review86108

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/menu/ActivityStreamContextMenu.java:149
(Diff revision 4)
> +                }
> +            }
> +        });
> +
> +
> +        // Set peek height

Redundant comment is redundant
Comment on attachment 8802760 [details]
menu_snapshort.png

@Bryan: Here's a screenshot of the current version of the context menu.
Attachment #8802760 - Flags: feedback?(bbell)
My current patches have a switch statement that uses Resource ID's. That's broken in |mach build|'s, but might be fixed in Bug 1311741. I can switch the switch for an if/else list if that bug doesn't land before this bug is ready for landing, but it would be nicer just to keep the switch if possible.
Status: NEW → ASSIGNED
Depends on: 1311741
Iteration: --- → 1.7
Blocks: 1312114
(In reply to Sebastian Kaspari (:sebastian) from comment #17)
> Comment on attachment 8791427 [details]
> Bug 1300144 - Import Activity Stream (VectorDrawable) menu icons
> 
> https://reviewboard.mozilla.org/r/78832/#review83520
> 
> I'm not really happy about changing all the icons in one place and keeping
> the "old" icons everywhere else. I'd prefer either changing all icons or
> none - to have a consistent set of icons everywhere. Maybe we should discuss
> this antlam and bbell?

I've filed Bug 1312114 to do this replacement, since it's not as simple as switching the drawables (some icons are slightly different, it's also not yet possible  to load VectorDrawable's everywhere, so I'm having to fix various parts of the code to get them working).
Comment on attachment 8798947 [details]
Bug 1300144 - Pre: pass mUrlOpenInBackgroundListener into HighlightsItem

https://reviewboard.mozilla.org/r/84282/#review86946
Attachment #8798947 - Flags: review?(s.kaspari) → review+
Comment on attachment 8791427 [details]
Bug 1300144 - Import Activity Stream (VectorDrawable) menu icons

https://reviewboard.mozilla.org/r/78832/#review86948
Attachment #8791427 - Flags: review?(s.kaspari) → review+
Comment on attachment 8791428 [details]
Bug 1300144 - Implement Activity Stream "context" bottomsheet menu

https://reviewboard.mozilla.org/r/78834/#review86950

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/menu/ActivityStreamContextMenu.java:90
(Diff revision 4)
> +        NavigationView nv = (NavigationView) findViewById(R.id.menu);
> +        nv.setNavigationItemSelectedListener(this);

Nit: Especially in longer methods I always find it hard to remember what short variables like "nv" in the scope are meaning (e.g. when autocompleting).

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/menu/ActivityStreamContextMenu.java:97
(Diff revision 4)
> +        ThreadUtils.postToBackgroundThread(new Runnable() {
> +            @Override
> +            public void run() {
> +                final boolean isAlreadyBookmarked = BrowserDB.from(context).isBookmark(context.getContentResolver(), url);
> +
> +                ThreadUtils.postToUiThread(new Runnable() {
> +                    @Override
> +                    public void run() {

If you are following a UI thread -> background thread -> UI thread pattern then an AsyncTask might be easier to use and read. However if manual threading has an advantage here then let's move some of the code to separate method to avoid this deep nesting.

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/menu/ActivityStreamContextMenu.java:121
(Diff revision 4)
> +        ThreadUtils.postToBackgroundThread(new Runnable() {
> +            @Override
> +            public void run() {
> +                final Cursor cursor = BrowserDB.from(context).getHistoryForURL(context.getContentResolver(), url);

Maybe another AsyncTask?

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/menu/ActivityStreamContextMenu.java:154
(Diff revision 4)
> +    public static void show(Context context, final String title, final String url,
> +                            HomePager.OnUrlOpenListener onUrlOpenListener,
> +                            HomePager.OnUrlOpenInBackgroundListener onUrlOpenInBackgroundListener,
> +                            final int tilesWidth, final int tilesHeight) {
> +        if (url == null) {
> +            // Data hasn't been loaded yet: this probably shouldn't ever happen:
> +            // RecyclerView.getViewForPosition() binds ViewHolder's immediately after
> +            // creating them, hence we shouldn't ever end up rendering an unbound ViewHolder.
> +            // The menu can only be selected once an item is rendered, so we can't end up here.

If this shouldn't happen then let's add @NonNull annotations.

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/menu/ActivityStreamContextMenu.java:176
(Diff revision 4)
> +
> +    @Override
> +    public boolean onNavigationItemSelected(MenuItem item) {
> +        switch (item.getItemId()) {
> +            case R.id.share:
> +                Telemetry.sendUIEvent(TelemetryContract.Event.SHARE, TelemetryContract.Method.LIST, "menu");

Only telemetry for share? :)

But that's something we can look at in bug 1301468.

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/menu/ActivityStreamContextMenu.java:201
(Diff revision 4)
> +                ThreadUtils.postToBackgroundThread(new Runnable() {
> +                    @Override
> +                    public void run() {
> +                        GeckoAppShell.createShortcut(title, url);
> +                    }
> +                });

This doesn't need to be called on a background thread afaik.

::: mobile/android/base/locales/en-US/android_strings.dtd:844
(Diff revision 4)
> +<!ENTITY activity_stream_dismiss "Dismiss">
> +<!ENTITY activity_stream_delete_history "Delete from History">

Maybe add a localization note so that is clear what dismiss does compared to delete.

::: mobile/android/base/resources/values/dimens.xml:225
(Diff revision 4)
> +    <item name="activity_stream_contextmenu_peek_height" type="dimen">380dp</item>
> +    <!-- note: max_menu_height only affects the scrolling menu, but doesnt' take into consideration
> +         the header above it. -->
> +    <item name="activity_stream_contextmenu_max_menu_height" type="dimen">350dp</item>

Does this work on all kind of devices? How about landscape?

Also: How does this look like on tablets? You mentioned Google Drive in one of the bugs as example for the bottom pager. On tablets they use a regular menu instead of the bottom pager - do we get this from the support library too?
Attachment #8791428 - Flags: review?(s.kaspari) → review+
Comment on attachment 8791428 [details]
Bug 1300144 - Implement Activity Stream "context" bottomsheet menu

https://reviewboard.mozilla.org/r/78834/#review86950

> This doesn't need to be called on a background thread afaik.

Ah, I never realised that! I'd copied this code from the existing context menu handling without checking - I've filed Bug 1312461 to fix existing uses.

> Does this work on all kind of devices? How about landscape?
> 
> Also: How does this look like on tablets? You mentioned Google Drive in one of the bugs as example for the bottom pager. On tablets they use a regular menu instead of the bottom pager - do we get this from the support library too?

In the current patch:
- On landscape, the menu shows full-width, and in fact full-screen.
- On tablet: menu shows full-width, still a bottom pager.

I'll file a followup bug to look into this more (FWIW google drive doesn't make the menu full-screen in landscape on a phone, but it's still full-width).

The support library doesn't automatically switch to a regular menu, but it shouldn't be too hard too switch since we can *hopefully* use the same code to setup a popup instead of the BottomSheet.
Blocks: 1312477
https://hg.mozilla.org/integration/fx-team/rev/4267a799727e687e28579f0f15661b80f6a882aa
Bug 1300144 - Increase touch area for menu button r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/e4e419b838bfac6d82d7cdac986d5919f17122b1
Bug 1300144 - Pre: pass mUrlOpenInBackgroundListener into HighlightsItem r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/08a6186c2842c471e85ac5e4060841431dcef572
Bug 1300144 - Import Activity Stream (VectorDrawable) menu icons r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/b6571211c807e209e550833eb47dfc08a4c6ae53
Bug 1300144 - Implement Activity Stream "context" bottomsheet menu r=sebastian
Attachment #8802760 - Flags: feedback?(bbell)
Blocks: 1314728
Blocks: 1314726
See Also: → 1310143
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: