Skip to content

Revisit v2 Cancelable Event Pattern - was KeyDown and MouseEvent no longer fires for Listbox #3950

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

Open
tznind opened this issue Mar 2, 2025 · 7 comments · May be fixed by #3955
Open

Revisit v2 Cancelable Event Pattern - was KeyDown and MouseEvent no longer fires for Listbox #3950

tznind opened this issue Mar 2, 2025 · 7 comments · May be fixed by #3955
Labels
bug v2 For discussions, issues, etc... relavant for v2
Milestone

Comments

@tznind
Copy link
Collaborator

tznind commented Mar 2, 2025

Describe the bug

Works for arrow keys but not letters

Issue is probably that keybindings, mousebindings and collection navigator consume the events before the users event.

I think that when the user has an explicit event tied to these then they should get priority.

Application.Init();

var w = new Window()
{
    Title = "Showcase"
};

var lv = new ListView()
{
    Width = Dim.Fill(),
    Height = Dim.Fill(),
};
w.Add(lv);
lv.SetSource(new ObservableCollection<string>(["cat","fish"]));

lv.MouseEvent += (_, e) =>
{
    if (e.IsSingleClicked)
    {
        // Also never happens
        MessageBox.Query("hey", "ok", "ok");
    }
};

lv.KeyDown += (_, e) =>
{
    MessageBox.Query("hey","ok","ok");
};

Application.Run(w);
Application.Shutdown();

To Reproduce
Steps to reproduce the behavior:

  1. Click into list
  2. press any letter key

Expected behavior
enter callback

Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

  • OS: [e.g. iOS]
  • Browser [e.g. chrome, safari]
  • Version [e.g. 22]

Smartphone (please complete the following information):

  • Device: [e.g. iPhone6]
  • OS: [e.g. iOS8.1]
  • Browser [e.g. stock browser, safari]
  • Version [e.g. 22]

Additional context
Add any other context about the problem here.

Set Project & Milestone
If you have access, please don't forget to set the right Project and Milestone.

@tznind tznind added bug v2 For discussions, issues, etc... relavant for v2 labels Mar 2, 2025
@tznind tznind changed the title KeyDown no longer fires for Listbox KeyDown and MouseEvent no longer fires for Listbox Mar 2, 2025
@BDisp
Copy link
Collaborator

BDisp commented Mar 2, 2025

Do you mean the user can by pass or change the ListView navigation behavior if he want?

@tznind
Copy link
Collaborator Author

tznind commented Mar 2, 2025

Yes, l think event should always have first priority since user can put anything in there.

BDisp added a commit to BDisp/Terminal.Gui that referenced this issue Mar 3, 2025
@tig tig changed the title KeyDown and MouseEvent no longer fires for Listbox Revisit v2 Cancelable Event Pattern - was KeyDown and MouseEvent no longer fires for Listbox Mar 3, 2025
@tig tig added this to the V2 Beta milestone Mar 8, 2025
@tznind
Copy link
Collaborator Author

tznind commented Apr 15, 2025

For ListView the code for collection navigator is in:

protected override bool OnKeyDown (Key a)

This means that even keybindings don't get priority over navigation, the ordering is controlled in base class so cannot be simply fixed for ListView alone:

// This is where navigation happens
if (RaiseKeyDown (key) || key.Handled)
{
    return true;
}

// This is where keybindings happen
if (InvokeCommands (key) is true || key.Handled)
{
    return true;
}

View.Keyboard.cs

This means that even this test fails:

[Fact]
public void ListViewCollectionNavigatorMatcher_KeybindingsOverrideNavigator ()
{
    ObservableCollection<string> source = new () { "apricot", "arm", "bat", "batman", "bates hotel", "candle" };
    ListView lv = new ListView { Source = new ListWrapper<string> (source) };

    lv.KeyBindings.Add (Key.B,Command.Down);

    Assert.Equal (-1, lv.SelectedItem);

    // Keys should be consumed to move down the navigation i.e. to apricot
    Assert.True (lv.NewKeyDownEvent (Key.B));
    Assert.Equal (0, lv.SelectedItem);

    // There is no keybinding for Key.C so it hits collection navigator i.e. we jump to candle
    Assert.True (lv.NewKeyDownEvent (Key.C));
    Assert.Equal (5, lv.SelectedItem);
}

@tig
Copy link
Collaborator

tig commented Apr 15, 2025

For ListView the code for collection navigator is in:

protected override bool OnKeyDown (Key a)
This means that even keybindings don't get priority over navigation, the ordering is controlled in base class so cannot be simply fixed for ListView alone:

// This is where navigation happens
if (RaiseKeyDown (key) || key.Handled)
{
return true;
}

// This is where keybindings happen
if (InvokeCommands (key) is true || key.Handled)
{
return true;
}
View.Keyboard.cs

This means that even this test fails:

[Fact]
public void ListViewCollectionNavigatorMatcher_KeybindingsOverrideNavigator ()
{
ObservableCollection source = new () { "apricot", "arm", "bat", "batman", "bates hotel", "candle" };
ListView lv = new ListView { Source = new ListWrapper (source) };

lv.KeyBindings.Add (Key.B,Command.Down);

Assert.Equal (-1, lv.SelectedItem);

// Keys should be consumed to move down the navigation i.e. to apricot
Assert.True (lv.NewKeyDownEvent (Key.B));
Assert.Equal (0, lv.SelectedItem);

// There is no keybinding for Key.C so it hits collection navigator i.e. we jump to candle
Assert.True (lv.NewKeyDownEvent (Key.C));
Assert.Equal (5, lv.SelectedItem);

}

Two things:

  1. In tests, avoid calling NewKeyDown on views. Instead use Application.RaiseKeyDownEvent. This ensures key bindings are sent to the right view.

  2. I think the problem is that the way PopupAutocomplete is implemented is fundamentally broken. It injects itself into the LV's subview heirachy as a subview. Then it uses custom drawing to draw outside of the LVs Viewport (which is supposed to be verboten in v2). And it has custom nav logic that DUPLICATES a ListView's functionality.

Instead, based on the new Popover design, it needs to be refactored.

The thing that shows the list of suggestions should just be a ListView, Registered with Application.Popover and shown via Application.Popover.Show when appropriate.

I believe this will fix this issue.

@tig
Copy link
Collaborator

tig commented Apr 15, 2025

That said, I do have this TODO in my latest MenuBar PR:

Image

@tznind
Copy link
Collaborator Author

tznind commented Apr 15, 2025

I think this is unrelated to popups, at least as far as the test above. It is about KeystrokeNavigator having precedence over both Keybindings and user KeyDown event handlers...

KeystrokeNavigator is the thing that moves selection when you press key i.e. to the next object that begins with that letter

@tig
Copy link
Collaborator

tig commented Apr 16, 2025

See here: #4027 (comment)

The fix does not belong to this Issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug v2 For discussions, issues, etc... relavant for v2
Projects
Status: No status
3 participants