fix: improved event flow management related to 'will-resize' event on Windows

[neo291]

Description of Change

This is change improve the compliance with Windows event management flow for the WM_SIZING event.
When we use the event.preventDefault() function we need to return true into the Windows event queue in order to signal to Windows that the event has been managed.
The solution is to add the missing 'return true' into the 'prevent default' condition.

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• PR title follows semantic commit guidelines

Release Notes

Notes: no-notes

[welcome]

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

• fix: don't overwrite prevent_default if default wasn't prevented
• feat: add app.isPackaged() method
• docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

• Follow the JavaScript, C++, and Python coding style.
• Run npm run lint locally to catch formatting errors earlier.
• Document any user-facing changes you've made following the documentation styleguide.
• Include tests when adding/changing behavior.
• Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

[nornagon]

Thanks for this! Looks like a good change. Would it be possible to add a test for this behaviour so we can avoid breaking it in some future refactor?

[neo291]

@nornagon you're welcome!
Unfortunately is a physical-mouse related event (mouse-down, drag and then mouse-up), for this reason I think that is complicated to set up tests for now, but would be a great idea for the future!
Moreover, it's my intention to look deep into the 'will-resize' and 'will-move' events, because the actual implementation is available only on macOS and Windows. Is my opinion that if I write code also to be run into Linux this can be nasty for a critical UI related feature like this, and become hard to use it for big projects, my hope is to find a solution that can fits to all the 3 platforms.
The my proposal is to merge that fix as is, that It doesn't hurts and improves the compliance with Windows flow.

[codebytere]

@neo291 looks like you'll need to rebase this – picked up some extra commits

[neo291]

@codebytere you're right, sorry, my fault! I hadn't noticed that mistake, now it should be fixed, thanks.

[codebytere]

Seeing quite a lot of test failures on MacOS with this change; it looks fine on its face but more investigation is in order before i'd feel comfortable merging this.

[neo291]

Yes I've seen them, but this change is related to Windows code only and even in the eventualty this change could affect macOS code, after a quick look to the failed tests, I think that are not related to something that can be affected by this change.
This change is only to make the Windows code works in the expected flow by Windows documentation.

[neo291]

After some more analysis I've changed the title and commit message with a more appropriate one, as the event prevention works correctly inside the main node process also with return false.
I've found that the case in which the prevention doesn't work is when we use/set the event into the renderer, in this case is used the IPC to trigger the callback inside the renderer, so, being the IPC to the renderer asynchronous and executed after the event management end make impossible to set the prevention flag in time to be used.
So now this change is only an improvement that make the code more compliant with the Windows documentation.
Despite the change is right and necessary anyway to improve, it not address the initial fix intention, I'm sorry for the inconvenience, but now all is more clear, I'll pay more attention in the future to the difference between the main node and the renderer.

[nornagon]

Does this change make any observable difference to the behaviour of the resize event now, or is it purely to comply with windows docs recommendation?

[neo291]

@nornagon no, this change doesn't make any observable difference to the behavior.
But also it is not a mere recommendation, it is mandatory as stated here in the WM_SIZING message documentation.
I report the row for completeness:

An application should return TRUE if it processes this message.

I think that is better to follow what is stated by Microsoft documentation.
In my experience with Windows there are many things that the documentation sometime whisper and even if are not followed all work properly for the most of the time, but sometimes can cause issues that is really complex to understand and that we will never think they can be originated by a such small things.
So IMHO is better to follow these small things when possible 😜.

[neo291]

@nornagon now the comment is appropriate.

[nornagon]

Hmmm, so I see your point about the docs, but reading the code, I'm not sure that return true is appropriate here. It looks to me like the order of events is:

1. User presses the mouse button over a drag region
2. User moves the mouse
3. Windows sends WM_SIZING to the window
4. Electron catches the message in PreHandleMSG
5. Electron notifies the app of the impending resize with will-resize
6. Electron checks whether the event's default action was prevented
   • If it was prevented, then we call ::GetWindowRect() to set the l_param (which is a RECT*) to the current size of the window, i.e., set the "resized" size to the current size, i.e. no change in size
7. Allow the event to continue processing

So the Windows docs don't necessarily tell us the right thing to do here. I wouldn't be surprised if returning true here would lead to some weird behaviour in a different part of the code (esp. some code within Chromium) which may expect to receive the WM_SIZING event always.

[nornagon]

/cc @poiru who wrote this code in the first place :)

[poiru]

LGTM.

[nornagon]

If @poiru's OK with it then so am I.

[nornagon]

Removing the 3-0-x and 4-0-x labels because this no longer addresses any observable issue.

[release-clerk]

No Release Notes