Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2722283004:
[Payments] Fix focus bug in the expiration date CVC unmaskp prompt. (Closed)
|
Created:
3 years, 9 months ago by sebsg Modified:
3 years, 9 months ago Reviewers:
please use gerrit instead CC:
chromium-reviews, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, agrieve+watch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Payments] Fix focus bug in the expiration date CVC unmaskp prompt.
BUG=697408
Review-Url: https://codereview.chromium.org/2722283004
Cr-Commit-Position: refs/heads/master@{#454594}
Committed: https://chromium.googlesource.com/chromium/src/+/298bf2e2d0ccfc87571bae60b2c0259d58f0d18e
Patch Set 1 #
Total comments: 4
Patch Set 2 : Record DidFocusOn instead of DidType #
Total comments: 3
Patch Set 3 : Only record focus changes #Messages
Total messages: 18 (9 generated)
sebsg@chromium.org changed reviewers: + rouslan@chromium.org
Hi Rouslan, PTAL?
https://codereview.chromium.org/2722283004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/2722283004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:510: // The user just finished typing in the month field and there iare no validation iare? https://codereview.chromium.org/2722283004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:512: if (mMonthInput.getText().length() == EXPIRATION_FIELDS_LENGTH) { You appear to be checking mMonthInput.getText().length() on lines 509 and 512. That seems strange. Please explain.
Hi Rouslan, Another look? By changing the logic from DidTypeIn* to DidFocusOn* I can remove all those sketchy double focus bugs :) https://codereview.chromium.org/2722283004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/2722283004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:510: // The user just finished typing in the month field and there iare no validation On 2017/03/02 20:55:27, ಠ_ಠ wrote: > iare? Done. https://codereview.chromium.org/2722283004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:512: if (mMonthInput.getText().length() == EXPIRATION_FIELDS_LENGTH) { On 2017/03/02 20:55:27, ಠ_ಠ wrote: > You appear to be checking mMonthInput.getText().length() on lines 509 and 512. > That seems strange. Please explain. More than strange, it was actually a mistake :P
This code is super tricky, so needs some more playing around etc. https://codereview.chromium.org/2722283004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/2722283004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:512: if (mYearInput.getText().length() == EXPIRATION_FIELDS_LENGTH) { Please verify something here: try typing in the current year first (17), then typing in January (01). Focus should stay on month and the date field should turn red with error message "invalid date". https://codereview.chromium.org/2722283004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:613: if (mDidFocusOnCvc && !mCardUnmaskInput.isFocused()) { Please verify something for me: type in any month, wait for focus to move to year, then type in a single digit, e.g., "2". Don't type the second digit yet. There should be no validation messages just yet, even though this date is not actually valid. Is this indeed in the case?
I uploaded two videos corresponding to your testing suggestions in the bug. I also did find another bug that is fixed with my patch2. To trigger: fill a valid exp month and year. The focus will move to CVC. Without typing anything, click on year. The focus will be on year and CVC. This used to happen because what the user requests as focused and what my code set as focused were different. In this case my code would send focus back on CVC sinc the user never typed in it and the exp date is valid. With my change from didUserTypeIn to DidFocusOn, it should not happen anymore.
lgtm % comment https://codereview.chromium.org/2722283004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/2722283004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:232: public void onTextChanged(CharSequence s, int start, int before, int count) { It's a bit confusing that onTextChanged flips a DidFocus. (Text change and focus are not the same.) Add a comment to clarify that this is intentional.
I checked and we don't need to record on text change, it makes more sense to record on focus changes :) and it's more clear.
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sebsg@chromium.org
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/2722283004/#ps40001 (title: "Only record focus changes")
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": 40001, "attempt_start_ts": 1488557941502190,
"parent_rev": "08f4cddf58834a8d8a7d7057e0b8d020c8ef73e1", "commit_rev":
"298bf2e2d0ccfc87571bae60b2c0259d58f0d18e"}
Message was sent while issue was closed.
Description was changed from ========== [Payments] Fix focus bug in the expiration date CVC unmaskp prompt. BUG=697408 ========== to ========== [Payments] Fix focus bug in the expiration date CVC unmaskp prompt. BUG=697408 Review-Url: https://codereview.chromium.org/2722283004 Cr-Commit-Position: refs/heads/master@{#454594} Committed: https://chromium.googlesource.com/chromium/src/+/298bf2e2d0ccfc87571bae60b2c0... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/298bf2e2d0ccfc87571bae60b2c0... |
