Make searchbar more keyboard-friendly #1752
Conversation
aa0072c to
2f62f04
Compare
c5a3079 to
ae81a21
Compare
|
This is ready to review on my side :) @fflorent @hexaltation if you wanna see |
|
Deployed commit |
| ) | ||
| ), | ||
| dom.onKeyDown({ | ||
| Escape$: () => { |
There was a problem hiding this comment.
Why is there a $ here?
Does it act like a regex or is it a typo for the key code Escape?
There was a problem hiding this comment.
It's a grainjs feature:
By default, registered keyboard events are stopped from bubbling with stopPropagation() and preventDefault(). If, however, you register a key with a "$" suffix (i.e. Enter$ instead of Enter), then the event is allowed to bubble normally.
There was a problem hiding this comment.
But your question shows I should add a comment here explaining why we don't want to stopPropagation here, you are right it's not obvious 👀
edit: added a comment to clarify
ae81a21 to
022983b
Compare
|
Deployed commit |
|
Thanks for working on this @manuhabitela. The |
022983b to
c22ce57
Compare
Woops, good catch thanks. I made a tiny change to make sure they work. |
|
Deployed commit |
|
Thanks for the quick fix. One other thing is that clicking the previous/next buttons or the search all pages checkbox no longer focuses the input. We have some tests not yet in this repo. Let me see about moving them here so you have more context. |
That one I kinda removed intentionally, felt like sticking with usual browser behavior was a good thing part of the cleanup. But I added it back with latest commit. This only applies when using the mouse in order to not mess up with keyboard nav where this behavior would be a bit weird :) |
00b2928 to
cfbebcc
Compare
|
Deployed commit |
cfbebcc to
0d2f84e
Compare
|
Deployed commit |
0d2f84e to
785457a
Compare
|
Deployed commit |
|
This should be better now! I tried something in the last commit, what do you think? 785457a |
|
LGTM. Thanks @manuhabitela!
Ok with me. Let me check with the rest of the team. |
|
@manuhabitela there's one subtle regression, I think. Before, if you had some cells selected it a table when you started searching, you could remove the selection after finishing searching by hitting Escape once more. Now, it doesn't do anything, and the selection remains until you paste it somewhere. |
these few added methods will help outside code react to the current region changing. From outside the RFS, we can now: - change the current region by simply giving a region id (no need to understand the internal structure of the state object) - listen to current region id changes - get the current region id
trapTabKey internally uses a really helpful function to determine if a given element is actually focusable or not (by checking not only the tag or attributes used on the el, but also if it's visible or not). this will be useful to have as a stand-alone util rather than it being kinda hidden in the trapTabKey code.
There was a bug in the RFS code. Let's say: - you focus the top panel with Ctrl+O - you tab through items and end up focused on a button, lets say "share" - you switch current region by clicking the main widget - the "share" button gets hidden for whatever reason. Still in the dom, but invisible - you use Ctrl+O again to go back on the top panel There, there was an error: we tried to focus back an unfocusable element because it was no longer visible. Now we correctly check if we can actually focus the element before focusing it, otherwise we focus back the panel element itself.
before: - we couldn't focus the search input when using the tab key after using Ctrl+O shortcut to navigate in the top panel - we couldn't navigate after the search button when using the tab key after using Ctrl+O. The navigation was stopped when arriving on the search input and it blocked users from accessing focusable elements appearing after the search bar, in the top panel (for example the share button) - we couldn't use the Tab key to focus the search bar items (the "all pages" checkbox, the arrow items, the close button) Now: - nav is not blocked anymore, we can access items after the searchbar when using Ctrl+O to focus the top panel (most blocking issue) - nav with Ctrl+O then Tab is working as expected to focus the search input - tabbing through items in the search bar works, we can toggle the "all pages" checkbox with keyboard only and focus the arrow and close buttons
as they were before, we make sure those are attached on the input itself so that they work correctly, not the wrapper div
this reapplies previous behavior before changing searchbar keyboard behavior, making sure to only do it when using mouse, in order to not disturb keyboard/screen reader users
when checking "search on all pages", the searchbar closed when switching page because it listened for the RFS changes and called `toggleMenu(false)`. Change the logic so that the searchbar stays open on page changes if we detect that it was opened before, + add a custom outside clicks listener to properly handle closing the searchbar when we dont want it anymore.
This is already doable with escape… But. Before, we could just press Tab to exit the searchbar easily. Now, pressing Tab correctly tabs through items next to the searchbar (like the "search all" checkbox). So this quick way of getting out the searchbar is gone. I felt like exposing a "submit search" action through Mod+Enter was a good idea, it seems it better fits the intent than "escaping" search bar. But I guess this is a bit nitpicking, we could also do without this?
|
What would Grist do without you George! Sorry about that. Working on that right now. |
hopefully this helps better understanding things
785457a to
28d89b6
Compare
|
Deployed commit |
This is a workaround exposing an issue that can happen with keyboard handling: When the RegionFocusSwitcher (RFS) focuses the ViewLayout, the ViewLayout registers its commands again, making those commands the "current" commands attached to the keyboard shortcuts. So, if before focusing the ViewLayout, we had registered commands for stuff that we would like to actually keep as "current" even after focusing back the ViewLayout, we are not happy, because they get overriden by the ViewLayout commands. Here we decide to manually handle the escape key of the ViewLayout so that it's only registered when we detect a maximized leaf, to fix a specific issue related to this. The issue was: - I select cells in a grids. cancel command is now clearSelection from the GridView - I then open the searchbar. RFS focuses top panel, - I then press Esc. The searchbar closes, RFS focuses the ViewLayout, making ViewLayout commands be the current ones, including the "cancel" one - I then press Esc again, expecting to clear my selection. I can't because the current cancel command is the ViewLayout's, basically doing nothing when not maximized. Now: - I select cells in a grids. cancel command is now clearSelection from the GridView) - I then open the searchbar. RFS focuses top panel, - I then press Esc. The searchbar closes, RFS focuses the ViewLayout, making ViewLayout commands be the current ones, except for the "cancel" one, because it activates only when the view is maximized, - I then press Esc again, expecting to clear my selection, and my selection is correctly cleared.
28d89b6 to
c3f57ef
Compare
|
Sooo the issue was kind of tricky, I explained it in details in the commit message, I'm not sure it warrants a novel as comments in the code as it's rather specific. I'm not that fond of this workaround but I feel it's okay as I don't think we should encounter too similar many problems in the future. And I'd say the fix is rather good as it makes more sense to register the view layout "cancel" command only when necessary anyway. What do you think? |
|
Deployed commit |
I'm ok with the workaround. Thanks for tracking down the issue and fixing it. |
|
Thanks for merging George 🎉 |
Context
The document search bar in the top panel has a few issues:
Proposed solution
The idea behind this PR is:
Has this been tested?
I did not add any specific test ; the few current tests making searches still pass. Not sure taking time to carefully test behavior is a must add here, I can add some if you think we must :)
Screenshots / Screencasts
Before:
searchbar.before.mp4
After:
search.mp4