fix: Fix menu.popup() bugs
#11385
fix: Fix menu.popup() bugs
#11385
Conversation
lib/browser/api/menu.js
Outdated
| @@ -45,6 +45,7 @@ Menu.prototype._init = function () { | |||
|
|
|||
| Menu.prototype.popup = function (window, x, y, positioningItem) { | |||
| let [newX, newY, newPosition, newWindow] = [x, y, positioningItem, window] | |||
| let opts | |||
|
|
|||
| // menu.popup(x, y, positioningItem) | |||
| if (!window) { | |||
There was a problem hiding this comment.
This doesn't look right to me. We used to check window != null, now we check !window
https://github.com/electron/electron/pull/10888/files
Old conditional:
if (window != null && (typeof window !== 'object' || window.constructor !== BrowserWindow)) {
Current conditional:
if (!window) {
 - // Shift.  + // shift over values
 - positioningItem = y  + if (typeof window !== 'object' || window.constructor !== BrowserWindow) {
There was a problem hiding this comment.
As far as I can tell, the old code here and the new code do the same thing. It's not super easy to read, but if you want a change in this PR, I'm happy to make one!
There was a problem hiding this comment.
Thanks for the better link 😄
I'm referring to where the old test (line 150 on lhs) of if (window != null ... is replaced with (line 50 rhs) if (!window) which is nearly the opposite test. This seems unintentional to me because the logic on the following conditionals are unchanged.
If I'm misreading this, that's fine, just correct me :)
If it's broken, a fix + regression test would be great.
There was a problem hiding this comment.
You know what, I think you're right. Let me beef up all the tests here to make sure that this PR fixes all the issues with the method right now.
| throw new Error(`Cannot open Menu without a BrowserWindow present`) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is more complicated before, and actually seems more complicated than the code was before #10888 as well.
So I'm trying to understand, is this a regression caused by #10888? If so, I think I'd prefer that we back that out + add your tests + then take another run at a refactor.
Alternately, was this broken before #10888 as well?
There was a problem hiding this comment.
This was broken before 10888, too. We default to showing in the focused window, but a focused window might not be present. I took the liberty here of going with the first window if we can't find one, but we could easily continue to throw and take this portion out.
The important portion here is really that we don't override newX just because x isn't a number (and so on).
There was a problem hiding this comment.
I'm not disagreeing with the choice of taking the first window, was just was trying to understand if this was a 10888 regression because the pre-10888 code and the code here didn't do the same things.
6535c9c to
dfd7598
Compare
|
@ckerr Slightly improved behavior: |
|
The linter is being such a linter today. 🙄
|
lib/browser/api/menu.js
Outdated
| if (typeof window !== 'object' || window.constructor !== BrowserWindow) { | ||
| [newPosition, newY, newX, newWindow] = [y, x, window, null] | ||
| } | ||
| if (window && typeof window !== 'object' || window.constructor !== BrowserWindow) { |
There was a problem hiding this comment.
Can this check be replaced with a simpler if (window && !(window instance of BrowserWindow))? It will also fix a linter error.
UPD see a comment below about if (window && ...)
There was a problem hiding this comment.
Yeah I'm wondering what prompted that typeof window !== 'object' clause in the first place
lib/browser/api/menu.js
Outdated
| if (typeof window !== 'object' || window.constructor !== BrowserWindow) { | ||
| [newPosition, newY, newX, newWindow] = [y, x, window, null] | ||
| } | ||
| if (window && typeof window !== 'object' || window.constructor !== BrowserWindow) { |
There was a problem hiding this comment.
if (window && ...
I guess it's a mistake.
This check was supposed to test that null of undefined were not explicitly provided as a window, right?
But what if it's a call in this form:
menu.popup(/* x */ 0, /* y */ 0, positioningItem)0 won't pass this check and the shift won't happen.
Let's change the check to if (window != null && ...
|
Excellent feedback, thanks a ton. Implemented! |
lib/browser/api/menu.js
Outdated
| } | ||
|
|
||
| // menu.popup({}) | ||
| if (window && window.constructor !== BrowserWindow) { |
There was a problem hiding this comment.
Numbers will pass this check too:
42 && 42..constructor !== BrowserWindow // true!(window != null) && (window.constructor === Object) should work fine.
| if (typeof window !== 'object' || window.constructor !== BrowserWindow) { | ||
| [newPosition, newY, newX, newWindow] = [y, x, window, null] | ||
| } | ||
| if (window != null && !(window instanceof BrowserWindow)) { |
There was a problem hiding this comment.
This is clearer than before.
Reading the code as it is now, this test looks very similar to the one in the next block five lines down, if (window && window.constructor !== BrowserWindow). If these are equivalent, they should be folded together for readability because right now it scans as if there is a subtle, uncommented difference between the two tests.
Nothing personal in throwing this back yet again. This code is already an improvement over what's in master.
UPD: Alexey's comment is more to the heart of the problem
There was a problem hiding this comment.
There is actually a subtle difference - the first one checks if it's anything other than a BrowserWindow, the second one if it's specifically an object 👍
There was a problem hiding this comment.
It does now but didn't when I made that comment 😈
Thanks for working through all these improvements and adding tests. 💚
|
Reverted for now until we get a clean CI pass. |
This PR does three things:
• Calling
menu.popup({})used to be okay, but broke in the last beta. This fixes that.• We assigned defaults incorrectly, overriding user input 😱
• Tests!
Closes #11376