Make ActionMode work on Gingerbread

RESOLVED FIXED in Firefox 28

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

unspecified
Firefox 28
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
I want actionmode to work for about:home customization. It also seems like it might be useful for some other stuff.
(Assignee)

Comment 1

6 years ago
Created attachment 687243 [details] [diff] [review]
Early WIP

This doesn't work completely yet, and I need to add a compatibility layer so that on HC+ we use the real native interfaces.
You might want to look at ActionBarSherlock's source code for inspiration or even for borrowing some code:

  https://github.com/JakeWharton/ActionBarSherlock
(Assignee)

Comment 3

6 years ago
Created attachment 688599 [details] [diff] [review]
WIP2

This is mostly working (but doesn't fall back to native on HC+, and maybe needs some theme love). The ViewFlipper animations also don't show. Wanted to start the review process for it now and try to handle other quirks in little follow up patches (my queue is getting to big and my fingers are in too many files at once).
Attachment #687243 - Attachment is obsolete: true
Attachment #688599 - Flags: review?(sriram)
Comment on attachment 688599 [details] [diff] [review]
WIP2

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

Overall:
1. I haven't seen ActionModeCompat. It's not | hg add | ed here.
2. ViewFlipper could be replaced with ViewAnimator.
3. We would be supporting a custom "action-mode" with blueeee color. Please talk to ibarlow about it.

Please test it on all phones/tablets. This patch would break few existing behaviors.
I like the approach of using ActionBarPresenter. That part looks good. You might want to rebase to get the new Menu refactored code from Bug 818229. That changes the menu-item click and display methods.

::: mobile/android/base/BrowserApp.java
@@ +9,5 @@
>  import org.mozilla.gecko.db.BrowserDB;
>  import org.mozilla.gecko.gfx.BitmapUtils;
>  import org.mozilla.gecko.util.GeckoAsyncTask;
>  import org.mozilla.gecko.util.GeckoBackgroundThread;
> +import org.mozilla.gecko.widget.ActionModeCompat;

Please rename it to GeckoActionMode. That's the notion we follow everywhere.

@@ +45,5 @@
> +import android.widget.Button;
> +import android.widget.ViewFlipper;
> +import android.widget.ImageButton;
> +import android.widget.ImageView;
> +

"import"s are done in alphabetical order.

@@ +209,5 @@
>          mAboutHomeStartupTimer = new Telemetry.Timer("FENNEC_STARTUP_TIME_ABOUTHOME");
>  
>          super.onCreate(savedInstanceState);
>  
> +        ViewFlipper actionBar = (ViewFlipper) getActionBarLayout();

I would suggest using ViewAnimator. Because, we aren't really flipping views (like a flip action). And I guess ViewFlipper does that. So, we don't want to accidentally allow users to flip to action-mode.

@@ +320,5 @@
>          if (Build.VERSION.SDK_INT >= 14 && !isTablet()) {
>              int index = mMainLayout.indexOfChild(mBrowserToolbar.getLayout());
>              mMainLayout.removeViewAt(index);
>  
> +            ViewFlipper actionBar = (ViewFlipper) getActionBarLayout();

ViewAnimator please.

@@ +1104,5 @@
>              }
>          }).execute();
>      }
> +
> +    public Object startActionMode(ActionModeCompat.Callback callback) {

This should return ActionModeCompat (now GeckoActionMode). Since Java would get confused between Android's startActionMode() signature and ours, we should probably rename this to something else. "startGeckoActionMode()" may be.

@@ +1105,5 @@
>          }).execute();
>      }
> +
> +    public Object startActionMode(ActionModeCompat.Callback callback) {
> +        // TODO: If this is a honeycomb machine, use native?

I was talking to ibarlow. He suggests a custom action mode for Firefox that's same across all devices.

@@ +1116,5 @@
> +
> +        mode.show();
> +        ((ViewFlipper)mBrowserToolbar.getLayout()).showNext();
> +        return mode;
> +        return null;

This should return the mode.

@@ +1119,5 @@
> +        return mode;
> +        return null;
> +    }
> +
> +    public void onActionModeFinished(ActionModeCompat mode) {

onGeckoActionModeFinished() is better. Also, shouldn't BrowserApp be a listener for ActionModeCompact.Callback? If so, please add "@Override" notations.

::: mobile/android/base/BrowserToolbar.java
@@ +123,5 @@
>          Tabs.registerOnTabsChangedListener(this);
>          mAnimateSiteSecurity = true;
>      }
>  
> +    public void from(ViewFlipper layout) {

This sounds a bit scary. We should be able to get the "mLayout" as the RelativeLayout in it and the "mActionMode" as another private variable. That makes things easier to understand.

@@ +1058,5 @@
>          }
>      }
>  
>      @Override
> +    public void addActionItem(GeckoMenuItem actionItem) {

I would refrain from using GeckoMenuItem directly. Ideally GeckoMenuItem is a logical entity. This listener should just know a "View" -- a physical entity -- that should be added.

::: mobile/android/base/GeckoMenu.java
@@ +20,5 @@
>  import android.widget.BaseAdapter;
>  import android.widget.LinearLayout;
>  import android.widget.ListView;
>  import android.widget.LinearLayout.LayoutParams;
> +import android.util.Log;

We don't log anything.

@@ +36,5 @@
>  
>      private Context mContext;
>  
>      public static interface ActionItemBarPresenter {
> +        public void addActionItem(GeckoMenuItem item);

GeckoMenuItem is a logical entity. Pleas use a physical generic entity -- View.

@@ +135,5 @@
>              setAdapter(mAdapter);
>          }
>  
>          mActionItems.add(menuItem);
> +        mActionItemBarPresenter.addActionItem(menuItem);

Old one is better.

@@ +371,5 @@
>              mItems = new ArrayList<View>();
>          }
>  
>          @Override
> +        public void addActionItem(GeckoMenuItem actionItem) {

View please.

::: mobile/android/base/GeckoMenuInflater.java
@@ +10,5 @@
>  import android.content.Context;
>  import android.content.res.TypedArray;
>  import android.content.res.XmlResourceParser;
>  import android.util.AttributeSet;
> +import android.util.Log;

We don't log.

@@ +134,5 @@
>          item.checkable = a.getBoolean(R.styleable.MenuItem_android_checkable, false);
>          item.checked = a.getBoolean(R.styleable.MenuItem_android_checked, false);
>          item.visible = a.getBoolean(R.styleable.MenuItem_android_visible, true);
>          item.enabled = a.getBoolean(R.styleable.MenuItem_android_enabled, true);
> +        item.showAsAction = a.getInt(R.styleable.MenuItem_showAsAction, 0);

The bug 818229 changed this. I would suggest building up on that.

if (Build.VERSION.SDK_INT >= 11)
    item.showAsAction = a.getInt(R.styleable.MenuItem_android_showAsAction, 0);
else
    item.showAsAction = a.getInt(R.styleable.MenuItem_showAsAction, 0);

@@ +146,5 @@
>                  .setEnabled(item.enabled)
>                  .setCheckable(item.checkable)
> +                .setIcon(item.iconRes);
> +        GeckoMenuItem gItem = (GeckoMenuItem)menuItem;
> +        gItem.setShowAsAction(item.showAsAction);

Cast only if it's GB and lower.

::: mobile/android/base/GeckoMenuItem.java
@@ +11,5 @@
>  import android.view.ContextMenu;
>  import android.view.MenuItem;
>  import android.view.SubMenu;
>  import android.view.View;
> +import android.util.Log;

No logs.

@@ +28,5 @@
>          public void setChecked(boolean checked);
>          public void setOnClickListener(View.OnClickListener listener);
>          public void setSubMenuIndicator(boolean hasSubMenu);
>          public void setVisibility(int visible);
> +        public View getViewLayout();

Is there a reason for changing this signature?

@@ +64,5 @@
> +    private static final int SHOW_AS_ACTION_IF_ROOM = 1;
> +    private static final int SHOW_AS_ACTION_ALWAYS = 2;
> +    private static final int SHOW_AS_ACTION_WITH_TEXT = 4;
> +    private static final int SHOW_AS_ACTION_COLLAPSE_ACTION_VIEW = 8;
> +

Please add the ones we support. Rest will default.

@@ +311,4 @@
>          mLayout.setId(mId);
>          mLayout.setOnClickListener(this);
>  
>          setTitle(mTitle);        

Space added?

@@ +315,5 @@
>          setVisible(mVisible);
>          setEnabled(mEnabled);
>          setCheckable(mCheckable);
>          setChecked(mChecked);
> +        ((MenuItemActionBar)mLayout).setShowText((actionEnum & SHOW_AS_ACTION_WITH_TEXT) > 0);

Casting is unnecessary.

An extra check in code: 
        if (mActionItem == (actionEnum > 0))
            return;
Should be handled properly as well. So, we need to store the actionEnum.

::: mobile/android/base/Makefile.in
@@ +180,5 @@
>    ui/PanZoomTarget.java \
>    ui/SimpleScaleGestureDetector.java \
>    ui/SubdocumentScrollHelper.java \
>    widget/DateTimePicker.java \
> +  widget/ActionModeCompat.java \

Nothing has gone into "widget/" directly yet. I would suggest moving it outside. Let the namespace change be separate. (And if done, I recommend shadowing Android's namespace. "org.mozilla.gecko.app.GeckoActionMode").

@@ -431,5 @@
>  RES_LAYOUT_LARGE_V11 = \
>    res/layout-large-v11/doorhangerpopup.xml \
>    res/layout-large-v11/site_identity_popup.xml \
>    res/layout-large-v11/tabs_panel_toolbar_menu.xml \
> -  res/layout-large-v11/abouthome_topsites_edit.xml \

Where did this go?

@@ +506,5 @@
>    res/drawable/favicon.png \
>    res/drawable/folder.png \
> +  res/drawable/ab_bottom_solid_light_holo.9.png \
> +  res/drawable/ab_transparent_dark_holo.9.png \
> +  res/drawable/ic_cab_done_holo_light.png \

ibarlow confirmed me that we would use Firefox specific custom action-mode. We don't need this resources. (Also, hdpi and xhdpi versions are missing).

@@ +566,5 @@
>    res/drawable/ic_clear_normal.png \
>    res/drawable/larry_blue.png \
>    res/drawable/larry_green.png \
>    res/drawable/menu.xml \
> +  res/drawable/menuicon.png \

What is this icon? Where is it shown in action-mode? We have our own icon.

@@ +585,5 @@
> +  res/drawable/menu_popup_bg.9.png \
> +  res/drawable/menu_popup_arrow.png \
> +  res/drawable/menu_item_check.png \
> +  res/drawable/menu_item_more.png \
> +  res/drawable/menu_item_uncheck.png \

This is done as a part of bug 818229.

@@ +1089,5 @@
>  MOZ_BRANDING_DRAWABLE_MDPI = $(shell if test -e $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/android-resources.mn; then cat $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/android-resources.mn | tr '\n' ' ';  fi)
>  MOZ_BRANDING_DRAWABLE_HDPI = $(shell if test -e $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/android-resources-hdpi.mn; then cat $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/android-resources-hdpi.mn | tr '\n' ' ';  fi)
>  MOZ_BRANDING_DRAWABLE_XHDPI = $(shell if test -e $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/android-resources-xhdpi.mn; then cat $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/android-resources-xhdpi.mn | tr '\n' ' ';  fi)
>  
> +RESOURCES=$(RES_LAYOUT) $(RES_LAYOUT_LAND_V14) $(RES_LAYOUT_V11) $(RES_LAYOUT_LARGE_V11) $(RES_LAYOUT_XLARGE_V11) $(RES_VALUES) $(RES_VALUES_LAND) $(RES_VALUES_V11) $(RES_VALUES_LARGE_V11) $(RES_VALUES_XLARGE_V11) $(RES_VALUES_LAND_V14) $(RES_XML) $(RES_ANIM) $(RES_DRAWABLE_NODPI) $(RES_DRAWABLE_BASE) $(RES_DRAWABLE_LDPI) $(RES_DRAWABLE_HDPI) $(RES_DRAWABLE_XHDPI) $(RES_DRAWABLE_MDPI_V11) $(RES_DRAWABLE_HDPI_V11) $(RES_DRAWABLE_XHDPI_V11) $(RES_DRAWABLE_LAND_V14) $(RES_DRAWABLE_LAND_MDPI_V14) $(RES_DRAWABLE_LAND_HDPI_V14) $(RES_DRAWABLE_LAND_XHDPI_V14) $(RES_DRAWABLE_LARGE_MDPI_V11) $(RES_DRAWABLE_LARGE_HDPI_V11) $(RES_DRAWABLE_LARGE_XHDPI_V11) $(RES_DRAWABLE_XLARGE_MDPI_V11) $(RES_DRAWABLE_XLARGE_HDPI_V11) $(RES_DRAWABLE_XLARGE_XHDPI_V11) $(RES_COLOR) $(RES_MENU)

RES_LAYOUT_V11 is added here. But it's not found anywhere else in Makefile.in.

::: mobile/android/base/MenuItemActionBar.java
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  package org.mozilla.gecko;
>  
> +import android.graphics.Color;

Move down. Alphabetisize (is that a word?)

@@ +15,3 @@
>  import android.widget.ImageButton;
>  import android.widget.ImageView;
> +import android.graphics.PorterDuff;

Move up.

@@ +18,2 @@
>  
> +public class MenuItemActionBar extends Button

I doubt using "Button" (reason below). It's better to re-use MenuItemDefault's layout if possible.

@@ +37,5 @@
>      }
>  
>      @Override
>      public void setIcon(Drawable icon) {
> +        setCompoundDrawablesWithIntrinsicBounds(icon,null,null,null);

This is 17 only. Will fail on ICS even. http://developer.android.com/reference/android/widget/TextView.html#setCompoundDrawablesRelativeWithIntrinsicBounds%28android.graphics.drawable.Drawable,%20android.graphics.drawable.Drawable,%20android.graphics.drawable.Drawable,%20android.graphics.drawable.Drawable%29 So, better with a LinearLayout.

The visibility checks are missing.

@@ +42,5 @@
>      }
>  
>      @Override
>      public void setIcon(int icon) {
> +        setCompoundDrawablesWithIntrinsicBounds(icon,0,0,0);

^^ See above.

@@ +46,5 @@
> +        setCompoundDrawablesWithIntrinsicBounds(icon,0,0,0);
> +    }
> +
> +    private String mTitle = "";
> +    private boolean mShowTitle = false;

This should be moved up.

@@ +49,5 @@
> +    private String mTitle = "";
> +    private boolean mShowTitle = false;
> +
> +    @Override
> +    public void setShowText(boolean aShow) {

We don't use "aShow" in Java generally. "show" is better.

@@ +50,5 @@
> +    private boolean mShowTitle = false;
> +
> +    @Override
> +    public void setShowText(boolean aShow) {
> +        Log.i(LOGTAG, "Set Show Text: " + aShow + " " + mTitle);

Please remove this.

@@ +52,5 @@
> +    @Override
> +    public void setShowText(boolean aShow) {
> +        Log.i(LOGTAG, "Set Show Text: " + aShow + " " + mTitle);
> +        if (aShow) setText(mTitle);
> +        else setText("");

This is why I feel a LinearLayout to be better. It gets a bit cryptic here.

@@ +53,5 @@
> +    public void setShowText(boolean aShow) {
> +        Log.i(LOGTAG, "Set Show Text: " + aShow + " " + mTitle);
> +        if (aShow) setText(mTitle);
> +        else setText("");
> +        mShowTitle = aShow;

Better to be first line in this method.

@@ +61,5 @@
>      public void setTitle(CharSequence title) {
>          // set accessibility contentDescription here
> +        mTitle = title.toString();
> +        if (mShowTitle)
> +            setText(mTitle);

A line break.

@@ +72,5 @@
> +
> +        Drawable[] drawables = getCompoundDrawables();
> +        for (int i = 0; i < drawables.length; i++) {
> +            if (drawables[i] != null)
> +                drawables[i].setColorFilter(enabled ? 0 : 0xFF999999, PorterDuff.Mode.MULTIPLY);

Why is this a MULTIPLY?

::: mobile/android/base/MenuItemDefault.java
@@ +70,5 @@
>              mIcon.setVisibility(GONE);
>          }
>      }
>  
> +    public void setShowText(boolean show) {

"@Override" annotation.
A comment like "//unused" or something.

::: mobile/android/base/resources/layout-land-v14/browser_toolbar.xml.in
@@ +170,5 @@
>      </RelativeLayout>
>  
> +    <include layout="@layout/actionbar"/>
> +
> +</ViewFlipper>

ViewAnimator maybe?

<include/> is expensive. And this is in startup path.

::: mobile/android/base/resources/layout-land-v14/browser_toolbar_menu.xml.in
@@ +196,5 @@
>      </RelativeLayout>
>  
> +    <include layout="@layout/actionbar"/>
> +
> +</ViewFlipper>

ViewAnimator maybe?

<include/> is expensive. And this is in startup path.

::: mobile/android/base/resources/layout-large-v11/browser_toolbar_menu.xml.in
@@ +197,5 @@
>      </RelativeLayout>
>  
> +    <include layout="@layout/actionbar"/>
> +
> +</ViewFlipper>

ViewAnimator maybe?

<include/> is expensive. And this is in startup path.

::: mobile/android/base/resources/layout-xlarge-v11/browser_toolbar_menu.xml.in
@@ +190,5 @@
>      </RelativeLayout>
>  
> +    <include layout="@layout/actionbar"/>
> +
> +</ViewFlipper>

ViewAnimator maybe?

<include/> is expensive. And this is in startup path.

::: mobile/android/base/resources/layout/actionbar.xml
@@ +1,5 @@
> +<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
> +              style="@style/GeckoActionBar"
> +              android:id="@+id/actionbar">
> +
> +    <RelativeLayout style="@style/GeckoActionBar.Internal"

Do we really need these styles? It's used in one place. We could just define them here.

@@ +4,5 @@
> +
> +    <RelativeLayout style="@style/GeckoActionBar.Internal"
> +                  android:id="@+id/actionbar_internal">
> +
> +        <!-- TODO: Add title and subtitle views -->

What's this?

@@ +6,5 @@
> +                  android:id="@+id/actionbar_internal">
> +
> +        <!-- TODO: Add title and subtitle views -->
> +
> +        <Button android:id="@+id/actionmode_close"

"done" is better. As the text would read so.

@@ +13,5 @@
> +        <LinearLayout style="@style/GeckoActionBar.Buttons"
> +                      android:id="@+id/actionbar_buttons"/>
> +
> +        <ImageButton style="@style/GeckoActionBar.Button.MenuButton"
> +                     android:id="@+id/actionbar_menu"/>

I haven't seen a menu button in action-mode. Could you point me to some screenshots?

::: mobile/android/base/resources/values-v11/styles.xml
@@ +43,5 @@
>      <!-- Lists in AwesomeBar -->
>      <style name="AwesomeBarList" parent="@style/GeckoList"/>
>   
>      <!-- ActionBar -->
> +    <style name="ActionBar" parent="android:style/Widget.Holo.ActionBar.Solid" />

Please test this change in "AwesomeBar".

@@ +47,5 @@
> +    <style name="ActionBar" parent="android:style/Widget.Holo.ActionBar.Solid" />
> +
> +    <style name="GeckoActionBar" parent="ActionBar">
> +        <item name="android:layout_width">fill_parent</item>
> +        <item name="android:layout_height">fill_parent</item>

This can go into actual XML.

@@ +52,5 @@
> +    </style>
> +
> +    <style name="GeckoActionBar.Internal" parent="android:style/Widget.Holo.ActionBar">
> +        <item name="android:layout_width">fill_parent</item>
> +        <item name="android:layout_height">fill_parent</item>

This can go into actual XML.

@@ +58,5 @@
> +
> +    <style name="GeckoActionBar.Button.CloseButton">
> +        <item name="android:layout_gravity">center</item>
> +        <item name="android:scaleType">fitCenter</item>
> +        <item name="android:text">Done</item>

Should be defined as a string.

@@ +60,5 @@
> +        <item name="android:layout_gravity">center</item>
> +        <item name="android:scaleType">fitCenter</item>
> +        <item name="android:text">Done</item>
> +        <item name="android:layout_alignParentLeft">true</item>
> +        <item name="android:drawableLeft">?android:attr/actionModeCloseDrawable</item>

Is this available all the way from 11? Anyways better to have an orange one :D

::: mobile/android/base/resources/values/attrs.xml
@@ +19,5 @@
>          <attr name="android:checkable"/>
>          <attr name="android:checked"/>
>          <attr name="android:visible"/>
>          <attr name="android:enabled"/>
> +        <attr name="description" format="string"/>

"android:decription" ?

@@ +21,5 @@
>          <attr name="android:visible"/>
>          <attr name="android:enabled"/>
> +        <attr name="description" format="string"/>
> +        <attr name="showAsAction">
> +          <flag name="never" value="0x00" />

4 space indent.

@@ +26,5 @@
> +          <flag name="ifRoom" value="0x01" />
> +          <flag name="always" value="0x02" />
> +          <flag name="withText" value="0x04" />
> +          <flag name="collapseActionView" value="0x04" />
> +        </attr>

My MenuInflater comment asked to check for build. This is how it works.

In this file, we would shadow the android's action-bar values.

Create a file in values-v11/attrs.xml which will have "android:showAsAction" under this. This particular change here will definitely break on tablets. Refresh/Bookmark will not be shown in BrowserToolbar.

::: mobile/android/base/resources/values/dimens.xml
@@ +30,5 @@
>      <dimen name="menu_item_row_height">44dp</dimen>
>      <dimen name="menu_item_row_width">240dp</dimen>
>      <dimen name="menu_popup_width">256dp</dimen>
>      <dimen name="menu_popup_offset">8dp</dimen>
> +    <dimen name="menu_popup_arrow_margin">5dip</dimen>

Bug 818229 does it.

::: mobile/android/base/resources/values/styles.xml
@@ +49,5 @@
>          <item name="android:layout_width">fill_parent</item>
>          <item name="android:layout_height">@dimen/browser_toolbar_height</item>
>          <item name="android:orientation">horizontal</item>
> +        <item name="android:inAnimation">?android:anim/slide_in_left</item>
> +        <item name="android:outAnimation">?android:anim/slide_out_right</item>

I think it should flip and not slide in/out.

@@ +67,5 @@
>          <item name="android:background">@android:color/transparent</item>
>      </style>
>  
> +    <style name="GeckoActionBar" parent="AddressBar">
> +        <item name="android:background">@drawable/ab_bottom_solid_light_holo</item>

Custom action-mode. Hence a blue background like "address_bar_url_bg"

@@ +71,5 @@
> +        <item name="android:background">@drawable/ab_bottom_solid_light_holo</item>
> +    </style>
> +
> +    <style name="GeckoActionBar.Internal" parent="AddressBar">
> +        <item name="android:background">@drawable/ab_transparent_dark_holo</item>

Custom action-mode. Hence a blue background like "address_bar_url_bg"

@@ +79,5 @@
> +        <item name="android:layout_height">fill_parent</item>
> +        <item name="android:layout_width">wrap_content</item>
> +        <item name="android:background">@android:color/transparent</item>
> +        <item name="android:layout_gravity">center_vertical</item>
> +        <item name="android:textColor">@+color/tabs_counter_color</item>

"@+" will define a new color. We already have that color defined. It's should be "@color". And this will change with blue (and oh! may be black for private-browsing).

@@ +85,5 @@
> +
> +    <style name="GeckoActionBar.Button.CloseButton">
> +        <item name="android:layout_gravity">center</item>
> +        <item name="android:scaleType">fitCenter</item>
> +        <item name="android:text">Done</item>

Should be an "@string/xyz" value.

@@ +89,5 @@
> +        <item name="android:text">Done</item>
> +        <item name="android:textColor">#000</item>
> +        <item name="android:layout_alignParentLeft">true</item>
> +        <item name="android:background">@android:color/transparent</item>
> +        <item name="android:drawableLeft">@drawable/ic_cab_done_holo_light</item>

Custom Firefox-ish "Done" button.

@@ +96,5 @@
> +    <style name="GeckoActionBar.Button.MenuButton">
> +        <item name="android:scaleType">center</item>
> +        <item name="android:layout_width">45dip</item>
> +        <item name="android:layout_height">fill_parent</item>
> +        <item name="android:layout_alignParentRight">true</item>

I always refrain from specifies rules in styles.xml.

@@ +97,5 @@
> +        <item name="android:scaleType">center</item>
> +        <item name="android:layout_width">45dip</item>
> +        <item name="android:layout_height">fill_parent</item>
> +        <item name="android:layout_alignParentRight">true</item>
> +        <item name="android:src">@drawable/menuicon</item>

Isn't this same as "@drawable/menu" ?

@@ +102,5 @@
> +    </style>
> +
> +    <style name="GeckoActionBar.Buttons">
> +        <item name="android:layout_gravity">right</item>
> +        <item name="android:layout_toLeftOf">@+id/actionbar_menu</item>

"@+" will add a new id. This id is defined. Also, it's good to have these rules in XML. It gets cryptic when such things are specified in styles.xml. We cannot know such a thing actually exists.

@@ +107,5 @@
> +        <item name="android:background">@android:color/transparent</item>
> +        <item name="android:textColor">#000</item>
> +    </style>
> +
> +    <style name="GeckoActionBar.Internal">

Already defined in same file.
Attachment #688599 - Flags: review?(sriram) → review-
(Assignee)

Comment 5

5 years ago
Fixed in bug 768667
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Assignee: nobody → wjohnston
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.