[Android] Increase the clickable area of direct share icon

chrome/android/java/res/layout/tabular_context_menu_row.xml

Index: chrome/android/java/res/layout/tabular_context_menu_row.xml
diff --git a/chrome/android/java/res/layout/tabular_context_menu_row.xml b/chrome/android/java/res/layout/tabular_context_menu_row.xml
index 5e4d200f2fdc6f3857bf2a00043407c70c884dd9..b638933dcff532653d2dc269414766767aed3f82 100644
--- a/chrome/android/java/res/layout/tabular_context_menu_row.xml
+++ b/chrome/android/java/res/layout/tabular_context_menu_row.xml
@@ -8,14 +8,15 @@
     android:layout_width="match_parent"
     android:layout_height="wrap_content"
     android:paddingStart="15dp"
-    android:paddingEnd="15dp"
     android:minHeight="40dp"
     tools:ignore="UseCompoundDrawables">
     <ImageView
         android:id="@+id/context_menu_icon"
         android:layout_width="20dp"
-        android:layout_height="20dp"
-        android:layout_gravity="center_vertical"
+        android:layout_height="match_parent"
+        android:paddingTop="10dp"

[Ted C, 2017/05/03 22:52:19]

here since this isn't clickable, I think you don't need padding(s)

[ltian, 2017/05/04 00:02:40]

I am curious if the height size is larger than 40dp, how would Android decides
the padding sizes for it if we remove these settings? Is it possible that this
icon will have a different size to the direct share icon because padding is not
specified?
Removing them seems have no UI difference, but just curious whether there is any
potential problem.

[Ted C, 2017/05/04 00:55:40]

I don't think the icon sizes would be any more different.  centerInside keeps
the
aspect ratio of the icons the same, so the visual difference between the two
should remain constant.

The only real way to enforce the icons remain the same size would be to ensure
the original assets are the same size.  Here, this icon has 20dp width and the
share
has 24dp width.

If you want to ensure that, I think what you had before was actually correct.

setting and explicit width and height and setting
layout_gravity="center_vertical"
both here and below is correct.   In that case, you won't have the click targets
increase in size if the text size increases, but it will enforce the correct
aspect
size.

+        android:paddingBottom="10dp"
+        android:scaleType="centerInside"
         android:contentDescription="@null" />
     <TextView
         android:id="@+id/context_text"
@@ -28,9 +29,17 @@
         android:textColor="#dd000000" />
     <ImageView
         android:id="@+id/context_menu_share_icon"
-        android:layout_width="20dp"
-        android:layout_height="20dp"
-        android:layout_gravity="center_vertical"
+        android:layout_width="51dp"
+        android:layout_height="match_parent"
+        android:paddingTop="8dp"
+        android:paddingBottom="8dp"

[Ted C, 2017/05/04 00:55:40]

actually, you shouldn't need paddingTop or paddingBottom since your height is
now match parent.  The full height will be clickable even if larger.

I guess by specifying the padding, you're ensuring the dimensions of the
drawable are correct.

Maybe you should keep padding top and padding bottom, move your layout_height
back to wrap_content and then add layout_gravity="center_vertical".

Then you can drop scaleType from both.  In that case, you're hardcoding the
size, vertically aligning the image and always ensuring the image sizes don't
change (if for some reason they were being compressed vertically).

[ltian, 2017/05/04 01:43:41]

Setting the height of ImageView to wrap_content makes the sharing row larger
than the others. Setting it explicitly to 40dp makes it work as expected, but my
another conern for that implementation is then if TextView is more than 40dp,
will it be a problem that there are some spaces above and below the ImageView
that handles click event for the parent container rather than the ImageView?

If so, is current implementation (explicitly set padding and set ImageView
height as match_parent)  better because even though the ratio between image and
padding changes, the whole space still only handles click event of ImageView?

[Ted C, 2017/05/04 14:37:56]

Indeed.  The current implementation will handle expanding the click area and
since
you are enforcing an explicit width (51 - 12 - 15), then you likely wouldn't see
changes in the image size if the row got larger (as the images "should" be
square
and not subject to scaling if the available height increases).

This approach is indeed fine, but it does have the potential for the image sizes
to change.

[ltian, 2017/05/04 18:27:23]

Looks like the two implementations all have their own drawbacks. I will keep
with current implementation since the row height and image size are not changed
when increasing the font size to the maximum on most devices.

+        android:paddingStart="12dp"
+        android:paddingEnd="15dp"
+        android:scaleType="centerInside"
         android:contentDescription="@null"
         android:visibility="gone"/>
+    <Space
+        android:id="@+id/context_menu_right_padding"
+        android:layout_width="15dp"
+        android:layout_height="wrap_content"/>
 </LinearLayout>