From 10e08c0a06ca3fcd43f536d8f1162f52d277f7ac Mon Sep 17 00:00:00 2001 From: petemill Date: Mon, 7 May 2018 18:06:58 -0700 Subject: [PATCH] Set zoomPercent on appState for each tab Fix #14045 Previously this was set on window state, but was not used, possibly because it was not reliable as it did not get the initial value which is set internally in muon based on origin. Instead, when this was working, it was read directly from the webview, which we try to avoid due to the webview's multi-purpose nature. For example, the webview may not be displaying the active tab - it may be displaying a preview, or waiting to display the active tab asynchronously. Since there is not yet a muon Tab event for 'zoom-changed', we must manually update the zoomPercent tabState property every time we knowingly change the zoom level. --- app/browser/reducers/tabsReducer.js | 7 +++++++ app/browser/tabs.js | 3 ++- app/common/actions/tabActions.js | 7 +++++++ app/common/constants/tabAction.js | 3 ++- app/common/state/tabState.js | 14 ++++++++++++++ .../components/common/contextMenu/contextMenu.js | 9 ++++++--- .../common/contextMenu/contextMenuItem.js | 6 +----- .../components/frame/guestInstanceRenderer.js | 9 ++++++--- app/renderer/rendererShortcutHandler.js | 15 +++++++++------ docs/state.md | 4 ++-- js/actions/windowActions.js | 15 --------------- js/constants/windowConstants.js | 1 - js/stores/windowStore.js | 3 --- test/unit/app/browser/tabsTest.js | 1 + test/unit/app/common/lib/historyUtilTest.js | 1 - .../app/renderer/components/frame/frameTest.js | 3 +-- test/unit/lib/fakeTab.js | 5 +++++ 17 files changed, 63 insertions(+), 43 deletions(-) diff --git a/app/browser/reducers/tabsReducer.js b/app/browser/reducers/tabsReducer.js index 9fffa7698a3..baeba2c270c 100644 --- a/app/browser/reducers/tabsReducer.js +++ b/app/browser/reducers/tabsReducer.js @@ -143,6 +143,13 @@ const tabsReducer = (state, action, immutableAction) => { }) break } + case tabActionConsts.ZOOM_CHANGED: + { + const tabId = tabState.resolveTabId(state, action.get('tabId')) + const zoomPercent = action.get('zoomPercent') + state = tabState.setZoomPercent(state, tabId, zoomPercent) + break + } case appConstants.APP_SET_STATE: state = tabs.init(state, action) break diff --git a/app/browser/tabs.js b/app/browser/tabs.js index 8b553927f79..1b673bc9af4 100644 --- a/app/browser/tabs.js +++ b/app/browser/tabs.js @@ -66,6 +66,7 @@ const getTabValue = function (tabId) { tabValue = tabValue.set('partition', tab.session.partition) tabValue = tabValue.set('partitionNumber', getPartitionNumber(tab.session.partition)) tabValue = tabValue.set('isPlaceholder', tab.isPlaceholder()) + tabValue = tabValue.set('zoomPercent', tab.getZoomPercent()) return tabValue.set('tabId', tabId) } } @@ -1420,7 +1421,7 @@ const api = { // communicate those out here. if (newTabValue && oldTabValue) { const changeInfo = {} - const rendererAwareProps = ['index', 'pinned', 'url', 'active'] + const rendererAwareProps = ['index', 'pinned', 'url', 'active', 'zoomPercent'] rendererAwareProps.forEach((prop) => { const newPropVal = newTabValue.get(prop) if (oldTabValue.get(prop) !== newPropVal) { diff --git a/app/common/actions/tabActions.js b/app/common/actions/tabActions.js index 6a5dcf5ab30..056beb61766 100644 --- a/app/common/actions/tabActions.js +++ b/app/common/actions/tabActions.js @@ -51,6 +51,13 @@ const tabActions = { dispatchAction(tabActionConstants.STOP_FIND_IN_PAGE_REQUEST, { tabId }) + }, + + zoomChanged (tabId, zoomPercent) { + dispatchAction(tabActionConstants.ZOOM_CHANGED, { + tabId, + zoomPercent + }) } } diff --git a/app/common/constants/tabAction.js b/app/common/constants/tabAction.js index 6465b224749..9731b9966f8 100644 --- a/app/common/constants/tabAction.js +++ b/app/common/constants/tabAction.js @@ -7,5 +7,6 @@ FINISH_NAVIGATION: 'tabActionsDidFinishNavigation', START_NAVIGATION: 'tabActionsDidStartNavigation', FIND_IN_PAGE_REQUEST: 'tabActionsFindInPageRequest', - STOP_FIND_IN_PAGE_REQUEST: 'tabActionsStopFindInPageRequest' + STOP_FIND_IN_PAGE_REQUEST: 'tabActionsStopFindInPageRequest', + ZOOM_CHANGED: 'tabActionsZoomChanged' } diff --git a/app/common/state/tabState.js b/app/common/state/tabState.js index 7a4579d935d..2983826a89b 100644 --- a/app/common/state/tabState.js +++ b/app/common/state/tabState.js @@ -680,6 +680,20 @@ const tabState = { } // handle set window return state.setIn(path, windowId) + }, + + setZoomPercent: (state, tabId, zoomPercent) => { + let path = tabState.getPathByTabId(state, tabId) + if (!path) { + console.error(`setZoomPercent: tab with ID ${tabId} not found!`) + return state + } + if (typeof zoomPercent !== 'number') { + console.error(`setZoomPercent: bad value for zoomPercent: ${zoomPercent}`) + return state + } + path = [...path, 'zoomPercent'] + return state.setIn(path, zoomPercent) } } diff --git a/app/renderer/components/common/contextMenu/contextMenu.js b/app/renderer/components/common/contextMenu/contextMenu.js index 7fee3668926..b5d024db3b0 100644 --- a/app/renderer/components/common/contextMenu/contextMenu.js +++ b/app/renderer/components/common/contextMenu/contextMenu.js @@ -17,9 +17,11 @@ const keyCodes = require('../../../../common/constants/keyCodes') // State const contextMenuState = require('../../../../common/state/contextMenuState') +const tabState = require('../../../../common/state/tabState') +const appStore = require('../../../../../js/stores/appStoreRenderer') +const { getCurrentWindowId } = require('../../../currentWindow') // Utils -const frameStateUtil = require('../../../../../js/state/frameStateUtil') const {separatorMenuItem} = require('../../../../common/commonMenu') const {wrappingClamp} = require('../../../../common/lib/formatUtil') @@ -219,12 +221,13 @@ class ContextMenu extends React.Component { mergeProps (state, ownProps) { const currentWindow = state.get('currentWindow') - const activeFrame = frameStateUtil.getActiveFrame(currentWindow) || Immutable.Map() + const selectedIndex = currentWindow.getIn(['ui', 'contextMenu', 'selectedIndex'], null) const contextMenuDetail = contextMenuState.getContextMenu(currentWindow) const props = {} - props.lastZoomPercentage = activeFrame.get('lastZoomPercentage') + const activeTab = tabState.getActiveTab(appStore.state, getCurrentWindowId()) + props.lastZoomPercentage = activeTab && activeTab.get('zoomPercent') props.contextMenuDetail = contextMenuDetail // TODO (nejc) only primitives props.selectedIndex = typeof selectedIndex === 'object' && Array.isArray(selectedIndex) && diff --git a/app/renderer/components/common/contextMenu/contextMenuItem.js b/app/renderer/components/common/contextMenu/contextMenuItem.js index ed6eb3f1e52..cd7ba220c69 100644 --- a/app/renderer/components/common/contextMenu/contextMenuItem.js +++ b/app/renderer/components/common/contextMenu/contextMenuItem.js @@ -167,11 +167,7 @@ class ContextMenuItem extends ImmutableComponent { return label } if (item.get('labelDataBind') === 'zoomLevel') { - const activeWebview = document.querySelector('.frameWrapper.isActive webview') - let percent = 100 - if (activeWebview) { - percent = activeWebview.getZoomPercent() - } + const percent = this.props.lastZoomPercentage || 100 return `${percent}%` } return '' diff --git a/app/renderer/components/frame/guestInstanceRenderer.js b/app/renderer/components/frame/guestInstanceRenderer.js index f5b4a6ec421..f937743c9b2 100644 --- a/app/renderer/components/frame/guestInstanceRenderer.js +++ b/app/renderer/components/frame/guestInstanceRenderer.js @@ -15,6 +15,7 @@ const WebviewDisplay = require('../../pooledWebviewDisplay') // Actions const windowActions = require('../../../../js/actions/windowActions') const webviewActions = require('../../../../js/actions/webviewActions') +const tabActions = require('../../../common/actions/tabActions') // state const frameStateUtil = require('../../../../js/state/frameStateUtil') @@ -105,8 +106,8 @@ class GuestInstanceRenderer extends React.Component { classNameWebview: css(styles.guestInstanceRenderer__webview), classNameWebviewAttached: css(styles.guestInstanceRenderer__webview_attached), classNameWebviewAttaching: css(styles.guestInstanceRenderer__webview_attaching), - onFocus: this.onFocus.bind(this), - onZoomChange: this.onUpdateZoom.bind(this) + onZoomChange: this.onUpdateZoom.bind(this), + onFocus: this.onFocus.bind(this) }) webviewActions.init(this.webviewDisplay) // treat the container as main frame position for mouse position @@ -127,7 +128,9 @@ class GuestInstanceRenderer extends React.Component { } onUpdateZoom (zoomPercent) { - windowActions.setLastZoomPercentage(this.props.frameKey, zoomPercent) + // TODO: better to respond to a muon Tab event `zoom-changed` via ZoomObserver + // if that is provided in the future + tabActions.zoomChanged(this.props.tabId, zoomPercent) } render () { diff --git a/app/renderer/rendererShortcutHandler.js b/app/renderer/rendererShortcutHandler.js index 11335bf7a77..da374918208 100644 --- a/app/renderer/rendererShortcutHandler.js +++ b/app/renderer/rendererShortcutHandler.js @@ -76,9 +76,10 @@ function handleShortcut (frameKey, shortcut, e, args) { const tabId = frameStateUtil.getTabIdByFrameKey(windowStore.state, frameKey) getWebContents(tabId, (webContents) => { if (webContents) { - const frame = frameStateUtil.getFrameByKey(windowStore.state, frameKey) webContents.zoomIn() - windowActions.setLastZoomPercentage(frame, webContents.getZoomPercent()) + // TODO: better to respond to a muon Tab event `zoom-changed` via ZoomObserver + // if that is provided in the future + tabActions.zoomChanged(tabId, webContents.getZoomPercent()) } }) break @@ -87,9 +88,10 @@ function handleShortcut (frameKey, shortcut, e, args) { const tabId = frameStateUtil.getTabIdByFrameKey(windowStore.state, frameKey) getWebContents(tabId, (webContents) => { if (webContents) { - const frame = frameStateUtil.getFrameByKey(windowStore.state, frameKey) webContents.zoomOut() - windowActions.setLastZoomPercentage(frame, webContents.getZoomPercent()) + // TODO: better to respond to a muon Tab event `zoom-changed` via ZoomObserver + // if that is provided in the future + tabActions.zoomChanged(tabId, webContents.getZoomPercent()) } }) break @@ -98,9 +100,10 @@ function handleShortcut (frameKey, shortcut, e, args) { const tabId = frameStateUtil.getTabIdByFrameKey(windowStore.state, frameKey) getWebContents(tabId, (webContents) => { if (webContents) { - const frame = frameStateUtil.getFrameByKey(windowStore.state, frameKey) webContents.zoomReset() - windowActions.setLastZoomPercentage(frame, webContents.getZoomPercent()) + // TODO: better to respond to a muon Tab event `zoom-changed` via ZoomObserver + // if that is provided in the future + tabActions.zoomChanged(tabId, webContents.getZoomPercent()) } }) break diff --git a/docs/state.md b/docs/state.md index a3bd701908d..d3795e7ea4e 100644 --- a/docs/state.md +++ b/docs/state.md @@ -589,7 +589,8 @@ AppStore suppress: boolean, // if true, show a suppress checkbox (defaulted to not checked) title: string, // title is the source; ex: "brave.com says:" }, - muted: boolean, // is the tab muted + muted: boolean, // is the tab muted, + zoomPercent: number, // current zoom levellast windowId: number // the windowId that contains the tab guestInstanceId: number, tabId: number @@ -739,7 +740,6 @@ WindowStore isPrivate: boolean, // private browsing tab key: number, lastAccessedTime: datetime, - lastZoomPercentage: number, // last value that was used for zooming loading: boolean, location: string, // the currently navigated location modalPromptDetail: object, diff --git a/js/actions/windowActions.js b/js/actions/windowActions.js index a7be8fe72ef..218afbef0be 100644 --- a/js/actions/windowActions.js +++ b/js/actions/windowActions.js @@ -579,21 +579,6 @@ const windowActions = { }) }, - /** - * Dispatches a message to store the last zoom percentage. - * This is mainly just used to trigger updates throughout React. - * - * @param {object} frameKey - The frame to set blocked info on - * @param {number} percentage - The new zoom percentage - */ - setLastZoomPercentage: function (frameKey, percentage) { - dispatch({ - actionType: windowConstants.WINDOW_SET_LAST_ZOOM_PERCENTAGE, - frameKey, - percentage - }) - }, - /** * Saves the position of the window in the window state * @param {Array} position - [x, y] diff --git a/js/constants/windowConstants.js b/js/constants/windowConstants.js index 82bdfd02fec..fa4e987d3c2 100644 --- a/js/constants/windowConstants.js +++ b/js/constants/windowConstants.js @@ -59,7 +59,6 @@ const windowConstants = { WINDOW_SET_REDIRECTED_BY: _, // Whether or not to show site info like redirected resources WINDOW_SET_SECURITY_STATE: _, WINDOW_SET_STATE: _, - WINDOW_SET_LAST_ZOOM_PERCENTAGE: _, WINDOW_SET_CLEAR_BROWSING_DATA_VISIBLE: _, WINDOW_SET_IMPORT_BROWSER_DATA_DETAIL: _, WINDOW_SET_IMPORT_BROWSER_DATA_SELECTED: _, diff --git a/js/stores/windowStore.js b/js/stores/windowStore.js index 03cfa639fef..1a5281c53e2 100644 --- a/js/stores/windowStore.js +++ b/js/stores/windowStore.js @@ -522,9 +522,6 @@ const doAction = (action) => { case windowConstants.WINDOW_SET_FAVICON: windowState = windowState.setIn(['frames', frameStateUtil.getFrameIndex(windowState, action.frameProps.get('key')), 'icon'], action.favicon) break - case windowConstants.WINDOW_SET_LAST_ZOOM_PERCENTAGE: - windowState = windowState.setIn(['frames', frameStateUtil.getFrameIndex(windowState, action.frameKey), 'lastZoomPercentage'], action.percentage) - break case windowConstants.WINDOW_SET_MOUSE_IN_TITLEBAR: windowState = windowState.setIn(['ui', 'mouseInTitlebar'], action.mouseInTitlebar) break diff --git a/test/unit/app/browser/tabsTest.js b/test/unit/app/browser/tabsTest.js index cfdb67224fa..4077193a568 100644 --- a/test/unit/app/browser/tabsTest.js +++ b/test/unit/app/browser/tabsTest.js @@ -81,6 +81,7 @@ describe('tabs API unit tests', function () { once: (event, cb) => { setImmediate(cb) }, + getZoomPercent: () => 100, isPlaceholder: () => false } if (tabId === 1) { diff --git a/test/unit/app/common/lib/historyUtilTest.js b/test/unit/app/common/lib/historyUtilTest.js index 6e550a6a39e..e1b5855187e 100644 --- a/test/unit/app/common/lib/historyUtilTest.js +++ b/test/unit/app/common/lib/historyUtilTest.js @@ -314,7 +314,6 @@ describe('historyUtil unit tests', function () { isFullScreen: false, isPrivate: false, key: 1, - lastZoomPercentage: 1, loading: true, location: 'https://brave.com', title: 'Brave' diff --git a/test/unit/app/renderer/components/frame/frameTest.js b/test/unit/app/renderer/components/frame/frameTest.js index ee7fe9e7477..24daa090667 100644 --- a/test/unit/app/renderer/components/frame/frameTest.js +++ b/test/unit/app/renderer/components/frame/frameTest.js @@ -47,8 +47,7 @@ describe.skip('Frame component unit tests', function () { const fakeWindowActions = { frameShortcutChanged: () => {}, setFindbarShown: () => {}, - setActiveFrame: () => {}, - setLastZoomPercentage: () => {} + setActiveFrame: () => {} } const domUtil = { diff --git a/test/unit/lib/fakeTab.js b/test/unit/lib/fakeTab.js index f4489c80426..0d0acea0ed1 100644 --- a/test/unit/lib/fakeTab.js +++ b/test/unit/lib/fakeTab.js @@ -18,6 +18,7 @@ function FakeTab (id, windowId, guestInstanceId = nextGuestInstanceId++) { this._canGoBack = false this._canGoForward = false this._isPlaceholder = false + this._zoomPercent = 100 } util.inherits(FakeTab, EventEmitter) @@ -51,4 +52,8 @@ proto.isPlaceholder = function () { return this._isPlaceholder } +proto.getZoomPercent = function () { + return this._zoomPercent +} + module.exports = FakeTab