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.

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

Any key press toggles module settings open/close #5915

Closed
aaemnnosttv opened this issue Sep 27, 2022 · 10 comments
Closed

Any key press toggles module settings open/close #5915

aaemnnosttv opened this issue Sep 27, 2022 · 10 comments
Labels
P1 Medium priority Rollover Issues which role over to the next sprint Type: Bug Something isn't working

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Sep 27, 2022

Bug Description

Currently the settings for connected services is overly sensitive to key down events. Any key press toggles the open/close of the settings when the header is focused.

The problematic line appears to be here, which uses the same handler for clicks as key down events.

Steps to reproduce

  1. Go to Settings > Connected Services
  2. Toggle open a module's settings
  3. Press any key
  4. See open/close toggling behavior when pressing any key

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The settings interface for a single connected module should only be open/closable by clicking the module's header or by specific keystrokes while focused: Enter/Return. Space could also be acceptable, but in this case it should be fine to have it expand only when using Enter/Return so users can still use spacebar to scroll the page.
  • When focused on the element a user can keyDown on with the Enter/Return key, aria attributes should be present letting the user know they can perform an action to expand the UI.

Implementation Brief

const onKeyDown = useCallback( ( { key } ) => {
		if ( document.activeElement === headerRef.current ) {
			if ( key === 'Enter' ) {
				onHeaderClick();
			}
	}, [] );
  • tabIndex in the Header container should be set to "0" for accessibility

(To consider: introduce :focus-within style for the Header to make the focus more noticeable.)

Test Coverage

Cover key down open/close events functionality with tests.

QA Brief

  • Go to Settings > Connected Services
  • Navigate via the keyboard to a module setting tab.
  • Press the ENTER key to toggle the tab.
  • Press the ESCAPE key to close a tab.
  • Ensure the tab doesn’t toggle for any other keypress.

Changelog entry

  • Fix module settings open/close issue when any key is pressed.
@aaemnnosttv aaemnnosttv added Type: Bug Something isn't working P1 Medium priority labels Sep 27, 2022
@aaemnnosttv
Copy link
Collaborator Author

@tofumatt would you please advise around what we should support for keystrokes here?

@sashadoes sashadoes assigned sashadoes and unassigned sashadoes Sep 29, 2022
@tofumatt tofumatt self-assigned this Oct 10, 2022
@tofumatt
Copy link
Collaborator

@sashadoes Looks good, just a question about this part of the IB:

tie up with container

What does this mean? I don't quite understand, sorry 😅

@tofumatt tofumatt assigned sashadoes and unassigned tofumatt Oct 10, 2022
@sashadoes
Copy link
Collaborator

@tofumatt Like -> Like: Put it in container, use the call back for the event in container.
I will rephrase it now, sorry :)

@sashadoes sashadoes assigned tofumatt and unassigned sashadoes Oct 10, 2022
@tofumatt
Copy link
Collaborator

Thanks, all good then 👍🏻

IB ✅

@tofumatt tofumatt removed their assignment Oct 12, 2022
@asvinb asvinb self-assigned this Oct 13, 2022
@asvinb asvinb removed their assignment Oct 20, 2022
@FlicHollis FlicHollis added the Rollover Issues which role over to the next sprint label Oct 23, 2022
@hussain-t hussain-t assigned hussain-t and asvinb and unassigned hussain-t Oct 25, 2022
@asvinb asvinb assigned hussain-t and unassigned asvinb Oct 26, 2022
@hussain-t hussain-t removed their assignment Oct 26, 2022
@mohitwp mohitwp self-assigned this Oct 27, 2022
@mohitwp
Copy link
Collaborator

mohitwp commented Oct 31, 2022

QA Update ❌

@asvinb

  • Verified on both latest and dev environment.
  • Able to reproduce issue on latest - connected services toggle getting open close by pressing any key and also keyboard navigation not working.
  • Verified on dev - The settings interface for a single connected module is getting only open/close by clicking the module's header or by specific keystrokes while focused: Enter/Return. Using enter key user can open/close and using esc key user can close single connected module.
  • Using 'Space' key user is able to scroll down the single connected module setting interface.

Issue > Now keyboard navigation is working but currently user is not able to know that they can perform an action to expand the UI. When I press tab key it is navigating me to the module settings but showing no indicator that I successfully switched to particular module header and I can expand settings interface for the module using keys.

OS : windows
Browser Chrome : Version 106.0.5249.119 (Official Build) (64-bit)

Recording.34.mp4

@mohitwp mohitwp assigned asvinb and unassigned asvinb Oct 31, 2022
@asvinb
Copy link
Collaborator

asvinb commented Oct 31, 2022

Thanks for raising that issue @mohitwp I have already created a new issue to tackle that: #6037 and accessibility as a whole :)

@mohitwp
Copy link
Collaborator

mohitwp commented Oct 31, 2022

@aaemnnosttv For the above issue I reported here we have separate ticket #6037 as per Asvin.
But reported issue is failing the point in AC - When focused on the element a user can keyDown on with the Enter/Return key, aria attributes should be present letting the user know they can perform an action to expand the UI. Currently, on clicking tab user is navigating to module setting header but header is not getting focus/highlight so user is not able to know that area is expandable. Should we fix this issue in this same PR or it will be ok if fix this in #6037 ?

cc @asvinb @wpdarren

@aaemnnosttv
Copy link
Collaborator Author

But reported issue is failing the point in AC - When focused on the element a user can keyDown on with the Enter/Return key, aria attributes should be present letting the user know they can perform an action to expand the UI. Currently, on clicking tab user is navigating to module setting header but header is not getting focus/highlight so user is not able to know that area is expandable. Should we fix this issue in this same PR or it will be ok if fix this in #6037 ?

@mohitwp the element does appear to have the necessary aria attributes as this point calls for. These are for screen readers though and do not provide any kind of styling on their own, nor does this issue call for stylistic changes. While this would be good to consider going forward, I believe the criteria here are fulfilled but I'll let @tofumatt confirm that as I believe he is the one who wrote them.

@aaemnnosttv aaemnnosttv assigned tofumatt and unassigned aaemnnosttv Nov 1, 2022
@tofumatt
Copy link
Collaborator

tofumatt commented Nov 1, 2022

Confirmed that this fulfils the ACs and further styling work can (and should) be part of #6037. Adding styles to this issue isn't needed and will complicate it, but for now screen readers will have the context that these elements are expandable.

Technically it's not great for sighted keyboard users, but at least they have the aria elements, so I think we can consider the QA passed for this and handle styling in the follow-up issue mentioned 👍🏻

@tofumatt tofumatt removed their assignment Nov 1, 2022
@mohitwp
Copy link
Collaborator

mohitwp commented Nov 2, 2022

QA Update ✅

Thank you @aaemnnosttv @tofumatt.
Issue Reported here - will get resolved through separate PR - #6037.

  • Verified on both latest and dev environment.
  • Able to reproduce issue on latest - connected services toggle getting open close by pressing any key and also keyboard navigation not working.
  • Verified on dev - The settings interface for a single connected module is getting only open/close by clicking the module's header or by specific keystrokes while focused: Enter/Return. Using enter key user can open/close and using esc key user can close single connected module.
  • Using 'Space' key user is able to scroll down the single connected module setting interface.
Recording.34.mp4

@mohitwp mohitwp removed their assignment Nov 2, 2022
@felixarntz felixarntz self-assigned this Nov 3, 2022
@felixarntz felixarntz removed their assignment Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority Rollover Issues which role over to the next sprint Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

9 participants