-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add keyboard controls for SiteSelector #5808
Conversation
c223d51
to
06209ba
Compare
Nice work @marekhrabe! It seems to be working rather well. One thing I noticed is that if I press the down arrow in the sidebar picker to move the highlighted site, and then wait, it eventually resets to the All Sites item being highlighted. |
@@ -100,24 +101,6 @@ | |||
} | |||
} | |||
} | |||
|
|||
// Highlight selected site | |||
&.is-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.
The behaviour of the sites dropdown is a bit awkward now, it should highlight the currently selected site when you open the dropdown, not the first in the list.
Ok, this seems to be related to the change "the first item is highlighted (previously currently opened site was highlghted)". What was the reason for that change? |
I don't think this is a good change. What was the reasoning for it? The picker works much better without search when you have only a few sites. Search should be an enhancement to navigation only when it makes sense. |
I suspect one of my latest changes to cause that (bc95cda1f8cd87c2414a359ebde40de805044e3f). I was able to reproduce and I'll push a fix for that soon.
I think this might be truer for the previous version than this one. Previously, the search was something additional and optional that you could use and then you had to come back to your mouse to confirm your selection. This time, I tried to make the search a core feature of new selectors. Even if you have 4 sites, I feel like having the ability to type, for example, just the first letter and hitting Enter is faster than arrow navigation through the list. At least that's how I use this kind of components. My interactions with Sublime's/Atom's Go to anything were my inspiration.
(Btw: that CSS has nothing to do with that. It was some legacy code that was not used, as selected sites were completely hidden in Dropdowns.) The reasoning was that if you start with the selected site, you might run easily into edge cases:
I thought that the most reasonable way (and working in all cases), is to select the first one. When there are more solutions, I aim for predictability and consistency in all scenarios and this was the case. As a little technical bonus, it allows us from having a double render of SiteSelector (see 329b83fa41dbf5dcacfe8a8a57f987da8025b71b), where we first need to render sites to know their order and after they are rendered, determine which one to highlight, save that to the state and rerender. Anyway, there is probably a way around that, but this is the most simple solution and they usually tend to work the best. The only component that is really affected by this change is probably the picker (left sidebar "Switch Site"). It behaves a little different than the previous implementation, but I don't think it is a bad thing. You still do? I think that users mainly use that component to change the current site, not to see what is selected. Thanks a lot for finding time to review this, @mtias. I would love to continue the discussion on points that I explained here if they change your point of view in some way. |
I've tested this and it seems to work well 👍 |
Which one do you prefer, @alisterscott? The Chrome one feels better to me. It works like that probably because Webkit implements (non-standard, but commonly used) Edit: I implemented cross-browser version that is now consistent across Chrome, Firefox & Safari that I tested. |
06209ba
to
8a8348c
Compare
The bug @mtias mentions should be fixed now. It now resets to the first available item only once you open the sidebar. Not in the middle of interactions. We can now discuss further if we want to change the behavior. |
Note that the fact that previously you had to use the mouse was an incomplete implementation of the design, so it was already meant to be disappearing when less than 6 sites AND still be triggered with enter. "Faster" here is a relative thing. We're talking of very very few keystrokes, and it's again an edge optimization that is... very edge: less than 6 sites and power user. Even there, just arrow down a few times and you're done. We're talking of a switch time still likely around the second or two. Given the marginal gain, I wouldn't add the extra UI for users with less than 6 sites. The search examples you make (Sublime / Atom) are geared for people with terminal experience, text-heavy and in an interface primarily textual. UIs are designed around users. So, in absence of any heavier weighting that makes tip the decision for search, I'd keep the optimization we have for a less cluttered UI, even if it costs a few milliseconds to an expert user.
The two edge cases aren't really issues. If the active site isn't in the list, we don't highlight anything. It's meant to work that way (plus we have an iteration 2 of the The reason we highlight the current site is acting as a reference point: "this is where you are". It's a relevant marker, and has a specific function. Also: there are two highlight statuses already to cover the cases: 1 — Highlight as selected — the dark background: 2 — Highlight as focus — the light background Keyboard navigation is a focus kind of highlight, so shouldn't touch or change the selected state. |
When you suggest I change the keyboard highlight color to the light colored version (the same as a mouse hover), don’t you think that might look confusing when combined with those mouse hovers? In one moment, you can have “selected site”, highlighted keyboard focus, and mouse hover in one component. I don’t say that it’s a common situation, but it might look funny and it might not be apparent what would be confirmed by pressing enter. What do you think? |
Someone moving with the keyboard and then using the mouse, and while hovering pressing enter seems super edge case. It could be addressed just by being stateful if the interaction is triggered by mouse (once you hover, the keyboard focus is lost) or keyboard (once you use the keyboard, the mouse hover gets overridden). Doable, but imho not worth the effort given the likelyhood of the issue. I'd just go for the double highlight for now (which I think it's going to be the outcome if we don't do anything special). |
That sounds reasonable, on it :) |
I opted for keeping the highlighted site state as index, instead of site object. It makes the state managment much more straightforward and prevents double-renders that we previously needed (first render to determine visbleSites, second to set first item as highlighted).
Correct css rules have been moved to SiteSelector and there is no need to repeat them.
f1d1bcc
to
efbccae
Compare
Testing this in Chrome and Firefox. In Chrome, Esc clears the search field when searching in the sidebar. In Firefox, Esc does not clear the search field. |
@ryanboren As far as I know that's a difference in browser implementation for the Can you double-check it from this demo? |
I can confirm that latest Firefox does not clear the search input on the demo page @folletto shared. I have same results with WordPress.com + Firefox = ESC does not clear the input. There is an issue for that #5771 that I think we should reopen, as it still describes a present problem that was not fixed. |
This PR adds keyboard support to every place where you can select sites. That means Up & Down arrows to move through sites in list, Enter to select and also Esc to close without changing a site.
These new keyboard features go well together with searching functionality to narrow results and that's why keystrokes are being listened directly on the search input.
Here is a quick preview of the implementation:
Testing Instructions
Places to test
SitePicker
)SitesPopover
)SitesDropdown
)SiteSelector
)Each place should follow these rules
Naming conventions
This is what "highlighted" site looks like. Site can be highlighted by navigating with arrow keys or by hovering on site with the mouse cursor.
This is what "selected" site looks like:
#### Altering between keyboard and mouse
I modeled those interactions after native system menus.
Test live!
Test live: https://calypso.live/posts?branch=add/site-selector-keyboard-accessibility
Closes #8
Closes #6024