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

Commit

Permalink
fixes to showing page host in titleMode
Browse files Browse the repository at this point in the history
Auditors: @bbondy
  • Loading branch information
diracdeltas committed Feb 8, 2016
1 parent a6776d0 commit 545c7a6
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 20 deletions.
24 changes: 10 additions & 14 deletions js/components/urlBar.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,29 +173,23 @@ class UrlBar extends ImmutableComponent {
this.updateDOM()
}

get titleValue () {
const parsedUrl = urlParse(this.props.activeFrameProps.get('location'))
let titlePrefix = ''
if (parsedUrl.protocol !== 'about:') {
titlePrefix += parsedUrl.host + ' | '
}
get hostValue () {
const parsed = urlParse(this.props.activeFrameProps.get('location'))
return parsed.host && parsed.protocol !== 'about:' ? parsed.host : ''
}

get titleValue () {
// For about:newtab we don't want the top of the browser saying New Tab
// Instead just show "Brave"
return ['about:blank', 'about:newtab'].includes(this.props.urlbar.get('location'))
? '' : titlePrefix + this.props.activeFrameProps.get('title')
? '' : this.props.activeFrameProps.get('title')
}

get locationValue () {
return ['about:newtab'].includes(this.props.urlbar.get('location'))
? '' : this.props.urlbar.get('location')
}

get inputValue () {
return this.props.titleMode
? this.titleValue : this.locationValue
}

get loadTime () {
let loadTime = ''
if (this.props.activeFrameProps.get('startLoadTime') &&
Expand Down Expand Up @@ -248,7 +242,9 @@ class UrlBar extends ImmutableComponent {
extendedValidation: this.extendedValidationSSL
})}/>
<div id='titleBar'>
{this.titleValue}
<span><strong>{this.hostValue}</strong></span>
<span>{this.hostValue && this.titleValue ? ' | ' : ''}</span>
<span>{this.titleValue}</span>
</div>
</div>
<input type='text'
Expand All @@ -259,7 +255,7 @@ class UrlBar extends ImmutableComponent {
onChange={this.onChange.bind(this)}
onClick={this.onClick.bind(this)}
onContextMenu={contextMenus.onUrlBarContextMenu.bind(this)}
value={this.inputValue}
value={this.locationValue}
data-l10n-id='urlbar'
className={cx({
insecure: !this.secure && this.props.loading === false && !this.isHTTPPage,
Expand Down
3 changes: 2 additions & 1 deletion test/lib/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,6 @@ module.exports = {
tabPage1: '[data-tab-page="0"]',
tabPage2: '[data-tab-page="1"]',
closeTab: '.closeTab',
urlbarIcon: '.urlbarIcon'
urlbarIcon: '.urlbarIcon',
titleBar: '#titleBar'
}
9 changes: 4 additions & 5 deletions test/navigationBarTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

const Brave = require('./lib/brave')
const Config = require('../js/constants/config').default
const {urlInput, activeWebview, activeTabFavicon, activeTab, navigatorLoadTime, urlbarIcon} = require('./lib/selectors')
const {urlInput, activeWebview, activeTabFavicon, activeTab, navigatorLoadTime, titleBar, urlbarIcon} = require('./lib/selectors')
const urlParse = require('url').parse
const assert = require('assert')

Expand Down Expand Up @@ -68,7 +68,7 @@ describe('urlbar', function () {
it('has title mode', function *() {
const host = this.host
yield this.app.client.waitUntil(function () {
return this.getValue(urlInput).then(val => val === host + ' | Page 1')
return this.getText(titleBar).then(val => val === host + ' | Page 1')
})
.isExisting(navigatorLoadTime).then(isExisting => assert(!isExisting))
})
Expand All @@ -86,7 +86,7 @@ describe('urlbar', function () {
yield this.app.client
.ipcSend('shortcut-focus-url', false)
.waitUntil(function () {
return this.getValue(urlInput).then(val => val === page1Url)
return this.getCssProperty(titleBar, 'display').then(display => display.value === 'none')
})
yield selectsText(this.app.client, page1Url)
})
Expand All @@ -102,10 +102,9 @@ describe('urlbar', function () {
})

it('does not have title mode', function *() {
let page_no_title = this.page_no_title
yield this.app.client
.waitUntil(function () {
return this.getValue(urlInput).then(val => val === page_no_title)
return this.getCssProperty(titleBar, 'display').then(display => display.value === 'none')
})
.waitForExist(navigatorLoadTime)
})
Expand Down

5 comments on commit 545c7a6

@bbondy
Copy link
Member

@bbondy bbondy commented on 545c7a6 Feb 8, 2016

Choose a reason for hiding this comment

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

++ thanks for doing these changes.

@bbondy
Copy link
Member

@bbondy bbondy commented on 545c7a6 Feb 9, 2016

Choose a reason for hiding this comment

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

I think with these changes the text at the top is slightly not centered. Please take a look and fix if not.

@diracdeltas
Copy link
Member Author

Choose a reason for hiding this comment

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

looks the same as before to me. do you have a test page?

screen shot 2016-02-08 at 5 50 37 pm

@bbondy
Copy link
Member

@bbondy bbondy commented on 545c7a6 Feb 9, 2016

Choose a reason for hiding this comment

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

oh ok I guess this is older.
I just measure the pixel count on the left and the right with Commadn+Shift+4
It is off by about 10 pixels I think because of the lock icon.

@bbondy
Copy link
Member

@bbondy bbondy commented on 545c7a6 Feb 9, 2016

Choose a reason for hiding this comment

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

I'll post a new issue since it isn't related to your changeset

Please sign in to comment.