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

Improved UI/UX of find bar #3315

Merged
merged 3 commits into from
Aug 27, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/state.md
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,8 @@ WindowStore
searchString: string, // the string being searched
caseSensitivity: boolean, // whether we are doing a case sensitive search
numberOfMatches: number, // Total number of matches on the page
activeMatchOrdinal: number // The current ordinal of the match
activeMatchOrdinal: number, // The current ordinal of the match
internalFindStatePresent: boolean // true if a find-first (ie findNext: false) call has been made
}
unloaded: boolean, // true if the tab is unloaded

Expand Down
25 changes: 25 additions & 0 deletions js/actions/webviewActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,31 @@ const webviewActions = {
webview.executeJavaScript('document.webkitRequestFullscreen()')
}
}
},

findInPage: function (searchString, caseSensitivity, forward, findNext, webview) {
webview = webview || getWebview()
if (!webview) {
return
}

if (searchString) {
webview.findInPage(searchString, {
matchCase: caseSensitivity,
forward,
findNext
})
} else {
webview.stopFindInPage('clearSelection')
}
},

stopFindInPage: function (webview) {
webview = webview || getWebview()
if (!webview) {
return
}
webview.stopFindInPage('keepSelection')
}
}

Expand Down
2 changes: 2 additions & 0 deletions js/components/button.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ class Button extends ImmutableComponent {
if (this.props.iconClass) {
return <span disabled={this.props.disabled}
data-l10n-id={this.props.l10nId}
style={this.props.inlineStyles}
className={cx({
browserButton: true,
fa: true,
Expand All @@ -22,6 +23,7 @@ class Button extends ImmutableComponent {
return <span disabled={this.props.disabled}
data-l10n-id={this.props.l10nId}
data-l10n-args={JSON.stringify(this.props.l10nArgs || {})}
style={this.props.inlineStyles}
className={cx({
browserButton: true,
[this.props.className]: !!this.props.className
Expand Down
99 changes: 64 additions & 35 deletions js/components/findbar.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,16 @@ const ImmutableComponent = require('./immutableComponent')
const Immutable = require('immutable')
const keyCodes = require('../constants/keyCodes')
const Button = require('./button.js')
const SwitchControl = require('../components/switchControl')
const windowActions = require('../actions/windowActions')
const windowStore = require('../stores/windowStore')
const {getTextColorForBackground} = require('../lib/color')

class FindBar extends ImmutableComponent {
constructor () {
super()
this.onBlur = this.onBlur.bind(this)
this.onClear = this.onClear.bind(this)
this.onKeyDown = this.onKeyDown.bind(this)
this.onChange = this.onChange.bind(this)
this.onFindPrev = this.onFindPrev.bind(this)
Expand All @@ -35,20 +38,20 @@ class FindBar extends ImmutableComponent {
onCaseSensitivityChange (e) {
windowActions.setFindDetail(this.frame, Immutable.fromJS({
searchString: this.searchString,
caseSensitivity: e.target.checked
caseSensitivity: !this.isCaseSensitive
}))
}

onFindFirst () {
this.props.onFind(this.searchString, this.isCaseSensitive)
this.props.onFind(this.searchString, this.isCaseSensitive, true, false)
}

onFindNext () {
this.props.onFind(this.searchString, this.isCaseSensitive, true)
this.props.onFind(this.searchString, this.isCaseSensitive, true, this.props.findDetail.get('internalFindStatePresent'))
}

onFindPrev () {
this.props.onFind(this.searchString, this.isCaseSensitive, false)
this.props.onFind(this.searchString, this.isCaseSensitive, false, this.props.findDetail.get('internalFindStatePresent'))
}

/**
Expand Down Expand Up @@ -97,6 +100,14 @@ class FindBar extends ImmutableComponent {
windowActions.setFindbarSelected(this.frame, false)
}

onClear () {
windowActions.setFindDetail(this.frame, Immutable.fromJS({
searchString: '',
caseSensitivity: this.isCaseSensitive
}))
this.focus()
}

get numberOfMatches () {
if (!this.props.findDetail || this.props.findDetail.get('numberOfMatches') === undefined) {
return -1
Expand Down Expand Up @@ -125,6 +136,14 @@ class FindBar extends ImmutableComponent {
return this.props.findDetail.get('searchString')
}

get backgroundColor () {
return this.props.paintTabs && (this.props.themeColor || this.props.computedThemeColor)
}

get textColor () {
return getTextColorForBackground(this.backgroundColor)
}

render () {
let findMatchText
if (this.numberOfMatches !== -1 && this.activeMatchOrdinal !== -1 && this.searchString) {
Expand All @@ -145,39 +164,49 @@ class FindBar extends ImmutableComponent {
data-l10n-id='findResultMatches' />
}

return <div className='findBar' onBlur={this.onBlur}>
<div className='searchStringContainer'>
<input type='text'
spellCheck='false'
ref={(node) => { this.searchInput = node }}
onKeyDown={this.onKeyDown}
onChange={this.onChange}
value={this.searchString} />
{findMatchText}
</div>
<Button iconClass='findButton fa-chevron-up'
className='findButton smallButton findPrev'
disabled={this.numberOfMatches === 0}

onClick={this.onFindPrev} />
<Button iconClass='findButton fa-chevron-down'
className='findButton smallButton findNext'
disabled={this.numberOfMatches === 0}
onClick={this.onFindNext} />
<Button iconClass='fa-times'
className='findButton smallButton hideButton'
onClick={this.props.onFindHide} />
<div className='caseSensitivityContainer'>
<input
const backgroundColor = this.backgroundColor
let findBarStyle = {}

if (backgroundColor) {
findBarStyle = {
background: backgroundColor,
color: this.textColor
}
}

return <div className='findBar' style={findBarStyle} onBlur={this.onBlur}>
<div className='searchContainer'>
<div className='searchStringContainer'>
<span className='searchStringContainerIcon fa fa-search'></span>
<input type='text'
spellCheck='false'
ref={(node) => { this.searchInput = node }}
onKeyDown={this.onKeyDown}
onChange={this.onChange}
value={this.searchString} />
<span className='searchStringContainerIcon fa fa-times'
onClick={this.onClear}></span>
</div>
<span className='findMatchText'>{findMatchText}</span>
<Button iconClass='findButton fa-caret-down'
inlineStyles={findBarStyle}
className='findButton smallButton findNext'
disabled={this.numberOfMatches <= 0}
onClick={this.onFindNext} />
<Button iconClass='findButton fa-caret-up'
inlineStyles={findBarStyle}
className='findButton smallButton findPrev'
disabled={this.numberOfMatches <= 0}
onClick={this.onFindPrev} />
<SwitchControl
id='caseSensitivityCheckbox'
type='checkbox'
className='caseSensitivityCheckbox'
checked={this.isCaseSensitive}
onChange={this.onCaseSensitivityChange} />
<label htmlFor='caseSensitivityCheckbox' data-l10n-id='caseSensitivity'>
{'Match case'}
</label>
checkedOn={this.isCaseSensitive}
onClick={this.onCaseSensitivityChange} />
<label htmlFor='caseSensitivityCheckbox' data-l10n-id='caseSensitivity' style={findBarStyle}></label>
</div>
<span className='findButton closeButton'
style={findBarStyle}
onClick={this.props.onFindHide}>+</span>
</div>
}
}
Expand Down
39 changes: 3 additions & 36 deletions js/components/frame.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
const React = require('react')
const urlParse = require('url').parse
const windowActions = require('../actions/windowActions')
const webviewActions = require('../actions/webviewActions')
const appActions = require('../actions/appActions')
const ImmutableComponent = require('./immutableComponent')
const Immutable = require('immutable')
Expand All @@ -22,7 +23,6 @@ const debounce = require('../lib/debounce.js')
const getSetting = require('../settings').getSetting
const config = require('../constants/config')
const settings = require('../constants/settings')
const FindBar = require('./findbar.js')
const { aboutUrls, isSourceAboutUrl, isTargetAboutUrl, getTargetAboutUrl, getBaseUrl, isNavigatableAboutPage } = require('../lib/appUrlUtil')
const { isFrameError } = require('../lib/errorUtil')
const locale = require('../l10n')
Expand All @@ -44,8 +44,6 @@ class Frame extends ImmutableComponent {
constructor () {
super()
this.onUpdateWheelZoom = debounce(this.onUpdateWheelZoom.bind(this), 20)
this.onFind = this.onFind.bind(this)
this.onFindHide = this.onFindHide.bind(this)
this.onFocus = this.onFocus.bind(this)
// Maps notification message to its callback
this.notificationCallbacks = {}
Expand Down Expand Up @@ -832,7 +830,7 @@ class Frame extends ImmutableComponent {

this.webview.addEventListener('did-navigate', (e) => {
if (this.props.findbarShown) {
this.onFindHide()
windowActions.setFindbarShown(this.frame, false)
}

for (let message in this.notificationCallbacks) {
Expand Down Expand Up @@ -993,15 +991,10 @@ class Frame extends ImmutableComponent {
}
const searchString = this.props.findDetail && this.props.findDetail.get('searchString')
if (searchString) {
this.onFind(searchString, this.props.findDetail && this.props.findDetail.get('caseSensitivity') || undefined, forward)
webviewActions.findInPage(searchString, this.props.findDetail && this.props.findDetail.get('caseSensitivity') || undefined, forward, this.props.findDetail.get('internalFindStatePresent'), this.webview)
}
}

onFindHide () {
windowActions.setFindbarShown(this.frame, false)
this.webview.stopFindInPage('keepSelection')
}

onUpdateWheelZoom () {
if (this.wheelDeltaY > 0) {
this.zoomIn()
Expand All @@ -1021,22 +1014,6 @@ class Frame extends ImmutableComponent {
}
}

onFind (searchString, caseSensitivity, forward) {
if (searchString) {
this.webview.findInPage(searchString, {
matchCase: caseSensitivity,
forward: forward !== undefined ? forward : true,
findNext: forward !== undefined
})
} else {
this.onClearMatch()
}
}

onClearMatch () {
this.webview.stopFindInPage('clearSelection')
}

get webRTCPolicy () {
return this.webview ? this.webview.getWebRTCIPHandlingPolicy() : WEBRTC_DEFAULT
}
Expand All @@ -1062,16 +1039,6 @@ class Frame extends ImmutableComponent {
? <FullScreenWarning location={this.props.location} />
: null
}
{
this.props.findbarShown && !this.props.isFullScreen
? <FindBar
onFind={this.onFind}
onFindHide={this.onFindHide}
frameKey={this.props.frameKey}
selected={this.props.findbarSelected}
findDetail={this.props.findDetail} />
: null
}
<div ref={(node) => { this.webviewContainer = node }}
className={cx({
webviewContainer: true,
Expand Down
36 changes: 35 additions & 1 deletion js/components/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const NavigationBar = require('./navigationBar')
const Frame = require('./frame')
const TabPages = require('./tabPages')
const TabsToolbar = require('./tabsToolbar')
const FindBar = require('./findbar.js')
const UpdateBar = require('./updateBar')
const NotificationBar = require('./notificationBar')
const DownloadsBar = require('./downloadsBar')
Expand Down Expand Up @@ -86,6 +87,8 @@ class Main extends ImmutableComponent {
this.onBraveMenu = this.onBraveMenu.bind(this)
this.onHamburgerMenu = this.onHamburgerMenu.bind(this)
this.onTabContextMenu = this.onTabContextMenu.bind(this)
this.onFind = this.onFind.bind(this)
this.onFindHide = this.onFindHide.bind(this)
this.checkForTitleMode = debounce(this.checkForTitleMode.bind(this), 20)
}
registerWindowLevelShortcuts () {
Expand Down Expand Up @@ -627,6 +630,24 @@ class Main extends ImmutableComponent {
windowActions.setUrlBarActive(false)
}

onFindHide () {
const activeFrame = FrameStateUtil.getActiveFrame(this.props.windowState)
windowActions.setFindbarShown(activeFrame, false)
webviewActions.stopFindInPage()
Copy link
Member

Choose a reason for hiding this comment

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

@bridiver this had to be called here to retain the existing behaviour.
stopFindInPage needs to be called to clear the highlight but to keep the current match in case you command+g.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the problem with this is that calling stopFindInPage resets the webcontents find state so you can't get back to the same search results

Copy link
Member

Choose a reason for hiding this comment

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

but it must keep it because it remembers the next position?

Copy link
Member

Choose a reason for hiding this comment

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

I think btw the webviewAction was used at one point and was committed out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it doesn't remember the next position, that was the bug. I started a slack conversation to explain

Copy link
Member

Choose a reason for hiding this comment

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

We can't remove the call because when we do the yellow highlight stays forever

windowActions.setFindDetail(this.frame, Immutable.fromJS({
internalFindStatePresent: false
}))
}

onFind (searchString, caseSensitivity, forward, findNext) {
webviewActions.findInPage(searchString, caseSensitivity, forward, findNext)
if (!findNext) {
windowActions.setFindDetail(this.frame, Immutable.fromJS({
internalFindStatePresent: true
}))
}
}

onTabContextMenu (e) {
const activeFrame = FrameStateUtil.getActiveFrame(this.props.windowState)
contextMenus.onTabsToolbarContextMenu(activeFrame, undefined, undefined, e)
Expand Down Expand Up @@ -901,6 +922,20 @@ class Main extends ImmutableComponent {
activeFrameKey={activeFrame && activeFrame.get('key') || undefined}
onMenu={this.onHamburgerMenu}
/>

{
activeFrame && activeFrame.get('findbarShown') && !activeFrame.get('isFullScreen')
? <FindBar
paintTabs={getSetting(settings.PAINT_TABS)}
themeColor={activeFrame.get('themeColor')}
computedThemeColor={activeFrame.get('computedThemeColor')}
frameKey={activeFrame.get('key')}
selected={activeFrame.get('findbarSelected')}
findDetail={activeFrame.get('findDetail')}
onFind={this.onFind}
onFindHide={this.onFindHide} />
: null
}
</div>
<div className='mainContainer'>
<div className='tabContainer'
Expand Down Expand Up @@ -933,7 +968,6 @@ class Main extends ImmutableComponent {
isFullScreen={frame.get('isFullScreen')}
showFullScreenWarning={frame.get('showFullScreenWarning')}
findbarShown={frame.get('findbarShown')}
findbarSelected={frame.get('findbarSelected')}
findDetail={frame.get('findDetail')}
hrefPreview={frame.get('hrefPreview')}
showOnRight={frame.get('showOnRight')}
Expand Down
Loading