943513 - Visual refinements to action bar mode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Ian Barlow (:ibarlow)]

We use the action bar for text selection controls now, which is awesome! Let's just make a few tweaks to polish the look. I'd like us to land these changes before we hit release with action bar mode. 

Before and after screens http://cl.ly/image/072Z1w3X0P1G

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This makes things look close to the mockup, plus a few other tiny bugs/tweaks. Takes a little extra work to style these buttons, since GeckoMenuItem tries to hide things by default.

1.) The Done button animates into view iff the urlbar is already showing when you flip to action mode. I also fade the actionbar, but that happens pretty quickly.

2.) Tweaked the bottom color of the bar to orange. Also tweaked the divider color since it was lighter in the mockup?

3.) Added a share icon stolen from Android. The share in their main repo has a little triangle in the corner to show its a menu ( http://androidxref.com/4.2.2_r1/xref/frameworks/base/core/res/res/drawable-mdpi/ic_menu_share_holo_light.png ). I erased it, but it means these are a little off center... They're also colored slightly different. I'm avoiding doing tweaks here unless UX asks me to, assuming they may want to design these icons?

4.) Added an mdpi select all menu item.

Screenshots:
HDPI ICS: http://dl.dropboxusercontent.com/u/72157/actionModeICS.png
MDPI GB:  http://dl.dropboxusercontent.com/u/72157/actionModeGB.png

Build:
http://people.mozilla.com/~wjohnston/actionmode.apk

Comment 2

[Wesley Johnston (:wesj)]

I should probably break these up into pieces. Let me do that for you lucas.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch 1/3 - Animation

And yeah, I know we could do this in a separate bug. For my sanity, I just wanted to do it all in one place.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch 2/3 - Add ability to style action bar items. Use it

This is the basis for most of this patch, letting us apply custom styles to these elements.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch 3/3 - Images and color tweaks

And this is the part that changes the background images, plus adds our missing icons.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8339561 [details] [diff] [review]
Patch 1/3 - Animation

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

Please wait for ibarlow's feedback before pushing.

::: mobile/android/base/ActionModeCompatView.java
@@ +160,5 @@
>  
> +    public void animateIn() {
> +        TranslateAnimation t = new TranslateAnimation(Animation.RELATIVE_TO_SELF, -0.5f, Animation.RELATIVE_TO_SELF, 0f,
> +                                                      Animation.RELATIVE_TO_SELF,  0f,   Animation.RELATIVE_TO_SELF, 0f);
> +        t.setDuration(getContext().getResources().getInteger(android.R.integer.config_shortAnimTime));

Gosh, we should probably add a util method somewhere with a shorter version to get shortAnimTime. Follow-up?

@@ +161,5 @@
> +    public void animateIn() {
> +        TranslateAnimation t = new TranslateAnimation(Animation.RELATIVE_TO_SELF, -0.5f, Animation.RELATIVE_TO_SELF, 0f,
> +                                                      Animation.RELATIVE_TO_SELF,  0f,   Animation.RELATIVE_TO_SELF, 0f);
> +        t.setDuration(getContext().getResources().getInteger(android.R.integer.config_shortAnimTime));
> +        mTitleView.startAnimation(t);

How does that interact with the viewflipper animation? Does this mean the animation to slide the title in will happen at the same time than the viewflipper's slide animation? Is this working well visually?

::: mobile/android/base/BrowserApp.java
@@ +2508,5 @@
> +                margins.showMargins(false);
> +            } else {
> +                // Otherwise, we animate the actionbar itself
> +                mActionBar.animateIn();
> +            }

nit: empty line here.

::: mobile/android/base/gfx/LayerMarginsAnimator.java
@@ +139,5 @@
>          mMarginsPinned = pin;
>      }
>  
> +    public boolean areMarginsShown() {
> +        ImmutableViewportMetrics metrics = mTarget.getViewportMetrics();

final

::: mobile/android/base/resources/anim/slide_in_ab.xml
@@ +1,1 @@
> +<?xml version="1.0" encoding="UTF-8"?>

The '_ab' suffix is not very obvious without context. '_actionbar' instead? Also, I wonder if prefixing makes more sense? Maybe actionbar_slide_out/actionbar_slide_in is more consistent?

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8339562 [details] [diff] [review]
Patch 2/3 - Add ability to style action bar items. Use it

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

I am giving r+ but please answer questions before pushing.

::: mobile/android/base/TextSelection.java
@@ +245,5 @@
>              for (int i = 0; i < length; i++) {
>                  try {
>                      final JSONObject obj = mItems.getJSONObject(i);
>                      final GeckoMenuItem menuitem = (GeckoMenuItem) menu.add(0, i, 0, obj.optString("label"));
> +                    menuitem.setShowAsAction(obj.optBoolean("showAsAction") ? 1 : 0, R.attr.menuItemActionModeStyle);

Just wondering: isn't the menu item style for action bars a property of the action bar itself? With this API you'd be able to set a different style for each menu item in the action mode bar, which feels a bit strange. Is that intended?

::: mobile/android/base/menu/GeckoMenuItem.java
@@ +342,5 @@
>      }
>  
>      @Override
>      public void setShowAsAction(int actionEnum) {
> +        setShowAsAction(actionEnum, org.mozilla.gecko.R.attr.menuItemActionBarStyle);

There's a potential mismatch between this default style here and the one defined in MenuItemActionBar's constructor, right? It would be good if we could avoid this duplication somehow. Thoughts?

::: mobile/android/base/resources/values/themes.xml
@@ +83,5 @@
>          <item name="topSitesThumbnailViewStyle">@style/Widget.TopSitesThumbnailView</item>
>          <item name="homeListViewStyle">@style/Widget.HomeListView</item>
>          <item name="menuItemDefaultStyle">@style/Widget.MenuItemDefault</item>
>          <item name="menuItemActionBarStyle">@style/Widget.MenuItemActionBar</item>
> +        <item name="menuItemActionModeStyle">@style/GeckoActionBar.Button</item>

Why isn't this called Widget.MenuItemActionMode or something similar? In any case, I'd expect the style name to have ActionMode in it ;-)

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8339564 [details] [diff] [review]
Patch 3/3 - Images and color tweaks

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

I'm a bit concerned about overdraws in this patch. Could you please check if there are any obvious regressions? Giving r- to get some input on this.

::: mobile/android/base/resources/layout/actionbar.xml
@@ +22,5 @@
> +    <View android:layout_height="fill_parent"
> +          android:layout_width="1dp"
> +          android:layout_marginTop="10dp"
> +          android:layout_marginBottom="10dp"
> +          android:background="@android:color/darker_gray"/>

Can't the same effect be achieved by using paddings/margins on existing views? What exactly is this view about?

::: mobile/android/base/resources/layout/gecko_app.xml
@@ +56,5 @@
>          </RelativeLayout>
>  
>          <ViewFlipper android:id="@+id/browser_actionbar"
>                  android:layout_width="fill_parent"
> +                android:background="@android:color/white"

This is likely yo cause overdraws in the toolbar area. Why is this necessary? How does the UI look (before and after) with 'Debug GPU overdraw' enabled?

::: mobile/android/base/resources/values/styles.xml
@@ +574,5 @@
>  
>      <style name="GeckoActionBar.Button">
>          <item name="android:layout_height">fill_parent</item>
>          <item name="android:layout_width">wrap_content</item>
> +        <item name="android:background">@drawable/action_bar_button</item>

This is also likely to cause overdraws, no?

::: mobile/android/chrome/content/SelectionHandler.js
@@ +358,5 @@
>          elt.value = elt.value.substring(0, start) + elt.value.substring(end)
>  
>          SelectionHandler._updateMenu();
>        },
> +      selector: ClipboardHelper.cutContext,

This is not about color/images, is it?

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch 1/3 - Animation

This animates the icons slightly too, which ian requested. Build is updated at http://people.mozilla.org/~wjohnston/actionmode.apk . I'm mostly following:

http://androidxref.com/4.2.2_r1/xref/frameworks/base/core/java/com/android/internal/widget/ActionBarContextView.java#402

which is using an ObjectAnimator. We could use PropertyAnimator here, but this always feels safer to me.

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 8341840 [details] [diff] [review]
Patch 1/3 - Animation

Actually, let me work through the other comments here first. Just wanted to send the build to ian.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch 1/3 - Animation stuff

I just pulled the duration into its own class. Only used here for now. I'll file a follow up to audit and use it in other places.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Patch 2/3 - Add styling ability

The overdraw aren't needed. I removed them. I also modified the style api to allow passing null. I agree this api is a bit weird. The strange thing is, we have two differently styled action bars now, so we have to be able to flip between both. Android makes it... not painless to do so. I don't know of a great way to do it.

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Patch 3/3 - Images and colors

The last patch here was the wrong one. Sorry, about that. To many similarly named patches in this queue :(

Comment 14

[Wesley Johnston (:wesj)]

(In reply to Wesley Johnston (:wesj) from comment #11)
> Created attachment 8341869 [details] [diff] [review]
> Patch 1/3 - Animation stuff
> 
> I just pulled the duration into its own class. Only used here for now. I'll
> file a follow up to audit and use it in other places.

I had to update this to initialize the duration to -1. Not going to add any more churn here for that.

Comment 15

[Wesley Johnston (:wesj)]

(In reply to Wesley Johnston (:wesj) from comment #11)
> Created attachment 8341869 [details] [diff] [review]
> Patch 1/3 - Animation stuff
> 
> I just pulled the duration into its own class. Only used here for now. I'll
> file a follow up to audit and use it in other places.

I also removed the fade here. I see Android doing one though (although I can't see it in their code...). Maybe it should be added back. Can't hurt to do this in smaller steps I guess.

Comment 16

[Lucas Rocha (:lucasr)]

Comment on attachment 8341869 [details] [diff] [review]
Patch 1/3 - Animation stuff

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

Looks good. I assume this has been ibarlow-approved?

::: mobile/android/base/animation/AnimationUtils.java
@@ +7,5 @@
> +package org.mozilla.gecko.animation;
> +
> +import android.content.Context;
> +
> +public class AnimationUtils {

The class name will clash with Android's own AnimationUtils. I wonder how annoying this will be in practice :-)

Comment 17

[Lucas Rocha (:lucasr)]

Comment on attachment 8341886 [details] [diff] [review]
Patch 3/3 - Images and colors

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

This is much simpler :-) Approved by ibarlow I assume.

Comment 18

[Lucas Rocha (:lucasr)]

Is 2/3 ready for review?

Comment 19

[Ian Barlow (:ibarlow)]

(In reply to Lucas Rocha (:lucasr) from comment #17)
> Comment on attachment 8341886 [details] [diff] [review]
> Patch 3/3 - Images and colors
> 
> Review of attachment 8341886 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is much simpler :-) Approved by ibarlow I assume.

Yes, I tried the build in comment 9 and it felt pretty solid :)

Comment 20

[Wesley Johnston (:wesj)]

Comment on attachment 8341885 [details] [diff] [review]
Patch 2/3 - Add styling ability

Yeah. The old patch was r+, but this is changed a bit.

Comment 21

[Lucas Rocha (:lucasr)]

Comment on attachment 8341885 [details] [diff] [review]
Patch 2/3 - Add styling ability

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

Nice.

::: mobile/android/base/resources/values/themes.xml
@@ +83,5 @@
>          <item name="topSitesThumbnailViewStyle">@style/Widget.TopSitesThumbnailView</item>
>          <item name="homeListViewStyle">@style/Widget.HomeListView</item>
>          <item name="menuItemDefaultStyle">@style/Widget.MenuItemDefault</item>
>          <item name="menuItemActionBarStyle">@style/Widget.MenuItemActionBar</item>
> +        <item name="menuItemActionModeStyle">@style/GeckoActionBar.Button</item>

Still wonder if GeckoActionBar.Button is a good name for this style. File a follow-up if you can think of a better name.

Comment 22

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/d379a9103c5b

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d379a9103c5b

Comment 24

[Wesley Johnston (:wesj)]

Grr. Forgot to make this change before landing, so animations all ran with zero duration:
https://hg.mozilla.org/integration/fx-team/rev/2c42e70b08c2

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2c42e70b08c2