-
Notifications
You must be signed in to change notification settings - Fork 309
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
controller/keyboard navigation support for <select> #566
base: master
Are you sure you want to change the base?
Conversation
- selectbox now scrolls when nav is being used - selectbox ensures that the selected option is scrolled into view, anchored to the top - "escape" can be used to cancel selection
80fdc9f
to
a34a52a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request!
I think scrolling inside select boxes is one of those things that haven't been tested a whole lot, nice to see it getting some needed attention.
I don't believe it is justified to diverge from the HTML behavior in terms of change
events and escape key. I think the current behavior here useful in many situations. So my suggest ion is to remove the value_last_selected
logic.
When testing this PR, I am getting quite some quirky behavior: The selection box keeps closing on me. Sometimes just when pressing arrow keys or when selecting an option. I tested this on the demo sample.
Another behavior that I noticed is that the scrollbar keeps flashing on and off as I press arrow keys to select new options. However, I see that this occurs in master too, so it is unrelated to this PR. But maybe you could take a look at that while you're at it? :)
selectbox now scrolls when nav is being used
Can you elaborate on this?
SetSelection(GetOption(value_last_selected), true); | ||
value_last_selected = -1; | ||
} | ||
ShowSelectBox(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think hiding the box is perfectly fine here. However, I don't think we should set the selection back. When we change the selected option (with arrow keys or otherwise), we commit it and send change
events. I don't think reverting that change is the right thing for escape. And as you mention too, web browser also seems to keep the selected value.
I think this behavior you want is better implemented locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove this from the PR. I kinda wish the change itself was deferred, though, because scrolling through the list on controller emits a ton of change events (they have to "focus" every element to get the active one). This is how browsers handle it, too ("change" isn't emitted until you actually press enter or click on an option), so this is one way we're currently diverging from browsers (the escape thing I can live with, especially if this is implemented).
if (selection != -1) | ||
{ | ||
Rml::ScrollIntoViewOptions scrollOptions { | ||
box_layout_dirty == DropDownBoxLayoutType::Open ? Rml::ScrollAlignment::Start : Rml::ScrollAlignment::Nearest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on the box_layout_dirty condition here, why do we need that?
Can you explain the motivation behind using Start
? I wonder if using Rml::ScrollAlignment::Nearest
always is the more appropriate choice. Especially when scrolling up it feels weird that it keeps selection at the bottom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a matter of preference thing, when I was using Nearest it felt awkward to have the selection start at the bottom of the list. Might also depend how you have the box styled or something. Looking at how browsers handle it, though, it seems they are using (the equivalent to) End on open (opening the box with a selection that has a scrolling area always puts it at the bottom of the list) rather than Nearest. I'll have to play more with this.
Nearest is used when scrolling normally, though, so the selection should 'float' like it does in browsers. At least that's the behavior I experienced when testing it locally (it won't scroll until the edges are reached): https://streamable.com/rwiyew
bool box_visible; | ||
int value_last_selected; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This member should be initialized.
I've not seen either of these locally, oddly enough. If I run into them I'll certainly fix them. I'll check the demo sample to see if I can repro it there.
The actual scroll box moves now if you are using KI_UP/KI_DOWN. Before, the box would just stay where it was, meaning you could not read the option being selected. |
Converting to draft for now as I won't have time for a while to poke these. |
I understand. I haven't been able to follow up on PRs and issues as much as I'd like lately, so things might have been a bit slow from my side too. There are some nice improvements here, so I hope you are able to resume at some point. But of course at you own discretion, cheers. |
Yeah I'm hoping to come back to this in a few weeks - just a lot of projects going on. I shall return, though! (EDIT: also super excited about the Filters/Effects PR seemingly being ready for merging; I've had my eye on it for a while!) |
Alright, I'm ready for this to get reviewed again. I removed the escape key functionality from this PR, but I still recommend we look into this; I'm using this functionality in our games and it's absolutely vital that escape/Back Button cancels the select box, because otherwise it switches menus which is not the behavior a user will expect from hitting escape/Back with a dropdown open. I can open it as a separate PR and we can figure out the details. I addressed the feedback that was left unaddressed before. Let me know what you think! |
Thanks for getting back at this one, very much appreciate that! I'll have to hold this one for a bit, right now I need to focus on the upcoming release, which hopefully is not too far out. After that I'll get back to this and the other PRs, cheers. |
This is a PR to address #565 as well as some other things I ran into while using <select>.
KI_ESCAPE can now be used to cancel a selectbox operation - this is currently done very naively, by simply storing the index of the current selection on open and restoring it on KI_ESCAPE if it is set (any change to the options list while it is open will remove the ability to cancel selection). This is a break from HTML standard behaviors, but I needed it for my project on all select boxes and it seemed like something that might be useful for others too (especially for something like a video resolution selector; in games people tend to expect a way to cancel a select box, esp on controller)(The third point was removed in the PR, but keeping this block below for posterity)
The third 'fix' is messy, and I wonder if diverging from the way browsers handling it is worth looking into:
this is a bit more than I wanted to shove into a single PR and wanted to discuss it further before I commit to changing any behaviors like that though.