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

[RFC] fix(popup): popup window relative to last focused window #8356

Merged
merged 1 commit into from
Apr 19, 2020

Conversation

brad-decker
Copy link
Contributor

Fixes #7314

Cause of bug

  • The popup window is triggered from the background chrome process for the extension which has no height, width, left or top position (actually all are 0)
  • The original logic attempted to use these to compute a good position for the popup.
  • The result is that on dual displays, the popup always opened in the top left most pixel of the leftmost display

Suggested resolution

  • use the browser extension API to grab the latest focused window
  • check the left and top values for that window
  • ???

I have defaulted to opening the popup perfectly aligned with the top left most pixel of the last focused window. This could end up being wonky. I would love to get an idea of what the best-case scenario would be from the product team @jacobcantele and of course would love input from the engineers on the approach here.

@brad-decker
Copy link
Contributor Author

Screen Shot 2020-04-17 at 1 32 28 PM

@rachelcope
Copy link

rachelcope commented Apr 17, 2020

I've always felt that the confirmation window appearing on the upper left corner is disorienting since all other MetaMask interactions appear in the upper right corner window (near the fox in the browser bar).

This decision (I believe) was made so it would be harder to fake a MetaMask popup. Does the left side position not have the same risks? Is there any other reason for the upper left popup location?

I'd propose instead that the MetaMask window always appear on the upper right corner. These UX principles apply here:

  1. consistency and principle of least surprise - (https://en.wikipedia.org/wiki/Principle_of_least_astonishment)
    Users should be able to anticipate that the MetaMask window will consistently show up in the same location. This would minimize surprise to where it might appear on their screen and reduce the learning time to get familiar with the product.

  2. proximity principle (https://www.nngroup.com/articles/closeness-of-actions-and-objects-gui/) - things that appear close together on the screen are seen as related. Having the MetaMask window appear near the fox extension icon will help users mentally connect the two.

Any thoughts on this? @brad-decker @jacobcantele @danfinlay @cjeria @omnat

** having the popup always appear on the window relative to last focus is obviously a huge improvement regardless!

@brad-decker
Copy link
Contributor Author

upper left is just easiest with the information the API returns, we can use other info to get the width of the browser and move the popup to the top-right if that is the desired effect.

@brad-decker brad-decker force-pushed the rfc-popupscreen branch 2 times, most recently from 3a8da85 to 1e83e46 Compare April 17, 2020 21:11
@brad-decker
Copy link
Contributor Author

Screen Shot 2020-04-17 at 4 11 35 PM

Moved to top right corner with latest push.

.nvmrc Outdated Show resolved Hide resolved
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Thanks, this looks like a great solution!

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

I agree that this is a good solution. I had one question about it.

app/scripts/lib/notification-manager.js Show resolved Hide resolved
Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

LGTM!

@whymarrh whymarrh merged commit f9989c5 into MetaMask:develop Apr 19, 2020
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.

Transaction confirmations/popups shown on primary instead of current monitor / display
4 participants