Issue #9053: Making country field in autofill a dropdown list instead of text input box. #10821

luixxiul · 2017-09-06T11:30:16Z

I mistakenly closed #10300 opened by @dfperry5. I picked up the 4 commits from the PR and added a commit to add CommonFormFullWidthDropdown. Apologies to @dfperry5.

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.

Test Plan:

Open about:autofill
Click Add Address
Click Country
Input united states
Make sure United States is selected
Hit enter key
Click Save
Open https://www.roboform.com/filling-test-all-fields
Select Country
Make sure United States appears on the candidate list

Automated test plan:

npm run test -- --grep="Autofill"

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

luixxiul · 2017-09-06T11:38:01Z

With this PR:

codecov-io · 2017-09-06T11:50:12Z

Codecov Report

Merging #10821 into master will decrease coverage by 0.01%.
The diff coverage is 10%.

@@            Coverage Diff             @@
##           master   #10821      +/-   ##
==========================================
- Coverage   54.17%   54.15%   -0.02%     
==========================================
  Files         247      247              
  Lines       21558    21575      +17     
  Branches     3342     3344       +2     
==========================================
+ Hits        11679    11684       +5     
- Misses       9879     9891      +12

Flag	Coverage Δ
#unittest	`54.15% <10%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
app/renderer/components/common/dropdown.js	`90% <ø> (-1.67%)`	⬇️
app/renderer/components/common/commonForm.js	`29.82% <0%> (-1.66%)`	⬇️
app/browser/menu.js	`52.19% <0%> (-0.55%)`	⬇️
app/locale.js	`35.23% <0%> (-0.34%)`	⬇️
js/l10n.js	`40.74% <50%> (-5.1%)`	⬇️

darkdh

lgtm

NejcZdovc

tests are failing, so I think that we need to check them out

https://travis-ci.org/brave/browser-laptop/jobs/272442182#L3535

luixxiul · 2017-09-06T17:07:55Z

~~I forgot to replace the data-test-id for CommonFormFullWidthDropdown. I'm fixing it soon.~~

luixxiul · 2017-09-06T18:39:28Z

It looks like the country is not correctly selected from the dropdown.

waitForElementFocus("[data-test-id="nameOnAddress"]", "undefined")
typeText([data-test-id="nameOnAddress"], Brave Lion, undefined)
typeText([data-test-id="organization"], Brave, undefined)
typeText([data-test-id="streetAddress"], 1161 Mission Street, #401, undefined)
typeText([data-test-id="city"], San Francisco, undefined)
typeText([data-test-id="state"], CA, undefined)
typeText([data-test-id="postalCode"], 94103-1550, undefined)
typeText([data-test-id="country"], US, undefined)
      2) "before each" hook for "adds an autofill address"
waitForBrowserWindow()
windowByUrl("chrome://brave/Users/Suguru/browser-laptop/app/extensions/brave/index.html")


  0 passing (58s)
  2 failing

Locally I noticed Uganda was selected instead of United States, leaving the dropdown open.

bsclifton · 2017-09-06T18:42:47Z

@luixxiul for the selection to work, you'd have to type what the actual value in the listbox is: ex "United" or "United States". Typing US is going to match countries like "us*"

luixxiul · 2017-09-07T01:23:59Z

@bsclifton I think I tried that too, but it did not work for me. I think I am doing something wrong. Would you mind helping me out? thanks!

bsclifton · 2017-09-09T06:57:49Z

@luixxiul I fixed the tests- unfortunately, your fork does not allow updates though. Did you specifically uncheck this box on purpose when creating the PR?

bsclifton · 2017-09-09T07:04:07Z

@luixxiul please cherry-pick this commit in:
3351c6d

luixxiul · 2017-09-09T08:51:14Z

@bsclifton I see! thanks for your commit. I'll check it out and cherry-pick it.

luixxiul · 2017-09-09T10:38:35Z

I get another new error which I'm not sure if this PR causes: https://travis-ci.org/brave/browser-laptop/jobs/273571165#L3754

  1) Autofill address autofills the updated address when edited:
     unknown error: Element is not clickable at point (1131, 189)
  Error: An unknown server-side error occurred while processing the command.
      at elementIdClick("0.05012787194082691-1") - click.js:20:22

