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

Only update tabPageIndex if user is not hovering other tab #9809

Merged
merged 2 commits into from
Jul 8, 2017
Merged

Only update tabPageIndex if user is not hovering other tab #9809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4dba751
Change per-frame hoverState to a single hoverTabIndex key
cezaraugusto Jul 7, 2017
51b876d
Only update tabPageIndex if user is not hovering other tab
cezaraugusto Jul 7, 2017
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
6 changes: 4 additions & 2 deletions app/common/state/tabContentState.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,14 +114,15 @@ const tabContentState = {
getTabIconColor: (state, frameKey) => {
const frame = frameStateUtil.getFrameByKey(state, frameKey)
const isActive = frameStateUtil.isFrameKeyActive(state, frameKey)
const hoverState = frameStateUtil.getTabHoverState(state, frameKey)

if (frame == null) {
return ''
}

const themeColor = frame.get('themeColor') || frame.get('computedThemeColor')
const activeNonPrivateTab = !frame.get('isPrivate') && isActive
const isPrivateTab = frame.get('isPrivate') && (isActive || frame.get('hoverState'))
const isPrivateTab = frame.get('isPrivate') && (isActive || hoverState)
const defaultColor = isPrivateTab ? styles.color.white100 : styles.color.black100
const isPaintTabs = getSetting(settings.PAINT_TABS)

Expand Down Expand Up @@ -159,7 +160,8 @@ const tabContentState = {
return false
}

return frame.get('hoverState') && hasBreakpoint(frame.get('breakpoint'), ['default', 'large'])
return frameStateUtil.getTabHoverState(state, frameKey) &&
hasBreakpoint(frame.get('breakpoint'), ['default', 'large'])
},

/**
Expand Down
6 changes: 1 addition & 5 deletions app/renderer/reducers/frameReducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ const closeFrame = (state, action) => {
}

const frameProps = frameStateUtil.getFrameByKey(state, action.frameKey)
const hoverState = state.getIn(['frames', index, 'hoverState'])
const hoverState = frameStateUtil.getTabHoverState(state, action.frameKey)

state = state.merge(frameStateUtil.removeFrame(
state,
Expand Down Expand Up @@ -122,14 +122,10 @@ const frameReducer = (state, action, immutableAction) => {
if (active != null) {
if (active) {
state = state.set('activeFrameKey', frame.get('key'))
if (frame.get('hoverState')) {
state = state.set('previewFrameKey', null)
}
if (frame.getIn(['ui', 'tabs', 'hoverTabPageIndex']) == null) {
state = state.deleteIn(['ui', 'tabs', 'previewTabPageIndex'])
}
state = state.setIn(['frames', index, 'lastAccessedTime'], new Date().getTime())

state = frameStateUtil.updateTabPageIndex(state, frame)
}
}
Expand Down
2 changes: 1 addition & 1 deletion docs/state.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,6 @@ WindowStore
history: array, // navigation history
hrefPreview: string, // show hovered link preview
httpsEverywhere: Object<string, Array<string>>, // map of XML rulesets name to redirected resources
hoverState: boolean, // wheter or not tab is being hovered
icon: string, // favicon url
isFullScreen: boolean, // true if the frame should be shown as full screen
isPrivate: boolean, // private browsing tab
Expand Down Expand Up @@ -665,6 +664,7 @@ WindowStore
},
size: array, // last known window size [x, y]
tabs: {
hoverTabIndex: number, // index of the current hovered tab
tabPageIndex: number, // index of the current tab page
previewTabPageIndex: number // index of the tab being previewed
},
Expand Down
65 changes: 59 additions & 6 deletions js/state/frameStateUtil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,15 +460,17 @@ function isPinned (state, frameKey) {
function updateTabPageIndex (state, frameProps) {
frameProps = makeImmutable(frameProps)
const index = getFrameTabPageIndex(state, frameProps.get('key'))
const isTabInHoverState = !!getHoverTabIndex(state)

if (index === -1) {
return state
}

state = state.setIn(['ui', 'tabs', 'tabPageIndex'], index)
state = state.deleteIn(['ui', 'tabs', 'previewTabPageIndex'])

return state
// Do not update tabPageIndex if user is in hover mode
Copy link
Member

Choose a reason for hiding this comment

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

Do you know why it even attempts to do this if in hover state mode?

Copy link
Member

Choose a reason for hiding this comment

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

The concern here is that updateTabPageIndex should just do what it says and not ignore for isTabInHoverState.

Instead we should just call updateTabPageIndex when the active state of a tab changes.
Because otherwise you could be on tab page 2 with an active tab on tab page 3. And as you're looking at tab page 2, audio could start playing on tab page 3 and it would change the active tab.

What you did mostly fixes it, but there are some edge cases that are missed.

I posted this issue for tracking the work, I'll merge this in the meantime:
#9911

You can do that task out of line with lower priority.

if (isTabInHoverState) {
return state
}
return state.setIn(['ui', 'tabs', 'tabPageIndex'], index)
}

const frameStatePath = (state, frameKey) => {
Expand Down Expand Up @@ -590,9 +592,10 @@ const setPreviewFrameKey = (state, frameKey, immediate = false) => {
const frame = getFrameByKey(state, frameKey)
const isActive = isFrameKeyActive(state, frameKey)
const previewTabs = getSetting(settings.SHOW_TAB_PREVIEWS)
const hoverState = getTabHoverState(state, frameKey)
let newPreviewFrameKey = frameKey

if (!previewTabs || frame == null || !frame.get('hoverState') || isActive) {
if (!previewTabs || frame == null || !hoverState || isActive) {
newPreviewFrameKey = null
}

Expand Down Expand Up @@ -638,10 +641,59 @@ const setTabPageHoverState = (state, tabPageIndex, hoverState) => {
return state
}

/**
* Checks if the current tab index is being hovered
* @param state {Object} - Application state
* @param frameKey {Number} - The current tab's frameKey
* @return Boolean - wheter or not hoverState is true
*/
const getTabHoverState = (state, frameKey) => {
const index = getFrameIndex(state, frameKey)
return getHoverTabIndex(state) === index
}

/**
* Gets the hovered tab index state
* This check will return null if no tab is being hovered
* and is used getTabHoverState to check if current index is being hovered.
* If the method to apply for does not know the right index
* this should be used instead of getTabHoverState
* @param state {Object} - Application state
* @return Immutable top level application state for hoverTabIndex
*/
const getHoverTabIndex = (state) => {
return state.getIn(['ui', 'tabs', 'hoverTabIndex'])
}

/**
* Sets the hover state for current tab index in top level state
* @param state {Object} - Application state
* @param frameKey {Number} - The current tab's frameKey
* @param hoverState {Boolean} - True if the current tab is being hovered.
* @return Immutable top level application state for hoverTabIndex
*/
const setHoverTabIndex = (state, frameKey, hoverState) => {
const frameIndex = getFrameIndex(state, frameKey)
if (!hoverState) {
state = state.setIn(['ui', 'tabs', 'hoverTabIndex'], null)
return state
}
return state.setIn(['ui', 'tabs', 'hoverTabIndex'], frameIndex)
}

/**
* Gets values from the window setTabHoverState action from the store
* and is used to apply both hoverState and previewFrameKey
* @param state {Object} - Application state
* @param frameKey {Number} - The current tab's frameKey
* @param hoverState {Boolean} - True if the current tab is being hovered.
* @return Immutable top level application state for hoverTabIndex
*/

const setTabHoverState = (state, frameKey, hoverState) => {
const frameIndex = getFrameIndex(state, frameKey)
if (frameIndex !== -1) {
state = state.setIn(['frames', frameIndex, 'hoverState'], hoverState)
state = setHoverTabIndex(state, frameKey, hoverState)
state = setPreviewFrameKey(state, frameKey)
}
return state
Expand All @@ -650,6 +702,7 @@ const setTabHoverState = (state, frameKey, hoverState) => {
module.exports = {
setTabPageHoverState,
setPreviewTabPageIndex,
getTabHoverState,
setTabHoverState,
setPreviewFrameKey,
getPreviewFrameKey,
Expand Down
40 changes: 23 additions & 17 deletions test/unit/app/common/state/tabContentStateTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const {braveExtensionId} = require('../../../../../js/constants/config')
const styles = require('../../../../../app/renderer/components/styles/global')

const frameKey = 1
const index = 0
const defaultWindowStore = Immutable.fromJS({
activeFrameKey: frameKey,
frames: [{
Expand All @@ -21,7 +22,8 @@ const defaultWindowStore = Immutable.fromJS({
location: 'http://brave.com'
}],
tabs: [{
key: frameKey
key: frameKey,
index: index
}],
framesInternal: {
index: {
Expand All @@ -30,6 +32,11 @@ const defaultWindowStore = Immutable.fromJS({
tabIndex: {
1: 0
}
},
ui: {
tabs: {
hoverTabIndex: index
}
}
})

Expand Down Expand Up @@ -340,6 +347,7 @@ describe('tabContentState unit tests', function () {
})

it('tab is private and active', function () {
const state = defaultWindowStore
getFrameByKeyMock = sinon.stub(frameStateUtil, 'getFrameByKey', () => {
return Immutable.fromJS({
isPrivate: true
Expand All @@ -348,10 +356,11 @@ describe('tabContentState unit tests', function () {
isFrameKeyActive = sinon.stub(frameStateUtil, 'isFrameKeyActive', () => {
return true
})
assert.equal(tabContentState.getTabIconColor(), styles.color.white100)
assert.equal(tabContentState.getTabIconColor(state), styles.color.white100)
})

it('tab is not private and active, but paint is disabled', function () {
const state = defaultWindowStore
getFrameByKeyMock = sinon.stub(frameStateUtil, 'getFrameByKey', () => {
return Immutable.fromJS({
isPrivate: false
Expand All @@ -361,10 +370,11 @@ describe('tabContentState unit tests', function () {
return true
})
defaultValue = false
assert.equal(tabContentState.getTabIconColor(), styles.color.black100)
assert.equal(tabContentState.getTabIconColor(state), styles.color.black100)
})

it('all valid', function () {
const state = defaultWindowStore
getFrameByKeyMock = sinon.stub(frameStateUtil, 'getFrameByKey', () => {
return Immutable.fromJS({
isPrivate: false,
Expand All @@ -374,7 +384,7 @@ describe('tabContentState unit tests', function () {
isFrameKeyActive = sinon.stub(frameStateUtil, 'isFrameKeyActive', () => {
return true
})
assert.equal(tabContentState.getTabIconColor(), 'white')
assert.equal(tabContentState.getTabIconColor(state), 'white')
})
})

Expand Down Expand Up @@ -427,33 +437,29 @@ describe('tabContentState unit tests', function () {
assert.equal(tabContentState.hasRelativeCloseIcon(state), false)
})

it('if not hovering', function () {
getFrameByKeyMock = sinon.stub(frameStateUtil, 'getFrameByKey', () => {
return Immutable.fromJS({
hoverState: false
})
})
assert.equal(tabContentState.hasRelativeCloseIcon(), false)
it('if not hovering (tabIndex !== hoverTabIndex)', function () {
const state = defaultWindowStore.setIn(['ui', 'tabs', 'hoverTabIndex'], null)
assert.equal(tabContentState.hasRelativeCloseIcon(state, frameKey), false)
})

it('if hovering and break point is small', function () {
it('if hovering (tabIndex === hoverTabIndex) and break point is small', function () {
const state = defaultWindowStore
getFrameByKeyMock = sinon.stub(frameStateUtil, 'getFrameByKey', () => {
return Immutable.fromJS({
hoverState: true,
breakpoint: 'small'
})
})
assert.equal(tabContentState.hasRelativeCloseIcon(), false)
assert.equal(tabContentState.hasRelativeCloseIcon(state, frameKey), false)
})

it('if hovering and break point is default', function () {
it('if hovering (tabIndex === hoverTabIndex) and break point is default', function () {
const state = defaultWindowStore
getFrameByKeyMock = sinon.stub(frameStateUtil, 'getFrameByKey', () => {
return Immutable.fromJS({
hoverState: true,
breakpoint: 'default'
})
})
assert.equal(tabContentState.hasRelativeCloseIcon(), true)
assert.equal(tabContentState.hasRelativeCloseIcon(state, frameKey), true)
})
})

Expand Down
9 changes: 8 additions & 1 deletion test/unit/app/renderer/components/tabs/content/closeTabIconTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const Immutable = require('immutable')
const fakeElectron = require('../../../../../lib/fakeElectron')
require('../../../../../braveUnit')

const index = 0
const tabId = 1
const frameKey = 1
const invalidFrameKey = 71
Expand Down Expand Up @@ -39,7 +40,8 @@ const defaultWindowStore = Immutable.fromJS({
location: 'http://brave.com'
}],
tabs: [{
key: frameKey
key: frameKey,
index: index
}],
framesInternal: {
index: {
Expand All @@ -48,6 +50,11 @@ const defaultWindowStore = Immutable.fromJS({
tabIndex: {
1: 0
}
},
ui: {
tabs: {
hoverTabIndex: index
}
}
})

Expand Down
Loading