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

Fix popup/notification when browser is in fullscreen, primarily on macOS. #9075

Merged
merged 2 commits into from
Jul 27, 2020

Conversation

tmashuang
Copy link
Contributor

@tmashuang tmashuang commented Jul 24, 2020

The issue was reported internally via Slack. User was running macOS Chrome in fullscreen mode where Chrome is created in a new Desktop workspace.

The issue reproduced on macOS Chrome in fullscreen/maximized view overrides the explicitly set width and height for windows.create(). Possibly not overrides, but creates a window based off of the window that it was created from. Related Chrome bugs 1 2

The fullscreen popup.left pixel will calculate the window position incorrectly since we set and assume the width of the created window. The incorrect left position the window and transition the focus Desktop/Workspace incorrectly and make it seem to lose focus of the new window/workspace. Incidentally this will make the popup full width/height, and create a new workspace for the view, which we have no control over until Chrome fixes it.

This will check if the popup is 'fullscreen', which it gets passed from the origin window, if so then don't reposition the window. If Chrome fixes the issue we can revert this change.

Screenshots are of the inconsistent scoped window attributes on Chrome and Firefox. Both are in Fullscreen/Desktop/Workspace and initiating a Connect Request, or any action that triggers popup/notification ui.

Chrome lastFocused Screen Shot 2020-07-24 at 1 38 28 PM
Firefox lastFocused Screen Shot 2020-07-24 at 1 41 59 PM
Chrome created popupWindow Screen Shot 2020-07-24 at 1 39 23 PM
Firefox created popupWindow Screen Shot 2020-07-24 at 1 42 19 PM
  1. Bug 1

  2. Bug 2

The issue was reported internally via Slack. User was running Mac OSX Chrome in fullscreen mode where Chrome is created in a new Desktop workspace.

The issue reproduced on OSX Chrome in fullscreen/maximized view overrides the explicitly set width and height for `windows.create()`. Possibly not overrides, but creates a window based off of the window that it was created from. Found a related [Chromium bug](https://bugs.chromium.org/p/chromium/issues/detail?id=263092&q=window%20create%20width%20os%3DMac&can=2).
The fullscreen `popup.left` pixel will calculate the window position incorrectly since we set and assume the width of the created window. The incorrect `left` position the window and transition the focus Desktop/Workspace incorrectly and make is seem to lose focus of the new window/workspace. Incidentally this will make the popup full width/height, and create a new workspace for the view, which we have no control over until Chrome
fixes it.

This will check if the popup is 'fullscreen', which it gets passed from the origin window, if so then don't reposition the window. If Chrome fixes the issue we can revert this change.
@tmashuang tmashuang requested a review from a team as a code owner July 24, 2020 21:30
@metamaskbot
Copy link
Collaborator

Builds ready [68bd4a5]
Page Load Metrics (657 ± 34 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31119462110
domContentLoaded4077626557134
load4137646577134
domInteractive4077626557134

@tmashuang tmashuang changed the title Fix popup/notification when browser is in fullscreen, primarily on OSX. Fix popup/notification when browser is in fullscreen, primarily on macOS. Jul 24, 2020
@kelonye

This comment has been minimized.

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.

Good catch! This additional window position update is definitely not desired on fullscreen.

I'm not entirely sure how those two linked Chrome bugs are related? The first seems to relate to max/min window sizes specifically (which is a feature of the Chrome App API, not the Extension API), and the second seems to be about a maximum popup size (which we aren't trying to exceed here). But nonetheless, this change is good.

app/scripts/lib/notification-manager.js Outdated Show resolved Hide resolved
Co-authored-by: Mark Stacey <[email protected]>
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!

@metamaskbot
Copy link
Collaborator

Builds ready [44d6e5d]
Page Load Metrics (649 ± 51 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31101562512
domContentLoaded39784464710551
load39984664910651
domInteractive39784464710551

@tmashuang tmashuang merged commit ef1b1d5 into develop Jul 27, 2020
@tmashuang tmashuang deleted the mac-fullscreen-chrome-popup-fix branch July 27, 2020 18:33
Gudahtt added a commit that referenced this pull request Aug 7, 2020
…cOS. (#9075)

* Fix popup/notification when browser is in fullscreen, primarily on OSX.

The issue was reported internally via Slack. User was running Mac OSX Chrome in fullscreen mode where Chrome is created in a new Desktop workspace.

The issue reproduced on OSX Chrome in fullscreen/maximized view overrides the explicitly set width and height for `windows.create()`. Possibly not overrides, but creates a window based off of the window that it was created from. Found a related [Chromium bug](https://bugs.chromium.org/p/chromium/issues/detail?id=263092&q=window%20create%20width%20os%3DMac&can=2).
The fullscreen `popup.left` pixel will calculate the window position incorrectly since we set and assume the width of the created window. The incorrect `left` position the window and transition the focus Desktop/Workspace incorrectly and make is seem to lose focus of the new window/workspace. Incidentally this will make the popup full width/height, and create a new workspace for the view, which we have no control over until Chrome
fixes it.

This will check if the popup is 'fullscreen', which it gets passed from the origin window, if so then don't reposition the window. If Chrome fixes the issue we can revert this change.

* Feedback commit

Co-authored-by: Mark Stacey <[email protected]>

Co-authored-by: Mark Stacey <[email protected]>
@metamaskbot metamaskbot mentioned this pull request Aug 7, 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.

4 participants