Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1441723002:
Notification custom layouts: paint the button icon for Material. (Closed)
|
Created:
5 years, 1 month ago by Michael van Ouwerkerk Modified:
5 years, 1 month ago CC:
chromium-reviews, Peter Beverloo, mlamouri+watch-notifications_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNotification custom layouts: paint the button icon for Material.
* a11y: TalkBack still says things like "Settings button" when tapped, as the Button is the view on top
* RTL is handled, the icon is placed on the right when the system is in RTL mode
BUG=554073
Committed: https://crrev.com/4bf4d613f09a253f35ab59059194faffb1a57b76
Cr-Commit-Position: refs/heads/master@{#360571}
Patch Set 1 #Patch Set 2 : Add TODOs. #Patch Set 3 : Fix build, some cleanups. #Patch Set 4 : Fix RTL and add unit test for paintBitmap. #Patch Set 5 : Documentation tweaks. #
Total comments: 30
Patch Set 6 : Address peter's and newt's comments. #Patch Set 7 : Delete unused constant. #
Total comments: 2
Patch Set 8 : Simplify setting a color filter. #
Messages
Total messages: 15 (3 generated)
Description was changed from ========== Notification custom layouts: paint the button icon. BUG=554073 ========== to ========== Notification custom layouts: paint the button icon for Material. * a11y: TalkBack still says things like "Settings button" when tapped, as the Button is the view on top * RTL is handled, the icon is placed on the right when the system is in RTL mode BUG=554073 ==========
mvanouwerkerk@chromium.org changed reviewers: + newt@chromium.org, peter@chromium.org
Peter, can you do a general review please? Newton, can you review as UI owner please?
Overall I'm OK with this, but having to do the porter/duff multiply manually is not great. Do you expect this to be sufficiently future-proof, also with non-Material layouts? https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java (right): https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:83: * The color named "secondary_text_default_material_light" with hex value #8a000000 converted to nit: don't name the value (that's up to the style to define) https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:86: private static final int BUTTON_ICON_COLOR_MATERIAL = -1979711488; _ID? https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:232: boolean ltr = ApiCompatibilityUtils.getLayoutDirection(resources.getConfiguration()) LocalizationUtils.isLayoutRtl() It's much more common for branches to check on RTL rather than LTR, and this allows you to re-use some existing infrastructure. Note that it does cache the value, which may affect your testing. https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:238: if (action.getIcon() != 0) { You mention in the XML file that a Bitmap is the format we get when loading an image from the network, but that feature actually doesn't exist (yet). It would be worthwhile to rephrase 243-245 as a TODO. (I think the comment in the layout adds value.) https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:240: options.inMutable = true; Why do we need this?
https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/web_notification_button.xml (right): https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/web_notification_button.xml:11: <FrameLayout Do these views need to overlap? If not, can't you just use a horizontal LinearLayout? If so, why do they need to overlap? Should the image be clickable? Could you use an ImageButton here? Or could you use compound drawables (see RemoteViews.setTextViewCompoundDrawables())? If you want both the image and the text to be clickable as part of the same button, then compound drawables are your friend. https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java (right): https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:86: private static final int BUTTON_ICON_COLOR_MATERIAL = -1979711488; You can just write this as "0x8a000000". Or you could use secondary_text_default_material_light directly if it's publicly exported or available via the support library (which seems to be the case), e.g. Resources.getColor(R.color.secondary_text_default_material_light) https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:232: boolean ltr = ApiCompatibilityUtils.getLayoutDirection(resources.getConfiguration()) On 2015/11/16 11:18:13, Peter Beverloo wrote: > LocalizationUtils.isLayoutRtl() > > It's much more common for branches to check on RTL rather than LTR, and this > allows you to re-use some existing infrastructure. Note that it does cache the > value, which may affect your testing. "isRtl" is the typical variable name in cases like this https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:247: paintBitmap(icon, BUTTON_ICON_COLOR_MATERIAL); Could you use "setColorFilter" on the ImageButton instead of doing the painting yourself? https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:256: + icon.getWidth(); Are we sure that the icon won't be unexpectedly wide? This is only a concern if you're using website-supplied icons. https://codereview.chromium.org/1441723002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java (right): https://codereview.chromium.org/1441723002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java:180: assertEquals(Color.parseColor("#ffffffff"), CustomNotificationBuilder.paintBitmap( You can just write "0xffffffff" instead of "Color.parseColor("#ffffffff")"
Thanks for the reviews... please take another look :-) https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/web_notification_button.xml (right): https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/web_notification_button.xml:11: <FrameLayout On 2015/11/16 17:50:08, newt wrote: > Do these views need to overlap? If not, can't you just use a horizontal > LinearLayout? If so, why do they need to overlap? Should the image be clickable? > Could you use an ImageButton here? Or could you use compound drawables (see > RemoteViews.setTextViewCompoundDrawables())? > > If you want both the image and the text to be clickable as part of the same > button, then compound drawables are your friend. I would have preferred to use RemoteViews#setTextViewCompoundDrawablesRelativeColorFilter(int viewId, int index, int color, PorterDuff.Mode mode) but that method is hidden. It's what the core framework uses for this situation. There is setTextViewCompoundDrawables(int viewId, int left, int top, int right, int bottom) but that does not apply a color filter. Then there is setTextViewCompoundDrawables(int viewId, Icon left, Icon top, Icon right, Icon bottom) but it is hidden and also uses Icon which is from API level 23 only. The _only_ way to apply a PorterDuffColorFilter (or an equivalent) to an image and somehow get that image into a RemoteViews, compatible from at least API level 16, is to acquire a Bitmap, paint that, and pass it to the RemoteViews. Then, the _only_ useful way to set that Bitmap is on an ImageView using setImageViewBitmap. So that's why this is a Button layered on top of an ImageView. It is intentional, though a bit more work than anticipated. From a UX perspective, the difference is undetectable: a11y is the same, look and feel are the same, RTL is the same. As the Button overlays the ImageView, it feels as if the image is clickable, as intended, it feels like it is part of the button. https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java (right): https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:83: * The color named "secondary_text_default_material_light" with hex value #8a000000 converted to On 2015/11/16 11:18:12, Peter Beverloo wrote: > nit: don't name the value (that's up to the style to define) Acknowledged. https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:86: private static final int BUTTON_ICON_COLOR_MATERIAL = -1979711488; On 2015/11/16 11:18:13, Peter Beverloo wrote: > _ID? This number is not an id, it's a color value. https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:86: private static final int BUTTON_ICON_COLOR_MATERIAL = -1979711488; On 2015/11/16 17:50:09, newt wrote: > You can just write this as "0x8a000000". Or you could use > secondary_text_default_material_light directly if it's publicly exported or > available via the support library (which seems to be the case), e.g. > Resources.getColor(R.color.secondary_text_default_material_light) Done (the getColor part). Nice. https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:232: boolean ltr = ApiCompatibilityUtils.getLayoutDirection(resources.getConfiguration()) On 2015/11/16 11:18:13, Peter Beverloo wrote: > LocalizationUtils.isLayoutRtl() > > It's much more common for branches to check on RTL rather than LTR, and this > allows you to re-use some existing infrastructure. Note that it does cache the > value, which may affect your testing. Done. https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:232: boolean ltr = ApiCompatibilityUtils.getLayoutDirection(resources.getConfiguration()) On 2015/11/16 17:50:09, newt wrote: > On 2015/11/16 11:18:13, Peter Beverloo wrote: > > LocalizationUtils.isLayoutRtl() > > > > It's much more common for branches to check on RTL rather than LTR, and this > > allows you to re-use some existing infrastructure. Note that it does cache the > > value, which may affect your testing. > > "isRtl" is the typical variable name in cases like this Done. https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:238: if (action.getIcon() != 0) { On 2015/11/16 11:18:13, Peter Beverloo wrote: > You mention in the XML file that a Bitmap is the format we get when loading an > image from the network, but that feature actually doesn't exist (yet). It would > be worthwhile to rephrase 243-245 as a TODO. (I think the comment in the layout > adds value.) Done. https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:240: options.inMutable = true; On 2015/11/16 11:18:12, Peter Beverloo wrote: > Why do we need this? Only mutable bitmaps can be painted, otherwise an additional mutable copy must be made. https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:247: paintBitmap(icon, BUTTON_ICON_COLOR_MATERIAL); On 2015/11/16 17:50:09, newt wrote: > Could you use "setColorFilter" on the ImageButton instead of doing the painting > yourself? But how could we? We only have RemoteViews here, and those do not expose all possible methods of the views, they must be annotated with RemotableViewMethod. https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:256: + icon.getWidth(); On 2015/11/16 17:50:08, newt wrote: > Are we sure that the icon won't be unexpectedly wide? This is only a concern if > you're using website-supplied icons. For now, we are sure it is ok, as we don't have web developer provided images yet. Once we do, we'll need to decide on what processing steps to apply. https://codereview.chromium.org/1441723002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java (right): https://codereview.chromium.org/1441723002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java:180: assertEquals(Color.parseColor("#ffffffff"), CustomNotificationBuilder.paintBitmap( On 2015/11/16 17:50:09, newt wrote: > You can just write "0xffffffff" instead of "Color.parseColor("#ffffffff")" Done. Nice cleanup.
On 2015/11/16 11:18:13, Peter Beverloo wrote: > Overall I'm OK with this, but having to do the porter/duff multiply manually is > not great. Do you expect this to be sufficiently future-proof, also with > non-Material layouts? > The APIs used here are widely compatible, and we have a lot of flexibility to manipulate the Bitmap as needed (resize, paint, whatever). It seems we can adapt if and when needed. What alternative is there, really, that is more adaptable and scalable? Certainly not hard coding the images and bundling multiple versions in the APK.
https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/web_notification_button.xml (right): https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/web_notification_button.xml:11: <FrameLayout On 2015/11/17 12:00:23, Michael van Ouwerkerk wrote: > On 2015/11/16 17:50:08, newt wrote: > > Do these views need to overlap? If not, can't you just use a horizontal > > LinearLayout? If so, why do they need to overlap? Should the image be > clickable? > > Could you use an ImageButton here? Or could you use compound drawables (see > > RemoteViews.setTextViewCompoundDrawables())? > > > > If you want both the image and the text to be clickable as part of the same > > button, then compound drawables are your friend. > > I would have preferred to use > RemoteViews#setTextViewCompoundDrawablesRelativeColorFilter(int viewId, int > index, int color, PorterDuff.Mode mode) but that method is hidden. It's what the > core framework uses for this situation. > > There is setTextViewCompoundDrawables(int viewId, int left, int top, int right, > int bottom) but that does not apply a color filter. > > Then there is setTextViewCompoundDrawables(int viewId, Icon left, Icon top, Icon > right, Icon bottom) but it is hidden and also uses Icon which is from API level > 23 only. > > The _only_ way to apply a PorterDuffColorFilter (or an equivalent) to an image > and somehow get that image into a RemoteViews, compatible from at least API > level 16, is to acquire a Bitmap, paint that, and pass it to the RemoteViews. > > Then, the _only_ useful way to set that Bitmap is on an ImageView using > setImageViewBitmap. > > So that's why this is a Button layered on top of an ImageView. It is > intentional, though a bit more work than anticipated. From a UX perspective, the > difference is undetectable: a11y is the same, look and feel are the same, RTL is > the same. As the Button overlays the ImageView, it feels as if the image is > clickable, as intended, it feels like it is part of the button. Yowsers. Thanks for the run down. I wasn't thinking about the fact that you need to combine a compound drawable with a color filter. Given that, I think the current solution is reasonable. It would probably be good to elaborate a bit on why you can't use compound drawables in the comment above. https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/web_notification_button.xml:29: <Button Will there always be button text, or will we have notifications where there's just an image but no text? https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/web_notification_button.xml:33: android:drawablePadding="0dp" not needed? (since there are no compound drawables) https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java (right): https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:247: paintBitmap(icon, BUTTON_ICON_COLOR_MATERIAL); On 2015/11/17 12:00:23, Michael van Ouwerkerk wrote: > On 2015/11/16 17:50:09, newt wrote: > > Could you use "setColorFilter" on the ImageButton instead of doing the > painting > > yourself? > > But how could we? We only have RemoteViews here, and those do not expose all > possible methods of the views, they must be annotated with RemotableViewMethod. setColorFilter(int) is @RemotableViewMethod, but it uses SRC_ATOP blending, which doesn't seem to work for your purposes, correct? https://codereview.chromium.org/1441723002/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java (right): https://codereview.chromium.org/1441723002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java:181: createWhiteBitmap(), 0x00000000).getPixel(0, 0)); fix indentation
Thanks! Please take another look. https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/web_notification_button.xml (right): https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/web_notification_button.xml:11: <FrameLayout On 2015/11/17 20:04:40, newt wrote: > On 2015/11/17 12:00:23, Michael van Ouwerkerk wrote: > > On 2015/11/16 17:50:08, newt wrote: > > > Do these views need to overlap? If not, can't you just use a horizontal > > > LinearLayout? If so, why do they need to overlap? Should the image be > > clickable? > > > Could you use an ImageButton here? Or could you use compound drawables (see > > > RemoteViews.setTextViewCompoundDrawables())? > > > > > > If you want both the image and the text to be clickable as part of the same > > > button, then compound drawables are your friend. > > > > I would have preferred to use > > RemoteViews#setTextViewCompoundDrawablesRelativeColorFilter(int viewId, int > > index, int color, PorterDuff.Mode mode) but that method is hidden. It's what > the > > core framework uses for this situation. > > > > There is setTextViewCompoundDrawables(int viewId, int left, int top, int > right, > > int bottom) but that does not apply a color filter. > > > > Then there is setTextViewCompoundDrawables(int viewId, Icon left, Icon top, > Icon > > right, Icon bottom) but it is hidden and also uses Icon which is from API > level > > 23 only. > > > > The _only_ way to apply a PorterDuffColorFilter (or an equivalent) to an image > > and somehow get that image into a RemoteViews, compatible from at least API > > level 16, is to acquire a Bitmap, paint that, and pass it to the RemoteViews. > > > > Then, the _only_ useful way to set that Bitmap is on an ImageView using > > setImageViewBitmap. > > > > So that's why this is a Button layered on top of an ImageView. It is > > intentional, though a bit more work than anticipated. From a UX perspective, > the > > difference is undetectable: a11y is the same, look and feel are the same, RTL > is > > the same. As the Button overlays the ImageView, it feels as if the image is > > clickable, as intended, it feels like it is part of the button. > > Yowsers. Thanks for the run down. I wasn't thinking about the fact that you need > to combine a compound drawable with a color filter. > > Given that, I think the current solution is reasonable. It would probably be > good to elaborate a bit on why you can't use compound drawables in the comment > above. Done. I've added some more docs here. https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/web_notification_button.xml:29: <Button On 2015/11/17 20:04:40, newt wrote: > Will there always be button text, or will we have notifications where there's > just an image but no text? Action (button) titles are required at the JavaScript API level. https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/web_notification_button.xml:33: android:drawablePadding="0dp" On 2015/11/17 20:04:40, newt wrote: > not needed? (since there are no compound drawables) Done. https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java (right): https://codereview.chromium.org/1441723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:247: paintBitmap(icon, BUTTON_ICON_COLOR_MATERIAL); On 2015/11/17 20:04:40, newt wrote: > On 2015/11/17 12:00:23, Michael van Ouwerkerk wrote: > > On 2015/11/16 17:50:09, newt wrote: > > > Could you use "setColorFilter" on the ImageButton instead of doing the > > painting > > > yourself? > > > > But how could we? We only have RemoteViews here, and those do not expose all > > possible methods of the views, they must be annotated with > RemotableViewMethod. > > setColorFilter(int) is @RemotableViewMethod, but it uses SRC_ATOP blending, > which doesn't seem to work for your purposes, correct? The top view needs to be a Button, so that TalkBack can read its text when tapped. So the view below can be an ImageView, no need to use an ImageButton for that part. Ah yes there is setColorFilter(int) for ImageView which can be called with RemoteViews#setInt(int viewId, String methodName, int value). When using a different color to achieve the same result with SRC_ATOP, this actually works. This is cool, simpler, thanks :-) We might have different requirements in future for web developer provided icons, but for now this will do. https://codereview.chromium.org/1441723002/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java (right): https://codereview.chromium.org/1441723002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java:181: createWhiteBitmap(), 0x00000000).getPixel(0, 0)); On 2015/11/17 20:04:40, newt wrote: > fix indentation Done.
lgtm
lgtm
The CQ bit was checked by mvanouwerkerk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441723002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441723002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/4bf4d613f09a253f35ab59059194faffb1a57b76 Cr-Commit-Position: refs/heads/master@{#360571} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
