Browser-laptop: Certain macOS / OS X menu items don't open a window if one isn't already open

[echosa]

- Did you search for similar issues before submitting this one?

Yes

• Describe the issue you encountered:

On OS X (and, I assume, macOS), if you have Brave open without any open windows and select File -> New Session Tab -> New Session Tab 1, no window opens. Selecting New Tab or New Private Tab will properly open a new window with the new tab, but New Session Tab does not.

Other menu items also do not open a window when one is not already present. Here's a list of ones I've found:

• [x] Brave -> About Brave
• [ ] Brave -> Import Browser Data
• [x] Brave -> Help Center
• [x] File -> New Session Tab -> New Session Tab 1
• [ ] File -> Open Location
• [ ] History -> Home
• [ ] History -> Clear Browsing Data
• [ ] History -> <any recently closed>
• [x] History -> Show History
• [ ] Bookmarks -> Import Browser Data
• [ ] Bookmarks -> <any actual bookmarked URL>
• [ ] Shields -> Site Shield Settings
• [x] Help -> Help Center

All the other menu items seem to work appropriately, but feel free to double check.

• Platform (Win7, 8, 10? macOS? Linux distro?):

OS X 10.11.6

• Brave Version (revision SHA):

0.14.1 (3de60d5)

• Steps to reproduce:

  1. Open Brave.
  2. Close all windows, but leave Brave running.
  3. Pick any of the menu items listed above.

• Actual result:

Nothing happens.

• Expected result:

A new window should open with the session tab or other appropriate page.

• Will the steps above reproduce in a fresh profile? If not what other info can be added?

Yes.

• Is this an issue in the currently released version?

Yes.

• Can this issue be consistently reproduced?

Yes.

[bsclifton]

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]

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]

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

Thanks!

[bsclifton]

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

[arsalankhalid]

OK Great :) Will take a stab

[arsalankhalid]

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]

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]

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

[kjozwiak]

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]

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!

[bsclifton]

@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]

@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]

@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]

@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!