New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 913360 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 21
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug



Sign in to add a comment

Mac PWAs: File > New opens Chrome window inappropriately

Project Member Reported by ccameron@chromium.org, Dec 10

Issue description

In a PWA, the File menu has a "New Window" item.

This menu item opens a new Chrome browser window.

We should decide if this should menu item should (1) be removed or (2) open a new instance of the PWA window (as it would if we were to launch again from chrome://apps).
 
Labels: -M-72 M-73
Cc: markchang@chromium.org
+ markchang@ since this seems like a product decision
Cc: pcovell@chromium.org mgiuca@chromium.org
Both 
- New Window
- Open File

don't seem to provide relevant functions for PWAs, and the outcomes (open a browser window, open a file in the window frame with no connection to the app). 

pcovell@, mgiuca@, markchang@ - can we make a call to not have these two menu items? 
Yeah I think we get rid of "Open file" altogether (since a PWA might be a file editor where there'd be an expectation of "Open file" opening a file *in* the app, not opening a web page in the browser). [Aside: I had no idea we had this feature. I don't see it on the Chrome menu on other platforms. Is this a macOS-only feature?]

"New Window" should open a new PWA app window at the start page. If that's too hard, we should nix it for this version, but bring it back in future.
We hook that up to IDC_NEW_WINDOW at:
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/main_menu_builder.mm?rcl=491e2671e35208ba3654d4211c775708df5c52f2&l=96

We then handle IDC_NEW_WINDOW by calling NewWindow at
https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_command_controller.cc?rcl=a4bd5ce4b8ed521ad948786963eacf35360405f4&l=344

And NewWindow calls NewEmptyWindow at
https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_commands.cc?rcl=9d37d5147d8b9d9a20bd1b55f2f7daad06ed0c3e&l=579

If we see that the browser is a hosted app, we should not create a NewEmptyWindow there. Doing the same thing we do for regular app launch (whatever that is) should work. When we launch from a shim we do it at
https://cs.chromium.org/chromium/src/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc?rcl=25578467440d7e7261f1a1a5c32173e561b57d8a&l=235

I'd test that, but I installed Mojave and, in exchange for awesomely-rounded PWA windows, my build is hosed.
Yeah, it should read CreateApplicationWindow there (possible with some ifdefs sprinkled in).
Owner: ccameron@chromium.org
Status: Assigned (was: Untriaged)
Project Member

Comment 8 by bugdroid1@chromium.org, Dec 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1860807b58e924ffc973ae03f3b2556361dddf2c

commit 1860807b58e924ffc973ae03f3b2556361dddf2c
Author: Christopher Cameron <ccameron@chromium.org>
Date: Fri Dec 21 15:34:28 2018

MacPWAs: Fix File>New and File>Open

Remove the Open item from the File menu -- the item isn't particularly
meaningful.

Make New Window in the File menu open a new instance of the PWA. Of note
is that the window opens in the exact same place as the initial window
of the PWA (as opposed to being offset), which should be fixed.

Bug:  913360 
Change-Id: I606852e4d8092046b6194870df22400b2eec0b1e
Reviewed-on: https://chromium-review.googlesource.com/c/1388003
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618529}
[modify] https://crrev.com/1860807b58e924ffc973ae03f3b2556361dddf2c/chrome/browser/ui/browser_commands.cc
[modify] https://crrev.com/1860807b58e924ffc973ae03f3b2556361dddf2c/chrome/browser/ui/cocoa/main_menu_builder.mm

Status: Fixed (was: Assigned)
I've filed issue 917416 on the fact that new windows show up in non-ideal (not tiled) places, as a non-P1.

Sign in to add a comment