Autofill should focus on first entry Fix #8280 #8510

kumarrishav · 2017-04-26T17:43:04Z

Test Plan:

Open browser and type about:autofill
Click on Add Address or Add Credit Card
Cursor will focus on first input field

Description

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).

kumarrishav · 2017-04-26T17:49:39Z

app/renderer/components/autofill/autofillAddressPanel.js

@@ -136,7 +138,7 @@ class AutofillAddressPanel extends ImmutableComponent {
              )}
                data-test-id='nameOnAddress'
                spellCheck='false' onKeyDown={this.onKeyDown} onChange={this.onNameChange}
-                value={this.props.currentDetail.get('name')}
+                value={this.props.currentDetail.get('name') || ''}


I added default value as {this.props.currentDetail.get('name') || ''} to avoid the warning (because of react 15 upgrade).
warning.js?8a56:36
Warning: AutofillAddressPanel is changing an uncontrolled input of type undefined to be controlled. Input elements should not switch from uncontrolled to controlled (or vice versa). Decide between using a controlled or uncontrolled input element for the lifetime of the component. More info: https://fb.me/react-controlled-components.

kumarrishav · 2017-04-26T17:49:51Z

app/renderer/components/autofill/autofillCreditCardPanel.js

@@ -115,7 +118,7 @@ class AutofillCreditCardPanel extends ImmutableComponent {
                  spellCheck='false'
                  onKeyDown={this.onKeyDown}
                  onChange={this.onNameChange}
-                  value={this.props.currentDetail.get('name')}
+                  value={this.props.currentDetail.get('name') || ''}


kumarrishav · 2017-04-26T18:32:54Z

Warning in console

darkdh

@kumarrishav could you also add test to test/contents/autofillTest.js ?

kumarrishav · 2017-04-26T21:00:10Z

@darkdh will test/contents/autofillTest.js be right place to test this? As name suggests this is to test the content in input field. I tried to find autofill related test test/app/renderer/components/ but couldn't find any.

darkdh · 2017-04-26T21:22:00Z

@kumarrishav you can add test there for now. All the autofill tests are in test/contents/autofillTest.js but we can do refactor later.

kumarrishav · 2017-04-26T21:28:06Z

@darkdh sure will do.
Btw i am facing this issue. what can be possible issue? https://community.brave.com/t/unable-to-run-test/2370

darkdh · 2017-04-26T21:34:53Z

sorry for the inconvenience. please do the following steps

make sure you have node v7.9.0
make sure your .npmrc contains this change 3f589aa

$ npm clean cache
$ rm -rf ~/.node-gyp
$ rm -rf ~/.electron
$ rm -rf node_modules
$ npm install

kumarrishav · 2017-04-26T22:00:51Z

@darkdh sorry it failed again.
Currently i am on master branch (latest code).
This time different error:

brave@0.15.0 test /Users/krishav/Documents/FOSS/Brave/browser-laptop
cross-env NODE_ENV=test mocha "test/**/*Test.js"

/Users/krishav/Documents/FOSS/Brave/browser-laptop/node_modules/bindings/bindings.js:83
throw e
^

Error: The module '/Users/krishav/Documents/FOSS/Brave/browser-laptop/node_modules/leveldown/build/Release/leveldown.node'
was compiled against a different Node.js version using
NODE_MODULE_VERSION 55. This version of Node.js requires
NODE_MODULE_VERSION 51. Please try re-compiling or re-installing
the module (for instance, using npm rebuild ornpm install).
at Object.Module._extensions..node (module.js:598:18)
at Module.load (module.js:488:32)
at tryModuleLoad (module.js:447:12)
at Function.Module._load (module.js:439:3)
at Module.require (module.js:498:17)
at require (internal/module.js:20:19)
at bindings (/Users/krishav/Documents/FOSS/Brave/browser-laptop/node_modules/bindings/bindings.js:76:44)
at Object. (/Users/krishav/Documents/FOSS/Brave/browser-laptop/node_modules/leveldown/leveldown.js:4:46)
at Module._compile (module.js:571:32)
at Module._extensions..js (module.js:580:10)

kumarrishav · 2017-04-26T22:04:33Z

why this version issue is happening?

darkdh · 2017-04-27T04:20:48Z

@kumarrishav it is because we have different ABI version. Please contain this commit 576921b and try again.

kumarrishav · 2017-04-27T10:49:39Z

@darkdh this fixes the issue.

kumarrishav · 2017-04-28T13:19:06Z

@darkdh I am not sure how to test the 'focus' thing in integration test. Also, i am getting this error while running test. I think unit test is right place to test it.

darkdh · 2017-04-30T16:57:11Z

@kumarrishav please rebase to master and try again. There will be some intermittent errors but at least Autofill address and Autofill credit card is testable
You can use waitForElementFocus to check element is focused and just add a it('check focus', function * () {}) to Autofill address and Autofill credit card before adds section.

kumarrishav · 2017-05-01T05:21:47Z

@darkdh sorry, but still same error. It fails at beforeEach hook. Though it fills the form, but fails at first it.('adds an autofill address' function * () {}) itself

NejcZdovc · 2017-05-01T07:58:27Z

@kumarrishav I just try running test on the master and it's working for me

luixxiul · 2017-05-01T10:44:08Z

@kumarrishav to run webdriver test open a console, run npm run watch-test. open another console and run the test specified above. It should work.

darkdh · 2017-05-01T15:56:45Z

@kumarrishav you can also run npm run watch-all which allows you to run test and launch browser-laptop

kumarrishav · 2017-05-03T13:09:04Z

Sorry @darkdh Still same error. Don't know why it's not working in my system (Mac OS, Node -v : 7.9.0 and npm 4.2.0). Can we move this test later to UT (and file another issue for same)


        Autofill should focus on first entry Fix #8280


        Add fist entry focus test

@kumarrishav

Auditors: @kumarrishav, @bsclifton

darkdh · 2017-05-03T16:19:51Z

@kumarrishav , I've added the test in d1f502a
Thanks for your work.

bsclifton

Changes look great! Tested manually and also ran npm run test -- --grep="Autofill"; everything is looking good 😄

bsclifton · 2017-05-03T20:28:10Z

Thanks, @kumarrishav! 😄

luixxiul added the feature/autofill label Apr 26, 2017

kumarrishav reviewed Apr 26, 2017

View changes

bsclifton assigned kumarrishav Apr 26, 2017

bsclifton added this to the 0.15.1 milestone Apr 26, 2017

darkdh self-requested a review Apr 26, 2017

darkdh suggested changes Apr 26, 2017

View changes

kumarrishav and others added 2 commits Apr 26, 2017

Autofill should focus on first entry Fix #8280

Verified

This commit was signed with a verified signature.

darkdh Anthony Tseng

GPG key ID: E14094A20D3AAEEC Learn about signing commits

a800fdb

Add fist entry focus test

Verified

This commit was signed with a verified signature.

darkdh Anthony Tseng

GPG key ID: E14094A20D3AAEEC Learn about signing commits

Loading status checks…

d1f502a

Auditors: @kumarrishav, @bsclifton

darkdh force-pushed the kumarrishav:autoFillautoFocusFix branch from ecf383c to d1f502a May 3, 2017

darkdh added the PR/ready-for-merge label May 3, 2017

darkdh self-assigned this May 3, 2017

darkdh approved these changes May 3, 2017

View changes

bsclifton self-requested a review May 3, 2017

bsclifton approved these changes May 3, 2017

View changes

bsclifton added QA/test-plan-specified release-notes/include and removed PR/ready-for-merge labels May 3, 2017

bsclifton merged commit 612350d into brave:master May 3, 2017
0 of 2 checks passed

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

continuous-integration/appveyor/pr AppVeyor build failed
Details

alexwykoff mentioned this pull request May 4, 2017

Autofill should automatically move cursor to first entry when adding a new record #8280

Closed

brave / browser-laptop Archived

Autofill should focus on first entry Fix #8280 #8510

Autofill should focus on first entry Fix #8280 #8510

kumarrishav commented Apr 26, 2017 •

edited by bsclifton

This comment has been minimized.

This comment has been minimized.

kumarrishav commented Apr 26, 2017

darkdh left a comment

kumarrishav commented Apr 26, 2017

darkdh commented Apr 26, 2017

kumarrishav commented Apr 26, 2017

darkdh commented Apr 26, 2017

kumarrishav commented Apr 26, 2017

kumarrishav commented Apr 26, 2017

darkdh commented Apr 27, 2017

kumarrishav commented Apr 27, 2017

kumarrishav commented Apr 28, 2017 •

edited

darkdh commented Apr 30, 2017

kumarrishav commented May 1, 2017

NejcZdovc commented May 1, 2017

luixxiul commented May 1, 2017

darkdh commented May 1, 2017

kumarrishav commented May 3, 2017

darkdh commented May 3, 2017

bsclifton left a comment

bsclifton commented May 3, 2017

brave / browser-laptop Archived

Join GitHub today

Autofill should focus on first entry Fix #8280 #8510

Autofill should focus on first entry Fix #8280 #8510

Conversation

kumarrishav commented Apr 26, 2017 • edited by bsclifton

Test Plan:

Description

This comment has been minimized.

kumarrishav Apr 26, 2017 • edited

This comment has been minimized.

kumarrishav Apr 26, 2017

kumarrishav commented Apr 26, 2017

darkdh left a comment

kumarrishav commented Apr 26, 2017

darkdh commented Apr 26, 2017

kumarrishav commented Apr 26, 2017

darkdh commented Apr 26, 2017

kumarrishav commented Apr 26, 2017

kumarrishav commented Apr 26, 2017

darkdh commented Apr 27, 2017

kumarrishav commented Apr 27, 2017

kumarrishav commented Apr 28, 2017 • edited

darkdh commented Apr 30, 2017

kumarrishav commented May 1, 2017

NejcZdovc commented May 1, 2017

luixxiul commented May 1, 2017

darkdh commented May 1, 2017

kumarrishav commented May 3, 2017

darkdh commented May 3, 2017

bsclifton left a comment

bsclifton commented May 3, 2017

kumarrishav commented Apr 26, 2017 •

edited by bsclifton

kumarrishav Apr 26, 2017 •

edited

kumarrishav commented Apr 28, 2017 •

edited