-
Notifications
You must be signed in to change notification settings - Fork 198
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
Remove eager selection of select option with arrow keys #855
Conversation
I agree that a change shouldn’t fire until the user has purposefully chosen an option, but with current select elements the displayed option in the triggering element does change. And that can be useful to keep. |
Does that typically happen for userland select menus? I feel (nothing more than a feeling) like I haven't seen that behavior anywhere but the builtin |
We resolved in the issue in the commit message to not do this, right? |
I feel like the preview should show the actually selected option. If it was to change with navigation but then disappear with escape (or clickoutside) I would find that confusing personally. But like mason that's not based on anything more than intuition and not having seen it in many custom implementations. |
I think it's also important to point out that the above statement - that the displayed option in the triggering element does change - is a Windows-specific observation. It is not true, for example, on a Mac. For one, the popover covers the in-page element. But if you're careful, you can observe what's underneath it (e.g. via sharing your screen in a Meet, because that does not share the popover) and you can see that it does not change as you use arrow keys to change options in the popover. Certainly a |
Yeah I thought we basically agreed to do the mac behavior. This is what I implemented in the chromium prototype if you look in canary: https://jsfiddle.net/xhsofq2z/4/ |
i'd submit you don't typically see this in most custom implementations similarly to why you don't see the ability to start typing chracters and have selection/focus move to the option that matches the typed characters - i'd submit it's an overlooked detail (which can be useful if the selected vs focused option has poor visual indicators) rather than a purposeful omission. Regardless, I'm not going to strongly advocate that we do this. I just didn't think that the decision to not have selection follow focus had to mean this behavior was off the table too. But if people don't see the value in it, then we can move on. Separately though, @josepharhar, i noticed in the demo you posted some oddities. rather than get into those here, i'll post a new issue |
So what is the next step to get this PR unblocked? @scottaohara said he won't push back here, the change aligns with the resolution in the issue. I'm going to approve and the issue can be re-opened if someone wants to change it. |
@josepharhar @lukewarlow friendly ping on this |
61a32e7
to
d23980d
Compare
I was concerned about #1087 and this contradicting each other but it seems that the desire is still to not do selection follow focus so this change is correct. Correct me if I'm wrong @josepharhar ? |
Given the issue I just mentioned though I think this probably does need some wider discussions. Specifically where do we draw the line between "platform convention" and actually having a good behaviour 😅 Either way I think we should remove this text as it's not currently the plan and won't always be correct regardless. |
This is correct, I approve this PR |
Fixes #742