Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2858433002:
[Android] Increase the clickable area of direct share icon (Closed)
Description[Android] Increase the clickable area of direct share icon
This CL increases the clickable area of direct share icon:
1. Change the icon size from 20dp to 24dp.
2. Add 8dp margin on both top and bottom of the icon.
3. If direct share icon is shown, remove the 15dp padding on the right
of the wrapping LinearLayout and add 15dp margin of direct share icon.
This makes the right area of direct share icon responses to the click
event of direct share rather than share.
The design spec is here: https://bbergher.googleplex.com/specs/context-menu/.
BUG=None
Review-Url: https://codereview.chromium.org/2858433002
Cr-Commit-Position: refs/heads/master@{#469415}
Committed: https://chromium.googlesource.com/chromium/src/+/a8bf5a1779446b4af653b276973b5c61a5a9b60c
Patch Set 1 #Patch Set 2 : Add design spec in the description. #
Total comments: 8
Patch Set 3 : Udpate based on Ted's comments. #
Total comments: 8
Patch Set 4 : Update based on Ted's new comments. #
Total comments: 7
Dependent Patchsets: Messages
Total messages: 20 (7 generated)
Description was changed from ========== [Android] Increase the clickable area of direct share icon This CL increases the clickable area of direct share icon: 1. Change the icon size from 20dp to 24dp. 2. Add 8dp margin on both top and bottom of the icon. 3. If direct share icon is shown, remove the 15dp padding on the right of the wrapping LinearLayout and add 15dp margin of direct share icon. This makes the right area of direct share icon responses to the click event of direct share rather than share. BUG=None ========== to ========== [Android] Increase the clickable area of direct share icon This CL increases the clickable area of direct share icon: 1. Change the icon size from 20dp to 24dp. 2. Add 8dp margin on both top and bottom of the icon. 3. If direct share icon is shown, remove the 15dp padding on the right of the wrapping LinearLayout and add 15dp margin of direct share icon. This makes the right area of direct share icon responses to the click event of direct share rather than share. The design spec is here: https://bbergher.googleplex.com/specs/context-menu/. BUG=None ==========
ltian@chromium.org changed reviewers: + tedchoc@chromium.org
Can you take a look of the changes in this CL? Thanks!
https://codereview.chromium.org/2858433002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/tabular_context_menu_row.xml (right): https://codereview.chromium.org/2858433002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_row.xml:26: android:layout_marginStart="10dp" I think you should also add 15dp marginEnd here. The somewhat annoying thing is that we always want 15dp space at the end. But here you need to conditionally put it based on whether the share icon exists. You could also add a <Space/> view with 15dp width that you hide or show based on the context menu. https://codereview.chromium.org/2858433002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_row.xml:34: android:layout_marginTop="8dp" do you want margin or padding? Margin is likely reducing your click target size. https://codereview.chromium.org/2858433002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_row.xml:36: android:layout_marginEnd="15dp" according to the spec, you also need 12dp start padding/margin https://codereview.chromium.org/2858433002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java (right): https://codereview.chromium.org/2858433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:45: mPadding = (int) (PADDING_DP * dpToPx); ah, so this is how you are going about the conditional padding...gotcha I'd be tempted to use the <Space> view in xml to make this a bit more self contained. By setting paddings here, we could potentially clobber anything set in XML and it will be not as easy to spot why.
https://codereview.chromium.org/2858433002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/tabular_context_menu_row.xml (right): https://codereview.chromium.org/2858433002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_row.xml:26: android:layout_marginStart="10dp" On 2017/05/02 16:51:37, Ted C wrote: > I think you should also add 15dp marginEnd here. The somewhat annoying thing is > that we always want 15dp space at the end. But here you need to conditionally > put it based on whether the share icon exists. > > You could also add a <Space/> view with 15dp width that you hide or show based > on the context menu. Done. https://codereview.chromium.org/2858433002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_row.xml:34: android:layout_marginTop="8dp" On 2017/05/02 16:51:37, Ted C wrote: > do you want margin or padding? Margin is likely reducing your click target > size. Sorry I just learn that padding is clickable while margin is not. So definitely padding is the right choice to use. Thanks! https://codereview.chromium.org/2858433002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_row.xml:36: android:layout_marginEnd="15dp" On 2017/05/02 16:51:37, Ted C wrote: > according to the spec, you also need 12dp start padding/margin Done. https://codereview.chromium.org/2858433002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java (right): https://codereview.chromium.org/2858433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:45: mPadding = (int) (PADDING_DP * dpToPx); On 2017/05/02 16:51:37, Ted C wrote: > ah, so this is how you are going about the conditional padding...gotcha > > I'd be tempted to use the <Space> view in xml to make this a bit more self > contained. By setting paddings here, we could potentially clobber anything set > in XML and it will be not as easy to spot why. Done.
https://codereview.chromium.org/2858433002/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/tabular_context_menu_row.xml (right): https://codereview.chromium.org/2858433002/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_row.xml:17: android:layout_marginTop="10dp" are these strictly necessary? And by setting top and bottom margins, doesn't this make the available height 0? 20 - 10 - 10 == 0 https://codereview.chromium.org/2858433002/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_row.xml:19: android:layout_gravity="center_vertical" if we need margins, is this needed anymore? https://codereview.chromium.org/2858433002/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_row.xml:33: android:layout_height="40dp" We should probably sync with UX, but what happens when the user bumps up their font size? In that case, the total row height could be taller than 40dp. Should we center the image in that case? I'm wondering if layout_height should be MATCH_PARENT then we could just set android:scaleType to something like centerInside. https://codereview.chromium.org/2858433002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java (right): https://codereview.chromium.org/2858433002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:100: viewHolder.mRightPadding.setVisibility(View.GONE); you need to set this to VISIBLE in the else case below
https://codereview.chromium.org/2858433002/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/tabular_context_menu_row.xml (right): https://codereview.chromium.org/2858433002/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_row.xml:17: android:layout_marginTop="10dp" On 2017/05/02 21:06:47, Ted C wrote: > are these strictly necessary? And by setting top and bottom margins, doesn't > this make the available height 0? > > 20 - 10 - 10 == 0 There are the settings in spec. The margin is placed outside the view so the whole height is 20 + 10 + 10 = 40. I can change it to use padding and the layout_height = 40 dp. Since this ImageView does not register for OnClick listener, I think there is no difference between these two options. https://codereview.chromium.org/2858433002/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_row.xml:19: android:layout_gravity="center_vertical" On 2017/05/02 21:06:47, Ted C wrote: > if we need margins, is this needed anymore? Oh, then both two ImageViews should not need this. https://codereview.chromium.org/2858433002/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_row.xml:33: android:layout_height="40dp" On 2017/05/02 21:06:47, Ted C wrote: > We should probably sync with UX, but what happens when the user bumps up their > font size? In that case, the total row height could be taller than 40dp. > Should we center the image in that case? > > I'm wondering if layout_height should be MATCH_PARENT then we could just set > android:scaleType to something like centerInside. Done. https://codereview.chromium.org/2858433002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java (right): https://codereview.chromium.org/2858433002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:100: viewHolder.mRightPadding.setVisibility(View.GONE); On 2017/05/02 21:06:47, Ted C wrote: > you need to set this to VISIBLE in the else case below Done.
lgtm https://codereview.chromium.org/2858433002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/tabular_context_menu_row.xml (right): https://codereview.chromium.org/2858433002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_row.xml:17: android:paddingTop="10dp" here since this isn't clickable, I think you don't need padding(s)
https://codereview.chromium.org/2858433002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/tabular_context_menu_row.xml (right): https://codereview.chromium.org/2858433002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_row.xml:17: android:paddingTop="10dp" On 2017/05/03 22:52:19, Ted C wrote: > here since this isn't clickable, I think you don't need padding(s) 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.
Hmm...yeah, I think I may have led you astray. Using gravity and explicit heights and paddings should ensure you control the aspect ratio and image bounds exactly, while keep the images aligned vertically. https://codereview.chromium.org/2858433002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/tabular_context_menu_row.xml (right): https://codereview.chromium.org/2858433002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_row.xml:17: android:paddingTop="10dp" On 2017/05/04 00:02:40, ltian wrote: > On 2017/05/03 22:52:19, Ted C wrote: > > here since this isn't clickable, I think you don't need padding(s) > > 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. 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. https://codereview.chromium.org/2858433002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_row.xml:35: android:paddingBottom="8dp" 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).
https://codereview.chromium.org/2858433002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/tabular_context_menu_row.xml (right): https://codereview.chromium.org/2858433002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_row.xml:35: android:paddingBottom="8dp" On 2017/05/04 00:55:40, Ted C wrote: > 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). 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?
https://codereview.chromium.org/2858433002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/tabular_context_menu_row.xml (right): https://codereview.chromium.org/2858433002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_row.xml:35: android:paddingBottom="8dp" On 2017/05/04 01:43:41, ltian wrote: > On 2017/05/04 00:55:40, Ted C wrote: > > 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). > > 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? 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.
https://codereview.chromium.org/2858433002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/tabular_context_menu_row.xml (right): https://codereview.chromium.org/2858433002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_row.xml:35: android:paddingBottom="8dp" On 2017/05/04 14:37:56, Ted C wrote: > On 2017/05/04 01:43:41, ltian wrote: > > On 2017/05/04 00:55:40, Ted C wrote: > > > 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). > > > > 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? > > 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. 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.
The CQ bit was checked by ltian@chromium.org
The CQ bit was unchecked by ltian@chromium.org
The CQ bit was checked by ltian@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493922475595220,
"parent_rev": "a4d96d0d8bdcc688451ad0376a7304df5b41904a", "commit_rev":
"a8bf5a1779446b4af653b276973b5c61a5a9b60c"}
Message was sent while issue was closed.
Description was changed from ========== [Android] Increase the clickable area of direct share icon This CL increases the clickable area of direct share icon: 1. Change the icon size from 20dp to 24dp. 2. Add 8dp margin on both top and bottom of the icon. 3. If direct share icon is shown, remove the 15dp padding on the right of the wrapping LinearLayout and add 15dp margin of direct share icon. This makes the right area of direct share icon responses to the click event of direct share rather than share. The design spec is here: https://bbergher.googleplex.com/specs/context-menu/. BUG=None ========== to ========== [Android] Increase the clickable area of direct share icon This CL increases the clickable area of direct share icon: 1. Change the icon size from 20dp to 24dp. 2. Add 8dp margin on both top and bottom of the icon. 3. If direct share icon is shown, remove the 15dp padding on the right of the wrapping LinearLayout and add 15dp margin of direct share icon. This makes the right area of direct share icon responses to the click event of direct share rather than share. The design spec is here: https://bbergher.googleplex.com/specs/context-menu/. BUG=None Review-Url: https://codereview.chromium.org/2858433002 Cr-Commit-Position: refs/heads/master@{#469415} Committed: https://chromium.googlesource.com/chromium/src/+/a8bf5a1779446b4af653b276973b... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/a8bf5a1779446b4af653b276973b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
