Closed Bug 1315937 Opened 8 years ago Closed 7 years ago

Custom tabs: Support custom menu items

Categories

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

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

Tracking

(firefox55 fixed)

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: sebastian, Assigned: walkingice)

References

Details

Attachments

(8 files, 3 obsolete files)

UX spec of customTab, Menu default options
763.98 KB, image/png
Details
Bug 1315937 - Only create option menu items once.
59 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1315937 - add theme to CustomTabsActivity
59 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1315937 - Add basic custom menu to CustomTabsActivity
59 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1315937 - Add custom items to menu of CustomTabsActivity
59 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1315937 - Add share item to menu if necessary
59 bytes, text/x-review-board-request
sebastian
: review+
Details
2017_04_05_14_15_55_Screenshot_20170405_135136.png_HTC.png
334.66 KB, image/png
Details
2017_04_05_14_19_35_Screenshot_2017_04_05_11_22_51.png_LG.png
845.42 KB, image/png
Details 
In bug 1279278 we implemented a simple toolbar for custom tabs. A client can define custom menu items to be displayed. That's something we do not support yet:

https://developer.chrome.com/multidevice/android/customtabs

https://developer.android.com/reference/android/support/customtabs/CustomTabsIntent.html
I started to work on this one.
Assignee: nobody → mail
Status: NEW → ASSIGNED
Attached patch 1315937-menu-items.patch (obsolete) — — Splinter Review 
The theme of the menu currently is still plain Android, does not look nice yet.
Probably I should use something like

 popupMenu = new GeckoPopupMenu(context);
 popupMenu.setOnMenuItemClickListener(this);

instead of using the default menu. What do you think?
GeckoPopupMenu is legacy code and for this new activity we might not need it anymore. And we should be able to get rid of most of it as soon as we start to work on a new iteration of the toolbar.

We currently only set a theme on activity level and not for the whole app. Does it look better when setting a Gecko*Theme in the manifest? Unfortunately our theme/styles code is a mess currently.
Should I rebase the patch on https://bugzilla.mozilla.org/show_bug.cgi?id=1317933 ? Or how to best handle the changes in the same class?
Hi Friedger, could you please help us rebase your patch in comment2 ?
Then we could proceed with the review process . 

Could you help us and make the patch available ASAP ? Thank you for your support !!
Flags: needinfo?(mail)
Attached patch 1315937_friedger.patch (obsolete) — — Splinter Review 
Patch has been rebased.
Attachment #8820241 - Attachment is obsolete: true
Flags: needinfo?(mail)
Attachment #8830356 - Flags: review?(s.kaspari)
Thanks for the updated patch! I'll try and review it.

(In reply to friedger from comment #8)
> Would you see the custom remote views as part of this bug as well?
> https://developer.android.com/reference/android/support/customtabs/
> CustomTabsIntent.Builder.html#setSecondaryToolbarViews(android.widget.
> RemoteViews,%20int[],%20android.app.PendingIntent)

Let's file a new bug for that.
Comment on attachment 8830356 [details] [diff] [review]
1315937_friedger.patch

Review of attachment 8830356 [details] [diff] [review]:
-----------------------------------------------------------------

Some apps add a share action to the toolbar and it doesn't seem to work with our implementation (e.g. Google+). This needs to be fixed and we should test with a variety of apps (I had problems finding apps on my phone that added more things than a share button). Apart from that I think only some code clean up is needed.

@friedger: Do you want to (and do you have the time to) push this over the finish line? Our Android team in Taipei is picking up some Custom Tabs bug this (and probably the next) sprints to finally get it ready to ship - so if you do not have the time they are interested in picking this up. :)

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java
@@ +63,5 @@
>      private int toolbarColor;
>      private String toolbarTitle;
> +    private ShareActionProvider shareActionProvider;
> +    private Intent shareIntent;
> +    private Map<Integer, PendingIntent> menuActions = new HashMap<>();

Do we need to keep all those things around (Intent, Map) or can't we just get them from the Intent if needed?

@@ +166,5 @@
>              actionBar.setTitle(toolbarTitle);
> +
> +            if (shareActionProvider != null) {
> +                setShareIntentText(tab.getURL());
> +            }

You can get the URL at any time from Tabs.getInstance().getSelectedTab() - so you might not need to update this here.

