Certain macOS / OS X menu items don't open a window if one isn't already open #8164

echosa · 2017-04-10T05:16:02Z

bsclifton · 2017-06-05T07:13:47Z

Marking as good first bug (also, I edited the original issue after fixing the new session tab issue)

The solution is pretty easy- commonMenu.js has a method we can call (named ensureAtLeastOneWindow) that will open a new window if needed.

Click here for an example that shows how to use the call properly (used for opening a new tab)

fraed · 2017-10-21T08:48:51Z

For me a lot of menu items are fixed for reopening with zero brave windows open. E.g.: "About brave".

"Import Browser Data" is still not working and adding ensureAtLeastOneWindow does not work as expected. The window opens but the Dialog is missing.

Probably the "hard to tackle" menu items are still not working. Eg.: "History home" lives in menu not commonMenu where ensureAtLeastOneWindow is missing. Even exporting ensureAtLeastOneWindow doesn't help to bring the "History Home" Tab up as clicking "History Home" throws an error in any case when no window is open:

TypeError: Cannot read property 'id' of null at getSetting.split.forEach (/Users/jan/Projects/brave/browser-laptop/app/browser/menu.js:303:66) at Array.forEach (<anonymous>) at click (/Users/jan/Projects/brave/browser-laptop/app/browser/menu.js:301:50) at MenuItem.click (/Users/jan/Projects/brave/browser-laptop/node_modules/electron-prebuilt/dist/Brave.app/Contents/Resources/electron.asar/browser/api/menu-item.js:59:9) at Function.executeCommand (/Users/jan/Projects/brave/browser-laptop/node_modules/electron-prebuilt/dist/Brave.app/Contents/Resources/electron.asar/browser/api/menu.js:121:15)

