[Payments] Update autofill/payments spacings

[Payments] Update autofill/payments spacings

* The fields felt a little squished.  Try to space them out a little better.

* Update the underline colors to match the TextInputLayers.

* Add support for half width controls in the display.

Screenshots here:
https://bugs.chromium.org/p/chromium/issues/detail?id=603635#c190

BUG=603635, 626451
Committed: https://crrev.com/975b62f4fe7cfd7f3de7ab6fc49a3cc20e7f49ce
Committed: https://crrev.com/815e8e8ecdfbbf1da2ef5ab958834b1644013911
Cr-Original-Commit-Position: refs/heads/master@{#404241}
Cr-Commit-Position: refs/heads/master@{#404446}

[gone, 2016-06-30 00:59:22]

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

[please use gerrit instead, 2016-06-30 01:03:38]

lgtm

[gone, 2016-07-01 01:05:40]

Description was changed from

==========
[Payments] Update autofill preference spacings

* The fields felt a little squished.  Try to space them out a little better.

* Update the underline colors to match the TextInputLayers

BUG=603635
==========

to

==========
[Payments] Update autofill preference spacings

* The fields felt a little squished.  Try to space them out a little better.

* Update the underline colors to match the TextInputLayers.

* Add support for half width controls in the display.

BUG=603635
==========

[gone, 2016-07-01 01:06:30]

dfalcantara@chromium.org changed reviewers:
+ ianwen@chromium.org

[gone, 2016-07-01 01:06:31]

Adding Ian to account for the expanded UI changes.

[gone, 2016-07-01 01:09:21]

(Also worth re-reviewing rouslan@)

[gone, 2016-07-01 01:09:37]

Description was changed from

==========
[Payments] Update autofill preference spacings

* The fields felt a little squished.  Try to space them out a little better.

* Update the underline colors to match the TextInputLayers.

* Add support for half width controls in the display.

BUG=603635
==========

to

==========
[Payments] Update autofill preference spacings

* The fields felt a little squished.  Try to space them out a little better.

* Update the underline colors to match the TextInputLayers.

* Add support for half width controls in the display.

Screenshots here:
https://bugs.chromium.org/p/chromium/issues/detail?id=603635#c190

BUG=603635
==========

[gone, 2016-07-01 01:10:37]

Description was changed from

==========
[Payments] Update autofill preference spacings

* The fields felt a little squished.  Try to space them out a little better.

* Update the underline colors to match the TextInputLayers.

* Add support for half width controls in the display.

Screenshots here:
https://bugs.chromium.org/p/chromium/issues/detail?id=603635#c190

BUG=603635
==========

to

==========
[Payments] Update autofill/payments spacings

* The fields felt a little squished.  Try to space them out a little better.

* Update the underline colors to match the TextInputLayers.

* Add support for half width controls in the display.

Screenshots here:
https://bugs.chromium.org/p/chromium/issues/detail?id=603635#c190

BUG=603635
==========

[please use gerrit instead, 2016-07-01 01:21:36]

Seal of approval

(lgtm)

[please use gerrit instead, 2016-07-01 06:14:13]

https://codereview.chromium.org/2114493002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java
(right):

https://codereview.chromium.org/2114493002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:168:
if (!invalidViews.contains(getCurrentFocus()))
focusInputField(invalidViews.get(0));
fyi, we should check for empty invalidViews here.

[gone, 2016-07-01 18:31:39]

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java
(right):

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:168:
if (!invalidViews.contains(getCurrentFocus()))
focusInputField(invalidViews.get(0));
On 2016/07/01 06:14:12, Rouslan (ツ) wrote:
> fyi, we should check for empty invalidViews here.

Should be fine; contains would fail wouldn't it?

[Ian Wen, 2016-07-01 19:19:04]

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
File chrome/android/java/res/layout/autofill_local_card_editor.xml (right):

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
chrome/android/java/res/layout/autofill_local_card_editor.xml:77:
<android.support.v7.widget.AppCompatSpinner
Correct me if I'm wrong. I thought the inflater we get from AppCompatActivity
should automatically convert Spinner in XML to AppCompatSpinner. Is it necessary
to specify "AppCompat" here?

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
chrome/android/java/res/layout/autofill_local_card_editor.xml:81:
android:focusable="true" />
Don't we need to set focusableInTouchmode to be true as well? If the purpose is
to make it focusable for keyboard navigation, "touch mode" should be a
requirement as well.

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
File chrome/android/java/res/layout/autofill_profile_editor.xml (right):

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
chrome/android/java/res/layout/autofill_profile_editor.xml:6: <!-- EditText
views in FloatLabelLayout require both hint and contentDescription to be set
After we stop using FloatLabelLayout, is it still true?

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java
(right):

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:105:
mHalfRowMargin = activity.getResources().getDimensionPixelSize(
Aha yes! I am always amazed at how Android can't handle such simplest spacing
matters in listviews and recycler views.

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:176:
private void validateForm(List<EditorTextField> invalidViews, ViewGroup root) {
This is essentially traversing the view hierarchy. Could we create a list of all
EditorFields, and traverse the list instead?

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:320:
R.layout.payment_request_editor_footer, null, false);
Use mLayout here as parent instead of null? If you specify parent in inflate(),
layout params will be correct and it might save you some trouble.

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java
(right):

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:65:
countriesLabel.setText(v.getContext().getString(R.string.autofill_profile_editor_country));
Q: can we set the string in XML?

[gone, 2016-07-01 23:00:50]

So your comment about that XML comment made me realize that I needed to make the
CL account for TalkBack.  The new patch set has a bunch of labelFors added
everywhere.  There's still some wonkiness where Android will automatically read
out an entire Dialog when it's opened, but I'm not sure of a workaround for that
and it's best left for another CL because it should already act like that.

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
File chrome/android/java/res/layout/autofill_local_card_editor.xml (right):

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
chrome/android/java/res/layout/autofill_local_card_editor.xml:77:
<android.support.v7.widget.AppCompatSpinner
On 2016/07/01 19:19:03, Ian Wen wrote:
> Correct me if I'm wrong. I thought the inflater we get from AppCompatActivity
> should automatically convert Spinner in XML to AppCompatSpinner. Is it
necessary
> to specify "AppCompat" here?

It doesn't always work.  Current guess is that we're passing around an
application Context instead of the correct themed context, which results in the
real spinner appearing instead of the AppCompatSpinner.  We had to do this in
infobars, for example.

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
chrome/android/java/res/layout/autofill_local_card_editor.xml:81:
android:focusable="true" />
On 2016/07/01 19:19:03, Ian Wen wrote:
> Don't we need to set focusableInTouchmode to be true as well? If the purpose
is
> to make it focusable for keyboard navigation, "touch mode" should be a
> requirement as well.

Focus in these XML files seems fairly wonky; I've tried to edit the ones that
might make sense.

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
File chrome/android/java/res/layout/autofill_profile_editor.xml (right):

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
chrome/android/java/res/layout/autofill_profile_editor.xml:6: <!-- EditText
views in FloatLabelLayout require both hint and contentDescription to be set
On 2016/07/01 19:19:03, Ian Wen wrote:
> After we stop using FloatLabelLayout, is it still true?

TextInputLayout doesn't seem to clear hints; removing the comment.

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java
(right):

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:105:
mHalfRowMargin = activity.getResources().getDimensionPixelSize(
On 2016/07/01 19:19:03, Ian Wen wrote:
> Aha yes! I am always amazed at how Android can't handle such simplest spacing
> matters in listviews and recycler views.

Lol well... the simplest case is 0 margin...

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:168:
if (!invalidViews.contains(getCurrentFocus()))
focusInputField(invalidViews.get(0));
On 2016/07/01 18:31:39, dfalcantara wrote:
> On 2016/07/01 06:14:12, Rouslan (ツ) wrote:
> > fyi, we should check for empty invalidViews here.
> 
> Should be fine; contains would fail wouldn't it?

D'oh, I get it.  Added an empty check.

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:176:
private void validateForm(List<EditorTextField> invalidViews, ViewGroup root) {
On 2016/07/01 19:19:03, Ian Wen wrote:
> This is essentially traversing the view hierarchy. Could we create a list of
all
> EditorFields, and traverse the list instead?

Huh.  Yeah, that makes a ton more sense.

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:320:
R.layout.payment_request_editor_footer, null, false);
On 2016/07/01 19:19:03, Ian Wen wrote:
> Use mLayout here as parent instead of null? If you specify parent in
inflate(),
> layout params will be correct and it might save you some trouble.

Problem is that we don't want to add it to the hierarchy, yet.  I doubt that the
View would keep the right kind of LayoutParams when it gets removed from its
parent.

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java
(right):

https://chromiumcodereview.appspot.com/2114493002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:65:
countriesLabel.setText(v.getContext().getString(R.string.autofill_profile_editor_country));
On 2016/07/01 19:19:03, Ian Wen wrote:
> Q: can we set the string in XML?

Not this one, no.  The layout includes a generic layout.

[Ian Wen, 2016-07-06 22:04:39]

lgtm.

Sorry for the delay.

https://chromiumcodereview.appspot.com/2114493002/diff/100001/chrome/android/...
File chrome/android/java/res/layout/autofill_profile_editor.xml (right):

https://chromiumcodereview.appspot.com/2114493002/diff/100001/chrome/android/...
chrome/android/java/res/layout/autofill_profile_editor.xml:37:
android:focusable="true"
Remove these since EditTexts are already focusable?

[gone, 2016-07-06 22:08:01]

https://codereview.chromium.org/2114493002/diff/100001/chrome/android/java/re...
File chrome/android/java/res/layout/autofill_profile_editor.xml (right):

https://codereview.chromium.org/2114493002/diff/100001/chrome/android/java/re...
chrome/android/java/res/layout/autofill_profile_editor.xml:37:
android:focusable="true"
On 2016/07/06 22:04:39, Ian Wen wrote:
> Remove these since EditTexts are already focusable?

Done.

[gone, 2016-07-06 22:08:06]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2016-07-06 22:08:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, ianwen@chromium.org
Link to the patchset: https://codereview.chromium.org/2114493002/#ps140001
(title: "Comments")

[commit-bot: I haz the power, 2016-07-06 22:09:30]

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-07-07 01:30:51]

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

[commit-bot: I haz the power, 2016-07-07 01:30:52]

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

[gone, 2016-07-07 18:52:23]

Had to make some minor changes to the PaymentRequestTestBase to get it to pass. 
Sending to CQ.

[gone, 2016-07-07 18:52:28]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2016-07-07 18:52:29]

The patchset sent to the CQ was uploaded after l-g-t-m from ianwen@chromium.org,
rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2114493002/#ps160001
(title: "Getting tests to pass")

[commit-bot: I haz the power, 2016-07-07 18:52:57]

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

[gone, 2016-07-07 20:57:41]

Description was changed from

==========
[Payments] Update autofill/payments spacings

* The fields felt a little squished.  Try to space them out a little better.

* Update the underline colors to match the TextInputLayers.

* Add support for half width controls in the display.

Screenshots here:
https://bugs.chromium.org/p/chromium/issues/detail?id=603635#c190

BUG=603635
==========

to

==========
[Payments] Update autofill/payments spacings

* The fields felt a little squished.  Try to space them out a little better.

* Update the underline colors to match the TextInputLayers.

* Add support for half width controls in the display.

Screenshots here:
https://bugs.chromium.org/p/chromium/issues/detail?id=603635#c190

BUG=603635,626451
==========

[commit-bot: I haz the power, 2016-07-07 22:07:40]

Description was changed from

==========
[Payments] Update autofill/payments spacings

* The fields felt a little squished.  Try to space them out a little better.

* Update the underline colors to match the TextInputLayers.

* Add support for half width controls in the display.

Screenshots here:
https://bugs.chromium.org/p/chromium/issues/detail?id=603635#c190

BUG=603635,626451
==========

to

==========
[Payments] Update autofill/payments spacings

* The fields felt a little squished.  Try to space them out a little better.

* Update the underline colors to match the TextInputLayers.

* Add support for half width controls in the display.

Screenshots here:
https://bugs.chromium.org/p/chromium/issues/detail?id=603635#c190

BUG=603635,626451
==========

[commit-bot: I haz the power, 2016-07-07 22:07:41]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-07-07 22:08:06]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-07 22:11:12]

Description was changed from

==========
[Payments] Update autofill/payments spacings

* The fields felt a little squished.  Try to space them out a little better.

* Update the underline colors to match the TextInputLayers.

* Add support for half width controls in the display.

Screenshots here:
https://bugs.chromium.org/p/chromium/issues/detail?id=603635#c190

BUG=603635,626451
==========

to

==========
[Payments] Update autofill/payments spacings

* The fields felt a little squished.  Try to space them out a little better.

* Update the underline colors to match the TextInputLayers.

* Add support for half width controls in the display.

Screenshots here:
https://bugs.chromium.org/p/chromium/issues/detail?id=603635#c190

BUG=603635,626451

Committed: https://crrev.com/975b62f4fe7cfd7f3de7ab6fc49a3cc20e7f49ce
Cr-Commit-Position: refs/heads/master@{#404241}
==========

[commit-bot: I haz the power, 2016-07-07 22:11:14]

Patchset 9 (id:??) landed as
https://crrev.com/975b62f4fe7cfd7f3de7ab6fc49a3cc20e7f49ce
Cr-Commit-Position: refs/heads/master@{#404241}

[gsennton, 2016-07-08 09:38:17]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/2137443002/ by gsennton@chromium.org.

The reason for reverting is: This patch breaks the Android Tests (dbg) bot, I've
filed bug crbug.com/626606 for this issue. It breaks in

C  857.741s Main  [FAIL]
org.chromium.chrome.browser.payments.PaymentRequestFreeShippingTest#testChangeCountryAddAddressAndPay:
C  857.741s Main  java.lang.AssertionError
C  857.741s Main  	at
org.chromium.chrome.browser.payments.ui.EditorView.addFieldViewToEditor(EditorView.java:293).

[gone, 2016-07-08 17:43:20]

Ian: This has the line that Rouslan suggested adding in EditorView.java, which
deals with the assert.  Can you take a quick look?

[gone, 2016-07-08 17:43:29]

Description was changed from

==========
[Payments] Update autofill/payments spacings

* The fields felt a little squished.  Try to space them out a little better.

* Update the underline colors to match the TextInputLayers.

* Add support for half width controls in the display.

Screenshots here:
https://bugs.chromium.org/p/chromium/issues/detail?id=603635#c190

BUG=603635,626451

Committed: https://crrev.com/975b62f4fe7cfd7f3de7ab6fc49a3cc20e7f49ce
Cr-Commit-Position: refs/heads/master@{#404241}
==========

to

==========
[Payments] Update autofill/payments spacings

* The fields felt a little squished.  Try to space them out a little better.

* Update the underline colors to match the TextInputLayers.

* Add support for half width controls in the display.

Screenshots here:
https://bugs.chromium.org/p/chromium/issues/detail?id=603635#c190

BUG=603635,626451

Committed: https://crrev.com/975b62f4fe7cfd7f3de7ab6fc49a3cc20e7f49ce
Cr-Commit-Position: refs/heads/master@{#404241}
==========

[Ian Wen, 2016-07-08 17:49:17]

lgtm again. 10th patchset is the revert

[gone, 2016-07-08 17:59:12]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2016-07-08 17:59:13]

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/2114493002/#ps180001
(title: "Fixing the assert")

[commit-bot: I haz the power, 2016-07-08 17:59:58]

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-07-08 18:27:49]

Description was changed from

==========
[Payments] Update autofill/payments spacings

* The fields felt a little squished.  Try to space them out a little better.

* Update the underline colors to match the TextInputLayers.

* Add support for half width controls in the display.

Screenshots here:
https://bugs.chromium.org/p/chromium/issues/detail?id=603635#c190

BUG=603635,626451

Committed: https://crrev.com/975b62f4fe7cfd7f3de7ab6fc49a3cc20e7f49ce
Cr-Commit-Position: refs/heads/master@{#404241}
==========

to

==========
[Payments] Update autofill/payments spacings

* The fields felt a little squished.  Try to space them out a little better.

* Update the underline colors to match the TextInputLayers.

* Add support for half width controls in the display.

Screenshots here:
https://bugs.chromium.org/p/chromium/issues/detail?id=603635#c190

BUG=603635,626451

Committed: https://crrev.com/975b62f4fe7cfd7f3de7ab6fc49a3cc20e7f49ce
Cr-Commit-Position: refs/heads/master@{#404241}
==========

[commit-bot: I haz the power, 2016-07-08 18:27:51]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-07-08 18:28:16]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 18:29:50]

Description was changed from

==========
[Payments] Update autofill/payments spacings

* The fields felt a little squished.  Try to space them out a little better.

* Update the underline colors to match the TextInputLayers.

* Add support for half width controls in the display.

Screenshots here:
https://bugs.chromium.org/p/chromium/issues/detail?id=603635#c190

BUG=603635,626451

Committed: https://crrev.com/975b62f4fe7cfd7f3de7ab6fc49a3cc20e7f49ce
Cr-Commit-Position: refs/heads/master@{#404241}
==========

to

==========
[Payments] Update autofill/payments spacings

* The fields felt a little squished.  Try to space them out a little better.

* Update the underline colors to match the TextInputLayers.

* Add support for half width controls in the display.

Screenshots here:
https://bugs.chromium.org/p/chromium/issues/detail?id=603635#c190

BUG=603635,626451

Committed: https://crrev.com/975b62f4fe7cfd7f3de7ab6fc49a3cc20e7f49ce
Committed: https://crrev.com/815e8e8ecdfbbf1da2ef5ab958834b1644013911
Cr-Original-Commit-Position: refs/heads/master@{#404241}
Cr-Commit-Position: refs/heads/master@{#404446}
==========

[commit-bot: I haz the power, 2016-07-08 18:29:52]

Patchset 10 (id:??) landed as
https://crrev.com/815e8e8ecdfbbf1da2ef5ab958834b1644013911
Cr-Commit-Position: refs/heads/master@{#404446}