@bsclifton @NejcZdovc do you know how to fix this?

bsclifton · 2017-09-09T17:05:33Z

I can't repro the issue locally- going to try on Linux

bsclifton · 2017-09-09T17:54:39Z

Also works when I run with Ubuntu 14.04 locally... hmm

bsclifton

Changes work great (great job, @dfperry5!) Looks and feels much better. Tests pass locally- I think we'll need to investigate more on why one test fails before merging.

luixxiul · 2017-09-09T18:25:40Z

related? https://www.drupal.org/node/2869464

dfperry5 · 2017-09-09T19:08:47Z

@bsclifton @luixxiul Hi guys, I've been keeping up with the thread - is there anything I can do to help? I know Brian said it could be something yall need to look at with the test.

bsclifton · 2017-09-09T22:12:07Z

@dfperry5 we're good to go from your end- maybe @NejcZdovc can help me figure out what's wrong with the test (or more specifically, where it's breaking). I'll be looking into it also 😄 Hopefully we'll get that squared away and get this merged for you. Thanks for hanging in there 😄

Creating a dropdown for the country list in the about:autofill panel Fix #9053

@dfperry5

Auditors: @dfperry5

@bbondy

- handle when locale is called with null/undefined/falsey value (before would fail on toLowerCase()) - handle situation where activeTab is not set and developer tools are opened (will be fixed soon by @bbondy) Auditors: @bbondy, @dfperry5

… is a blank modified: app/renderer/components/autofill/autofillAddressPanel.js modified: app/renderer/components/autofill/autofillAddressPanel.js modified: app/renderer/components/autofill/autofillAddressPanel.js modified: app/renderer/components/autofill/autofillAddressPanel.js

@cezaraugusto

Auditors: @cezaraugusto Test Plan: 1. Open about:autofill 2. Click 'Add Address'

@dfperry5

…stead of a text field Auditors: @dfperry5, @darkdh, @luixxiul Test Plan: `npm run test -- --grep="Autofill"

bsclifton · 2017-09-10T05:49:05Z

Looking back at travis-ci, previous builds failed on this same issue
https://travis-ci.org/brave/browser-laptop/jobs/273315807#L3610

I created an issue to track the failing test (since it has been present for a while)
#10879

bsclifton

++

Because the tests work locally and the same one failed BEFORE this PR, I believe this is safe to merge 😄

bsclifton · 2017-09-10T05:52:01Z

Congrats on your first contribution, @dfperry5 😄 🎉

Let us know if there's another issue we can help with (and apologies this one took so long to get sorted out)

NejcZdovc · 2017-09-10T12:15:12Z

@bsclifton we started getting this errors with this commit 3e38aee and it's not related to this PR. I am still figuring out what is going on and once I do it will fix multiple failing tests.

luixxiul added feature/autofill l10n needs-triage labels Sep 6, 2017

luixxiul requested a review from bsclifton September 6, 2017 11:30

luixxiul mentioned this pull request Sep 6, 2017

Issue #9053: Making country field in autofill a dropdown list instead of text input box. #10300

Closed

8 tasks

luixxiul requested a review from darkdh September 6, 2017 11:31

srirambv mentioned this pull request Sep 6, 2017

Make Autofill country field into a dropdown list #9053

Closed

luixxiul added this to the 0.21.x (Nightly Channel) milestone Sep 6, 2017

luixxiul removed the needs-triage label Sep 6, 2017

darkdh approved these changes Sep 6, 2017

View reviewed changes

NejcZdovc suggested changes Sep 6, 2017

View reviewed changes

luixxiul added the PR/work-in-progress ⚒ label Sep 6, 2017

luixxiul added help wanted The PR/issue opener needs help to complete/report the task. and removed PR/work-in-progress ⚒ labels Sep 6, 2017

bsclifton removed the help wanted The PR/issue opener needs help to complete/report the task. label Sep 9, 2017

bsclifton approved these changes Sep 9, 2017

View reviewed changes

dfperry5 and others added 6 commits September 9, 2017 22:25

Converted Country Textbox to Select Dropdown

282f45d

Creating a dropdown for the country list in the about:autofill panel Fix #9053

Small cleanup for autofill country list per discussion in PR

defd43c

Auditors: @dfperry5

Fix two issues

316eb8d

- handle when locale is called with null/undefined/falsey value (before would fail on toLowerCase()) - handle situation where activeTab is not set and developer tools are opened (will be fixed soon by @bbondy) Auditors: @bbondy, @dfperry5

Add 'CommonFormFullWidthDropdown' to commonForm.js

8a3c843

Auditors: @cezaraugusto Test Plan: 1. Open about:autofill 2. Click 'Add Address'

Update autofill webdriver tests to use Country as a dropdown value in…

eb8d792

…stead of a text field Auditors: @dfperry5, @darkdh, @luixxiul Test Plan: `npm run test -- --grep="Autofill"

bsclifton approved these changes Sep 10, 2017

View reviewed changes

bsclifton merged commit 79df4ed into brave:master Sep 10, 2017

luixxiul deleted the countryfield branch September 10, 2017 06:00

bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Oct 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #9053: Making country field in autofill a dropdown list instead of text input box. #10821

Issue #9053: Making country field in autofill a dropdown list instead of text input box. #10821

luixxiul commented Sep 6, 2017 •

edited by bsclifton

Loading

luixxiul commented Sep 6, 2017

codecov-io commented Sep 6, 2017 •

edited

Loading

darkdh left a comment

NejcZdovc left a comment

luixxiul commented Sep 6, 2017 •

edited

Loading

luixxiul commented Sep 6, 2017

bsclifton commented Sep 6, 2017

luixxiul commented Sep 7, 2017

bsclifton commented Sep 9, 2017

bsclifton commented Sep 9, 2017

luixxiul commented Sep 9, 2017

luixxiul commented Sep 9, 2017

bsclifton commented Sep 9, 2017

bsclifton commented Sep 9, 2017

bsclifton left a comment

luixxiul commented Sep 9, 2017

dfperry5 commented Sep 9, 2017

bsclifton commented Sep 9, 2017

bsclifton commented Sep 10, 2017

bsclifton left a comment •

edited

Loading

bsclifton commented Sep 10, 2017

NejcZdovc commented Sep 10, 2017

Issue #9053: Making country field in autofill a dropdown list instead of text input box. #10821

Issue #9053: Making country field in autofill a dropdown list instead of text input box. #10821

Conversation

luixxiul commented Sep 6, 2017 • edited by bsclifton Loading

Submitter Checklist:

Test Plan:

Automated test plan:

Reviewer Checklist:

luixxiul commented Sep 6, 2017

codecov-io commented Sep 6, 2017 • edited Loading

Codecov Report

darkdh left a comment

Choose a reason for hiding this comment

NejcZdovc left a comment

Choose a reason for hiding this comment

luixxiul commented Sep 6, 2017 • edited Loading

luixxiul commented Sep 6, 2017

bsclifton commented Sep 6, 2017

luixxiul commented Sep 7, 2017

bsclifton commented Sep 9, 2017

bsclifton commented Sep 9, 2017

luixxiul commented Sep 9, 2017

luixxiul commented Sep 9, 2017

bsclifton commented Sep 9, 2017

bsclifton commented Sep 9, 2017

bsclifton left a comment

Choose a reason for hiding this comment

luixxiul commented Sep 9, 2017

dfperry5 commented Sep 9, 2017

bsclifton commented Sep 9, 2017

bsclifton commented Sep 10, 2017

bsclifton left a comment • edited Loading

Choose a reason for hiding this comment

bsclifton commented Sep 10, 2017

NejcZdovc commented Sep 10, 2017

luixxiul commented Sep 6, 2017 •

edited by bsclifton

Loading

codecov-io commented Sep 6, 2017 •

edited

Loading

luixxiul commented Sep 6, 2017 •

edited

Loading

bsclifton left a comment •

edited

Loading