If anyone can offer help how to deal with ensureAtLeastOneWindow in menu.js and why "Import Browser Data" ist not working using ensureAtLeastOneWindow` I would give it a try!

arsalankhalid · 2018-01-29T20:59:40Z

Is this still available, may I take this over? @bsclifton

Thanks!

bsclifton · 2018-02-13T16:47:08Z

@arsalankhalid absolutely 😄 I'll self-assign to reserve it for you

arsalankhalid · 2018-02-15T03:33:48Z

OK Great :) Will take a stab

arsalankhalid · 2018-02-15T21:57:30Z

I'm not able to re-create this, i've removed all my apps, started up again and then attempted to start a new session. But no banana, are we sure this issue still exists?

echosa · 2018-02-15T22:07:25Z

Yes, this still an issue. Here's a GIF of me showing/testing Brave -> About Brave and Brave -> Import Browser Data. Notice that when I click them without an existing Brave window open (I close the window at the start of the GIF), clicking those entries do nothing. I expect that they would open a window and show the correct page/content.

arsalankhalid · 2018-02-16T02:08:45Z

Ah much clearer, thank you. Will give it a shot!

kjozwiak · 2018-03-22T20:12:34Z

Still an issue using 0.22.6 e6ff4ea. You'll get the following TypeError when attempting to export bookmarks when there's no windows currently opened:

STR:

launch brave on macOS and close all the windows, the application should still be running
select Bookmarks -> Export Bookmarks

[TypeError: window can not be null
    at Object.showDialog (/Applications/Brave-Beta.app/Contents/Resources/electron.asar/browser/api/dialog.js:56:13)
    at Object.showDialog (/Applications/Brave-Beta.app/Contents/Resources/app.asar/app/browser/bookmarksExporter.js:37:10)
    at process.on (/Applications/Brave-Beta.app/Contents/Resources/app.asar/app/index.js:377:25)
    at emitNone (events.js:86:13)
    at process.emit (events.js:188:7)
    at click (/Applications/Brave-Beta.app/Contents/Resources/app.asar/app/common/commonMenu.js:267:17)
    at MenuItem.click (/Applications/Brave-Beta.app/Contents/Resources/electron.asar/browser/api/menu-item.js:59:9)
    at Function.executeCommand (/Applications/Brave-Beta.app/Contents/Resources/electron.asar/browser/api/menu.js:121:15)]

armaanahluwalia · 2018-03-23T16:01:07Z

Hi @bsclifton . I've been going through the good-first-bugs and identified this as something I'd like to try my hand at. ( This would be my first contribution to the project as well as open source in general for a while so I hope you don't mind ).

Anyway I debugged this issue and I've found out that it's being caused ( at least in the case of the "About Brave" action ) due to the existence of the 'Buffer Window'. The app thinks that a window is still open in the getAllWindows() call. I'm going to try my best to put in a PR tonight. I hope you wouldn't mind.

Thanks!

When closing last window, make sure buffer window is closed so that app exits

bsclifton · 2018-03-26T05:28:14Z

@armaanahluwalia your help is definitely welcomed 😄👍

The buffer window optimization is fairly new- so I doubt this is the root cause for the issue. You should be able to check out the 0.21.x branch and reproduce the issue there too

armaanahluwalia · 2018-03-26T18:06:27Z

@bsclifton I'll look into it further but as per my last investigation, the reason that the about brave option wasn't working is because on macOS the getAllWindows() call thinks there is a window open even though there isn't. This is caused by the buffer window not being closed when all windows are closed due to line 341 of commit 12aa8b .

Will double check I'm right and address the other parts of this bug too but thought I'd put this down in case it helps anyone else out. Thanks for the info though regarding the reasoning for the buffer window.

armaanahluwalia · 2018-03-26T18:09:36Z

@bsclifton I think that also brings up another point of wrapping the getAllWindows function to exclude the buffer window since people calling the function should be shielded from the workings of the buffer window solution. Perhaps have another api call such as getAllRealWindows or something like that and if you want the buffer window to be included then call the original function

petemill · 2018-03-26T19:24:17Z

@armaanahluwalia I agree, I think I may have considered a function in the past but didn't see a need for it yet. getAllWindows should not be used, as there can be other kind of windows running background tasks. Instead app/browser/windows.js: getAllRendererWindows() should be used, and we can add a flag to that to filter out buffer window. It should probaby default to excluding buffer window, e.g. getAllRendererWindows(excludeBufferWindow = true). Thanks for digging in!

Fix #8164 Fix #13689 This bug was caused because menu items were using a hidden window for their new tabs. We should _not_ use `electron{.remote}.BrowserWindow.getAllWindows()` or `.get{Active, Focused}Window` directly. There are two reasons that could have bad results: 1. We create BrowserWindows which are not normal tabbed renderer windows (e.g. some code that runs on Bookmark creation) 2. We have a hidden window sometimes (the Buffer Window) Instead call `app/browser/window.js`: `getAllRendererWindows` and `getActiveWindowId`. Fix for main menu actions, which can be called when there are no windows, but when searching for `BrowserWindow.` Test Plan: - Check all menu items open new tab in currently active window - Check all menu items open new tab in currently active window, when app is not focused - Check all menu items open new tab in new window if there is no visible window - Check tabs in all windows are persisted on restart when app / windows close - Check process quits in Windows when last visible window is closed

luixxiul added the OS/macOS label Apr 10, 2017

bsclifton added the bug/good-first-bug label Jun 5, 2017

bsclifton added this to the Triage Backlog milestone Nov 27, 2017

bsclifton self-assigned this Feb 13, 2018

bsclifton removed their assignment Mar 20, 2018

armaanahluwalia referenced this issue Mar 23, 2018

Merge pull request #13242 from brave/fix/quit-on-close-buffer-window

12aa8bb

When closing last window, make sure buffer window is closed so that app exits

bsclifton mentioned this issue Mar 26, 2018

When closing last window, make sure buffer window is closed so that app exits #13242

Merged

10 tasks

bsclifton mentioned this issue Apr 3, 2018

New Tab doesn't work without an open window on macOS #13689

Closed

petemill mentioned this issue Apr 5, 2018

Menu items which need a window will create / show one if needed #13749

Merged

10 tasks

petemill self-assigned this Apr 5, 2018

petemill modified the milestones: Triage Backlog, 0.22.x Release 2 (Beta Channel) Apr 6, 2018

petemill added the PR/pending-review label Apr 6, 2018

bsclifton modified the milestones: 0.22.x Release 2 (Beta Channel), 0.22.x Release 3 Apr 6, 2018

bsclifton added release-notes/include QA/test-plan-specified labels Apr 6, 2018

bsclifton unassigned petemill Apr 6, 2018

bsclifton removed PR/pending-review QA/test-plan-specified release-notes/include labels Apr 6, 2018

bsclifton modified the milestones: 0.22.x Release 3, Triage Backlog Apr 6, 2018

NejcZdovc added the help wanted The PR/issue opener needs help to complete/report the task. label May 7, 2018

bsclifton added wontfix fixed-with-brave-core This issue will automatically resolved with the replacement of Muon with Brave Core. labels Aug 26, 2018

bsclifton removed this from the Triage Backlog milestone Aug 26, 2018

bsclifton closed this as completed Aug 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Certain macOS / OS X menu items don't open a window if one isn't already open #8164

Certain macOS / OS X menu items don't open a window if one isn't already open #8164

echosa commented Apr 10, 2017 •

edited by bsclifton

Loading

bsclifton commented Jun 5, 2017 •

edited

Loading

fraed commented Oct 21, 2017

arsalankhalid commented Jan 29, 2018

bsclifton commented Feb 13, 2018

arsalankhalid commented Feb 15, 2018

arsalankhalid commented Feb 15, 2018

echosa commented Feb 15, 2018 •

edited

Loading

arsalankhalid commented Feb 16, 2018

kjozwiak commented Mar 22, 2018

armaanahluwalia commented Mar 23, 2018

bsclifton commented Mar 26, 2018 •

edited

Loading

armaanahluwalia commented Mar 26, 2018

armaanahluwalia commented Mar 26, 2018

petemill commented Mar 26, 2018

Certain macOS / OS X menu items don't open a window if one isn't already open #8164

Certain macOS / OS X menu items don't open a window if one isn't already open #8164

Comments

echosa commented Apr 10, 2017 • edited by bsclifton Loading

bsclifton commented Jun 5, 2017 • edited Loading

fraed commented Oct 21, 2017

arsalankhalid commented Jan 29, 2018

bsclifton commented Feb 13, 2018

arsalankhalid commented Feb 15, 2018

arsalankhalid commented Feb 15, 2018

echosa commented Feb 15, 2018 • edited Loading

arsalankhalid commented Feb 16, 2018

kjozwiak commented Mar 22, 2018

armaanahluwalia commented Mar 23, 2018

bsclifton commented Mar 26, 2018 • edited Loading

armaanahluwalia commented Mar 26, 2018

armaanahluwalia commented Mar 26, 2018

petemill commented Mar 26, 2018

echosa commented Apr 10, 2017 •

edited by bsclifton

Loading

bsclifton commented Jun 5, 2017 •

edited

Loading

echosa commented Feb 15, 2018 •

edited

Loading

bsclifton commented Mar 26, 2018 •

edited

Loading