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

Popup may sometimes be not activating #7051

Closed
danfinlay opened this issue Aug 21, 2019 · 3 comments · Fixed by #8314
Closed

Popup may sometimes be not activating #7051

danfinlay opened this issue Aug 21, 2019 · 3 comments · Fixed by #8314
Assignees
Labels

Comments

@danfinlay
Copy link
Contributor

In a user research session, clicking Connect in a dapp seemed to not trigger the popup.

Are there cases where the popup is not being activated? I know, this is hard to investigate without a concrete repro, but opening the issue because the OpenSea dev said they have heard of this reported by users.

It could have just been that a subsequent click hid the popup, making this part of #7022.

@jennypollack
Copy link
Contributor

possibly related: just got this failure in an e2e firefox test on circleci from an unrelated code change

  1. MetaMask
    Send ETH from dapp using advanced gas controls
    initiates a send from the dapp:
    Error: No window with title: MetaMask Notification
    at switchToWindowWithTitle (test/e2e/helpers.js:110:11)
    at switchToWindowWithTitle (test/e2e/helpers.js:119:18)
    at process._tickCallback (internal/process/next_tick.js:68:7)

@bdresser
Copy link
Contributor

From #7670

This is not a bug, I just think the way MM behaves is "odd"
If the MM pops up because a website has initiated a transaction, (ofc due ultimately to the user's interaction with the website), this is good. If a subsequent transaction is initiated, the popup appears once again, this is also good and what I would expect.

However:

If the MM popup is rejected, subsequent transactions initiated by the website do NOT launch the popup, instead the popup is hidden, all that shows is a notification on the MM icon in the bar at the top (chrome). I think this is wrong, the popup should appear again, every time. It is confusing because it is inconsistent behaviour.

@Gudahtt
Copy link
Member

Gudahtt commented Apr 3, 2020

I was able to reproduce this by clicking "Connect" on the test dapp, the closing the notification window quickly while it was still loading. Subsequent notification windows would not appear.

@Gudahtt Gudahtt self-assigned this Apr 9, 2020
Gudahtt added a commit that referenced this issue Apr 9, 2020
MetaMask would sometimes get into a state where the notification popup
would never open. This could happen if the notification window was
closed shortly after being opened. After this happened, no popups would
show up until after the extension was reset.

This was happening because the background thought the popup was already
open. The variable it uses to track whether the popup was open or not
was being set to `true` immediately after the background asked the
browser to open a new window, before a handler was attached that could
respond to the window being closed.

Removing this line seems to solve the problem.

This line was added originally in #5437, which dealt with batch
transactions. Batches of transactions seem to work just fine without
this line though (from local testing), and I can't think of why this
would be required.

Closes #7051
Gudahtt added a commit that referenced this issue Apr 9, 2020
MetaMask would sometimes get into a state where the notification popup
would never open. This could happen if the notification window was
closed shortly after being opened. After this happened, no popups would
show up until after the extension was reset.

This was happening because the background thought the popup was already
open. The variable it uses to track whether the popup was open or not
was being set to `true` immediately after the background asked the
browser to open a new window, before a handler was attached that could
respond to the window being closed.

Removing this line seems to solve the problem.

This line was added originally in #5437, which dealt with batch
transactions. Batches of transactions seem to work just fine without
this line though (from local testing), and I can't think of why this
would be required.

Closes #7051
Gudahtt added a commit that referenced this issue Apr 28, 2020
This is a backport of #8314. Here's the original description:

MetaMask would sometimes get into a state where the notification popup
would never open. This could happen if the notification window was
closed shortly after being opened. After this happened, no popups would
show up until after the extension was reset.

This was happening because the background thought the popup was already
open. The variable it uses to track whether the popup was open or not
was being set to `true` immediately after the background asked the
browser to open a new window, before a handler was attached that could
respond to the window being closed.

Removing this line seems to solve the problem.

This line was added originally in #5437, which dealt with batch
transactions. Batches of transactions seem to work just fine without
this line though (from local testing), and I can't think of why this
would be required.

Closes #7051
Gudahtt added a commit that referenced this issue Apr 29, 2020
This is a backport of #8314. Here's the original description:

MetaMask would sometimes get into a state where the notification popup
would never open. This could happen if the notification window was
closed shortly after being opened. After this happened, no popups would
show up until after the extension was reset.

This was happening because the background thought the popup was already
open. The variable it uses to track whether the popup was open or not
was being set to `true` immediately after the background asked the
browser to open a new window, before a handler was attached that could
respond to the window being closed.

Removing this line seems to solve the problem.

This line was added originally in #5437, which dealt with batch
transactions. Batches of transactions seem to work just fine without
this line though (from local testing), and I can't think of why this
would be required.

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

Successfully merging a pull request may close this issue.

4 participants