fix: Fix tab focus when other elements are displayed next to text that are within a focus trap #5281
Conversation
juliusknorr
requested review from
max-nextcloud,
JuliaKirschenheuter,
ShGKme and
szaimen
|
Additional related question that came up while testing: Do we need a active focus indicator for the actual content editable? Currently that does not have any indicator except for the cursor position Something like this would be easy to achieve but I'm unsure as this is quite a visual design change cc @nextcloud/designers 
|
|
Checking the cypress failure |
|
Thank you @juliushaertl for taking over! No, we don't need an active focus indicator for the actual content editable. Just tested (i don't know if this PR is ready) but there seems to be 2 invisible focus stops between content editable and sidebar elements: same behavior for forward and backward navigation |
Which browser did you try there? I haven't seen that happening yesterday. |
juliusknorr
force-pushed
the
fix/sidebar-focustrap
branch
from
b4f7e90 to
5dbd121
Compare
|
tested again, but still can reproduce in Chrome ;( |
|
Ok, this turns out more tricky. Apparently we cannot just prevent the focus trap event listener form triggering as it is registered with https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#capture false so it will be executed even before we ever reach the event listeners from text. pause/unpause of the focus trap could work but seems not very clean Maybe we need to create our own trap for the content editable so we can control if and how it would be exited - just assuming for now that the parent trap would then just be picked up properly, but needs more work and investigation. |
|
https://github.com/focus-trap/focus-trap/blob/master/docs/js/nested.js is probably a good sample for this if we handle the activation once the editor gets focus and the deactivation on the Tab/Shift-Tab keypress (if no native editor function like list indent is feasible) |
|
@juliushaertl sounds like this still needs some work, right? Turning into |
juliusknorr
force-pushed
the
fix/sidebar-focustrap
branch
2 times, most recently
from
7059511 to
95fe30c
Compare
|
All right, I went for a slightly different approach. The viewer focus trap needs to be paused on the fly in order to properly handle tab commands in the editor, as we have no control over if a tab key event is reaching the editor otherwise. This is because the focus trap registers a capture listener (https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#capture), so whenever we reach a tab command in the editor the focus trap will already have captured the event. We also cannot work around this by pushing our own focus trap to the stack, as the focus trap package does not offer any reliable way to programmatically focus the next element of the parent trap if we allow tabbing out of the editor. Therefore a new tiptap extension now pauses the focus trap whenever the tab key should be catched by editor actions (indent lists, navigate to the next table cell) and otherwise unpauses so that the actual next tabbable element in the trap containers is getting its focus. @ShGKme Would appreciate your feedback here since you likely have better knowledge of focus trap handling. Does that approach make sense? At least function wise this seems to be the only way to achieve consistent tab handling I could find. |
|
@juliushaertl I have been just working on fixing the conflict between Maybe that's what you need? nextcloud-libraries/nextcloud-vue#4953
This is exactly what I have done. I'm pausing the focus trap stack and unpausing it back. |
|
Ok, similar approach then. The fix here is unrelated to actions so we still need our own implementation, but then I'd set this back to review state |
|
Ah just tests need some fixes |
| const trapStack = window._nc_focus_trap ?? [] | ||
| const activeTrap = trapStack[trapStack.length - 1] | ||
|
|
||
| const possibleEditorTabCommand = editor.can().sinkListItem('listItem') |
There was a problem hiding this comment.
This will throw an exception when listItem is not a registered node type:
Error: There is no node type named 'listItem'. Maybe you forgot to add the extension?
We probably either have to catch the exception or check if the node type exists first.
There was a problem hiding this comment.
Fixed by only using it for the rich text editing. Plaintext has a different problem, but I'll address that separately.
…t are within a focus trap Signed-off-by: Julius Härtl <jus@bitgrid.net>
juliusknorr
force-pushed
the
fix/sidebar-focustrap
branch
from
95fe30c to
2c9e7a0
Compare
|
/backport to stable28 |
Instead of manually messing with the focus trap which breaks the focus order when trying to tab out of the editor into the sidebar next to the viewer, we can just use the default viewer handling and instead prevent event propagation for cases where the Tab key issues a command within the editor.
Additional fix for https://github.com/nextcloud/andy/issues/8