Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undo operation in DemoConsole causes ContextMenuStrip to become unusable #12591

Closed
Nora-Zhou01 opened this issue Dec 5, 2024 · 7 comments · Fixed by #12729
Closed

Undo operation in DemoConsole causes ContextMenuStrip to become unusable #12591

Nora-Zhou01 opened this issue Dec 5, 2024 · 7 comments · Fixed by #12729
Assignees
Labels
💥 regression-preview Regression from a preview release 🚧 work in progress Work that is current in progress
Milestone

Comments

@Nora-Zhou01
Copy link
Member

Nora-Zhou01 commented Dec 5, 2024

.NET version

.NET 10.0 main branch of WinForms repo

Did it work in .NET Framework?

Not tested/verified

Did it work in any of the earlier releases of .NET Core or .NET 5+?

Yes, this is a regression issue. Not repro on .NET 9.0 from Release/9.0 branch of WinForms repo.

Issue description

In the DemoConsole test application in the Winforms repo, the Undo operation causes the ContextMenuStrip to be unusable.

.NET 10

democonsole.undo.main.mp4

.NET 9.0

democonsole.undo.9.0.mp4

Steps to reproduce

Test sample: DemoConsole in Winforms repo

  1. Click the form designer after Type Text for the ToolstripMenuItem in the ContextMenuStrip
  2. Click Undo/Ctrl + Z
@Nora-Zhou01 Nora-Zhou01 added 💥 regression-preview Regression from a preview release untriaged The team needs to look at this issue in the next triage labels Dec 5, 2024
@LeafShi1 LeafShi1 removed the untriaged The team needs to look at this issue in the next triage label Dec 6, 2024
@LeafShi1 LeafShi1 self-assigned this Dec 9, 2024
@LeafShi1
Copy link
Member

LeafShi1 commented Dec 9, 2024

This issue caused by PR #11358

@elachlan
Copy link
Contributor

CC: @kirsan31

@kirsan31
Copy link
Contributor

Will look as soon as I can (most likely on the weekend).

@LeafShi1 LeafShi1 removed their assignment Dec 16, 2024
@Epica3055 Epica3055 self-assigned this Jan 6, 2025
@kirsan31
Copy link
Contributor

kirsan31 commented Jan 6, 2025

After brief look:
The problem is that "Type Here" element is not returned to the collection during Undo.
The problem only occur if ContextMenuStrip not selected (not visible) during undo.

There are a lot of additions and deletions going on there (clearly more than necessary). Need more deep investigation...

---------------------UPD---------------------

"Type Here" element Inserted in this method: ToolStripDropDownDesigner.OnSelectionChanged -> ToolStripDropDownDesigner.ShowMenu -> ToolStripDropDownDesigner.ShowMenu -> ToolStripMenuItemDesigner.CreatetypeHereNode only when _initialized flag is not set.
And _initialized flag is set to false in ToolStripDropDownDesigner.DropDownItem_DropDownClosed which is called from ToolStripDropDownDesigner.OnUndone -> ToolStripDropDownDesigner.HideMenu -> ToolStripDropDownDesigner.HideDropDown method only if _dropDown.Visible is true!
And after #4808 fix _dropDown.Visible is false.
Need more time...

---------------------UPD2---------------------

The problem is in ToolStripDropDown.SetVisibleCore:

CancelEventArgs openEventArgs = new(cancel: DisplayedItems.Count == 0);

After #4808 fix DisplayedItems is empty and therefore we will not show dropdown - _dropDown.Visible is false.
So previously (before #11358) with some conditions we can show empty dropdown (if dropdown is empty but DisplayedItems not). But after fix we can't show non empty dropdown if DisplayedItems is empty.
Need more time...

---------------------UPD3---------------------

The call sequence is:

  • remove test element. This lead to empty DisplayedItems after 4808 fix.
  • ShowMenu -> CreatetypeHereNode
    • add "Type Here" element
    • ShowDropDown. This will not show dropdown after 4808 fix because of empty DisplayedItems and next steps will not execute.
    • SetDisplayedItems
  • HideDropDown
  • ...

So theoretically we can change the order of SetDisplayedItems and ShowDropDown and all will work.
But it's becoming more and more complicated. And I advice to revert 4808 fix.

@Tanya-Solyanik
Copy link
Member

@Nora-Zhou01 - do you see any similar problems in the Visual Studio designer for applications targeting NET9?

@kirsan31
Copy link
Contributor

kirsan31 commented Jan 7, 2025

@Nora-Zhou01 - do you see any similar problems in the Visual Studio designer for applications targeting NET9?

Them shouldn't be there, the problem is in the fix described above (main branch). And I suggested to roll it back.

kirsan31 added a commit to kirsan31/winforms that referenced this issue Jan 8, 2025
kirsan31 added a commit to kirsan31/winforms that referenced this issue Jan 8, 2025
Tanya-Solyanik pushed a commit that referenced this issue Jan 8, 2025
Fix #12591.
This reverts #4808 fix commit from #11358 PR.

The disposal of DisplayedItems after removal has already been implemented in DoLayoutIfHandleCreated but only if handle was created. So we have a leak only if handle is destroyed (#4808 case) and we didn't take this into account.
ricardobossan pushed a commit to ricardobossan/winforms that referenced this issue Jan 9, 2025
…et#12729)

Fix dotnet#12591.
This reverts dotnet#4808 fix commit from dotnet#11358 PR.

The disposal of DisplayedItems after removal has already been implemented in DoLayoutIfHandleCreated but only if handle was created. So we have a leak only if handle is destroyed (dotnet#4808 case) and we didn't take this into account.
@John-Qiao
Copy link
Member

Verified this issue on winforms repo from main branch, it has been fixed: the ContextMenuStrip works well after clicked Undo.

testresult.mp4

@MelonWang1 MelonWang1 added this to the 10.0 Preview1 milestone Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💥 regression-preview Regression from a preview release 🚧 work in progress Work that is current in progress
Projects
None yet
8 participants