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

Tab between TextBoxes #606

Open
psFried opened this issue Oct 27, 2015 · 8 comments
Open

Tab between TextBoxes #606

psFried opened this issue Oct 27, 2015 · 8 comments

Comments

@psFried
Copy link
Contributor

psFried commented Oct 27, 2015

I think it would be nice to be able to move between text boxes just using the keyboard by hitting tab or enter. I've got something hacked together that works, but it's really ugly, and it seems like there must be a better way. Can anyone think of a non-hacky way to implement this?

Ideally, something like Ui::force_focus_on(&mut self, idx: Into<Index>) would be possible, where idx would refer to the widget to focus on. Something like, TextBox::steal_focus(mut self) would also work. That is about what I have right now, but like I said, it doesn't feel like the cleanest implementation. Any ideas for a better approach?

@psFried
Copy link
Contributor Author

psFried commented Nov 6, 2015

I opened up this PR as a first draft of how this could work. Currently, this works as follows:

let mut focus_next: Option<WidgetId> = None;
TextBox::new(&mut my_string)
    .react(|new_val| {
        focus_next = Some(SECOND_TEXT_BOX);
    })
    .set(FIRST_TEXT_BOX, ui);

// ...

if let Some(next_widget) = focus_next {
    ui.change_focus_to(next_widget);
}

This works, but it doesn't seem like a really good solution. It requires people to manually manage which Widget will take focus. I think it would be better if the Ui could just handle that. Also, right now there are a bunch of warnings printed to stdout due to TextBoxes trying to uncapture when the next TextBox has already captured the keyboard. We could solve this just by removing the warning, but it seems like there's a cleaner solution for handling user input that would make all this simpler...

@bvssvni
Copy link
Member

bvssvni commented Nov 6, 2015

In Window Forms this is solved by assigning each control a tab order index.

@mitchmindtree
Copy link
Contributor

Perhaps we could add a method to the UiCell that can be called like ui.capture_next_of_kind("TextBox") from within the Widget::update?

This way we could just check for a tab input and then call ui.capture_next_of_kind("TextBox") and then the Ui would change the capturing from the current widget::Index to the next index that has the kind "TextBox", cycling back to the first index when the last is past.

Edit: We could take this further too by adding methods such as ui.capture_next_in_direction(Direction::Right), etc.

@psFried
Copy link
Contributor Author

psFried commented Nov 6, 2015

@mitchmindtree I like that idea! Definitely adding whatever method to UiCell makes sense, too. Doesn't seem like Widgets should really ever interact with the Ui directly.

I'm thinking, though, that the 'kind' of widget shouldn't really matter. I like the idea of either using the Widget Index, or else just defining the 'next' widget explicitly. Maybe something like:

TextBox::new(&mut my_string)
    .next_widget(NEXT_WIDGET_ID)
    .react(|new_str| {...

Then, when the user hits tab, the TextBox can just call UiCell::change_capture_to(NEXT_WIDGET_ID) or something like that?

@psFried
Copy link
Contributor Author

psFried commented Nov 6, 2015

@bvssvni Scratch I like this idea, too. Just assigning a tab order sounds better. Then the widget graph could just keep track of all the tab orders and provide the next one on request. At first blush, I'd almost be tempted to use the widget index itself as the tab order. Maybe I'll start off using the widget index and then change it to a separate tab order once the proof of concept looks good.

@psFried
Copy link
Contributor Author

psFried commented Jan 3, 2016

I finally got around to really working on this, and it's going to be a rather large pull request when it's done. I've struggled to find a clean way of implementing this without getting mired in refactoring how widgets capture/receive keyboard/mouse input. Currently, widgets such as the TextBox are responsible for calling UiCell::capture_mouse when they want to handle drag events. This is because the user could drag slightly outside of the widget's rectangle and the widget would no longer receive the input in the UpdateArgs. This means that the widget is also effectively storing the state of whether it is currently capturing the mouse/keyboard so that it can also know when to call UiCell::uncapture_mouse/keyboard. So, if the Ui changes who's capturing the keyboard behind the scenes, then we end up with inconsistent states. The next update cycle, a widget that thinks it's capturing the keyboard now isn't anymore. This isn't technically a huge deal, but it definitely doesn't feel right either.

So, there's two parts to the idea. One, is that whether a widget is capturing the keyboard/mouse should be solely determined by the Ui. Currently, the Ui passes input to the widget, which then inspects the input and determines if it should ask to capture the keyboard or mouse, then the ui checks if something else is already capturing and makes the final decision. I think this could be simplified if the ui always just gives keyboard/mouse input to whichever widget has been clicked on.

The second piece would be to simplify how widgets receive input, from the mouse especially. Each widget is not only storing the state of whether it is capturing the keyboard/mouse, but it's also storing the state of the mouse itself. This also seems wrong. I've started adding semantic mouse events to the Mouse itself, which seems like it will result in a lot of code being removed from the widgets themselves. This currently looks something like:

pub enum SimpleMouseEvent {
  Click(ClickInfo),
  Drag(DragInfo),
  Scroll(ScrollInfo),
}

The Mouse then has a method simple_event_iterator() that returns an iterator over the events since the last update. So, all a widget has to do is something like:

let input: UserInput = update_args.input();
let mouse_events = input.maybe_mouse.iter().flat_map(|mouse| mouse.simple_event_iterator());
for event in mouse_events {
  match event {
    Click(click_info) if click_info.mouse_button == MouseButton::Left => {
      handle_left_click_at(click_info.position);
    },
    _ => {}
  }
}

This finally gets us to the point where we can just have two simple boolean fields on the UpdateArgs struct that tells the widget whether it is currently capturing the keyboard/mouse. Of course at this point, actually capturing the mouse should only be necessary in rare circumstances. Most widgets should just be able to handle the SimpleMouseEvents and not have to worry about anything more complex than that. This is because Drag events would automatically be sent to the widget that was under the starting point of the drag. So, for the example of the TextBox needing to capture the mouse in order to ensure that it still gets mouse events when the user drags slightly off the text area, this is simply handled by the Mouse. The only widget I can think of that should need to capture the mouse is the DropDownList. This is because we want the list to expand when a user clicks and stay expanded until they select an item or click somewhere else.

It seems pretty weird to start by refactoring mouse input in order to handle tabbing between text boxes, but I really think that it's the right way to proceed. I had two different branches with working proof of concepts for tabbing between boxes, but they both seemed to be adding too much complexity to the whole capturing/uncapturing flow.

Once I have the input refactoring finished, I'll put up a pull request for that before I actually finish the tabbing between text boxes functionality. All said, it should end up getting rid of a lot of code in some of the widgets for handling mouse input. Another side benefit of the Mouse refactoring is that it will enable using all the available buttons on the mouse instead of just left, right, middle, and unknown. Should also make it a lot easier to handle double-clicks and hovers on down the line. Here's a link to the branch I'm working on. Let me know your thoughts on the matter.

@bvssvni
Copy link
Member

bvssvni commented Jan 3, 2016

@psFried Sounds nice! After making design changes, I often open up an issue and label it with "Information" to make it easy to find later.

@bradacina
Copy link

In the mean time, could we have a new event in TextBox Event::LostFocus and emit it when we detect a Tab key press just like Event::Enter is being emitted?

input::Key::Return => events.push(Event::Enter),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants