Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixes for keyboard navigation #2122

Merged
merged 12 commits into from
Aug 28, 2019
Merged

Fixes for keyboard navigation #2122

merged 12 commits into from
Aug 28, 2019

Conversation

danxuliu
Copy link
Member

Draft until #2117 is merged (first commit of this pull request is Do not select again an already selected tab)

"Requires" (that is, the password button works better with) nextcloud/server#16863

This pull request fixes several elements not being focusable (or being focusable when they should not be) or highlighted when focused:

  • Tab headers in the sidebar
  • Select2 containers (Add participant and New conversation...)
  • Copy link and password buttons
  • Edit button in room name and guest name
  • Sidebar trigger
  • Participants in the Participants tab of the sidebar

This can be tested by navigating through the main Talk UI by pressing tab and checking that

  • the items are properly highlighted when focused
  • pressing Enter or space when an element is focused triggers the action associated to that element

Some things to keep in mind:

  • The link element in the tab header should be changed to a span, but I kept it as a link for now to be closer to the server.
  • When a Select2 list is open pressing tab adds the current item instead of changing to the next element. However, note that the Select2 list can be closed by pressing Esc and then pressing tab works as expected.
  • When pressing enter on the Copy link button the focus is lost; this seems to be caused by the tooltip changing the parent element when shown or something like that, but I have not investigated it.
  • After focusing the edit button in room name or guest name pressing Enter shows the input. However, if Enter is kept pressed it shows and hides the input again and again. Unfortunately the browsers constantly trigger keydown and keyup events when a key is kept pressed, so once the input is shown that elements receives the repeated Enter keyup events; theoretically the repeat property should be set on the event to make possible to distinguish it, but in practice it seems that is is not set, so this can not be detected nor handled (or, at least, I could not find how to do it ;-) ).
  • The tab order can still be improved, as the button to show the sidebar and the button to enable fullscreen are not navigated one after the other (due to their position in the DOM). However that involves a deeper change and I did not want to mess with explicit tab order, so for now I left it as is.
  • I have not enabled keyboard navigation on the avatars or mentions in the chat view to trigger the contacts menu, as I think that would make the navigation much slower for little gain (as the contacts menu can be triggered from the participants list)
  • The message list in the chat view is not highlighted when focused (focusing it allows to scroll using the keyboard); maybe it would be good to add a dashed border or something like that, but only if it was focused with keyboard navigation and not by clicking on the list. Probably this could be accomplished by tweaking this Keyboard-Only focus technique (StackOverflow has a nice summary), but it may need some JavaScript too to redirect the key events from the wrapper to the message list itself. In any case... that type of highlighting of scrollable elements it not widespread as far as I know, so for the time being I left it as is.

@danxuliu danxuliu added this to the 💚 Next Major milestone Aug 28, 2019
@danxuliu danxuliu mentioned this pull request Aug 28, 2019
Merged
@danxuliu danxuliu mentioned this pull request Aug 28, 2019
Merged
@nickvergessen
Copy link
Member

Is this required for the lobby PR? Doesn't look like

danxuliu added 12 commits August 28, 2019 09:31
@danxuliu
Do not select again an already selected tab
89921bc 
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Fix keyboard navigation in tab headers
819b5a0 
The tab headers contain a link that was focusable using keyboard
navigation. However, the CSS styles that highlight the tab header were
associated to the whole tab header, so even if the link was focused the
tab header was not highlighted.

Now the link is no longer focusable and the tab header itself is the
element that gets the focus, which causes it to be highlighted when
navigating with the keyboard. Also, the same selection behaviour applied
when the tab header is clicked is now applied when pressing space or
enter.

Finally, as the tab header can not be selected again when it is already
selected the tab header is not focusable either when it is already
selected.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Fix select2 containers not highlighted when focused
e45e375 
The select2 containers have the same size as the "oca-spreedme-add-XXX"
wrappers that contain them, so the borders can be moved from those
wrappers to the containers themselves. This makes possible to set a
stronger border when the select2 container is active/focused.

Besides that now the text is stronger too when the select2 container is
focused to show that more clearly.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Fix "copy link" button not focusable
671feab 
The "copy link" button was not an actual button but a span, so it was
ignored in keyboard navigation.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Fix edit button in EditableTextLabel not focusable
04d2a47 
The edit button was not an actual button but a span, so it was ignored
in keyboard navigation. Moreover, the opacity was applied to its parent
element instead of to the button itself, so it was not highlighted when
focused. Also, after the input is hidden now the focus is given back to
the edit button.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Fix sidebar trigger not focusable
0f5392a 
The sidebar trigger was not an actual button but a div, so it was
ignored in keyboard navigation.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Remove reference to inexistent event handler
472d1a0 
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Fix password button not focusable
1bf31f5 
The password button was not an actual button but a span, so it was
ignored in keyboard navigation. Also, after the password menu is hidden
now the focus is given back to the password button.

The button was also wrapped in a div with the "menutoggle" CSS class,
which causes the menu to be hidden when clicking again on the toggle.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Remove unused CSS rule
5e79ab9 
The first link child of items in the participants list is the
participant information, which includes the avatar, name... However,
there is no direct "img" child of that element (the avatar image is a
child of the avatar element), so the rule has no effect.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Fix participant items being focusable
5b89513 
The participant items were link elements, so they were taken into
account in keyboard navigation. However, although it is possible to
interact with some of its children, it is not possible to interact
with the participant item as a whole, so they should not be focusable.
As the link in the participant items led nowhere they are now simple
span elements instead of links.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Fix pointer cursor not shown on participants with a contacts menu
9692378 
When the current user is an actual user and not a guest clicking on the
avatar of a participant shows the contacts menu (unless the participant
is the current user or a guest), so when hovering on the avatar the
cursor should be a pointer to imply a possible interaction.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Fix avatars of participants with contacts menu not focusable
bf87c25 
The avatar is a div, so it is not focusable by default. However, if it
were focused it could not be properly "highlighted" with a border; the
whole avatar is dimmed for offline participants, so the border colour
would be different when "highlighting" the avatar of online and offline
participants. Due to this the avatar is now wrapped in another div which
is the focusable one (provided the avatar has a contacts menu) and the
one that gets the border. Finally, when the Enter or space keys are
pressed on the wrapper a click is triggered in the avatar, which shows
the contacts menu.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu danxuliu force-pushed the fixes-for-keyboard-navigation branch from e38a627 to bf87c25 Compare August 28, 2019 07:33
@danxuliu
Copy link
Member Author

It is.

I have rebased but cancelled the Drone job (it would have passed, as the changes in master did not affect this pull request) as well as removed the call to backport bot for now.

@danxuliu danxuliu marked this pull request as ready for review August 28, 2019 07:36
@nickvergessen
Copy link
Member

Works surprisingly well. It's only weird that when the participant/room list reloads you have to start at the top again when you where in that sector.

@nickvergessen nickvergessen merged commit b7b6c9e into master Aug 28, 2019
@delete-merged-branch delete-merged-branch bot deleted the fixes-for-keyboard-navigation branch August 28, 2019 08:01
@danxuliu
Copy link
Member Author

It's only weird that when the participant/room list reloads you have to start at the top again when you where in that sector.

Right, it is caused by rendering the views again; the element with the focus is removed, so the browser moves the focus back to the closest parent. It happens too in the CallInfoView when the room type changes and things like that. Could be fixed, yes, but... well, future ;-)

@nickvergessen
Copy link
Member

/backport to stable16

@backportbot-nextcloud
Copy link

The backport to stable16 failed. Please do this backport manually.

@danxuliu danxuliu mentioned this pull request Aug 29, 2019
Merged
@nextcloud nextcloud deleted a comment from backportbot-nextcloud bot Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@Ivansss Ivansss Awaiting requested review from Ivansss

Assignees
No one assigned
Labels
3. to review bug feature: frontend 🖌️ "Web UI" client
Projects
None yet
Milestone
7.0.0-beta1
Development

Successfully merging this pull request may close these issues.
2 participants
@danxuliu @nickvergessen