Terra-core: jsx-a11y eslint plugin reports various lint issues with paginator

[bjankord]

Bug Report

Description

The following lint errors show up for the paginator component after updating eslint / eslint-jsx-a11y plugin:

• jsx-a11y/no-static-element-interactions
• jsx-a11y/anchor-is-valid
• jsx-a11y/no-noninteractive-tabindex

Steps to Reproduce

1. Run eslint with eslint v4+ and jsx-a11y plugin v6.1.0+

Expected Behavior

Lint errors are addressed and resolved

Possible Solution

Currently, the pagination component uses anchor elements without href attributes. Switching over to using the button component could resolve these lint issues.

Environment

• Component Name and Version: terra-paginator v1.0.0+

[nramamurth]

I have an initial rough draft to resolve this issue.

Overall, I believe the following are the changes that I believe that need to be made while translating all the anchor tags to be of type Button.

1. Translations

The text 'First', 'Next', 'Previous', 'Last' need to be internationalised, methinks.

https://github.com/cerner/terra-core/blob/25696dcf0fb49d22356219252f13ef0ef83b98e7/packages/terra-paginator/src/Paginator.jsx#L150-L152

https://github.com/cerner/terra-core/blob/25696dcf0fb49d22356219252f13ef0ef83b98e7/packages/terra-paginator/src/Paginator.jsx#L163-L165

1. Transformation of First anchor tag to First button.

- <a
+ <Button
      aria-disabled={selectedPage === 1}
      aria-label="first" // There is an issue.
+     isDisabled={selectedPage === 1}
-     className={cx(['nav-link', 'left-controls', selectedPage === 1 && 'is-disabled'])}
+     className={cx(['nav-link',  selectedPage === 1 && 'is-disabled'])}
      tabIndex={selectedPage === 1 ? null : '0'}
      onClick={this.handlePageChange(1)}
      onKeyDown={this.handleOnKeyDown(1)}
+     text="First" // Internationalised.
-  >
+ />
-  First
- </a>

Similar changes need to be made for Last button.

1. Transformation of Previous anchor tag to Previous button.

- <a
+ <Button
     aria-disabled={selectedPage === 1}
     aria-label="previous" // There is an issue.
-    className={cx(['nav-link', 'left-controls', 'previous', selectedPage === 1 && 'is-disabled'])}
+    className={cx(['nav-link', 'previous', selectedPage === 1 && 'is-disabled'])}
     tabIndex={selectedPage === 1 ? null : '0'}
     onClick={this.handlePageChange(previousPageIndex)}
     onKeyDown={this.handleOnKeyDown(previousPageIndex)}
+    text="Previous" // Internationalised
+    icon={<IconPrevious />}
+    isDisabled={selectedPage === 1}
-  >
+ />
-          <span className={cx('icon')} />
- Previous
- </a>

• Similar changes need to be made for Next button but with isReversed prop so the <IconNext /> appears to the right of the text.
• Add isIconOnly to these buttons for the "Reduced Paginator".

1. Evaluate CSS Changes:

https://github.com/cerner/terra-core/commit/4e9bba3187f3f902a567d2a38fea5fb81ce19a94#diff-dbd3b75a65ed5bc7cbb843b08ef6e93c

I have removed most of the CSS that were not needed imo. @neilpfeiffer Could you please re-evaluate them for me?

@bjankord @neilpfeiffer @ryanthemanuel @ShettyAkarsh Please let me know your thoughts.

[nramamurth]

Upon further initial refactoring, I ran into an issue with how the methods handlePageChange and handleOnKeyDown handle navigation in a paginator w/o pages.

https://github.com/cerner/terra-core/blob/25696dcf0fb49d22356219252f13ef0ef83b98e7/packages/terra-paginator/src/Paginator.jsx#L76-L80

I noticed that for a default Button, the aria-label supplied by the consumer is nuked which causes an issue in the above code because the supplied aria-label to the buttons 'Next' and 'Previous' are nuked.

https://github.com/cerner/terra-core/blob/25696dcf0fb49d22356219252f13ef0ef83b98e7/packages/terra-button/src/Button.jsx#L246

I am curious if there was a conscious decision made during the design of the Button component. If this is by design, then we cannot depend on the aria-label to figure out the direction in a paginator without pages.

@bjankord @neilpfeiffer What are your thoughts?

[nramamurth]

Updates:

1. Offline, Neil confirmed that this issue mentioned in https://github.com/cerner/terra-core/issues/1689#issuecomment-483123764 should be fixed.

2. Neil was also wondering if Paginator will need its own buttons since the designs that we have to achieve in the Paginator component may not (or do not) completely match designs that we have been given in the button component and consistent in all themes (Clinical Theme, MPage Theme, Default Terra theme).

To evaluate the styles further on all themes, I have an "almost working" branch - https://github.com/cerner/terra-core/compare/paginator-lint.

@neilpfeiffer Please feel free to update it if I have quoted you wrong :)

[bjankord]

Yeah, I'd recommend we don't use terra-button for this. I looked through the design UX has provided and it looks like this will need it's own button styles. We should use an button HTML element and apply pagination button styles to that.

[nramamurth]

@neilpfeiffer Here's a new branch with changes using HTML button elements - https://github.com/cerner/terra-core/compare/paginator-buttons.

I think it is _important_ to point out that the current behaviour of the mouse-click action on _Paginator_ is not consistent across all browsers:

• Chrome: the page number links do not persist a border and a shadow when they are clicked.
• Safari / Firefox / IE 11 : the page number links persist a border and a shadow when they are clicked.

Safari:

Firefox:

Chrome:

IE 11:

With buttons, this behaviour has changed. Please let me know the expected behaviour across all browsers for both keyboard navigation and mouse-click so I could address the issue accordingly.

[nramamurth]

@neilpfeiffer With new button implementation on IE 11 🤯💥

[ryanthemanuel]

Let's make sure we're keeping https://github.com/cerner/terra-core/issues/1837 in mind with this as well.

[bjankord]

Cross Post from #1837

For every page, it should say “Goto Page #”
For the current page it should say “Current Page, Page #”
First, Previous, next and last stay the same
These rule apply for both paginates

https://medium.com/@jessebeach/beware-smushed-off-screen-accessible-text-5952a4c2cbfe