1348718 - Tweak images of ActionBar in CustomTabsActivity

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Julian Chu [:walkingice]]

According to Material Design and requirements from TDC designers, some images of ActionBar in CustomTabsActivity need to be adjust

* Home button: change from Arrow-image to Cross(Close)-image
* Image size of Action-button and Menu-option-button

Comment 1

[Julian Chu [:walkingice]]

Attachment: Bug 1348718 - Use cross icon as close button

Move this change to ActionBarPresenter. Use cross icon as close button
to match design spec.

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

Comment 2

[Julian Chu [:walkingice]]

Attachment: Bug 1348718 - Build option menu button in same way

There might be up to 2 Always-show-as-action button in ActionBar.
One for menu-options, one for 3rd-party app action-button, if any.

According to our design spec, the two buttons appearance should be
similiar, therefore now we create them by same method.

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

Comment 3

[Julian Chu [:walkingice]]

Attachment: Bug 1348718 - Remove useless method

No need to export text primary color anymore.

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

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8855780 [details]
Bug 1348718 - Use cross icon as close button

https://reviewboard.mozilla.org/r/127676/#review130398

::: mobile/android/base/java/org/mozilla/gecko/customtabs/ActionBarPresenter.java:152
(Diff revision 1)
>          return mTextPrimaryColor;
>      }
>  
> +    private void initIndicator() {
> +        mActionBar.setDisplayHomeAsUpEnabled(true);
> +        Drawable indicator = mActionBar.getThemedContext().getDrawable(R.drawable.ic_close_light);

nit: final

::: mobile/android/base/java/org/mozilla/gecko/customtabs/ActionBarPresenter.java:154
(Diff revision 1)
>  
> +    private void initIndicator() {
> +        mActionBar.setDisplayHomeAsUpEnabled(true);
> +        Drawable indicator = mActionBar.getThemedContext().getDrawable(R.drawable.ic_close_light);
> +        DrawableCompat.setTint(indicator, mTextPrimaryColor);
> +        mActionBar.setHomeAsUpIndicator(indicator);

I saw that there's a custom tabs API for setting a custom icon: setCloseButtonIcon(). Are we going to support this? Do we have a bug filed for that already?

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8855781 [details]
Bug 1348718 - Build option menu button in same way

https://reviewboard.mozilla.org/r/127678/#review130400

https://www.youtube.com/watch?v=9jK-NcRmVcw :)

::: mobile/android/base/java/org/mozilla/gecko/customtabs/ActionBarPresenter.java:133
(Diff revision 1)
> +        int size = res.getDimensionPixelSize(R.dimen.custom_tab_action_button_size);
> +        int padding = res.getDimensionPixelSize(R.dimen.custom_tab_action_button_padding);

final final :)

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java:218
(Diff revision 1)
>      public boolean onCreatePanelMenu(final int id, final Menu menu) {
> -        insertActionButton(menu, startIntent, actionBarPresenter.getTextPrimaryColor());
>  
> -        popupMenu = createCustomPopupMenu();
> +        // if 3rd-party app asks to add an action button
> +        if (IntentUtil.hasActionButton(startIntent)) {
> +            Bitmap bitmap = IntentUtil.getActionButtonIcon(startIntent);

final?

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8855782 [details]
Bug 1348718 - Remove useless method

https://reviewboard.mozilla.org/r/127680/#review130402

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Just saw that there's a test failure:
> :app:compileAutomationDebugUnitTestJavaWithJavac/home/worker/workspace/build/src/mobile/android/tests/background/junit4/src/org/mozilla/gecko/customtabs/TestCustomTabsActivity.java:124: error: cannot find symbol

Comment 8

[Julian Chu [:walkingice]]

Comment on attachment 8855781 [details]
Bug 1348718 - Build option menu button in same way

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

Comment 9

[Julian Chu [:walkingice]]

Comment on attachment 8855782 [details]
Bug 1348718 - Remove useless method

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

Comment 10

[Julian Chu [:walkingice]]

Comment on attachment 8855780 [details]
Bug 1348718 - Use cross icon as close button

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

Comment 11

[Julian Chu [:walkingice]]

Comment on attachment 8855781 [details]
Bug 1348718 - Build option menu button in same way

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

Comment 12

[Julian Chu [:walkingice]]

Comment on attachment 8855782 [details]
Bug 1348718 - Remove useless method

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

Comment 13

[Julian Chu [:walkingice]]

Comment on attachment 8855780 [details]
Bug 1348718 - Use cross icon as close button

https://reviewboard.mozilla.org/r/127676/#review130398

> I saw that there's a custom tabs API for setting a custom icon: setCloseButtonIcon(). Are we going to support this? Do we have a bug filed for that already?

Cannot believe I missed this method. Already file a bug for this(Bug 1354766), thanks!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/500fcd4b01fb
Use cross icon as close button r=sebastian
https://hg.mozilla.org/integration/autoland/rev/0aca46064d80
Build option menu button in same way r=sebastian
https://hg.mozilla.org/integration/autoland/rev/aa6ffa5c5b2c
Remove useless method r=sebastian

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/500fcd4b01fb
https://hg.mozilla.org/mozilla-central/rev/0aca46064d80
https://hg.mozilla.org/mozilla-central/rev/aa6ffa5c5b2c