twolfson / karma-electron

Karma launcher and preprocessor for Electron
The Unlicense
59 stars 21 forks source link

Regardless of the `browserWindowOptions.show` value, a window always opens. #54

Closed trusktr closed 3 years ago

trusktr commented 3 years ago

I thought that browserWindowOptions.show being set to false would hide the window. However regardless of the value, windows always appear during testing.

It's not a big deal though: on CI I use xvfb-maybe to make it "headless" regardless, but I was curious as to what the actual result of browserWindowOptions.show should be.

twolfson commented 3 years ago

I've confirmed that this is a bug by running:

npm run test-karma-continuous -- --browsers VisibleElectron

and toggling show: true/false in the karma.conf.js

Though, it's strange because there's 1 less window when we do true/false, so it makes me think something is working -- just unsure what has changed ._.

I'm pretty busy so I can't guarantee an immediate turnaround time, maybe by the end of next week? (probably will be a lot sooner, I don't like known bugs sticking around too long)

twolfson commented 3 years ago

Trying to tackle this, though kind of tired. We'll see how far I get

twolfson commented 3 years ago

So far:

twolfson commented 3 years ago
twolfson commented 3 years ago

Yep, this is window.open misbehaving in Electron:

twolfson commented 3 years ago

Behavior still persists on electron@15. Seems like this is a bug in Electron so will see if there's anything existing to watch

twolfson commented 3 years ago

Ah, it seems there's a windowOpenHandler we can configure so going to do that

https://github.com/electron/electron/blob/v12.1.2/docs/api/window-open.md#windowopenurl-framename-features

https://github.com/electron/electron/blob/v12.1.2/docs/api/web-contents.md#contentssetwindowopenhandlerhandler

twolfson commented 3 years ago

Well that did remedy the issue, but now the tests are failing so it might have overdone it -- and I'm noticing a nativeWindowOpen a few lines up that I'd like to understand. But I'm out of energy for tonight =/ Will resume another day

https://github.com/twolfson/karma-electron/blob/bc60482ba3d835099fa97d72ff3f3e1c9890266b/lib/electron-launcher.js#L95-L102

twolfson commented 3 years ago

Resuming the journey on this

twolfson commented 3 years ago

So it looks like the only piece of the tests that are failing right now is a Node.js parity one (i.e. module.parent is undefined as expected but that's not an actually defined attribute (i.e. no hasOwnProperty), which is inconsistent =/

That being said, the nativeWindowOpen: true a few lines up that I'd noticed was introduced in 7.0.0 as part of supporting electron@12 so that's prob fine

twolfson commented 3 years ago

Ah, the module.parent property was moved to module.__proto__. But that also seems to just be an electron@12 -> electron@15 change so we're fine =P

I think the patch setWindowOpenHandler will work fine. Going to create a clean branch, test it, and :shipit:

twolfson commented 3 years ago

This has been patched with setWindowOpenHandler, tested, and released in 7.1.0. Thanks again for the bug report! =D