[Payments] Float card scanner icon at the end of EditText instead of compound drawable

Float card scanner icon at the end of EditText instead of compound drawable

This gives better user experience since compound drawable do not support click event.

BUG=657178, 640430
Committed: https://crrev.com/c3da4b51497fca50ebed5e4adbec3f7ada68ceda
Cr-Commit-Position: refs/heads/master@{#427222}

[gogerald1, 2016-10-20 22:10:12]

Patchset #1 (id:1) has been deleted

[gogerald1, 2016-10-20 22:10:17]

Patchset #1 (id:20001) has been deleted

[gogerald1, 2016-10-20 22:10:26]

The CQ bit was checked by gogerald@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 22:10:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gogerald1, 2016-10-20 22:16:10]

Description was changed from

==========
header


draft

BUG=
==========

to

==========
Float card scanner icon at the end of EditText instead of compound drawable

This gives better user experience since compound drawable do not support click
event.

BUG=657178
==========

[gogerald1, 2016-10-20 22:29:46]

The CQ bit was checked by gogerald@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 22:30:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gogerald1, 2016-10-20 22:43:09]

Patchset #1 (id:40001) has been deleted

[gogerald1, 2016-10-20 22:43:14]

Patchset #1 (id:60001) has been deleted

[gogerald1, 2016-10-20 22:43:23]

The CQ bit was checked by gogerald@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 22:44:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gogerald1, 2016-10-20 22:44:30]

gogerald@chromium.org changed reviewers:
+ rouslan@chromium.org

[gogerald1, 2016-10-20 22:44:31]

Hi, PTAL,

[please use gerrit instead, 2016-10-20 22:52:04]

https://codereview.chromium.org/2434383002/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/payments_request_editor_textview.xml
(right):

https://codereview.chromium.org/2434383002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/payments_request_editor_textview.xml:30:
android:contentDescription="@string/autofill_scan_credit_card"
The content description should ideally be set from Java. See my other comment.

https://codereview.chromium.org/2434383002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java
(right):

https://codereview.chromium.org/2434383002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java:64:
mActionIcon.setVisibility(VISIBLE);
mActionIcon.setContentDescription(fieldModel.getActionDescriptionForAccessibility());

https://codereview.chromium.org/2434383002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java:163:
// Align the bottom of mActionIcon to the bottom of mInput.
Add a comment about why this is necessary. I think it's because the text field
changes in size on focus change, but you probably know the reasons better.

[commit-bot: I haz the power, 2016-10-20 23:34:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 23:34:11]

Dry run: This issue passed the CQ dry run.

[gogerald1, 2016-10-20 23:44:34]

The CQ bit was checked by gogerald@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 23:45:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gogerald1, 2016-10-20 23:48:09]

Patchset #2 (id:100001) has been deleted

[gogerald1, 2016-10-20 23:48:29]

The CQ bit was checked by gogerald@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 23:49:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gogerald1, 2016-10-21 01:03:38]

The CQ bit was checked by gogerald@chromium.org to run a CQ dry run

[gogerald1, 2016-10-21 01:04:24]

Patchset #2 (id:120001) has been deleted

[commit-bot: I haz the power, 2016-10-21 01:04:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gogerald1, 2016-10-21 01:08:51]

https://codereview.chromium.org/2434383002/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/payments_request_editor_textview.xml
(right):

https://codereview.chromium.org/2434383002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/payments_request_editor_textview.xml:30:
android:contentDescription="@string/autofill_scan_credit_card"
On 2016/10/20 22:52:04, rouslan wrote:
> The content description should ideally be set from Java. See my other comment.

Done.

https://codereview.chromium.org/2434383002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java
(right):

https://codereview.chromium.org/2434383002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java:64:
mActionIcon.setVisibility(VISIBLE);
On 2016/10/20 22:52:04, rouslan wrote:
>
mActionIcon.setContentDescription(fieldModel.getActionDescriptionForAccessibility());

Done.

https://codereview.chromium.org/2434383002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java:163:
// Align the bottom of mActionIcon to the bottom of mInput.
On 2016/10/20 22:52:04, rouslan wrote:
> Add a comment about why this is necessary. I think it's because the text field
> changes in size on focus change, but you probably know the reasons better.

Done.

[commit-bot: I haz the power, 2016-10-21 01:50:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 01:50:04]

