Add basic ARIA mark-up #12

Menelion · 2018-05-12T23:03:08Z

I've added basic ARIA mark-up for links and buttons.

The only thing I'm doubtful about is how to organize threads and replies. I experimented a bit, but this functionality needs to be tested and might be reverted in future.

I also didn't mark up user name links that toggle user IDs: I might implement it in future, but it needs also a description, otherwise a blind user won't understand what the click on a user name does.

igoradamenko

Good PR, thanks.

But I think that we can make it even better.

igoradamenko · 2018-05-12T23:21:52Z

web/app/components/comment/comment.jsx

@@ -306,6 +307,8 @@ export default class Comment extends Component {
            <span className={b('comment__score', {}, { view: o.score.view })}>
              <span
                className={b('comment__vote', {}, { type: 'up', selected: scoreIncreased, disabled: isGuest || isCurrentUser })}
+                role="button"
+                aria-disabled={isGuest || isCurrentUser ? 'true' : 'false'}


You can use just isGuest || isCurrentUser as a value here, can't you?

I thought about it. However, in all specifications I always saw aria-something="true" or "false" (note the quotes). So would it be OK to leave it as you suggest? If so, I'll be glad to remove those unnecessary boolean values :)

If you leave just a boolean value in attribute, it will be processed to string: "true" or "false". Check this demo: https://codepen.io/igoradamenko/pen/QrxvGe?editors=0010

So, isGuest and isCurrentUser are always boolean, and I think you can remove unnecessary ternary operator here :-)

igoradamenko · 2018-05-12T23:22:43Z

web/app/components/comment/comment.jsx

@@ -286,6 +286,7 @@ export default class Comment extends Component {
                <a
                  className="comment__link-to-parent"
                  href={`${o.locator.url}#${COMMENT_NODE_CLASSNAME_PREFIX}${o.pid}`}
+                  aria-label="Parent comment"


Go to parent comment? And maybe it's better to add title here too.

Agreed. Did that just for conciseness, our screen readers tend to say quite a lot of things at once :).

igoradamenko · 2018-05-12T23:24:34Z

web/app/components/comment/comment.jsx

@@ -244,7 +244,7 @@ export default class Comment extends Component {

    if (mods.view === 'preview') {
      return (
-        <div className={b('comment', props, defaultMods)}>
+        <div className={b('comment', props, defaultMods)} role="listitem article" aria-level={mods.level}>


Instead of role="article" I think you should change div with article.

But don't forget to add display: block to .comment in this case.

igoradamenko · 2018-05-12T23:28:23Z

web/app/components/auth-panel/auth-panel.jsx

@@ -46,7 +46,7 @@ export default class AuthPanel extends Component {
              <strong className="auth-panel__username" onClick={this.toggleUserId}>{user.name}</strong>
              {isUserIdVisible && <span className="auth-panel__user-id"> ({user.id})</span>}.
              {' '}
-              <span className="auth-panel__pseudo-link" onClick={props.onSignOut}>Sign out?</span>
+              <span className="auth-panel__pseudo-link" role="link" onClick={props.onSignOut}>Sign out?</span>


If you add role="link" you also should add tabIndex="0" because if you don't it's senselessly, isn't it?

igoradamenko · 2018-05-12T23:31:26Z

web/app/components/comment/comment.jsx

@@ -244,7 +244,7 @@ export default class Comment extends Component {

    if (mods.view === 'preview') {
      return (
-        <div className={b('comment', props, defaultMods)}>
+        <div className={b('comment', props, defaultMods)} role="listitem article" aria-level={mods.level}>


BTW, why do you think that listitem must be used here? Are you sure that it's OK to mix roles here?

Oh, I've reread your first comment. As far as I understand, it's the experimental part what you were talking about.

What if we use article only for Comment component, list for .root__threads in Root? And for Thread we can always use listitem and list only when it has some replies inside. So in this case we will have structure like this one:

list (.root__threads) — listitem list (Thread with Threads inside) — — article (Comment) — — listitem (Thread without Threads inside) — — — article (Comment) — — listitem (Thread without Threads inside) — — — article (Comment) — listitem (Thread without Threads inside) — — article (Comment)

I'm not an expert in a11y, but it sounds OK for me. What do you think?

Sounds perfect to me, thanks! And, it seems, this way we don't need to bother ourselves with manually setting aria-level :)

Oh yes, and I also need some help, sorry (I'm not a (P)React expert so far). How do we separate those threads with nested threads and without them in the thread component?

Ah, it's pretty simple.

Here we have Threads inside Thread: https://github.com/umputun/remark/blob/master/web/app/components/thread/thread.jsx#L33-L41

So, you can see that nested Threads generated from replies array. You can use replies.length as a condition for adding list role. For example:

const roles = ['listitem'].concat(replies.length ? 'list' : []); return ( <div className={b('thread', props)} role={roles}>

or even simpler:

return ( <div className={b('thread', props)} role={['listitem'].concat(replies.length ? 'list' : [])} >

But I'm not sure should we add list if child Threads are collapsed? If we shouldn't then conditional must be like this: (!collapsed && replies.length) ? 'list' : []. I don't know what the usual a11y pattern for situation like this is.

Thanks! Usually if something is expanded or collapsed, we add aria-expanded which is either true or false. BTW, where are the Expand/Collapse (pseudo-)links located?

Here it is: https://github.com/umputun/remark/blob/master/web/app/components/comment/comment.jsx#L343-L350

It's a bit messy, but we have it inside Comment in Thread, and when you click on it, Thread hides / shows nested Threads. But the expand/collapse button is inside Comment ¯\_(ツ)_/¯.

igoradamenko · 2018-05-12T23:33:11Z

web/app/components/root/root.jsx

@@ -223,7 +223,7 @@ export default class Root extends Component {

                {
                  !!pinnedComments.length && (
-                    <div className="root__pinned-comments">
+                    <div className="root__pinned-comments" role="region" aria-label="Pinned Comments">


Pinned comments, please (second word is lowercased).

igoradamenko · 2018-05-12T23:34:29Z

web/app/components/root/root.jsx

@@ -268,13 +268,13 @@ export default class Root extends Component {

          {
            isBlockedVisible && (
-              <div className="root__main">
+              <div className="root__main" role="region" aria-label="Blocked Users">


Blocked users.

And may be it's better to move all this things to BlockedUsers component root node.

igoradamenko · 2018-05-13T18:46:01Z

LGTM. @Menelion, have you finished with it? May I merge it?

Menelion · 2018-05-13T20:12:26Z

@igoradamenko, Yes, please. However, I still don't get why the CI is failing :).

igoradamenko · 2018-05-13T20:17:54Z

That's because we don't have CI for pull requests now. Somewhen we will — #14.

But now — thank you for PR, it's awesome 🐨

Andre Polykanine A.K.A. Menelion Elensúlë added 2 commits May 13, 2018 01:49

Add basic ARIA mark-up

e6aa411

Resolve conflicts

e96f571

igoradamenko requested changes May 12, 2018

View reviewed changes

Changed according to code review by @igoradamenko

158cc3f

igoradamenko approved these changes May 13, 2018

View reviewed changes

igoradamenko merged commit ef618a5 into umputun:master May 13, 2018

Menelion deleted the aria-links branch May 13, 2018 21:25

igoradamenko mentioned this pull request Jun 23, 2018

Add a11y linter #97

Closed

paskal added this to the v1.3 milestone Jan 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic ARIA mark-up #12

Add basic ARIA mark-up #12

Menelion commented May 12, 2018

igoradamenko left a comment

igoradamenko May 12, 2018

Menelion May 13, 2018

igoradamenko May 13, 2018

igoradamenko May 12, 2018

Menelion May 13, 2018

igoradamenko May 12, 2018

igoradamenko May 12, 2018

igoradamenko May 12, 2018

igoradamenko May 12, 2018

Menelion May 13, 2018

Menelion May 13, 2018

igoradamenko May 13, 2018

Menelion May 13, 2018

igoradamenko May 13, 2018 •

edited

igoradamenko May 12, 2018

igoradamenko May 12, 2018

igoradamenko commented May 13, 2018

Menelion commented May 13, 2018

igoradamenko commented May 13, 2018

Add basic ARIA mark-up #12

Add basic ARIA mark-up #12

Conversation

Menelion commented May 12, 2018

igoradamenko left a 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

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

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

igoradamenko May 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igoradamenko commented May 13, 2018

Menelion commented May 13, 2018

igoradamenko commented May 13, 2018

igoradamenko May 13, 2018 •

edited