Improving usability on onboarding screen when using keyboard and screen reader #10661

Rafi993 · 2020-10-06T12:10:57Z

What type of PR is this?

Description

Improving usability when using keyboard and screen reader

Related Tickets & Documents

Closes #9585

QA Instructions, Screenshots, Recordings

Go to url http://localhost:3000/onboarding
Try to complete the on-boarding journey with keyboard (with screen reader on)

Added tests?

yes
no, because they aren't needed
no, because I need help

Added to documentation?

docs.forem.com
readme
no documentation needed

What gif best describes this PR or how it makes you feel?

Rafi993 · 2020-10-06T17:34:53Z

app/assets/stylesheets/preact/onboarding-modal.scss

@@ -246,7 +246,7 @@
 }
 // modal overflow scrolling
 .onboarding-modal-scroll-container {
-  height: calc(100vh - 289px);
+  height: calc(100vh - 332px);


Since it was causing two scrollbars I changed it to so that it has only one scrollbar

Rafi993 · 2020-10-07T03:03:31Z

app/javascript/shared/components/focusTrap.jsx

+import { createFocusTrap } from 'focus-trap';
+import { defaultChildrenPropTypes } from '../../common-prop-types';
+
+const FocusTrap = ({ selector, children }) => {


I created this wrapper component that uses library 'focus-trap' to trap focus. Ideally I love to convert it to a hook rather than a component but since it is used by class components too it can't be a hook.

Rafi993 · 2020-10-07T03:04:33Z

app/javascript/shared/components/focusTrap.jsx

+import PropTypes from 'prop-types';
+import { h, Fragment } from 'preact';
+import { useEffect, useRef } from 'preact/hooks';
+import { createFocusTrap } from 'focus-trap';


I added this library https://bundlephobia.com/result?p=focus-trap@6.1.2 It's size is 2.3kb minified and gzipped (but should be much smaller since I don't use all functionality of that lib and it is tree shakabale) is this ok?

Rafi993 · 2020-10-07T03:05:16Z

app/javascript/shared/components/focusTrap.jsx

+};
+
+FocusTrap.defaultProps = {
+  selector: '.crayons-modal',


I made the default selector value '.crayon-modal' since this component will be mostly be used by modals (rarely in other places where different selector string could be passed)

If you open a second modal over the top of an existing one I think the tabbing will be locked to the first.
That being said I believe, with how you've set it up, you can do some something like

<FocusTrap selector={reference.current}> <div id="locked-in" ref={reference}></div> </FocusTrap>

Ref would work too. Incase modal appears another modal it can pass another selector string like id of the modal and it would also work.

<FocusTrap selector="#secondModal"> <div id="secondModal"> second modal </div> </FocusTrap>

But on the other hand nested modals might not be used anywhere (but I could be wrong). Having no prop needed to be passed for this component by default makes API much simpler.

Rafi993 · 2020-10-07T03:06:06Z

app/javascript/onboarding/components/FollowTags.jsx

+                </h2>
+              </header>
+              <div className="onboarding-modal-scroll-container">
+                <ul data-testid="onboarding-tags" className="onboarding-tags">


I changed div to ul and li so that I could be read by screen reader

rhymes

Haven't reviewed the code but I wonder if we can improve the behavior in the third slide

I have to tab 3 times before I can press space to select the "follow" button:

It seems to work nice otherwise. There are a few violations found in the Accessibility panel in Firefox but I think they are beyond the objective of this PR.

Thanks @Rafi993!

Link2Twenty · 2020-10-07T09:56:20Z

FocusTrap would be useful for #10610 too.

Link2Twenty · 2020-10-07T10:00:37Z

app/javascript/shared/components/focusTrap.jsx

+import { createFocusTrap } from 'focus-trap';
+import { defaultChildrenPropTypes } from '../../common-prop-types';
+
+const FocusTrap = ({ selector, children }) => {


Is it worth having a bool active prop?
Meaning in the future we could add the ability to lock focus if we want to,

Thank you @Link2Twenty. I think boolean active can be added to FocusTrap component and made optional value with default value being true. But right now it is not used here but it could be added later to the component if needed without affecting existing code.

Rafi993 · 2020-10-08T07:24:06Z

Haven't reviewed the code but I wonder if we can improve the behavior in the third slide

I have to tab 3 times before I can press space to select the "follow" button:

It seems to work nice otherwise. There are a few violations found in the Accessibility panel in Firefox but I think they are beyond the objective of this PR.

Thanks @Rafi993!

Thank you @rhymes. When "follow" button is selected focus is jumping to the previous element causing this issue. Let me search for reason it is happening.

Rafi993 · 2020-10-08T13:40:41Z

Haven't reviewed the code but I wonder if we can improve the behavior in the third slide

I have to tab 3 times before I can press space to select the "follow" button:

It seems to work nice otherwise. There are a few violations found in the Accessibility panel in Firefox but I think they are beyond the objective of this PR.

Thanks @Rafi993!

Similar issue is also happening in tag selection too. I couldn't figure out why it is happening. Any thoughts @forem/oss

Thanks for you help

Link2Twenty · 2020-10-08T14:13:55Z

app/javascript/onboarding/components/FollowUsers.jsx

+              >
+                {users.map((user) => (
+                  <li key={user.name}>
+                    <button


I think this button should have tabindex="-1".
I think we only want the follow button to be in the tab order.

Here is the flow with the tabindex -1

only 1 tab to get to the next follow button

Thank you @Link2Twenty. But there is one issue though the screen reader is not reading out items in the list (the name of the user and the description below now) it just says follow. This might cause confusion for users who are using screen readers since they don't know which user did they just followed. I can add aria-label to the follow button saying "Follow Jere Thompson" it will help but it will still miss out the description but I don't know if that is ok. Any thoughts?

When I remove the handler this.handleClick it is not happening I did try adding both event.preventDefault() and event.stopPropagation() they don't seem to affect this. It is something to do with the state change. Let me keep searching.

It is a bug in the FocusTrap component that is causing this when I remove it is working properly. Let me keep searching further.

I was able to fix this by moving FocusTrap component one level up to app/javascript/onboarding/Onboarding.jsx there are some tests failing (even though what the test is looking for is working when tested manually) let me fix those too.

I have fixed the breaking tests too

To answer your comment regarding the button name, @Rafi993, an aria-label would help to uniquely identify the individual follow buttons. That would address some instances of an issue that exists throughout the site where controls are repeated (i.e. "Links with the same name have a similar purpose" in the axe extension).

Thank you @marcysutton I have made the change. I did not know about this before.

Rafi993 · 2020-10-10T03:37:55Z

Haven't reviewed the code but I wonder if we can improve the behavior in the third slide

I have to tab 3 times before I can press space to select the "follow" button:

It seems to work nice otherwise. There are a few violations found in the Accessibility panel in Firefox but I think they are beyond the objective of this PR.

Thanks @Rafi993!

I have resolved this now

Rafi993 · 2020-10-12T09:22:55Z

I think some unrelated test failed.

…lity

rhymes · 2020-10-29T07:11:25Z

@Rafi993 the tests are fine, it's Codeclimate complaining :) We should likely ignore it in the context of this PR

rhymes

It looks good, thanks @Rafi993, only a question still on the "follow people slide". Is it possible to go down from "2 tabs + space to select follow" to "1 tab plus space"?

Rafi993 · 2020-10-29T08:35:49Z

It looks good, thanks @Rafi993, only a question still on the "follow people slide". Is it possible to go down from "2 tabs + space to select follow" to "1 tab plus space"?

Solution @Link2Twenty suggested above does it #10661 (comment) . But when I was using screen reader it just read out the text in the "Follow" button alone and it did not read the user name or the description below.

Link2Twenty · 2020-10-29T10:00:13Z

app/javascript/onboarding/components/FollowUsers.jsx

+                        data-testid="onboarding-user-following-status"
+                        type="button"
+                        className="user-following-status"
+                        aria-pressed={selectedUsers.includes(user)}


Here would could add

aria-label={`${selectedUsers.includes(user) ? 'Unfollow' : 'Follow'} ${user.name}`}

This will tell the screen reader what to read.

This would be to coincide with the tabIndex change.

Here would could add

aria-label={`${selectedUsers.includes(user) ? 'Unfollow' : 'Follow'} ${user.name}`}

This will tell the screen reader what to read.

But it would miss the description for each user

I guess the three options are

We remove something from the tabIndex be it the inner or outer button

We accept multiple tabs is a thing that people have to deal with

We wait for A11y: Add keyboard navigation to article feed #10468 and use the useListNavigation hook to add j and k navigation support.

nickytonline · 2020-11-03T14:31:58Z

I'm looking to give this a review this week. Hacktoberfest was pretty busy. 😅

…lity

nickytonline · 2021-01-20T15:04:43Z

package.json

@@ -155,7 +155,7 @@
    "@honeybadger-io/webpack": "^1.2.0",
    "@rails/webpacker": "5.2.1",
    "@testing-library/user-event": "^12.1.7",
-    "ahoy.js": "^0.3.7",
+    "ahoy.js": "^0.3.8",


refactor (blocking): This looks like a bad merge or maybe ensure you have merged latest as this does not appear to be related to the work being done in this PR.

nickytonline · 2021-01-20T15:04:47Z

yarn.lock

-  version "5.4.6"
-  resolved "https://registry.yarnpkg.com/table/-/table-5.4.6.tgz#1292d19500ce3f86053b05f0e8e7e4a3bb21079e"
-  integrity sha512-wmEc8m4fjnob4gt5riFRtTu/6+4rSe12TpAELNSqHMfF3IqnA+CH37USM6/YR3qRZv7e56kAEAtd6nKZaxe0Ug==
+table@^6.0.4:


refactor (blocking): This looks like a bad merge or maybe ensure you have merged latest as this does not appear to be related to the work being done in this PR.

Rafi993 · 2021-01-21T04:21:55Z

As we discussed on the stream it is better to split this PR into multiple small PR's so that it will be easy to review. I'll start creating separate small PR's.

Rafi993 · 2021-01-27T03:58:10Z

Closing this PR in favour of smaller PRs that have been raised.

Makings links focusable

531f798

pr-triage bot added the PR: draft bot applied label for PR's that are a work in progress label Oct 6, 2020

Rafi993 added 4 commits October 6, 2020 17:58

Removing unwanted scroll

9457015

Converting div to ul and li

1fafce0

Fixing tags for screen reader

7b282ea

Fixing following users for screen reader

5d8e652

Rafi993 commented Oct 6, 2020

View reviewed changes

Rafi993 added 3 commits October 7, 2020 06:30

Adding roles, title and subtitle to modal

906aa92

Focus trap component

cc40cbb

Fixing casing of FocusTrap

390d5eb

Rafi993 commented Oct 7, 2020

View reviewed changes

Adding tests for focus trap component

fdbdb3d

Rafi993 marked this pull request as ready for review October 7, 2020 03:40

Rafi993 requested a review from nickytonline as a code owner October 7, 2020 03:40

Rafi993 requested a review from a team October 7, 2020 03:40

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: draft bot applied label for PR's that are a work in progress labels Oct 7, 2020

Rafi993 changed the title ~~Improving usability on onboarding screen~~ Improving usability on onboarding screen when using keyboard and screen reader Oct 7, 2020

rhymes requested review from ludwiczakpawel, jacobherrington and vaidehijoshi and removed request for a team October 7, 2020 06:30

rhymes reviewed Oct 7, 2020

View reviewed changes

Link2Twenty reviewed Oct 7, 2020

View reviewed changes

Link2Twenty reviewed Oct 8, 2020

View reviewed changes

Rafi993 added 3 commits October 10, 2020 06:56

Removing undefined function

bd2929a

Fixing focus jumping

2645559

Fixing breaking tests

c1062d4

Merge branch 'master' into Rafi993/improving-on-boarding-screen-usabi…

3eee989

…lity

rhymes approved these changes Oct 29, 2020

View reviewed changes

pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 29, 2020

Link2Twenty reviewed Oct 29, 2020

View reviewed changes

Rafi993 mentioned this pull request Nov 11, 2020

Keyboard access and usability in Onboarding screens #9585

Closed

Merge branch 'master' into Rafi993/improving-on-boarding-screen-usabi…

bdb82b3

…lity

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Nov 17, 2020

nickytonline removed their request for review November 18, 2020 03:20

Rafi993 added 3 commits December 3, 2020 22:21

aria-label for follow user button

8df6433

Merge branch 'master' into Rafi993/improving-on-boarding-screen-usabi…

54a6a30

…lity

Merge branch 'master' into Rafi993/improving-on-boarding-screen-usabi…

97a17e4

…lity

Rafi993 requested a review from a team January 20, 2021 14:32

nickytonline suggested changes Jan 20, 2021

View reviewed changes

pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 20, 2021

Rafi993 closed this Jan 27, 2021

Improving usability on onboarding screen when using keyboard and screen reader #10661

Improving usability on onboarding screen when using keyboard and screen reader #10661

Rafi993 commented Oct 6, 2020 •

edited

Rafi993 Oct 6, 2020

Rafi993 Oct 7, 2020

Rafi993 Oct 7, 2020 •

edited

Rafi993 Oct 7, 2020

Link2Twenty Oct 7, 2020 •

edited

Rafi993 Oct 8, 2020 •

edited

Rafi993 Oct 7, 2020

rhymes left a comment

Link2Twenty commented Oct 7, 2020

Link2Twenty Oct 7, 2020

Rafi993 Oct 8, 2020 •

edited

Rafi993 commented Oct 8, 2020 •

edited

Rafi993 commented Oct 8, 2020

Link2Twenty Oct 8, 2020 •

edited

Link2Twenty Oct 8, 2020

Rafi993 Oct 8, 2020 •

edited

Rafi993 Oct 10, 2020

Rafi993 Oct 10, 2020

Rafi993 Oct 10, 2020

Rafi993 Oct 10, 2020

marcysutton Dec 3, 2020 •

edited

Rafi993 Dec 3, 2020

Rafi993 commented Oct 10, 2020

Rafi993 commented Oct 12, 2020

rhymes commented Oct 29, 2020

rhymes left a comment

Rafi993 commented Oct 29, 2020

Link2Twenty Oct 29, 2020

Link2Twenty Oct 29, 2020

Rafi993 Oct 29, 2020

Link2Twenty Oct 29, 2020

nickytonline commented Nov 3, 2020

nickytonline Jan 20, 2021

nickytonline Jan 20, 2021

Rafi993 commented Jan 21, 2021

Rafi993 commented Jan 27, 2021

Improving usability on onboarding screen when using keyboard and screen reader #10661

Improving usability on onboarding screen when using keyboard and screen reader #10661

Conversation

Rafi993 commented Oct 6, 2020 • edited

What type of PR is this?

Description

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Added tests?

Added to documentation?

What gif best describes this PR or how it makes you feel?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rafi993 Oct 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Link2Twenty Oct 7, 2020 • edited

Choose a reason for hiding this comment

Rafi993 Oct 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhymes left a comment

Choose a reason for hiding this comment

Link2Twenty commented Oct 7, 2020

Choose a reason for hiding this comment

Rafi993 Oct 8, 2020 • edited

Choose a reason for hiding this comment

Rafi993 commented Oct 8, 2020 • edited

Rafi993 commented Oct 8, 2020

Link2Twenty Oct 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rafi993 Oct 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcysutton Dec 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rafi993 commented Oct 10, 2020

Rafi993 commented Oct 12, 2020

rhymes commented Oct 29, 2020

rhymes left a comment

Choose a reason for hiding this comment

Rafi993 commented Oct 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickytonline commented Nov 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rafi993 commented Jan 21, 2021

Rafi993 commented Jan 27, 2021

Rafi993 commented Oct 6, 2020 •

edited

Rafi993 Oct 7, 2020 •

edited

Link2Twenty Oct 7, 2020 •

edited

Rafi993 Oct 8, 2020 •

edited

Rafi993 Oct 8, 2020 •

edited

Rafi993 commented Oct 8, 2020 •

edited

Link2Twenty Oct 8, 2020 •

edited

Rafi993 Oct 8, 2020 •

edited

marcysutton Dec 3, 2020 •

edited