Updating roles & tabindexes based on #403 (#469)

Co-authored-by: Ben Hudson <ben@wedo.digital>
express-labs · Jul 25, 2024 · 5529d17 · 5529d17
1 parent 0d8799c
commit 5529d17
Show file tree

Hide file tree

Showing 4 changed files with 10 additions and 62 deletions.
diff --git a/src/Slide/Slide.jsx b/src/Slide/Slide.jsx
@@ -21,8 +21,8 @@ const Slide = class Slide extends React.PureComponent {
     onFocus: PropTypes.func,
     orientation: CarouselPropTypes.orientation.isRequired,
     slideSize: PropTypes.number.isRequired,
+    role: PropTypes.string,
     style: PropTypes.object,
-    tabIndex: PropTypes.number,
     tag: PropTypes.string,
     totalSlides: PropTypes.number.isRequired,
     visibleSlides: PropTypes.number.isRequired,
@@ -40,8 +40,8 @@ const Slide = class Slide extends React.PureComponent {
     innerTag: 'div',
     onBlur: null,
     onFocus: null,
+    role: 'option',
     style: {},
-    tabIndex: null,
     tag: 'div',
     isIntrinsicHeight: false,
   }
@@ -104,7 +104,6 @@ const Slide = class Slide extends React.PureComponent {
       orientation,
       slideSize,
       style,
-      tabIndex,
       tag: Tag,
       totalSlides,
       visibleSlides,
@@ -155,16 +154,12 @@ const Slide = class Slide extends React.PureComponent {
       innerClassName,
     ]);
 
-    const defaultTabIndex = this.isVisible() ? 0 : -1;
-    const newTabIndex = typeof tabIndex === 'number' ? tabIndex : defaultTabIndex;
-
     return (
       <Tag
         ref={(el) => { this.tagRef = el; }}
-        tabIndex={newTabIndex}
         aria-selected={this.isVisible()}
         aria-label={ariaLabel}
-        role="option"
+        role={this.props.role}
         onFocus={this.handleOnFocus}
         onBlur={this.handleOnBlur}
         className={newClassName}

diff --git a/src/Slide/__tests__/Slide.test.jsx b/src/Slide/__tests__/Slide.test.jsx
@@ -58,21 +58,7 @@ describe('<Slide />', () => {
     const wrapper = shallow(<Slide {...props} orientation="vertical" />);
     expect(wrapper.find('.slide').prop('style').width).toBe('100%');
   });
-  it('should have a tabIndex of -1 if the slide is not visible within the slideTray (index < currentSlide)', () => {
-    const wrapper = shallow((
-      <Slide
-        currentSlide={1}
-        index={0}
-        naturalSlideHeight={400}
-        naturalSlideWidth={300}
-        orientation="horizontal"
-        slideSize={25}
-        totalSlides={6}
-        visibleSlides={2}
-      />
-    ));
-    expect(wrapper.find('.slide').prop('tabIndex')).toBe(-1);
-  });
+
   it('should apply any supplied classes to hidden slides', () => {
     const wrapper = shallow((
       <Slide
@@ -109,30 +95,10 @@ describe('<Slide />', () => {
     expect(wrapper.find('.slide').hasClass('i-be-visible')).toBe(true);
     expect(wrapper.find('.slide').hasClass('carousel__slide--visible')).toBe(true);
   });
-  it('should have a tabIndex of -1 if the slide is not visible within the slideTray (index >= currentSlide + visibleSlides)', () => {
-    const wrapper = shallow((
-      <Slide
-        currentSlide={1}
-        index={3}
-        naturalSlideHeight={400}
-        naturalSlideWidth={300}
-        orientation="horizontal"
-        slideSize={25}
-        totalSlides={6}
-        visibleSlides={2}
-      />
-    ));
-    expect(wrapper.find('.slide').prop('tabIndex')).toBe(-1);
-  });
-  it('if a tabIndex prop is supplied, set the tabIndex to that value and ignore our internally computed value.', () => {
-    // this is for testing only.
-    // eslint-disable-next-line jsx-a11y/tabindex-no-positive
-    const wrapper = shallow(<Slide {...props} tabIndex={7} />);
-    expect(wrapper.find('.slide').prop('tabIndex')).toBe(7);
-  });
+
   it('should correctly set styles, if isIntrinsicHeight is set', () => {
     // this is for testing only.
-    // eslint-disable-next-line jsx-a11y/tabindex-no-positive
+
     const wrapper = shallow(<Slide {...props} isIntrinsicHeight />);
     const slideStyle = wrapper.find('.slide').prop('style');
     expect(slideStyle.paddingBottom).toBe('unset');
@@ -143,7 +109,6 @@ describe('<Slide />', () => {
   });
   it('should correctly set styles, in vertical mode if isIntrinsicHeight is set', () => {
     // this is for testing only.
-    // eslint-disable-next-line jsx-a11y/tabindex-no-positive
     const wrapper = shallow(<Slide {...props} orientation="vertical" isIntrinsicHeight />);
     const slideStyle = wrapper.find('.slide').prop('style');
     expect(slideStyle.paddingBottom).toBe('unset');

diff --git a/src/Slider/Slider.jsx b/src/Slider/Slider.jsx
@@ -38,12 +38,12 @@ const Slider = class Slider extends React.Component {
     orientation: CarouselPropTypes.orientation.isRequired,
     playDirection: CarouselPropTypes.direction.isRequired,
     privateUnDisableAnimation: PropTypes.bool,
+    role: PropTypes.string,
     slideSize: PropTypes.number.isRequired,
     slideTraySize: PropTypes.number.isRequired,
     spinner: PropTypes.func,
     step: PropTypes.number.isRequired,
     style: PropTypes.object,
-    tabIndex: PropTypes.number,
     totalSlides: PropTypes.number.isRequired,
     touchEnabled: PropTypes.bool.isRequired,
     trayProps: PropTypes.shape({
@@ -78,9 +78,9 @@ const Slider = class Slider extends React.Component {
     moveThreshold: 0.1,
     onMasterSpinner: null,
     privateUnDisableAnimation: false,
+    role: 'listbox',
     spinner: null,
     style: {},
-    tabIndex: null,
     trayProps: {},
     trayTag: 'div',
     visibleSlides: 1,
@@ -582,7 +582,6 @@ const Slider = class Slider extends React.Component {
       slideTraySize,
       spinner,
       style,
-      tabIndex,
       totalSlides,
       touchEnabled,
       trayProps,
@@ -654,7 +653,7 @@ const Slider = class Slider extends React.Component {
       classNameTray,
     ]);
 
-    const newTabIndex = tabIndex !== null ? tabIndex : 0;
+
 
     // remove invalid div attributes
     const {
@@ -691,10 +690,9 @@ const Slider = class Slider extends React.Component {
         className={sliderClasses}
         aria-live="polite"
         aria-label={ariaLabel}
+        role={this.props.role}
         style={sliderStyle}
-        tabIndex={newTabIndex}
         onKeyDown={this.handleOnKeyDown}
-        role="listbox"
         {...rest}
       >
         <div className={trayWrapClasses} style={trayWrapStyle}>

diff --git a/src/Slider/__tests__/Slider.test.jsx b/src/Slider/__tests__/Slider.test.jsx
@@ -935,16 +935,6 @@ describe('<Slider />', () => {
       expect(wrapper.prop('carouselStore').state.currentSlide).toBe(1);
     });
 
-    it('the .carousel__slider should have a default tabIndex of 0', () => {
-      const wrapper = shallow(<Slider {...props} />);
-      expect(wrapper.find('.carousel__slider').prop('tabIndex')).toBe(0);
-    });
-
-    it('override the default tabIndex for .carousel__slider if a tabIndex prop is passed to this component', () => {
-      const wrapper = shallow(<Slider {...props} tabIndex={-1} />);
-      expect(wrapper.find('.carousel__slider').prop('tabIndex')).toBe(-1);
-    });
-
     it('should not call this.focus() if totalSlides <= visibleSlides', () => {
       const wrapper = shallow(<Slider {...props} totalSlides={2} visibleSlides={2} />);
       const instance = wrapper.instance();