Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Commit

Permalink
Make findbar not a partially controlled component
Browse files Browse the repository at this point in the history
This gets rid of the warning that React gives about a controlled component.  It also fixes the problem with when state updates because a background tab changes, it should not re-select text

Fix #5363
  • Loading branch information
bbondy committed Nov 4, 2016
1 parent 9566bcd commit d73dd6e
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 15 deletions.
13 changes: 6 additions & 7 deletions js/components/findbar.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,16 +79,20 @@ class FindBar extends ImmutableComponent {
}

componentDidMount () {
this.searchInput.value = this.searchString
this.focus()
this.select()
windowActions.setFindbarSelected(false)
}

componentWillUpdate (nextProps) {
this.didFrameChange = nextProps.frameKey !== this.props.frameKey
if (nextProps.frameKey !== this.props.frameKey) {
this.searchInput.value = nextProps.findDetail && nextProps.findDetail.get('searchString') || ''
}
}

componentDidUpdate (prevProps) {
if (this.props.selected) {
if (this.props.selected && !prevProps.selected) {
this.focus()
// Findbar might already be focused, so make sure select happens even if no
// onFocus event happens.
Expand Down Expand Up @@ -203,10 +207,6 @@ class FindBar extends ImmutableComponent {
}
}

const inputValue = this.didFrameChange
? this.searchString || undefined
: undefined

return <div className='findBar' style={findBarStyle} onBlur={this.onBlur}>
<div className='searchContainer'>
<div className='searchStringContainer'>
Expand All @@ -215,7 +215,6 @@ class FindBar extends ImmutableComponent {
spellCheck='false'
onContextMenu={this.onContextMenu}
ref={(node) => { this.searchInput = node }}
value={inputValue}
onFocus={this.onInputFocus}
onKeyDown={this.onKeyDown}
onKeyUp={this.onKeyUp} />
Expand Down
41 changes: 33 additions & 8 deletions test/components/frameTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ describe('findbar', function () {
yield setup(this.app.client)
const url = Brave.server.url('find_in_page.html')
yield this.app.client
.tabByUrl(Brave.newTabUrl)
.tabByIndex(0)
.url(url)
.waitForUrl(url)
.windowParentByUrl(url)
Expand Down Expand Up @@ -163,6 +163,30 @@ describe('findbar', function () {
assert.equal(match, '2 of 2')
})

it('typing while another frame is loading', function * () {
const url2 = Brave.server.url('find_in_page2.html')
yield this.app.client
.showFindbar()
.waitForElementFocus(findBarInput)
.ipcSend(messages.SHORTCUT_NEW_FRAME, url2, { openInForeground: false })
.setValue(findBarInput, 'test')
.waitUntil(function () {
return this.getValue(findBarInput).then((val) => val === 'test')
})
.showFindbar()
.waitForElementFocus(findBarInput)
.keys('x')
.waitUntil(function () {
return this.getValue(findBarInput).then((val) => val === 'x')
})
.keys('y')
.keys('z')
.waitUntil(function () {
return this.getValue(findBarInput).then((val) => val === 'xyz')
})
.ipcSend(messages.SHORTCUT_CLOSE_FRAME, 2)
})

it('findbar input remembered but no active ordinals after navigation until RETURN key', function * () {
const url2 = Brave.server.url('find_in_page2.html')
yield this.app.client
Expand All @@ -177,7 +201,7 @@ describe('findbar', function () {

yield this.app.client
.waitForVisible(findBarMatches)
.tabByUrl(Brave.newTabUrl)
.tabByIndex(0)
.url(url2)
.waitForUrl(url2)
.windowParentByUrl(url2)
Expand All @@ -199,17 +223,18 @@ describe('findbar', function () {
it('remembers findbar input when switching frames', function * () {
const url = Brave.server.url('find_in_page.html')
yield this.app.client
.tabByIndex(0)
.url(url)
.waitForUrl(url)
.windowParentByUrl(url)
.showFindbar()
.waitForElementFocus(findBarInput)
.setValue(findBarInput, 'test')
yield this.app.client
.ipcSend(messages.SHORTCUT_NEW_FRAME, url)
.waitForTabCount(2)
.tabByIndex(1)
.waitForUrl(url)
.windowParentByUrl(url)
.waitUntil(function () {
return this.getAttribute('webview[data-frame-key="2"]', 'src').then((src) => src === url)
})
.waitForElementFocus('webview[data-frame-key="2"]')
yield this.app.client
.showFindbar(true, 2)
.waitForElementFocus(findBarInput)
.setValue(findBarInput, 'abc')
Expand Down

2 comments on commit d73dd6e

@bbondy
Copy link
Member Author

@bbondy bbondy commented on d73dd6e Nov 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auditors: @diracdeltas

@diracdeltas
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Please sign in to comment.