Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show popup if notification or activeMMtab is true #6223

Merged
merged 3 commits into from
Mar 5, 2019
Merged

Conversation

tmashuang
Copy link
Contributor

@tmashuang tmashuang commented Feb 28, 2019

but not if popup(extension view) is open

Fixes #6221

@tmashuang tmashuang changed the title Show popup if notification or activeMMtab is true but not if popup is… Show popup if notification or activeMMtab is true Feb 28, 2019
@metamaskbot
Copy link
Collaborator

Builds ready [5a82212]: mascara, chrome, firefox, edge, opera

@danjm
Copy link
Contributor

danjm commented Feb 28, 2019

I think that the change that was made in #6183 was a mistake. (And my mistake for approving it)

I think the logic before that change reflects what we desire: new popup windows should not be opened if there is a metamask tab currently opened, if there is a notification window currently open or if the extension popup is open.

What we need to do is address the case when there is a notification window open but not in focus. It needs to be brought into focus.

I think it would be good if we revert the change made in #6183 and then find an alternate solution for bringing an out of focus notification window into focus.

@@ -448,7 +448,7 @@ function setupController (initState, initLangCode) {
function triggerUi () {
extension.tabs.query({ active: true }, tabs => {
const currentlyActiveMetamaskTab = Boolean(tabs.find(tab => openMetamaskTabsIDs[tab.id]))
if ((!popupIsOpen || !notificationIsOpen) && !currentlyActiveMetamaskTab) {
if ((!popupIsOpen || notificationIsOpen) && !currentlyActiveMetamaskTab) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will result in multiple notification windows being opened if multiple txs are created before any one is confirmed/rejected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not the case.

mar-04-2019 08-29-53

Copy link
Contributor

Choose a reason for hiding this comment

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

So maybe notifcationIsOpen can be removed entirely from the if expression?

@metamaskbot
Copy link
Collaborator

Builds ready [8e99a53]: mascara, chrome, firefox, edge, opera

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants