Fix Form snap layout when in maximized state

[jbhensley]

Fixes #6153.

Proposed changes

System.Windows.Forms.Form caches size and location upon becoming maximized and restores it when exiting maximized state. This creates a problem when using Windows snap layout since the form will be brought out of a maximized state and "docked" to a specific location by Windows only have its size and location subsequently changed to the internally cached values when the form processes WM_WINDOWPOSCHANGED.

If it were possible to not use internally cached values when coming out of a maximized state due to a snap layout command, this problem should be resolved. It does not appear that Windows provides any way to detect that. Instead, this PR assumes that any position change not initiated by the user or by a programmatic change of Form.WindowState is instead initiated by a window manager and cached positional values are not used.

Note: for determining if the change was initiated by the user, it was observed that a window message of WM_SYSCOMMAND containing a wParam of SC_RESTORE was sent regardless of whether the max/min button was clicked, the title bar was double-clicked, or if the restore command was used from the window's context menu. I was not able to find any way for the user to exit maximized state without triggering the WM_SYSCOMMAND. If that were possible, this fix would not work.

This PR fixes the issue by removing the unconditional caching of size/location that occurred when a form is maximized/minimized. By only attempting to set these values when necessary (e.g. a form is minimized and Size or Location is changed programmatically) it allows Windows to otherwise handle positioning.

The original attempt to fix left unconditional caching alone and only addressed the scenario of a form coming out of maximized state due to Windows snap layout. However, there were additional scenarios identified in which Windows attempted to set the geometry only to have been overridden by unconditionally cached values.

Customer Impact

Customers should observe that Form objects respond properly when using snap layout while in a maximized state.

Regression?

• No

Risk

Risk is that manner of determining whether Form position change is due to Windows snap layout versus user/programmatic change is deficient and does not cover all scenarios.

• It is unclear why form geometry was unconditionally cached and restored. There is risk that the original author had an presently unidentified edge case in mind. This change should be tested thoroughly.

Test methodology

• Unit tests
• UI integration tests
• Manually manipulated a Form object with the mouse and keyboard to verify behavior

Microsoft Reviewers: Open in CodeFlow

[Tanya-Solyanik]

Looks reasonable. On what versions of windows had it been tested? WIn11/10/7?

[RussKie]

Nice!

Now I wonder if we could write integration tests (UIIntegrationTests) that would interact with the new snap layout options.

[jbhensley]

Interesting. Seems like we'd need to know where to position the mouse. I tried calculating center of the maximize button using SystemInformation.CaptionButtonSize in conjunction with Width and then with ClientRectangle.Width, but the numbers don't quite come out right either way. The crude method I'm using is below.

protected override void OnPreviewKeyDown(PreviewKeyDownEventArgs e)
{
	base.OnPreviewKeyDown(e);

	if (e.KeyCode != Keys.A)
		return;

	var builder = new StringBuilder();
	builder.AppendLine(PointToClient(MousePosition).ToString());
	builder.AppendLine($"\tWidth: {Width}");
	builder.AppendLine($"\tClientRectangle: {ClientRectangle.Width}");
	builder.AppendLine($"\tButton width: {SystemInformation.CaptionButtonSize.Width}");
	builder.AppendLine($"\tCenter from width: {Width - 8 - (SystemInformation.CaptionButtonSize.Width * 1.5)}");
	builder.AppendLine($"\tCenter from client width: {ClientRectangle.Width - 8 - (SystemInformation.CaptionButtonSize.Width * 1.5)}");

	Debug.WriteLine(builder.ToString());
}

With my cursor reasonably centered:

I get:

{X=730,Y=-17}
Width: 816
ClientRectangle: 800
Button width: 36
Center from width: 754
Center from client width: 738

I'm afraid things would get worse from there when trying to move to the layouts. Of course, I could be going about this all wrong.

[jbhensley]

Quick follow up. The snap layout options panel has built in delays. I think that would be the biggest hurtle.

As far as mouse, it is possible to get needed mouse position to hover over the max button by sending WM_GETTITLEBARINFOEX message to the window. Then you have to account for the hover delay. Positioning from there to the layout you want might be tricky.

For keyboard, you can send Win+Z to open the menu and navigate with arrow keys. I started down this route, but also encountered timing issues.

[WinFormsFact]
public async Task Form_RespondsToSnapLayoutAsync()
{
	await RunEmptyFormTestAsync(async form =>
	{
		form.Location = new Point(20, 21);
		form.Size = new Size(300, 310);

		// Windows Key + Z opens the snap layout menu
		await InputSimulator.SendAsync(
			form,
			inputSimulator => inputSimulator.Keyboard.ModifiedKeyStroke(VirtualKeyCode.LWIN, VirtualKeyCode.VK_Z));

		// 65 or less and the snap layout panel is not yet ready for keyboard input
		await Task.Delay(66);

		// Right arrow selects the first snap layout (dock to left)
		await InputSimulator.SendAsync(
			form,
			inputSimulator => inputSimulator.Keyboard.KeyPress(VirtualKeyCode.RIGHT));

		// Press enter to snap
		await InputSimulator.SendAsync(
			form,
			inputSimulator => inputSimulator.Keyboard.KeyPress(VirtualKeyCode.RETURN));

		// Pause here so we can visually observe the form
		await Task.Delay(5000);
	});
}

I'm concerned that the needed delay may vary.

RunEmptyFormTestAsync is a method I added to ControlTestBase just for this.

[dreddy-work]

May be we should rely on windows notification here. Have you noticed any windows messages/notifications when snap layout pops up?

[jbhensley]

Doesn't look like it.

I modified RunEmptyFormTestAsync to be generic so I could use a custom form that dumps WndProc to the debugger. Changing the test code a bit to:

form.LogMessagesToDebug = true;
Diagnostics.Debug.WriteLine($"{DateTime.Now.Millisecond}: logging on");

await InputSimulator.SendAsync(
	form,
	inputSimulator => inputSimulator.Keyboard.ModifiedKeyStroke(VirtualKeyCode.LWIN, VirtualKeyCode.VK_Z));

Diagnostics.Debug.WriteLine($"{DateTime.Now.Millisecond}: input sent");

// inputSimulator.Sleep appears wildly inconsistent with snap panel timing. Task.Delay does not
await Task.Delay(SnapLayoutDelayMS);

form.LogMessagesToDebug = false;
Diagnostics.Debug.WriteLine($"{DateTime.Now.Millisecond}: logging off");

I get:

MS	msgid	msg/descr
376		logging on
383	70	WM_WINDOWPOSCHANGING
392	70	WM_WINDOWPOSCHANGING
396	133	WM_NCPAINT
399	20	WM_ERASEBKGND
404	14	WM_GETTEXTLENGTH
407	13	WM_GETTEXT
411	71	WM_WINDOWPOSCHANGED
415	15	WM_PAINT
421	14	WM_GETTEXTLENGTH
423	13	WM_GETTEXT
441	135	WM_GETDLGCODE
450	135	WM_GETDLGCODE
455	256	WM_KEYFIRST
464	257	WM_KEYUP
468	257	WM_KEYUP
477		input sent
487	134	WM_NCACTIVATE
491	6	WM_ACTIVATE
493	28	WM_WININICHANGE
495	8	WM_KILLFOCUS
498	14	WM_GETTEXTLENGTH
500	13	WM_GETTEXT
502	14	WM_GETTEXTLENGTH
505	13	WM_GETTEXT
516	641	WM_IME_SETCONTEXT
523	642	WM_IME_NOTIFY
995		logging off

So there's only 46 milliseconds between when the code resumes execution from sending the key sequence and when the last WndProc msg is received. The smallest workable delay I've observed was 66 ms.

[jbhensley]

Looks reasonable. On what versions of windows had it been tested? WIn11/10/7?

Tested:

• Win10 21H1 (19043.1415)
• Win11 21H2 (22000.376)

[RussKie]

Nice! Thank you for adding such tests. I don't think we need to exercise all of them, one or two will be sufficient to ensure it is working in general. RE: timing - I think it's totally acceptable to account for a delay. IIRC the InputSimulator has an extension to add a wait fluently. I think I saw it in some other test. I'm currently OOF, and won't be able to formally review your change for a few weeks.

[jbhensley]

Added a couple of snap layout UI integration tests. They do feel a bit fragile, but are passing on my box.

[RussKie]

🚀

[RussKie]

We have another PR in flight that uses the same method https://github.com/dotnet/winforms/pull/6262/files#diff-b701242799ecc2572782b1786e9da6de99a8b2894ee42a6e96bf472abede001a

[jbhensley]

Awesome. I can wait for that PR to merge and then use RunFormWithoutControlAsync introduced there. Looks like it might be about ready.

Let me know if you want me to keep this PR rebased to a single commit or if it just gets squashed on merge to main.

Lastly, no pressure to review if your still OOF

[jbhensley]

Pulled changes from #6262 and refactored

[RussKie]

Overall it is working much better, though I seem to have a scenario where the original form geometry isn't getting restored

1. Maximise a .NET form
2. Select one of the snap layouts and fill the other positions.
3. If we drag the form from its placement the form retains the original (i.e. start geometry).

If instead of 3. we maximise the form again and then drag it - the form will be restored to the snap layout's geometry, not to the original.

I'll add a recording later.

[jbhensley]

Ah, yes, that makes sense. The form caches geometry when it becomes maximized, so the snap position was the last known geometry at the time.

Maybe this should be set about a different way. The purpose for caching is apparently to allow size/location change while not in normal state, but yet size/location is cached unconditionally upon maximize (and minimize):

winforms/src/System.Windows.Forms/src/System/Windows/Forms/Form.cs

Lines 4748 to 4754 in 6fb22da

	// If someone set Location or Size while the form was maximized or minimized,
	// we had to cache the new value away until after the form was restored to normal size.
	// This function is called after WindowState changes, and handles the above logic.
	// In the normal case where no one sets Location or Size programmatically,
	// Windows does the restoring for us.
	//
	private void RestoreWindowBoundsIfNecessary()

winforms/src/System.Windows.Forms/src/System/Windows/Forms/Form.cs

Lines 5918 to 5920 in 6fb22da

	// If we used to be normal and we just became minimized or maximized,
	// stash off our current bounds so we can properly restore.
	if (oldState == FormWindowState.Normal && WindowState != FormWindowState.Normal)

Perhaps this logic should be smarter. Windows does indeed handle restoring position of native windows, including in the scenario you have outlined. This can be tested with Notepad. My original attempt was to be minimally invasive and provide the least opportunity to break something. At this point, though, it feels like we're fighting Windows. I can explore the alternative approach.

[jbhensley]

I have updated the approach and changed the PR description to match.

This feels more like the "right" way in that we let Windows do what it does and only take over when necessary. That said, it also feels more risky since the original author obviously had something in mind when they added the code that has been removed. Even though unconditionally caching geometry seems wrong and is definitely the source of our snap layout problems, there may have been a reason for it. I don't suppose there is any way we can access history for Framework 4.8 and earlier to get an idea why this was there?

[jbhensley]

/azp run

[azure-pipelines]

Commenter does not have sufficient privileges for PR 6335 in repo dotnet/winforms

[jbhensley]

I'm a lowly peon. I don't suppose sudo /azp run would work 😆

[RussKie]

I'm a lowly peon. I don't suppose sudo /azp run would work 😆

No, sorry. It requires WRITE access to re-run the build this way. Mere mortals need to close/re-open PRs to restart the build. :)

[RussKie]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[RussKie]

Apologies for the delay, we've not forgotten about this one.

[RussKie]

I think we should retain the original behaviour for all OSes, and only disable it for Win11.

[jbhensley]

That's probably safer, with any potential side effect of the change only affecting a subset of users. Code updated to remove unconditional caching of geometry for Win11 or greater.

[RussKie]

@jbhensley apologies, it fell off my radar. Do you mind rebasing on top of the latest main, so we can do a test pass over this change before we commit it. Thank you.

[dreddy-work]

@Olina-Zhang , can you help validate this change before we commit?

[Olina-Zhang]

@Olina-Zhang , can you help validate this change before we commit?

Will test it today.

[John-Qiao]

@dreddy-work Tested the built private binaries for this pull based on latest .NET 7.0 SDK, now snap layout works correctly when initiated from the maximized window, no new issue found during manual testing. But the migrated automation cases blocked by the known issue: 6952, we will re-run the migrated automation cases once that known issue be fixed and update result here.

[RussKie]

Thank you @John-Qiao, #6952 is likely caused by an unrelated change.

[RussKie]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[ghost]

Hello @RussKie!

Because this pull request has the :octocat: automerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[ghost]

Happy to oblige

[RussKie]

Thank you @jbhensley for you work and your patience.