Improve keyboard navigation #1440
Conversation
manuhabitela
force-pushed
the
issue-1251-keyboard-nav
branch
from
9ce2d5c to
35b7a19
Compare
manuhabitela
force-pushed
the
issue-1251-keyboard-nav
branch
6 times, most recently
from
2826389 to
d531f28
Compare
|
Ok this is starting to get way bigger than I expected (again). I think I'm getting close to something that works but this needs way more actual testing at this point (manual testing I mean for now). |
manuhabitela
force-pushed
the
issue-1251-keyboard-nav
branch
3 times, most recently
from
9c140c4 to
f9ec279
Compare
|
Ok, update! In the end there is quite a bit of code. It's rather contained in its own class though. The whole logic is still similar to what I wrote in the PR description, with new things mostly to handle mouse interactions and fine-tune the details. If you want details, you can check the second commit message, it explains everything. The code should also be rather easy to read, but I know it's starting to get quite busy… The whole idea is:
Help appreciated about grainjs/knockout observables and grist doc loadingOne thing I was stuck on is related to the grist document loading. The RegionFocusSwitcher class knows whether or not it needs to handle the special case of a grist doc view layout, because we can optionally pass it the Thing is, at this point the grist doc/viewLayout is not done loading. So I have some weird
I'm sure the way I load the grist doc in the class is not that great, but oh well, if anyone has 15 minutes to help on that it would be much appreciated. Next steps for meI'll now see where tests break to see if the logic is viable or if it's too… ambitious. Especially the mouse logic of totally disabling view commands on panel click, I fear that this breaks multiples things I don't know about. I didn't test quite enough all interactions, especially when FocusLayer are added/removed. And I'm not sure the idea of disabling view commands when focusing panels is actually what we would want. Any insight is appreciated. |
|
Deployed commit |
|
Deployed commit |
manuhabitela
force-pushed
the
issue-1251-keyboard-nav
branch
from
dcb8a95 to
4d69b18
Compare
|
Deployed commit |
manuhabitela
force-pushed
the
issue-1251-keyboard-nav
branch
from
4d69b18 to
d4441ec
Compare
|
Deployed commit |
manuhabitela
force-pushed
the
issue-1251-keyboard-nav
branch
from
d4441ec to
05ff0ea
Compare
|
Deployed commit |
manuhabitela
force-pushed
the
issue-1251-keyboard-nav
branch
from
05ff0ea to
adeefb8
Compare
|
Deployed commit |
manuhabitela
force-pushed
the
issue-1251-keyboard-nav
branch
from
adeefb8 to
a355820
Compare
|
Deployed commit |
manuhabitela
force-pushed
the
issue-1251-keyboard-nav
branch
from
a355820 to
c481f42
Compare
|
Deployed commit |
manuhabitela
force-pushed
the
issue-1251-keyboard-nav
branch
from
c481f42 to
8993a4e
Compare
manuhabitela
force-pushed
the
issue-1251-keyboard-nav
branch
from
5bc5ad2 to
fce83fe
Compare
|
Deployed commit |
|
Okay, update:
So, what I need to do:
|
manuhabitela
force-pushed
the
issue-1251-keyboard-nav
branch
from
fce83fe to
e760c4c
Compare
1) Disable the clipboard catch-all element when there is no need to enable it (we conclude we don't need it when we see no copy/put/paste command is set). This means it is disabled on most pages except the document view. 2) Make sure interactive elements are highlighted when focusing them with the keyboard. Lots of elements have `outline: none` forced in the codebase to prevent showing unwanted outlines when clicking with the mouse. But it prevents keyboard users to see where they are. Have a tiny JS script checking for keyboard interactions to force showing the outline on actual kb focus.
RegionFocusSwitcher makes it possible for the user to cycle through app panels and document widgets (named "regions" as a whole) with a couple keyboard shortcuts. 1) On a grist document page, pressing the Ctrl+o (or Ctrl+Shift+o) shortcut jumps your keyboard focus from widget to widget, but also panel to panel. You go to widget1, widget 2, then left panel, top panel, then widget 1, widget 2, etc. This shortcut is necessary to reach stuff outside widgets: because by default, usual kb shortcuts like tab or arrow keys are highjacked by widgets and you can't go out ot them. On other pages (that don't have widgets), pressing the shortcut jumps your keyboard focus from panel to panel, but this time it also includes the main content area as a "panel". The shortcut is not _that_ necessary on other pages since you can now use Tab normally. But I guess it's better to keep the shortcut always work whatever the page. 2) The new creatorPanel command allows to keyboard focus from/to the creator panel with Ctrl+Alt+o. This is a separate command from the regions-cycling, as the creator panel is tied to the currently focused viewLayout section. So we must be able to focus a section, and then directly go to the creator panel in order to configure it. If we notice the user just spammed the Ctrl+O shortcut too much, we assume he just tried accessing the creator panel and we show him the keyboard shortcut in a notification. 3) Pressing Esc while on a panel will first focus back the panel, then restore focus to the view layout. 4) The grist doc view now keeps track of this new stuff, in order to disable the view section commands when focusing panels. This is an important trick that lets us enable the Tab key and others when focusing the panels. 5) The new navigation shortcuts are now always on, even in inputs. Using ctrl+o is somewhat problematic because it is the default browser shortcut for opening files, so in cases where we don't catch the keypress, the filechooser appears. We want to avoid that as much as possible. 6) One main trick allowing us to easily allow Tab navigation on panels, while on a grist docs page, is the change in Clipboard.js. We can now set a 'clipboard_group_focus' class somewhere in the dom, and the clipboard will allow focusing elements inside the one having the class. 6) On a grist doc, mouse interactions also drastically impact keyboard navigation: they now silently toggle the view layout commands. If I click on the left panel, it wont actually force a focus change, but it will enable keyboard nav inside the panel. This is not done yet: - needs more actual usage testing to finetune the behavior and make sure it is what we want. - needs tests
we now check if the passed doc observer is actually ready before doing anything
i actually don't understand why I didn't stumble on this sooner but oh well…
- we basically dont want to focus back the current panel with esc, if we didn't go on it via the cycle commands before. just directly reset when we dont come from cycle commands.
before, we listened for clicks anywhere in the document. It seems that some e2e tests do some trickery by specifically clicking on out-of-the-app-ui stuff… Do not interfer when doing that. This is more of a precaution in order to make sure we only do things on purpose.
before, panel content was always focusable. This made some unintended behavior, like clicking a "a" element would keep the focus, and make the `input` command stop working. now if we just click a "a" element inside a non-focused panel, it wont grab focus.
sometimes, we want to run "current view"-related commands via keyboard shortcuts, while we are focusing ie. the creator panel. this commit makes it possible, by distinguishing "globally active" view commands from "only user-focused" view commands. in order to have the "globally active" commands more reliably work, we make sure the active section is focused by the region switcher automatically. it's not an ideal behavior for the user, as he will then need to manually go focus back the panel he was on ; but this is more done as a safety measure to not end up in a state where `body` is focused while we'd expect otherwise.
since most kb-related things in grist views are handled manually with specific js, we dont want a default focus ring on stuff that might get focus inside them. before that, a focus ring was added on celleditor_text_editor for example.
the idea is to fix all the interactions where we start focused on the creator panel and the app ends up focusing the clipboard element by itself, for example when transforming a column type (after applying the transform). In those cases for now we end up in a weird state where the regionFocusSwitcher says we are on the creator panel, but the document.activeElement is the hidden textarea (clipboard el).
- two interactions now need the user to first press esc to get out of current focus before continuing (I'd say an acceptable change?) - sometimes when we want to return on the "app focus", it won't mean the clipboard element anymore
waits were already done here and there in the code but not everywhere, making tests breaking randomly when opening dropdowns
some tests break because we want to click too early on elements not there yet, because they appear in dropdown menus right after clicking on buttons showing them. just wait a bit
- now the RFS is created in the App, I guess it makes more sense here, because it's also where the Clipboard is, and now we can more easilly access it across the app if needed… - …but I have a whole mess when initializing now (more than before), to make sure panelAttrs are applied correctly after the grist doc loaded… i'm sure this can be improved somehow? - no more dirty window.regionFocusSwitcher
not sure this is a good way to handle that, but it was a bit convoluted to keep track of state change initiator in "_currentUpdateInitiator". Now that it's in the state object it's more logical I think. But i'm not sure of the implications of putting a mouse event object in an observable?
more advanced tests are missing, but this is a good start
manuhabitela
force-pushed
the
issue-1251-keyboard-nav
branch
from
e760c4c to
dd5b990
Compare
|
Deployed commit |
Just trying to disable the mouse-specific stuff to see if tests pass or not right now.
manuhabitela
force-pushed
the
issue-1251-keyboard-nav
branch
from
8f33c7e to
fa075c2
Compare
|
Deployed commit |
| const noCopyPasteCommands = copy._activeFunc === _.noop && cut._activeFunc === _.noop && paste._activeFunc === _.noop; | ||
| if (elem && elem.classList.contains('clipboard_forbid_focus')) { | ||
| return false; | ||
| } | ||
| if (noCopyPasteCommands) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
this is a maybe too optimistic assumption to make (i'd say it's okay since I wrote it but just putting some lights on this), but what this means is: if we notice we don't have any copy/paste commands registered, we assume we don't have to steal usual browser focus. This is the main thing resulting in the default browser focus being possible on non-document pages.
| region: undefined, | ||
| initiator: undefined, | ||
| }); | ||
| this._initiated = Observable.create(this, false); |
There was a problem hiding this comment.
The whole way I initialize the RegionFocusSwitcher (RFS) is a bit… convoluted, to say the least. The RFS is initialized in the App.
More info about the requirements of it all:
- the RFS applies classes on page panel dom elements via the
panelAttrsmethod, and those classes depend on whether or not a grist doc is loaded. I get the grist doc via the App page model - the RFS listens for clicks made on the page container
- we listen for clipboard-related events that are fired on the App
I didn't find a clean way to be sure to start everything only once the grist doc has finished initializing the first time, while still being able to access App related stuff.
Note the panel classes need to listen for the grist doc changes, because the classes change depending on whether the grist doc is currently actually shown or the current page is a "special page" like the code view.
This is why I end up with multiple listeners that are just there to start things when stuff has finished loading. And the _initiated internal variable is a listener because I also listen to it in the panelAttrs method.
And to top it of, when the app.pageModel is reset, I manually also reset stuff inside the RFS. This whole thing is not enough though as I still have failing tests when the pageModel gets reset somewhere else and I don't act accordingly. It's just yet another thing showing this is not the correct way to initialize all this.
@dsagal this is my main pain point on this PR, if you or maybe @georgegevoian, @berhalak have any insight on how to initialize this correctly, I'm all ears 🙏
The gist of it is: the RegionFocusSwitcher needs to know if there is a current grist document, and if the view sections are currently shown or not.
| dom.cls('kb-focus-highlighter-group', use => { | ||
| use(this._initiated); | ||
| // highlight focused elements everywhere except in the grist doc views | ||
| if (id !== 'main') { | ||
| return true; | ||
| } | ||
| const gristDoc = this._gristDocObs ? use(this._gristDocObs) : null; | ||
| if (!gristDoc) { | ||
| return true; | ||
| } | ||
| if (gristDoc) { | ||
| use(gristDoc.activeViewId); | ||
| } | ||
| return isSpecialPage(gristDoc); | ||
| }), | ||
| dom.cls('clipboard_group_focus', use => { | ||
| use(this._initiated); | ||
| const gristDoc = this._gristDocObs ? use(this._gristDocObs) : null; | ||
| // if we are not on a grist doc, whole page is always focusable | ||
| if (!gristDoc) { | ||
| return true; | ||
| } | ||
| const current = use(this._state).region; | ||
| // on a grist doc, panel content is focusable only if it's the current region | ||
| if (current?.type === 'panel' && current.id === id) { | ||
| return true; | ||
| } | ||
| if (gristDoc) { | ||
| use(gristDoc.activeViewId); | ||
| } | ||
| // on a grist doc, main panel is focusable only if we are not the actual document view | ||
| return isSpecialPage(gristDoc); | ||
| }), | ||
| dom.cls('clipboard_forbid_focus', use => { | ||
| use(this._initiated); | ||
| // forbid focus only on the main panel when on an actual document view (and not a special page) | ||
| if (id !== 'main') { | ||
| return false; | ||
| } | ||
| const gristDoc = this._gristDocObs ? use(this._gristDocObs) : null; | ||
| if (!gristDoc) { | ||
| return false; | ||
| } | ||
| if (gristDoc) { | ||
| use(gristDoc.activeViewId); | ||
| } | ||
| if (isSpecialPage(gristDoc)) { | ||
| return false; | ||
| } | ||
| return true; | ||
| }), | ||
| dom.cls(`${cssFocusedPanel.className}-focused`, use => { | ||
| use(this._initiated); | ||
| const current = use(this._state); | ||
| return current.initiator?.type === 'cycle' && current.region?.type === 'panel' && current.region.id === id; | ||
| }), |
There was a problem hiding this comment.
in this, I use the cls helper multiple times to apply different classes. But the content of each callback is rather similar, they mostly subscribe to the same observables.
Is there a way for me to write only one callback to apply different classes? I feel this could end up running way less code each time an observable changes.
| return dom.create(WelcomePage, appModel, appObj); | ||
| } else if (pageType === 'account') { | ||
| return domAsync(loadAccountPage().then(ap => dom.create(ap.AccountPage, appModel))); | ||
| return domAsync(loadAccountPage().then(ap => dom.create(ap.AccountPage, appModel, appObj))); | ||
| } else if (pageType === 'admin') { | ||
| return domAsync(loadAdminPanel().then(m => dom.create(m.AdminPanel, appModel))); | ||
| return domAsync(loadAdminPanel().then(m => dom.create(m.AdminPanel, appModel, appObj))); | ||
| } else if (pageType === 'activation') { | ||
| return domAsync(loadActivationPage().then(ap => dom.create(ap.getActivationPage(), appModel))); | ||
| } else if (pageType === 'audit-logs') { | ||
| return domAsync(loadAuditLogsPage().then(m => dom.create(m.AuditLogsPage, appModel))); | ||
| return domAsync(loadAuditLogsPage().then(m => dom.create(m.AuditLogsPage, appModel, appObj))); |
There was a problem hiding this comment.
I didn't update all the pages because it seems some are related to the EE edition but if this gets merged as is, they would need to be updated to benefit from the region focus switcher.
this is yet another thing showing I don't initialize stuff where I should… help appreciated
|
Deployed commit |
Context
See #1251. The gist of it is:
Proposed solution
This tries to handle both the above tasks.
First commit enables classic keyboard navigation when not on a grist doc page. This is done by disabling the Clipboard catch-all system when we conclude we don't need it ; and by forcing visible focus-rings to be applied if we detect keyboard usage. Just doing that is enough, as navigation-related commands that intercept Tab and others are not loaded on pages other than a grist doc.
Second commit is bigger and takes care of implementing a new behavior: pressing Ctrl+o and Ctrl+shift+o, now instead of cycling through widgets, cycles through widgets and page panels. Also, a new shortcut, Ctrl+alt+o, allows to focus to and from the creator panel.
With added logic on the view model allowing to keep track of whether it is the currently "focused panel" or not, we can easily toggle off each view commands when we focus the left or top panel, and toggle them back on when we focus back the main widgets area. This is the trick making the "Tab" key and others usable on panels when viewing a grist document.
This PR also makes sure the Ctrl+o shortcuts always work, even inside inputs, so that we don't open the browser file chooser by mistake when navigating. You could say needing to do that asks the question of why we use this keybinding in the first place (good question), but at least for now making sure it's totally overridden makes more sense in my pov.
Status
This is still WIP:
I'm not that proud of the code written, mostly because I created a tight link between PagePanels and ViewLayout, while they actually don't know about each other. And theedit: code is now much cleaner, new code should mostly remain in the RegionFocusSwitcher class.focusSectioncommand I added is basically used as a global function to do as I please. I am currently rewriting stuff so that it's cleaner.But I think this is overall a pretty good start (close to the finish line actually). I feel like the main idea of the code wouldn't change that much after some cleaning. Some more grist-trained eyes will be appreciated when I'm done for sure!
Impact
Merging this would drastically improve exhaustive keyboard navigation for sighted users, as it directly enables lots of interactive elements to now be focused and triggered through the keyboard.
After this step, some more work would be needed for specific elements and components where things like unfocusable divs are used. But this handles like 70% of the cases (random data out of my magic hat).
Related issues
Fixes #1251.
Has this been tested?
Screenshots / Screencasts
grist-kb-nav.mp4
ping @dsagal if you are curious!