1279278 - Implement urlbar features for chrome custom tabs

Status: RESOLVED FIXED

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

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

You can specify a toolbar color, additional menu items, etc.

Comment 1

[Jonathan Almeida [:jonalmeida]]

Attachment: Custom Tabs toolbar using color fron Intent f?droeh

MozReview-Commit-ID: EbwtRsOIMLp

Comment 2

[Jonathan Almeida [:jonalmeida]]

Attachment: toolbar-color.png

I've put together a small patch of what the toolbar would look start to look like and implemented the bits that would make use of the toolbar color that 3rd party apps would send.

The image on the left shows the default look if a colour was not set (I've just used the colour from our current toolbar for now), and the right is what it would look like if a toolbar colour was set.

Ideally, the next part would be to wrap the Toolbar bit in a CoordinatorLayout and set a custom Behaviour[1] so that we can intercept touch events and have the toolbar hide when we scroll down and show up when we scroll up. There's a blogpost[2] that shows how this can be done.

Feel free to also ignore the patch attached above. I mainly wanted some aesthetic look to fennec when some of the apps I use impl Custom Tabs. :)
Alternatively, if no one would be working on this, I'd be happy to take a shot at it as well.

[1]: https://developer.android.com/reference/android/support/design/widget/CoordinatorLayout.Behavior.html
[2]: https://medium.com/google-developers/intercepting-everything-with-coordinatorlayout-behaviors-8c6adc140c26

Comment 3

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8792337 [details] [diff] [review]
Custom Tabs toolbar using color fron Intent f?droeh

Requesting feedback from droeh who seems to be working on this.

Comment 4

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8792337 [details] [diff] [review]
Custom Tabs toolbar using color fron Intent f?droeh

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

Looks good to me.

Comment 5

[Dylan Roeh (:droeh) (he/him)]

Attachment: Custom Tabs URL bar (part 1)

This rebases Jon's patch. Coupled with part 2 (below) it puts the Custom Tabs toolbar in pretty good shape.

Comment 6

[Dylan Roeh (:droeh) (he/him)]

Attachment: Custom Tabs URL bar (part 2)

In this we use the domain name as the title in the custom tab toolbar (ellipsizing on the left to avoid spoofing) and handle the toolbar navigation the same we handle onBackButtonPressed.

Comment 7

[Dylan Roeh (:droeh) (he/him)]

Attachment: Screenshot with domain name in toolbar

Comment 8

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8806896 [details]
Screenshot with domain name in toolbar

Anthony, does this look reasonable for a version 0.1 sort of thing? (Note the toolbar's background color can be chosen by the app that launches the custom tab.)

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8806894 [details] [diff] [review]
Custom Tabs URL bar (part 1)

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

I just tried the patch and it's pretty cool! :)

I noticed that when rotating the device then the color is lost and we show a black toolbar. This is something we should fix in a follow-up bug.

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java
@@ +82,5 @@
> +
> +    public boolean onOptionsItemSelected(MenuItem item) {
> +        switch (item.getItemId()) {
> +            case android.R.id.home:
> +                finish();

nit: We should return true here after handling this call.

@@ +101,5 @@
> +        if (color != NO_COLOR) {
> +            toolbar.setBackgroundColor(color);
> +        }
> +        final Window window = getWindow();
> +        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {

I guess we shouldn't update the status bar color if the color is NO_COLOR?

@@ +103,5 @@
> +        }
> +        final Window window = getWindow();
> +        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
> +            window.addFlags(WindowManager.LayoutParams.FLAG_DRAWS_SYSTEM_BAR_BACKGROUNDS);
> +            window.setStatusBarColor(darken(color, 0.25));

0.25 seems to be darker than what Android/Appcompat does normally. If you use Google+ or Yahnac and launch a custom tab then the toolbar has the same color as the app but the status bar is darker than in the app. But we could file a follow-up bug for that.

@@ +107,5 @@
> +            window.setStatusBarColor(darken(color, 0.25));
> +        }
> +    }
> +
> +    private int darken(final int color, final double fraction) {

darken() and  darkenColor() should be moved to a *utils class. We do not have ColorUtils anymore - but maybe it's time to bring it back.

::: mobile/android/base/resources/layout/customtabs_activity.xml
@@ +16,5 @@
>          in this tree. In a perfect world this should just include a GeckoView.
>      -->
>  
> +    <android.support.v7.widget.Toolbar
> +        android:id="@+id/action_bar"

super nit: "toolbar" might be a better id here :)

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8806895 [details] [diff] [review]
Custom Tabs URL bar (part 2)

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

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java
@@ +37,3 @@
>      private static final int NO_COLOR = -1;
>      private Toolbar toolbar;
> +    private TextView textView;

This textview is only used in onTabChanged() and we do not need to keep a reference here.

@@ +38,5 @@
>      private Toolbar toolbar;
> +    private TextView textView;
> +    private int tabId = -1;
> +
> +    private View.OnClickListener listener = new View.OnClickListener() {

This listener can be inlined (following the style of other files).

@@ +52,5 @@
>          toolbar = (Toolbar) findViewById(R.id.action_bar);
>          updateActionBarWithToolbar(toolbar);
>          updateToolbarColor();
> +        toolbar.setNavigationOnClickListener(listener);
> +        Tabs.registerOnTabsChangedListener(this);

We register for tab events but we never unregister.

@@ +87,5 @@
> +                // setSupportActionBar.
> +                Field f = toolbar.getClass().getDeclaredField("mTitleTextView");
> +                f.setAccessible(true);
> +                textView = (TextView) f.get(toolbar);
> +                textView.setEllipsize(TextUtils.TruncateAt.START);

This is pretty hacky and can break in the future. The Toolbar allows to add a custom view to it. We could add our own TextView and configure it however we like. Adding a custom view is something we want to do eventually anyways - to display the lock icon etc.

Maybe it's possible to use ToolbarDisplayLayout here - or share some code.

Sounds like a bigger task for a follow-up bug too.

@@ +90,5 @@
> +                textView = (TextView) f.get(toolbar);
> +                textView.setEllipsize(TextUtils.TruncateAt.START);
> +            } catch (Exception e) {
> +                Log.i(LOGTAG, "Failed to get Toolbar TextView, using default title.");
> +                title = "Firefox";

I'd start by just displaying the full uri for now. It's still better than displaying nothing at all. We should also handle the case where uri.getHost() returns null or an empty string.

@@ +94,5 @@
> +                title = "Firefox";
> +                // TODO -- this probably needs more thought, maybe try to manually truncate the domain?
> +            }
> +            toolbar.setTitle(title);
> +            updateActionBarWithToolbar(toolbar);

I do not understand why we do this every time. We should need to set the toolbar as actionbar only once. If this didn't work without setting it again: This might need to be called on the UI thread and/or this needs to be called on the actionbar returned by getSupportActionBar().

@@ +100,5 @@
> +            toolbar.setNavigationOnClickListener(listener);
> +        }
> +    }
> +
> +    private void backPressedImpl() {

What's the advantage of using backPressedImpl()? Can't this code be in onBackPressed() and be called from the system as well as the click listener?

Comment 11

[Dylan Roeh (:droeh) (he/him)]

Attachment: Custom Tabs URL bar (part 1) v1.1

Carrying over Sebastian's r+. 

Losing the toolbar custom color when device orientation changes will be addressed in bug 1315348, status bar darkness will be addressed in bug 1315349. Everything else is addressed in this version of the patch.

Comment 12

[Dylan Roeh (:droeh) (he/him)]

Attachment: Custom Tabs URL bar (part 2) v1.1

Carrying over Sebastian's r+.

I filed bug 1315350 for the hacky approach I took to ellipsizing here.

Other complaints are fixed, with the exception of displaying the full uri in the event that we can't ellipsize at the start of the string. I don't think we want a situation in which "totallylegitimatesite.com.phishing.ru" could get truncated to "totallylegitimatesite.com..." in the title. In practice, when we have a less ugly approach to ellipsizing this will probably be unnecessary, so it should be dealt with as part of bug 1315350. But for now, I've left "Firefox" as the default title here for that case and also when we cannot getHost() for whatever reason.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Dylan Roeh (:droeh) from comment #12)
> Other complaints are fixed, with the exception of displaying the full uri in
> the event that we can't ellipsize at the start of the string. I don't think
> we want a situation in which "totallylegitimatesite.com.phishing.ru" could
> get truncated to "totallylegitimatesite.com..." in the title.

I absolutely agree but I think this will be fixed by when changing the implementation in bug 1315350. Nothing is shipping yet. However if we go with the app name for now, then let's use AppConstants.MOZ_APP_BASENAME instead of hardcoding 'Firefox'.

Comment 14

[Dylan Roeh (:droeh) (he/him)]

(In reply to Sebastian Kaspari (:sebastian) from comment #13)
> I absolutely agree but I think this will be fixed by when changing the
> implementation in bug 1315350. Nothing is shipping yet. However if we go
> with the app name for now, then let's use AppConstants.MOZ_APP_BASENAME
> instead of hardcoding 'Firefox'.

Good point, will do.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d18a718eb96f
Custom Tabs toolbar using color fron Intent r=sebastian
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c529127c7a4
Use domain name as a Toolbar title and behave correctly when Toolbar navigation is used. r=sebastian

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d18a718eb96f
https://hg.mozilla.org/mozilla-central/rev/1c529127c7a4

Comment 17

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8806896 [details]
Screenshot with domain name in toolbar

Yes, if this bug is only about supporting the color use. This makes sense as a V0.1