Merge pull request #3995 from MetaMask/i3759-popup

Fix confirmation popup not always opening

12 files changed, 104 insertions, 68 deletions

diff --git a/app/scripts/background.js b/app/scripts/background.js
index 35c484ec5..6550e8944 100644
--- a/app/scripts/background.js
+++ b/app/scripts/background.js
@@ -21,6 +21,11 @@ const setupMetamaskMeshMetrics = require('./lib/setupMetamaskMeshMetrics')
 const EdgeEncryptor = require('./edge-encryptor')
 const getFirstPreferredLangCode = require('./lib/get-first-preferred-lang-code')
 const getObjStructure = require('./lib/getObjStructure')
+const {
+  ENVIRONMENT_TYPE_POPUP,
+  ENVIRONMENT_TYPE_NOTIFICATION,
+  ENVIRONMENT_TYPE_FULLSCREEN,
+} = require('./lib/enums')
 
 const STORAGE_KEY = 'metamask-config'
 const METAMASK_DEBUG = process.env.METAMASK_DEBUG
@@ -43,7 +48,7 @@ const isEdge = !isIE && !!window.StyleMedia
 
 let popupIsOpen = false
 let notificationIsOpen = false
-let openMetamaskTabsIDs = {}
+const openMetamaskTabsIDs = {}
 
 // state persistence
 const diskStore = new LocalStorageStore({ storageKey: STORAGE_KEY })
@@ -172,7 +177,7 @@ function setupController (initState, initLangCode) {
     return versionedData
   }
 