Dry run: This issue passed the CQ dry run.

[please use gerrit instead, 2016-10-21 13:42:55]

lgtm

[gogerald1, 2016-10-21 13:51:31]

The CQ bit was checked by gogerald@chromium.org

[commit-bot: I haz the power, 2016-10-21 13:51:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[please use gerrit instead, 2016-10-21 13:54:45]

I don't own files in chrome/android/java/res/layout/.

[gogerald1, 2016-10-21 13:55:01]

The CQ bit was unchecked by gogerald@chromium.org

[gogerald1, 2016-10-21 14:48:47]

gogerald@chromium.org changed reviewers:
+ tedchoc@chromium.org

[gogerald1, 2016-10-21 14:48:48]

tedchoc@chromium.org: Please review changes in 

chrome/android/java/res/layout/*

[gogerald1, 2016-10-21 15:48:15]

Description was changed from

==========
Float card scanner icon at the end of EditText instead of compound drawable

This gives better user experience since compound drawable do not support click
event.

BUG=657178
==========

to

==========
Float card scanner icon at the end of EditText instead of compound drawable

This gives better user experience since compound drawable do not support click
event.

BUG=657178,640430
==========

[Ted C, 2016-10-21 17:50:43]

https://codereview.chromium.org/2434383002/diff/140001/chrome/android/java/re...
File chrome/android/java/res/layout/payments_request_editor_textview.xml
(right):

https://codereview.chromium.org/2434383002/diff/140001/chrome/android/java/re...
chrome/android/java/res/layout/payments_request_editor_textview.xml:11:
<org.chromium.chrome.browser.widget.CompatibilityTextInputLayout
what happens if this text is very long?  Won't the icon overlap it and
potentially prevent it from being visible?

I feel this needs android:paddingEnd="48dp" or something to ensure that isn't
the case.

[gogerald1, 2016-10-21 18:22:03]

Patchset #3 (id:160001) has been deleted

[gogerald1, 2016-10-21 19:54:59]

Patchset #3 (id:180001) has been deleted

[gogerald1, 2016-10-21 20:01:35]

https://codereview.chromium.org/2434383002/diff/140001/chrome/android/java/re...
File chrome/android/java/res/layout/payments_request_editor_textview.xml
(right):

https://codereview.chromium.org/2434383002/diff/140001/chrome/android/java/re...
chrome/android/java/res/layout/payments_request_editor_textview.xml:11:
<org.chromium.chrome.browser.widget.CompatibilityTextInputLayout
On 2016/10/21 17:50:43, Ted C wrote:
> what happens if this text is very long?  Won't the icon overlap it and
> potentially prevent it from being visible?
> 
> I feel this needs android:paddingEnd="48dp" or something to ensure that isn't
> the case.

Done. Add padding programmatically in EditorTextField.onWindowFocusChanged when
action_icon is enabled.

[Ted C, 2016-10-21 20:13:50]

a couple questions about the java code (that you might have already tried, but
more trying to get a better understanding)

https://codereview.chromium.org/2434383002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java
(right):

https://codereview.chromium.org/2434383002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java:64:
Drawable icon = ApiCompatibilityUtils.getDrawable(
do you happen to know if TintedDrawable's would work here?

In particular, this API "should" do what you want...hopefully.
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

https://codereview.chromium.org/2434383002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java:172:
if (hasWindowFocus && mActionIcon != null) {
did you try overwriting onLayout (or measure) instead?  This seems to be layout
related stuff and that would be more consistent with android's normal pattern.

Decent write up about the process if you're not familiar:
https://developer.android.com/guide/topics/ui/how-android-draws.html

[gogerald1, 2016-10-21 20:57:12]

The CQ bit was checked by gogerald@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 20:57:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gogerald1, 2016-10-21 20:58:07]

https://codereview.chromium.org/2434383002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java
(right):

https://codereview.chromium.org/2434383002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java:64:
Drawable icon = ApiCompatibilityUtils.getDrawable(
On 2016/10/21 20:13:50, Ted C wrote:
> do you happen to know if TintedDrawable's would work here?
> 
> In particular, this API "should" do what you want...hopefully.
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

Done. Did not know this convenient class, turns out it's default tint mode is
what I need here, very good to know.

https://codereview.chromium.org/2434383002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java:172:
if (hasWindowFocus && mActionIcon != null) {
On 2016/10/21 20:13:50, Ted C wrote:
> did you try overwriting onLayout (or measure) instead?  This seems to be
layout
> related stuff and that would be more consistent with android's normal pattern.
> 
> Decent write up about the process if you're not familiar:
> https://developer.android.com/guide/topics/ui/how-android-draws.html

Done. Just chose a moment late enough :(. Seems onMeasure is late enough in this
case.

[commit-bot: I haz the power, 2016-10-21 22:23:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 22:23:57]

Dry run: This issue passed the CQ dry run.

[Ted C, 2016-10-24 17:41:41]

https://codereview.chromium.org/2434383002/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java
(right):

https://codereview.chromium.org/2434383002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java:165:
public void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
welcome to the wild world of android layouts, so forgive me for the rambling
reply.

setting the padding actually will retrigger a layout pass as it affects the
measurement.

I have a couple potential suggestions:
1.) In the XML file, pull this android:layout_width="48dp" out to use a @dimen
reference.  Then instead of relying on mActionIcon.getWidth(), which will only
be resolved after a full measure and layout pass, you would just pull the same
value of of dimens.
2.) Instead of relying on a full multi-pass measure/layout, you would do
something like the follows:

onMeasure(...) {
  // This assumes the width never changes, potentially an unneeded
  // optimization.
  if (mActionIcon != null && mActionIcon.getMeasuredWidth() == 0) {
      // Using the existing measure specs is fine in this case because we
      // know the view size is bounded.
      mActionIcon.onMeasure(widthMeasureSpec, heightMeasureSpec);
  }
  if (mActionIcon != null) {
      // FYI: I'm using getMeasuredWidth() here as it doesn't rely on the
      // layout pass.
      ApiCompatibilityUtils.setPaddingRelative(mInput,
              ApiCompatibilityUtils.getPaddingStart(mInput),
mInput.getPaddingTop(),
              mActionIcon.getMeasuredWidth(), mInput.getPaddingBottom());
  }
  super.onMeasure(widthMeasureSpec, heightMeasureSpec);
}

// The problem with using getY() and getHeight(), getTop() is that it
// requires running layout first, so it won't be right inside of
// onMeasure (it might have worked because setPadding was causing a
// double trigger, but that isn't something we should rely on).
//
// Since setTranslation[X|Y] only affect draw positioning, it doesn't
// trigger a relayout and is therefore safe to do relatively unguarded
// here.
onLayout(boolean changed, ...) {
  super.onLayout(...);
  if (changed && mActionIcon != null) {
      float offset = mInputLayout.getY() + mInput.getY() + (float)
mInput.getHeight()
              - (float) mActionIcon.getHeight() - mActionIcon.getTop();
      mActionIcon.setTranslationY(offset);
  }
}

I "hope" that is somewhat clear about the flow of these APIs, but let me know if
not.  I tried to add inline comments describing it for the use of the review,
but not something that should necessarily go in code.

https://codereview.chromium.org/2434383002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java:198:
public AutoCompleteTextView getEditText() {
for non-private methods, it's a best practice to add javadocs

[gogerald1, 2016-10-24 20:00:01]

Patchset #5 (id:240001) has been deleted

[gogerald1, 2016-10-24 20:11:15]

Patchset #5 (id:260001) has been deleted

[gogerald1, 2016-10-24 20:12:11]

The CQ bit was checked by gogerald@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 20:13:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gogerald1, 2016-10-24 20:41:52]

Patchset #5 (id:280001) has been deleted

[gogerald1, 2016-10-24 20:56:37]

https://codereview.chromium.org/2434383002/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java
(right):

https://codereview.chromium.org/2434383002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java:165:
public void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
On 2016/10/24 17:41:41, Ted C (OOO 10.25 - 30) wrote:
> welcome to the wild world of android layouts, so forgive me for the rambling
> reply.
> 
> setting the padding actually will retrigger a layout pass as it affects the
> measurement.
> 
> I have a couple potential suggestions:
> 1.) In the XML file, pull this android:layout_width="48dp" out to use a @dimen
> reference.  Then instead of relying on mActionIcon.getWidth(), which will only
> be resolved after a full measure and layout pass, you would just pull the same
> value of of dimens.
> 2.) Instead of relying on a full multi-pass measure/layout, you would do
> something like the follows:
> 
> onMeasure(...) {
>   // This assumes the width never changes, potentially an unneeded
>   // optimization.
>   if (mActionIcon != null && mActionIcon.getMeasuredWidth() == 0) {
>       // Using the existing measure specs is fine in this case because we
>       // know the view size is bounded.
>       mActionIcon.onMeasure(widthMeasureSpec, heightMeasureSpec);
>   }
>   if (mActionIcon != null) {
>       // FYI: I'm using getMeasuredWidth() here as it doesn't rely on the
>       // layout pass.
>       ApiCompatibilityUtils.setPaddingRelative(mInput,
>               ApiCompatibilityUtils.getPaddingStart(mInput),
> mInput.getPaddingTop(),
>               mActionIcon.getMeasuredWidth(), mInput.getPaddingBottom());
>   }
>   super.onMeasure(widthMeasureSpec, heightMeasureSpec);
> }
> 
> // The problem with using getY() and getHeight(), getTop() is that it
> // requires running layout first, so it won't be right inside of
> // onMeasure (it might have worked because setPadding was causing a
> // double trigger, but that isn't something we should rely on).
> //
> // Since setTranslation[X|Y] only affect draw positioning, it doesn't
> // trigger a relayout and is therefore safe to do relatively unguarded
> // here.
> onLayout(boolean changed, ...) {
>   super.onLayout(...);
>   if (changed && mActionIcon != null) {
>       float offset = mInputLayout.getY() + mInput.getY() + (float)
> mInput.getHeight()
>               - (float) mActionIcon.getHeight() - mActionIcon.getTop();
>       mActionIcon.setTranslationY(offset);
>   }
> }
> 
> I "hope" that is somewhat clear about the flow of these APIs, but let me know
if
> not.  I tried to add inline comments describing it for the use of the review,
> but not something that should necessarily go in code.

Done.
Clear enough. mAction will soon be grouped into a LinearLayout to add additional
icon which will show and gone dynamically
(https://codereview.chromium.org/2442933003/), so I would not fix the width by
@dimen.

https://codereview.chromium.org/2434383002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java:198:
public AutoCompleteTextView getEditText() {
On 2016/10/24 17:41:41, Ted C (OOO 10.25 - 30) wrote:
> for non-private methods, it's a best practice to add javadocs

Done.

[Ted C, 2016-10-24 21:04:56]

lgtm

[gogerald1, 2016-10-24 21:07:29]

The CQ bit was checked by gogerald@chromium.org

[gogerald1, 2016-10-24 21:07:29]

The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2434383002/#ps300001
(title: "add comments, move setTranslationY")

[commit-bot: I haz the power, 2016-10-24 21:07:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 00:21:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 00:21:55]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[gogerald1, 2016-10-25 00:44:47]

The CQ bit was checked by gogerald@chromium.org

[commit-bot: I haz the power, 2016-10-25 00:45:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 01:37:41]

Description was changed from

==========
Float card scanner icon at the end of EditText instead of compound drawable

This gives better user experience since compound drawable do not support click
event.

BUG=657178,640430
==========

to

==========
Float card scanner icon at the end of EditText instead of compound drawable

This gives better user experience since compound drawable do not support click
event.

BUG=657178,640430
==========

[commit-bot: I haz the power, 2016-10-25 01:37:42]

Committed patchset #5 (id:300001)

[commit-bot: I haz the power, 2016-10-25 01:41:38]

Description was changed from

==========
Float card scanner icon at the end of EditText instead of compound drawable

This gives better user experience since compound drawable do not support click
event.

BUG=657178,640430
==========

to

==========
Float card scanner icon at the end of EditText instead of compound drawable

This gives better user experience since compound drawable do not support click
event.

BUG=657178,640430

Committed: https://crrev.com/c3da4b51497fca50ebed5e4adbec3f7ada68ceda
Cr-Commit-Position: refs/heads/master@{#427222}
==========

[commit-bot: I haz the power, 2016-10-25 01:41:40]

Patchset 5 (id:??) landed as
https://crrev.com/c3da4b51497fca50ebed5e4adbec3f7ada68ceda
Cr-Commit-Position: refs/heads/master@{#427222}