Remove text button container from MenuItems
Right now, with bordered MenuItems, the rotary
highligh is a box around the bordered button
in a way that doesn't quite line up with it.
To fix that, we should be focusing the actual
clickable buttons, not containers, and then
setting the rotary highlight correctly on
those buttons.
This cl removes the text container, and makes
the focus apply directly to each button/switch.
The icon is a little different because it
has both a foreground and background ImageView.
Note that this cl reverts the blue rotary
box around the textbutton to a simple ripple.
We should update the buttonStyle and switchStyle
in Theme.DeviceDefault to include a rotary
highlight that makes sense for them.
Test: Manually
Bug: 174490425
Change-Id: Ic047ddbb57263ec430dcf55cdb03ff158200f5b6
diff --git a/car-ui-lib/car-ui-lib/src/main/java/com/android/car/ui/toolbar/MenuItemRenderer.java b/car-ui-lib/car-ui-lib/src/main/java/com/android/car/ui/toolbar/MenuItemRenderer.java
index 41b6bef..ca3364b 100644
--- a/car-ui-lib/car-ui-lib/src/main/java/com/android/car/ui/toolbar/MenuItemRenderer.java
+++ b/car-ui-lib/car-ui-lib/src/main/java/com/android/car/ui/toolbar/MenuItemRenderer.java
@@ -64,12 +64,9 @@
private View mIconContainer;
private ImageView mIconView;
private Switch mSwitch;
- private View mTextContainer;
private TextView mTextView;
private TextView mTextWithIconView;
-
- /** Whether the layout file supports rotary mode. */
- private boolean mIsRotaryEnabledLayout;
+ private boolean mIndividualClickListeners;
MenuItemRenderer(MenuItem item, ViewGroup parentView) {
mMenuItem = item;
@@ -110,13 +107,11 @@
requireViewByRefId(mView, R.id.car_ui_toolbar_menu_item_icon_container);
mIconView = requireViewByRefId(mView, R.id.car_ui_toolbar_menu_item_icon);
mSwitch = requireViewByRefId(mView, R.id.car_ui_toolbar_menu_item_switch);
- // mTextContainer is only available in rotary enabled layout.
- mTextContainer =
- CarUiUtils.findViewByRefId(mView, R.id.car_ui_toolbar_menu_item_text_container);
- mIsRotaryEnabledLayout = mTextContainer != null;
mTextView = requireViewByRefId(mView, R.id.car_ui_toolbar_menu_item_text);
mTextWithIconView =
requireViewByRefId(mView, R.id.car_ui_toolbar_menu_item_text_with_icon);
+ mIndividualClickListeners = mView.getContext().getResources()
+ .getBoolean(R.bool.car_ui_toolbar_menuitem_individual_click_listeners);
updateView();
callback.accept(mView);
@@ -144,38 +139,31 @@
mView.setVisibility(View.VISIBLE);
mView.setContentDescription(mMenuItem.getTitle());
- int iconContainerVisibility = View.GONE;
- int textContainerVisibility = View.GONE;
- mTextView.setVisibility(View.GONE);
- mTextWithIconView.setVisibility(View.GONE);
- mSwitch.setVisibility(View.GONE);
+ View clickTarget;
if (checkable) {
mSwitch.setChecked(mMenuItem.isChecked());
- mSwitch.setVisibility(View.VISIBLE);
- if (mIsRotaryEnabledLayout) {
- textContainerVisibility = View.VISIBLE;
- } else {
- iconContainerVisibility = View.VISIBLE;
- }
+ clickTarget = mSwitch;
} else if (hasText && hasIcon && textAndIcon) {
mMenuItem.getIcon().setBounds(0, 0, mMenuItemIconSize, mMenuItemIconSize);
mTextWithIconView.setCompoundDrawables(mMenuItem.getIcon(), null, null, null);
mTextWithIconView.setText(mMenuItem.getTitle());
- mTextWithIconView.setVisibility(View.VISIBLE);
- textContainerVisibility = View.VISIBLE;
+ clickTarget = mTextWithIconView;
} else if (hasIcon) {
mIconView.setImageDrawable(mMenuItem.getIcon());
- iconContainerVisibility = View.VISIBLE;
+ clickTarget = mIconContainer;
} else { // hasText will be true
mTextView.setText(mMenuItem.getTitle());
- mTextView.setVisibility(View.VISIBLE);
- textContainerVisibility = View.VISIBLE;
+ clickTarget = mTextView;
}
- // Unlike other views, we should only update the visibility of mIconContainer and
- // mTextContainer once, otherwise rotary focus might break.
- mIconContainer.setVisibility(iconContainerVisibility);
- if (mTextContainer != null) {
- mTextContainer.setVisibility(textContainerVisibility);
+
+ mIconContainer.setVisibility(clickTarget == mIconContainer ? View.VISIBLE : View.GONE);
+ mTextView.setVisibility(clickTarget == mTextView ? View.VISIBLE : View.GONE);
+ mTextWithIconView.setVisibility(clickTarget == mTextWithIconView
+ ? View.VISIBLE : View.GONE);
+ mSwitch.setVisibility(clickTarget == mSwitch ? View.VISIBLE : View.GONE);
+
+ if (!mIndividualClickListeners) {
+ clickTarget = mView;
}
if (!mMenuItem.isTinted() && hasIcon) {
@@ -185,19 +173,13 @@
recursiveSetEnabledAndDrawableState(mView);
mView.setActivated(mMenuItem.isActivated());
- View clickTarget = null;
- if (mIsRotaryEnabledLayout) {
- clickTarget = iconContainerVisibility == View.VISIBLE ? mIconContainer : mTextContainer;
- } else {
- clickTarget = mView;
- }
if (mMenuItem.getOnClickListener() != null
|| mMenuItem.isCheckable()
|| mMenuItem.isActivatable()) {
clickTarget.setOnClickListener(v -> mMenuItem.performClick());
- } else {
- clickTarget.setOnClickListener(null);
- clickTarget.setClickable(false);
+ } else if (clickTarget == mView) {
+ mView.setOnClickListener(null);
+ mView.setClickable(false);
}
}
diff --git a/car-ui-lib/car-ui-lib/src/main/res-overlayable/values/overlayable.xml b/car-ui-lib/car-ui-lib/src/main/res-overlayable/values/overlayable.xml
index 3ad5398..b6f00f0 100644
--- a/car-ui-lib/car-ui-lib/src/main/res-overlayable/values/overlayable.xml
+++ b/car-ui-lib/car-ui-lib/src/main/res-overlayable/values/overlayable.xml
@@ -84,6 +84,7 @@
<item type="bool" name="car_ui_preference_show_chevron"/>
<item type="bool" name="car_ui_scrollbar_enable"/>
<item type="bool" name="car_ui_toolbar_logo_fills_nav_icon_space"/>
+ <item type="bool" name="car_ui_toolbar_menuitem_individual_click_listeners"/>
<item type="bool" name="car_ui_toolbar_nav_icon_reserve_space"/>
<item type="bool" name="car_ui_toolbar_show_logo"/>
<item type="bool" name="car_ui_toolbar_tab_flexible_layout"/>
@@ -347,7 +348,6 @@
<item type="id" name="car_ui_toolbar_menu_item_icon_container"/>
<item type="id" name="car_ui_toolbar_menu_item_switch"/>
<item type="id" name="car_ui_toolbar_menu_item_text"/>
- <item type="id" name="car_ui_toolbar_menu_item_text_container"/>
<item type="id" name="car_ui_toolbar_menu_item_text_with_icon"/>
<item type="id" name="car_ui_toolbar_menu_items_container"/>
<item type="id" name="car_ui_toolbar_nav_icon"/>
diff --git a/car-ui-lib/car-ui-lib/src/main/res/values/bools.xml b/car-ui-lib/car-ui-lib/src/main/res/values/bools.xml
index b34d620..ccf9c4d 100644
--- a/car-ui-lib/car-ui-lib/src/main/res/values/bools.xml
+++ b/car-ui-lib/car-ui-lib/src/main/res/values/bools.xml
@@ -30,6 +30,8 @@
row, replacing the title -->
<bool name="car_ui_toolbar_tabs_on_second_row">false</bool>
+ <bool name="car_ui_toolbar_menuitem_individual_click_listeners">false</bool>
+
<!-- CarUiRecyclerView -->
<!-- Whether to display the Scroll Bar or not. Defaults to true. If this is set to false,
diff --git a/car-ui-lib/car-ui-lib/src/main/res/values/ids.xml b/car-ui-lib/car-ui-lib/src/main/res/values/ids.xml
index 44a162e..5b6f651 100644
--- a/car-ui-lib/car-ui-lib/src/main/res/values/ids.xml
+++ b/car-ui-lib/car-ui-lib/src/main/res/values/ids.xml
@@ -17,9 +17,6 @@
<!-- Id used for the search button when using Toolbar.createSearch() method -->
<item name="search" type="id"/>
- <!-- Id used for in car_ui_toolbar_menu_item.xml -->
- <item name="car_ui_toolbar_menu_item_text_container" type="id"/>
-
<!-- WideScreen Keyboard IDs-->
<item type="id" name="car_ui_wideScreenInputArea"/>
<item type="id" name="car_ui_imeWideScreenInputArea"/>
diff --git a/car-ui-lib/referencedesign/res/layout/car_ui_toolbar_menu_item.xml b/car-ui-lib/referencedesign/res/layout/car_ui_toolbar_menu_item.xml
index be95cc1..0a6b24c 100644
--- a/car-ui-lib/referencedesign/res/layout/car_ui_toolbar_menu_item.xml
+++ b/car-ui-lib/referencedesign/res/layout/car_ui_toolbar_menu_item.xml
@@ -24,7 +24,7 @@
style="@style/Widget.CarUi.Toolbar.MenuItem.IndividualContainer"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
- android:background="@drawable/car_ui_toolbar_menu_item_icon_ripple">
+ android:layout_gravity="center">
<ImageView
android:layout_width="match_parent"
android:layout_height="match_parent"
@@ -39,39 +39,26 @@
android:tint="@color/car_ui_toolbar_menu_item_icon_color"
android:tintMode="src_in"/>
</FrameLayout>
-
- <FrameLayout
- android:id="@+id/car_ui_toolbar_menu_item_text_container"
- style="@style/Widget.CarUi.Toolbar.MenuItem.IndividualContainer"
+ <com.android.car.ui.uxr.DrawableStateSwitch
+ android:id="@+id/car_ui_toolbar_menu_item_switch"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
- android:background="?android:attr/selectableItemBackground">
- <!-- These buttons must have clickable="false" or they will steal the click events from the container -->
- <com.android.car.ui.uxr.DrawableStateSwitch
- android:id="@+id/car_ui_toolbar_menu_item_switch"
- android:layout_width="wrap_content"
- android:layout_height="wrap_content"
- android:layout_gravity="center"
- android:background="@null"
- android:focusable="false"
- android:clickable="false"/>
- <com.android.car.ui.uxr.DrawableStateButton
- android:id="@+id/car_ui_toolbar_menu_item_text"
- style="@style/Widget.CarUi.Toolbar.TextButton"
- android:layout_width="wrap_content"
- android:layout_height="match_parent"
- android:layout_gravity="center"
- android:background="@null"
- android:focusable="false"
- android:clickable="false"/>
- <com.android.car.ui.uxr.DrawableStateButton
- android:id="@+id/car_ui_toolbar_menu_item_text_with_icon"
- style="@style/Widget.CarUi.Toolbar.TextButton.WithIcon"
- android:layout_width="wrap_content"
- android:layout_height="match_parent"
- android:layout_gravity="center"
- android:background="@null"
- android:focusable="false"
- android:clickable="false"/>
- </FrameLayout>
+ android:layout_gravity="center"
+ android:clickable="false"/>
+
+ <!-- These buttons must have clickable="false" or they will steal the click events from the container -->
+ <com.android.car.ui.uxr.DrawableStateButton
+ android:id="@+id/car_ui_toolbar_menu_item_text"
+ style="@style/Widget.CarUi.Toolbar.TextButton"
+ android:layout_width="wrap_content"
+ android:layout_height="wrap_content"
+ android:layout_gravity="center"
+ android:clickable="false"/>
+ <com.android.car.ui.uxr.DrawableStateButton
+ android:id="@+id/car_ui_toolbar_menu_item_text_with_icon"
+ style="@style/Widget.CarUi.Toolbar.TextButton.WithIcon"
+ android:layout_width="wrap_content"
+ android:layout_height="wrap_content"
+ android:layout_gravity="center"
+ android:clickable="false"/>
</FrameLayout>
diff --git a/car-ui-lib/referencedesign/res/layout/car_ui_toolbar_menu_item_primary.xml b/car-ui-lib/referencedesign/res/layout/car_ui_toolbar_menu_item_primary.xml
index 4364271..716c8ee 100644
--- a/car-ui-lib/referencedesign/res/layout/car_ui_toolbar_menu_item_primary.xml
+++ b/car-ui-lib/referencedesign/res/layout/car_ui_toolbar_menu_item_primary.xml
@@ -24,7 +24,7 @@
style="@style/Widget.CarUi.Toolbar.MenuItem.IndividualContainer"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
- android:background="@drawable/car_ui_toolbar_menu_item_icon_ripple">
+ android:layout_gravity="center">
<ImageView
android:layout_width="match_parent"
android:layout_height="match_parent"
@@ -38,37 +38,27 @@
android:layout_gravity="center"
android:tint="@color/car_ui_toolbar_menu_item_icon_color"
android:tintMode="src_in"/>
- <com.android.car.ui.uxr.DrawableStateSwitch
- android:id="@+id/car_ui_toolbar_menu_item_switch"
- android:layout_width="wrap_content"
- android:layout_height="wrap_content"
- android:layout_gravity="center"
- android:background="@null"
- android:focusable="false"
- android:clickable="false"/>
</FrameLayout>
-
- <FrameLayout
- android:id="@+id/car_ui_toolbar_menu_item_text_container"
- style="@style/Widget.CarUi.Toolbar.MenuItem.IndividualContainer"
+ <com.android.car.ui.uxr.DrawableStateSwitch
+ android:id="@+id/car_ui_toolbar_menu_item_switch"
android:layout_width="wrap_content"
- android:layout_height="wrap_content">
- <!-- These buttons must have clickable="false" or they will steal the click events from the container -->
- <com.android.car.ui.uxr.DrawableStateButton
- android:id="@+id/car_ui_toolbar_menu_item_text"
- style="@style/Widget.CarUi.Toolbar.TextButton.Primary"
- android:layout_width="wrap_content"
- android:layout_height="match_parent"
- android:layout_gravity="center"
- android:focusable="false"
- android:clickable="false"/>
- <com.android.car.ui.uxr.DrawableStateButton
- android:id="@+id/car_ui_toolbar_menu_item_text_with_icon"
- style="@style/Widget.CarUi.Toolbar.TextButton.Primary"
- android:layout_width="wrap_content"
- android:layout_height="match_parent"
- android:layout_gravity="center"
- android:focusable="false"
- android:clickable="false"/>
- </FrameLayout>
+ android:layout_height="wrap_content"
+ android:layout_gravity="center"
+ android:clickable="false"/>
+
+ <!-- These buttons must have clickable="false" or they will steal the click events from the container -->
+ <com.android.car.ui.uxr.DrawableStateButton
+ android:id="@+id/car_ui_toolbar_menu_item_text"
+ style="@style/Widget.CarUi.Toolbar.TextButton.Primary"
+ android:layout_width="wrap_content"
+ android:layout_height="wrap_content"
+ android:layout_gravity="center"
+ android:clickable="false"/>
+ <com.android.car.ui.uxr.DrawableStateButton
+ android:id="@+id/car_ui_toolbar_menu_item_text_with_icon"
+ style="@style/Widget.CarUi.Toolbar.TextButton.Primary"
+ android:layout_width="wrap_content"
+ android:layout_height="wrap_content"
+ android:layout_gravity="center"
+ android:clickable="false"/>
</FrameLayout>
diff --git a/car-ui-lib/referencedesign/res/values/styles.xml b/car-ui-lib/referencedesign/res/values/styles.xml
index ba37aac..b33d11a 100644
--- a/car-ui-lib/referencedesign/res/values/styles.xml
+++ b/car-ui-lib/referencedesign/res/values/styles.xml
@@ -34,7 +34,7 @@
<style name="Widget.CarUi.Toolbar.MenuItem.IndividualContainer">
<item name="android:minHeight">76dp</item>
<item name="android:minWidth">76dp</item>
- <item name="android:layout_gravity">center</item>
+ <item name="android:background">@drawable/car_ui_toolbar_menu_item_icon_ripple</item>
<item name="android:focusable">true</item>
</style>
diff --git a/car-ui-lib/referencedesign/res/values/values.xml b/car-ui-lib/referencedesign/res/values/values.xml
index 0cbb8ee..5dbf018 100644
--- a/car-ui-lib/referencedesign/res/values/values.xml
+++ b/car-ui-lib/referencedesign/res/values/values.xml
@@ -12,4 +12,6 @@
<dimen name="car_ui_toolbar_logo_size">44dp</dimen>
<dimen name="car_ui_toolbar_nav_icon_size">44dp</dimen>
+
+ <bool name="car_ui_toolbar_menuitem_individual_click_listeners">true</bool>
</resources>
diff --git a/car-ui-lib/referencedesign/res/xml/overlays.xml b/car-ui-lib/referencedesign/res/xml/overlays.xml
index 363d389..6332b67 100644
--- a/car-ui-lib/referencedesign/res/xml/overlays.xml
+++ b/car-ui-lib/referencedesign/res/xml/overlays.xml
@@ -50,6 +50,7 @@
<item target="bool/car_ui_toolbar_logo_fills_nav_icon_space" value="@bool/car_ui_toolbar_logo_fills_nav_icon_space" />
<item target="bool/car_ui_toolbar_tab_flexible_layout" value="@bool/car_ui_toolbar_tab_flexible_layout" />
<item target="bool/car_ui_toolbar_tabs_on_second_row" value="@bool/car_ui_toolbar_tabs_on_second_row" />
+ <item target="bool/car_ui_toolbar_menuitem_individual_click_listeners" value="@bool/car_ui_toolbar_menuitem_individual_click_listeners" />
<item target="id/car_ui_toolbar_background" value="@id/car_ui_toolbar_background" />
<item target="id/car_ui_toolbar_nav_icon_container" value="@id/car_ui_toolbar_nav_icon_container" />
@@ -68,7 +69,6 @@
<item target="id/car_ui_toolbar_menu_item_icon_container" value="@id/car_ui_toolbar_menu_item_icon_container"/>
<item target="id/car_ui_toolbar_menu_item_icon" value="@id/car_ui_toolbar_menu_item_icon"/>
<item target="id/car_ui_toolbar_menu_item_switch" value="@id/car_ui_toolbar_menu_item_switch"/>
- <item target="id/car_ui_toolbar_menu_item_text_container" value="@id/car_ui_toolbar_menu_item_text_container"/>
<item target="id/car_ui_toolbar_menu_item_text" value="@id/car_ui_toolbar_menu_item_text"/>
<item target="id/car_ui_toolbar_menu_item_text_with_icon" value="@id/car_ui_toolbar_menu_item_text_with_icon"/>
<item target="id/seekbar" value="@id/seekbar"/>
diff --git a/car-ui-lib/tests/apitest/current.xml b/car-ui-lib/tests/apitest/current.xml
index 461ce2c..769b040 100644
--- a/car-ui-lib/tests/apitest/current.xml
+++ b/car-ui-lib/tests/apitest/current.xml
@@ -19,6 +19,7 @@
<public type="bool" name="car_ui_preference_show_chevron"/>
<public type="bool" name="car_ui_scrollbar_enable"/>
<public type="bool" name="car_ui_toolbar_logo_fills_nav_icon_space"/>
+ <public type="bool" name="car_ui_toolbar_menuitem_individual_click_listeners"/>
<public type="bool" name="car_ui_toolbar_nav_icon_reserve_space"/>
<public type="bool" name="car_ui_toolbar_show_logo"/>
<public type="bool" name="car_ui_toolbar_tab_flexible_layout"/>
@@ -282,7 +283,6 @@
<public type="id" name="car_ui_toolbar_menu_item_icon_container"/>
<public type="id" name="car_ui_toolbar_menu_item_switch"/>
<public type="id" name="car_ui_toolbar_menu_item_text"/>
- <public type="id" name="car_ui_toolbar_menu_item_text_container"/>
<public type="id" name="car_ui_toolbar_menu_item_text_with_icon"/>
<public type="id" name="car_ui_toolbar_menu_items_container"/>
<public type="id" name="car_ui_toolbar_nav_icon"/>