-  function persistData(state) {
+  function persistData (state) {
     if (!state) {
       throw new Error('MetaMask - updated state is missing', state)
     }
@@ -191,33 +196,53 @@ function setupController (initState, initLangCode) {
   //
   // connect to other contexts
   //
-
   extension.runtime.onConnect.addListener(connectRemote)
+
+  const metamaskInternalProcessHash = {
+    [ENVIRONMENT_TYPE_POPUP]: true,
+    [ENVIRONMENT_TYPE_NOTIFICATION]: true,
+    [ENVIRONMENT_TYPE_FULLSCREEN]: true,
+  }
+
+  const isClientOpenStatus = () => {
+    return popupIsOpen || Boolean(Object.keys(openMetamaskTabsIDs).length) || notificationIsOpen
+  }
+
   function connectRemote (remotePort) {
-    const isMetaMaskInternalProcess = remotePort.name === 'popup' || remotePort.name === 'notification'
+    const processName = remotePort.name
+    const isMetaMaskInternalProcess = metamaskInternalProcessHash[processName]
     const portStream = new PortStream(remotePort)
+
     if (isMetaMaskInternalProcess) {
       // communication with popup
-      popupIsOpen = popupIsOpen || (remotePort.name === 'popup')
       controller.isClientOpen = true
       controller.setupTrustedCommunication(portStream, 'MetaMask')
-      // record popup as closed
-      if (remotePort.sender.url.match(/home.html$/)) {
-        openMetamaskTabsIDs[remotePort.sender.tab.id] = true
-      }
-      if (remotePort.name === 'popup') {
+
+      if (processName === ENVIRONMENT_TYPE_POPUP) {
+        popupIsOpen = true
+
         endOfStream(portStream, () => {
           popupIsOpen = false
-          if (remotePort.sender.url.match(/home.html$/)) {
-            openMetamaskTabsIDs[remotePort.sender.tab.id] = false
-          }
-          controller.isClientOpen = popupIsOpen ||
-            Object.keys(openMetamaskTabsIDs).some(key => openMetamaskTabsIDs[key])
+          controller.isClientOpen = isClientOpenStatus()
         })
       }
-      if (remotePort.name === 'notification') {
+
+      if (processName === ENVIRONMENT_TYPE_NOTIFICATION) {
+        notificationIsOpen = true
+
         endOfStream(portStream, () => {
           notificationIsOpen = false
+          controller.isClientOpen = isClientOpenStatus()
+        })
+      }
+
+      if (processName === ENVIRONMENT_TYPE_FULLSCREEN) {
+        const tabId = remotePort.sender.tab.id
+        openMetamaskTabsIDs[tabId] = true
+
+        endOfStream(portStream, () => {
+          delete openMetamaskTabsIDs[tabId]
+          controller.isClientOpen = isClientOpenStatus()
         })
       }
     } else {
@@ -260,10 +285,11 @@ function setupController (initState, initLangCode) {
 
 // popup trigger
 function triggerUi () {
-  extension.tabs.query({ active: true }, (tabs) => {
-    const currentlyActiveMetamaskTab = tabs.find(tab => openMetamaskTabsIDs[tab.id])
-    if (!popupIsOpen && !currentlyActiveMetamaskTab && !notificationIsOpen) notificationManager.showPopup()
-    notificationIsOpen = true
+  extension.tabs.query({ active: true }, tabs => {
+    const currentlyActiveMetamaskTab = Boolean(tabs.find(tab => openMetamaskTabsIDs[tab.id]))
+    if (!popupIsOpen && !currentlyActiveMetamaskTab && !notificationIsOpen) {
+      notificationManager.showPopup()
+    }
   })
 }
 
diff --git a/app/scripts/lib/enums.js b/app/scripts/lib/enums.js
new file mode 100644
index 000000000..0a3afca47
--- /dev/null
+++ b/app/scripts/lib/enums.js
@@ -0,0 +1,9 @@
+const ENVIRONMENT_TYPE_POPUP = 'popup'
+const ENVIRONMENT_TYPE_NOTIFICATION = 'notification'
+const ENVIRONMENT_TYPE_FULLSCREEN = 'fullscreen'
+
+module.exports = {
+  ENVIRONMENT_TYPE_POPUP,
+  ENVIRONMENT_TYPE_NOTIFICATION,
+  ENVIRONMENT_TYPE_FULLSCREEN,
+}
diff --git a/app/scripts/lib/environment-type.js b/app/scripts/lib/environment-type.js
deleted file mode 100644
index 7966926eb..000000000
--- a/app/scripts/lib/environment-type.js
+++ /dev/null
@@ -1,10 +0,0 @@
-module.exports = function environmentType () {
-  const url = window.location.href
-  if (url.match(/popup.html$/)) {
-    return 'popup'
-  } else if (url.match(/home.html$/)) {
-    return 'responsive'
-  } else {
-    return 'notification'
-  }
-}
diff --git a/app/scripts/lib/is-popup-or-notification.js b/app/scripts/lib/is-popup-or-notification.js
deleted file mode 100644
index ad3e825c0..000000000
--- a/app/scripts/lib/is-popup-or-notification.js
+++ /dev/null
@@ -1,12 +0,0 @@
-module.exports = function isPopupOrNotification () {
-  const url = window.location.href
-  // if (url.match(/popup.html$/) || url.match(/home.html$/)) {
-  // Below regexes needed for feature toggles (e.g. see line ~340 in ui/app/app.js)
-  // Revert below regexes to above commented out regexes before merge to master
-  if (url.match(/popup.html(?:\?.+)*$/) ||
-    url.match(/home.html(?:\?.+)*$/) || url.match(/home.html(?:#.*)*$/)) {
-    return 'popup'
-  } else {
-    return 'notification'
-  }
-}
diff --git a/app/scripts/lib/util.js b/app/scripts/lib/util.js
index 6dee9edf0..df815906f 100644
--- a/app/scripts/lib/util.js
+++ b/app/scripts/lib/util.js
@@ -1,20 +1,27 @@
 const ethUtil = require('ethereumjs-util')
 const assert = require('assert')
 const BN = require('bn.js')
-
-module.exports = {
-  getStack,
-  sufficientBalance,
-  hexToBn,
-  bnToHex,
-  BnMultiplyByFraction,
-}
+const {
+  ENVIRONMENT_TYPE_POPUP,
+  ENVIRONMENT_TYPE_NOTIFICATION,
+  ENVIRONMENT_TYPE_FULLSCREEN,
+} = require('./enums')
 
 function getStack () {
   const stack = new Error('Stack trace generator - not an error').stack
   return stack
 }
 
+const getEnvironmentType = (url = window.location.href) => {
+  if (url.match(/popup.html(?:\?.+)*$/)) {
+    return ENVIRONMENT_TYPE_POPUP
+  } else if (url.match(/home.html(?:\?.+)*$/) || url.match(/home.html(?:#.*)*$/)) {
+    return ENVIRONMENT_TYPE_FULLSCREEN
+  } else {
+    return ENVIRONMENT_TYPE_NOTIFICATION
+  }
+}
+
 function sufficientBalance (txParams, hexBalance) {
   // validate hexBalance is a hex string
   assert.equal(typeof hexBalance, 'string', 'sufficientBalance - hexBalance is not a hex string')
@@ -42,3 +49,12 @@ function BnMultiplyByFraction (targetBN, numerator, denominator) {
   const denomBN = new BN(denominator)
   return targetBN.mul(numBN).div(denomBN)
 }
+
+module.exports = {
+  getStack,
+  getEnvironmentType,
+  sufficientBalance,
+  hexToBn,
+  bnToHex,
+  BnMultiplyByFraction,
+}
diff --git a/app/scripts/ui.js b/app/scripts/ui.js
index c326ca1c3..bdab29c1e 100644
--- a/app/scripts/ui.js
+++ b/app/scripts/ui.js
@@ -3,7 +3,8 @@ const OldMetaMaskUiCss = require('../../old-ui/css')
 const NewMetaMaskUiCss = require('../../ui/css')
 const startPopup = require('./popup-core')
 const PortStream = require('./lib/port-stream.js')
-const isPopupOrNotification = require('./lib/is-popup-or-notification')
+const { getEnvironmentType } = require('./lib/util')
+const { ENVIRONMENT_TYPE_NOTIFICATION } = require('./lib/enums')
 const extension = require('extensionizer')
 const ExtensionPlatform = require('./platforms/extension')
 const NotificationManager = require('./lib/notification-manager')
@@ -27,7 +28,7 @@ async function start() {
   // injectCss(css)
 
   // identify window type (popup, notification)
-  const windowType = isPopupOrNotification()
+  const windowType = getEnvironmentType(window.location.href)
   global.METAMASK_UI_TYPE = windowType
   closePopupIfOpen(windowType)
 
@@ -69,7 +70,7 @@ async function start() {
 
 
   function closePopupIfOpen (windowType) {
-    if (windowType !== 'notification') {
+    if (windowType !== ENVIRONMENT_TYPE_NOTIFICATION) {
       // should close only chrome popup
       notificationManager.closePopup()
     }
diff --git a/old-ui/app/conf-tx.js b/old-ui/app/conf-tx.js
index 67eb31c37..dc1dc0538 100644
--- a/old-ui/app/conf-tx.js
+++ b/old-ui/app/conf-tx.js
@@ -6,8 +6,9 @@ const actions = require('../../ui/app/actions')
 const NetworkIndicator = require('./components/network')
 const LoadingIndicator = require('./components/loading')
 const txHelper = require('../lib/tx-helper')
-const isPopupOrNotification = require('../../app/scripts/lib/is-popup-or-notification')
 const log = require('loglevel')
+const { ENVIRONMENT_TYPE_NOTIFICATION } = require('../../app/scripts/lib/enums')
+const { getEnvironmentType } = require('../../app/scripts/lib/util')
 
 const PendingTx = require('./components/pending-tx')
 const PendingMsg = require('./components/pending-msg')
@@ -51,7 +52,7 @@ ConfirmTxScreen.prototype.render = function () {
 
   var txData = unconfTxList[props.index] || {}
   var txParams = txData.params || {}
-  var isNotification = isPopupOrNotification() === 'notification'
+  var isNotification = getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_NOTIFICATION
 
   log.info(`rendering a combined ${unconfTxList.length} unconf msg & txs`)
   if (unconfTxList.length === 0) return h(Loading, { isLoading: true })
diff --git a/ui/app/components/modals/modal.js b/ui/app/components/modals/modal.js
index 9250cc77e..43dcd20ae 100644
--- a/ui/app/components/modals/modal.js
+++ b/ui/app/components/modals/modal.js
@@ -5,7 +5,8 @@ const connect = require('react-redux').connect
 const FadeModal = require('boron').FadeModal
 const actions = require('../../actions')
 const isMobileView = require('../../../lib/is-mobile-view')
-const isPopupOrNotification = require('../../../../app/scripts/lib/is-popup-or-notification')
+const { getEnvironmentType } = require('../../../../app/scripts/lib/util')
+const { ENVIRONMENT_TYPE_POPUP } = require('../../../../app/scripts/lib/enums')
 
 // Modal Components
 const BuyOptions = require('./buy-options-modal')
@@ -162,7 +163,7 @@ const MODALS = {
     ],
     mobileModalStyle: {
       width: '95%',
-      top: isPopupOrNotification() === 'popup' ? '52vh' : '36.5vh',
+      top: getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_POPUP ? '52vh' : '36.5vh',
     },
     laptopModalStyle: {
       width: '449px',
@@ -179,7 +180,7 @@ const MODALS = {
     ],
     mobileModalStyle: {
       width: '95%',
-      top: isPopupOrNotification() === 'popup' ? '52vh' : '36.5vh',
+      top: getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_POPUP ? '52vh' : '36.5vh',
     },
     laptopModalStyle: {
       width: '449px',
@@ -196,7 +197,7 @@ const MODALS = {
     ],
     mobileModalStyle: {
       width: '95%',
-      top: isPopupOrNotification() === 'popup' ? '52vh' : '36.5vh',
+      top: getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_POPUP ? '52vh' : '36.5vh',
     },
     laptopModalStyle: {
       width: '449px',
@@ -208,7 +209,7 @@ const MODALS = {
     contents: h(ConfirmResetAccount),
     mobileModalStyle: {
       width: '95%',
-      top: isPopupOrNotification() === 'popup' ? '52vh' : '36.5vh',
+      top: getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_POPUP ? '52vh' : '36.5vh',
     },
     laptopModalStyle: {
       width: '473px',
diff --git a/ui/app/components/pages/unlock.js b/ui/app/components/pages/unlock.js
index b3320da21..567b72518 100644
--- a/ui/app/components/pages/unlock.js
+++ b/ui/app/components/pages/unlock.js
@@ -11,7 +11,8 @@ const {
   setNetworkEndpoints,
   setFeatureFlag,
 } = require('../../actions')
-const environmentType = require('../../../../app/scripts/lib/environment-type')
+const { ENVIRONMENT_TYPE_POPUP } = require('../../../../app/scripts/lib/enums')
+const { getEnvironmentType } = require('../../../../app/scripts/lib/util')
 const getCaretCoordinates = require('textarea-caret')
 const EventEmitter = require('events').EventEmitter
 const Mascot = require('../mascot')
@@ -131,7 +132,7 @@ class UnlockScreen extends Component {
             this.props.markPasswordForgotten()
             this.props.history.push(RESTORE_VAULT_ROUTE)
 
-            if (environmentType() === 'popup') {
+            if (getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_POPUP) {
               global.platform.openExtensionInBrowser()
             }
           },
diff --git a/ui/app/first-time/init-menu.js b/ui/app/first-time/init-menu.js
index 692b01c8c..3df040922 100644
--- a/ui/app/first-time/init-menu.js
+++ b/ui/app/first-time/init-menu.js
@@ -8,7 +8,8 @@ const actions = require('../actions')
 const Tooltip = require('../components/tooltip')
 const getCaretCoordinates = require('textarea-caret')
 const { RESTORE_VAULT_ROUTE, DEFAULT_ROUTE } = require('../routes')
-const environmentType = require('../../../app/scripts/lib/environment-type')
+const { getEnvironmentType } = require('../../../app/scripts/lib/util')
+const { ENVIRONMENT_TYPE_POPUP } = require('../../../app/scripts/lib/enums')
 const { OLD_UI_NETWORK_TYPE } = require('../../../app/scripts/config').enums
 
 class InitializeMenuScreen extends Component {
@@ -180,7 +181,7 @@ class InitializeMenuScreen extends Component {
 
   showRestoreVault () {
     this.props.markPasswordForgotten()
-    if (environmentType() === 'popup') {
+    if (getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_POPUP) {
       global.platform.openExtensionInBrowser()
     }
 
diff --git a/ui/app/reducers/metamask.js b/ui/app/reducers/metamask.js
index e65dc2d11..5f965fbe0 100644
--- a/ui/app/reducers/metamask.js
+++ b/ui/app/reducers/metamask.js
@@ -1,7 +1,8 @@
 const extend = require('xtend')
 const actions = require('../actions')
 const MetamascaraPlatform = require('../../../app/scripts/platforms/window')
-const environmentType = require('../../../app/scripts/lib/environment-type')
+const { getEnvironmentType } = require('../../../app/scripts/lib/util')
+const { ENVIRONMENT_TYPE_POPUP } = require('../../../app/scripts/lib/enums')
 const { OLD_UI_NETWORK_TYPE } = require('../../../app/scripts/config').enums
 
 module.exports = reduceMetamask
@@ -15,7 +16,7 @@ function reduceMetamask (state, action) {
     isUnlocked: false,
     isAccountMenuOpen: false,
     isMascara: window.platform instanceof MetamascaraPlatform,
-    isPopup: environmentType() === 'popup',
+    isPopup: getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_POPUP,
     rpcTarget: 'https://rawtestrpc.metamask.io/',
     identities: {},
     unapprovedTxs: {},
diff --git a/ui/app/unlock.js b/ui/app/unlock.js
index 84d8b7e7c..1325656af 100644
--- a/ui/app/unlock.js
+++ b/ui/app/unlock.js
@@ -7,7 +7,8 @@ const actions = require('./actions')
 const getCaretCoordinates = require('textarea-caret')
 const EventEmitter = require('events').EventEmitter
 const { OLD_UI_NETWORK_TYPE } = require('../../app/scripts/config').enums
-const environmentType = require('../../app/scripts/lib/environment-type')
+const { getEnvironmentType } = require('../../app/scripts/lib/util')
+const { ENVIRONMENT_TYPE_POPUP } = require('../../app/scripts/lib/enums')
 
 const Mascot = require('./components/mascot')
 
@@ -77,7 +78,7 @@ UnlockScreen.prototype.render = function () {
       h('p.pointer', {
         onClick: () => {
           this.props.dispatch(actions.markPasswordForgotten())
-          if (environmentType() === 'popup') {
+          if (getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_POPUP) {
             global.platform.openExtensionInBrowser()
           }
         },