@@ +212,5 @@
>          return super.onOptionsItemSelected(item);
>      }
>  
> +    @Override
> +    public boolean onPrepareOptionsMenu(Menu menu) {

I wonder why we do this in onPrepareOptionsMenu() every time before we show the "menu" and not onCreateOptionsMenu()?
Attachment #8830356 - Flags: review?(s.kaspari) → feedback+
I used the demo app of Samsung: https://github.com/SamsungInternet/examples/blob/master/custom-tab-demo for testing.

Go ahead and bring this feature to firefox at a faster speed than I can provide right now. Especially adding test, I don't know (yet).
(In reply to friedger from comment #11)
> I used the demo app of Samsung:
> https://github.com/SamsungInternet/examples/blob/master/custom-tab-demo for
> testing.

Ah, that's helpful!


(In reply to friedger from comment #11)
> Go ahead and bring this feature to firefox at a faster speed than I can
> provide right now. Especially adding test, I don't know (yet).

Okay. Thanks for the preparatory patch, Friedger! :)
Assignee: mail → nobody
Status: ASSIGNED → NEW
Julian, you have been working on another custom tab bug - are you taking this over too?
Flags: needinfo?(walkingice0204)
OK let me take this over. And thank you @friedger !
Assignee: nobody → walkingice0204
Flags: needinfo?(walkingice0204)
Attachment #8830356 - Attachment is obsolete: true
Comment on attachment 8842811 [details]
Bug 1315937 - Only create option menu items once.

https://reviewboard.mozilla.org/r/116584/#review119182

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java:209
(Diff revision 1)
> +    // Usually should use onCreateOptionsMenu() to initialize menu items. But GeckoApp overwrite
> +    // it to support custom menu(Bug 739412). Then the parameter *menu* in this.onCreateOptionsMenu()
> +    // and this.onPrepareOptionsMenu() are different instances - GeckoApp.onCreatePanelMenu() changed it.
> +    // CustomTabsActivity only use standard menu in ActionBar, so initialize menu here.
>      @Override
> -    public boolean onPrepareOptionsMenu(Menu menu) {
> +    public boolean onCreatePanelMenu(final int id, final Menu menu) {

Oh, that's a bit annoying. I was hoping that we could build a new menu without having to care about the old stuff we have in Browser/GeckoApp.
Attachment #8842811 - Flags: review?(s.kaspari) → review+
Comment on attachment 8842812 [details]
Bug 1315937 - add theme to CustomTabsActivity

https://reviewboard.mozilla.org/r/116586/#review119184

::: mobile/android/base/resources/values/themes.xml:127
(Diff revision 1)
> +    <style name="CustomTabs" parent="Theme.AppCompat.NoActionBar">
> +    </style>
> +
> +    <style name="CustomTabs.Light" parent="Theme.AppCompat.Light.NoActionBar">

All our other themes use a "Gecko" prefix. Maybe use that here too?
Comment on attachment 8842812 [details]
Bug 1315937 - add theme to CustomTabsActivity

https://reviewboard.mozilla.org/r/116586/#review119188
Attachment #8842812 - Flags: review?(s.kaspari) → review+
Comment on attachment 8842813 [details]
Bug 1315937 - Add basic custom menu to CustomTabsActivity

https://reviewboard.mozilla.org/r/116588/#review119192

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java:340
(Diff revision 1)
> +        final GeckoPopupMenu popupMenu = new GeckoPopupMenu(this);
> +        final GeckoMenu geckoMenu = popupMenu.getMenu();

I'm wondering whether GeckoMenu etc. is something we actually need here or whether we could use some of the system APIs? Most of this code was written before there were ActionBar or Toolbar available. Maybe we could try getting rid of this and having CustomTabActivity as first use case? But don't waste time if this seems to be a too big task. :)

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java:358
(Diff revision 1)
> +            Intent browserIntent = new Intent(Intent.ACTION_VIEW, Uri.parse("http://"));
> +            ResolveInfo info = getPackageManager()
> +                    .resolveActivity(browserIntent, PackageManager.MATCH_DEFAULT_ONLY);
> +            String name = info.loadLabel(getPackageManager()).toString();

nit: final?

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java:358
(Diff revision 1)
> +            Intent browserIntent = new Intent(Intent.ACTION_VIEW, Uri.parse("http://"));
> +            ResolveInfo info = getPackageManager()
> +                    .resolveActivity(browserIntent, PackageManager.MATCH_DEFAULT_ONLY);

Do we always want the default browser here or do we want to switch to _this_ build's browser view? In most cases this will be the same because a custom tab is usually opened in the default browser - but it doesn't need to be that way.

::: mobile/android/base/locales/en-US/android_strings.dtd:286
(Diff revision 1)
>       Instead of switching to the browser it appears as if the user stays in the third-party app.
>       For more see: https://developer.chrome.com/multidevice/android/customtabs -->
>  <!ENTITY pref_custom_tabs "Custom Tabs">
>  <!ENTITY pref_custom_tabs_summary3 "Allow apps to open websites using a customized version of &brandShortName;">
> +<!ENTITY custom_tabs_menu_item_open_in "Open in &formatS;">
> +<!ENTITY custom_tabs_menu_footer "Powered by Firefox">

Instead of "Firefox" use &brandShortName; here.
Comment on attachment 8842813 [details]
Bug 1315937 - Add basic custom menu to CustomTabsActivity

https://reviewboard.mozilla.org/r/116588/#review119192

> I'm wondering whether GeckoMenu etc. is something we actually need here or whether we could use some of the system APIs? Most of this code was written before there were ActionBar or Toolbar available. Maybe we could try getting rid of this and having CustomTabActivity as first use case? But don't waste time if this seems to be a too big task. :)

Agree, I do prefer using system API. So far designer asks to build a Option-menu with Buttons and a footer.(And it also looks like GeckoApp's option menu), so I just reuse the widget GeckoMenu. As far as I know, it is not feasible by just using system API (Maybe I am wrong). If you think there is any obvious refactoring task for GeckoMenu/GeckoPopupMenu, or any system API I can use in here, I am willing to do that! :D

> Do we always want the default browser here or do we want to switch to _this_ build's browser view? In most cases this will be the same because a custom tab is usually opened in the default browser - but it doesn't need to be that way.

So far the requirement I received is to use default browser.(and this is the same behavior as Chrome)
Comment on attachment 8842812 [details]
Bug 1315937 - add theme to CustomTabsActivity

https://reviewboard.mozilla.org/r/116586/#review119184

> All our other themes use a "Gecko" prefix. Maybe use that here too?

OK, I changed to 'GeckoCustomTabs'. Or 'Gecko.CustomTabs' would be better? I cannot distinguishi the difference from our code base
Comment on attachment 8842813 [details]
Bug 1315937 - Add basic custom menu to CustomTabsActivity

https://reviewboard.mozilla.org/r/116588/#review119192

> Agree, I do prefer using system API. So far designer asks to build a Option-menu with Buttons and a footer.(And it also looks like GeckoApp's option menu), so I just reuse the widget GeckoMenu. As far as I know, it is not feasible by just using system API (Maybe I am wrong). If you think there is any obvious refactoring task for GeckoMenu/GeckoPopupMenu, or any system API I can use in here, I am willing to do that! :D

Okay, that's fine! Just think about whether you would implement it differently. This was build when Android versions were popular that we do not even support anymore (<= 2.3) - so if there's something better we can build (maybe on top of system APIs) then the CustomTabsActivity is a good "playground" for that. There are plans to update the toolbar and style in the main browser too and this would be a great opportunity to bring learnings from CustomTabActivity back to the main browser again. But yeah, if GeckoMenu/GeckoPopupMenu are still the best option we have then let's go with it.
Comment on attachment 8842813 [details]
Bug 1315937 - Add basic custom menu to CustomTabsActivity

https://reviewboard.mozilla.org/r/116588/#review120936

::: mobile/android/base/locales/en-US/android_strings.dtd:285
(Diff revision 3)
>  <!-- Custom Tabs is an Android API for allowing third-party apps to open URLs in a customized UI.
>       Instead of switching to the browser it appears as if the user stays in the third-party app.
>       For more see: https://developer.chrome.com/multidevice/android/customtabs -->
>  <!ENTITY pref_custom_tabs "Custom Tabs">
>  <!ENTITY pref_custom_tabs_summary3 "Allow apps to open websites using a customized version of &brandShortName;">
> +<!ENTITY custom_tabs_menu_item_open_in "Open in &formatS;">

Please add a comment for our translators so that they now what the placeholder is and where this might show up.
Attachment #8842813 - Flags: review?(s.kaspari) → review+
Comment on attachment 8842814 [details]
Bug 1315937 - Add custom items to menu of CustomTabsActivity

https://reviewboard.mozilla.org/r/116590/#review120940
Attachment #8842814 - Flags: review?(s.kaspari) → review+
Comment on attachment 8842815 [details]
Bug 1315937 - Add share item to menu if necessary

https://reviewboard.mozilla.org/r/116592/#review120944

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java:453
(Diff revision 3)
> +
> +    /**
> +     * Callback for Share menu item.
> +     */
> +    private void onShareClicked() {
> +        final String url = getIntent().getDataString();

Shouldn't we get the URL from the currently selected tab? e.g. Tabs.getInstance().getSelectedTab().getUrl()?

By now the user might be on a different site.
Attachment #8842815 - Flags: review?(s.kaspari) → review+
Comment on attachment 8842815 [details]
Bug 1315937 - Add share item to menu if necessary

https://reviewboard.mozilla.org/r/116592/#review120944

> Shouldn't we get the URL from the currently selected tab? e.g. Tabs.getInstance().getSelectedTab().getUrl()?
> 
> By now the user might be on a different site.

You are right! It makes more sense, and is alos what Chrome did.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9c111cf48f5
Only create option menu items once. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/c8e221d67726
add theme to CustomTabsActivity r=sebastian
https://hg.mozilla.org/integration/autoland/rev/4391ccb1488e
Add basic custom menu to CustomTabsActivity r=sebastian
https://hg.mozilla.org/integration/autoland/rev/90c92cd73ae8
Add custom items to menu of CustomTabsActivity r=sebastian
https://hg.mozilla.org/integration/autoland/rev/3794be0ad393
Add share item to menu if necessary r=sebastian
Keywords: checkin-needed
Verified as fixed on latest Nightly 55.0a1 (2017-04-04);
Tested with HTC Desire 820 (Android 6.0.1) on following apps: Gmail, Google+, Chromer, Yahnac, Google News & Weather.
Verified as fixed on latest Nightly 55.0a1 (2017-04-04);
Tested with LG G4 (Android 5.1) on following apps: Gmail, Google+, Chromer, Yahnac, Google News & Weather.
Status: RESOLVED → VERIFIED
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: