-
Notifications
You must be signed in to change notification settings - Fork 492
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix(@clayui/pagination): LPD-1285 accessibility issues on prev, next … #5779
Conversation
@@ -164,6 +164,7 @@ const ClayPaginationWithBasicItems = React.forwardRef<HTMLUListElement, IProps>( | |||
disabled={internalActive === 1} | |||
href={previousHref} | |||
onClick={() => setActive(previousPage)} | |||
role={internalActive === 1 ? undefined : 'button'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In both these cases shouldn't we be checking if the href is actually empty or null? Don't we sometimes use an href for going to the previous or next page, in which case the role should stay a link and not a button, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll update it
…and ellipsis buttons
chore(@clayui/core): LPD-1285 regen snapshots
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -164,6 +164,11 @@ const ClayPaginationWithBasicItems = React.forwardRef<HTMLUListElement, IProps>( | |||
disabled={internalActive === 1} | |||
href={previousHref} | |||
onClick={() => setActive(previousPage)} | |||
role={ | |||
previousHref || internalActive === 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a previousHref, it will be a string which would be a truthy value. Then this would be set as the role, right? So I don't think this logic wouldn't be correct when previousHref exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If previousHref
exists, the pagination component outputs <a class="page-link" href="/url">
. In this case, we don't need role="button"
because it's a hyperlink.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay... looks good. Sorry I was incorrectly interpreting the precedence of the operators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -164,6 +164,11 @@ const ClayPaginationWithBasicItems = React.forwardRef<HTMLUListElement, IProps>( | |||
disabled={internalActive === 1} | |||
href={previousHref} | |||
onClick={() => setActive(previousPage)} | |||
role={ | |||
previousHref || internalActive === 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay... looks good. Sorry I was incorrectly interpreting the precedence of the operators.
…and ellipsis buttons
https://liferay.atlassian.net/browse/LPD-1285
This adds
role="button"
to the prev and next buttons. Axe flags these because there is no href attribute. This also addsaria-controls="clay-id-#"
on the ellipsis button when it is active. The dropdown-menu is lazy loaded and Axe can't associatearia-controls
to the dropdown-menu.Axe Errors

After Fix
