Skip to content

Commit

Permalink
fix: Different approach
Browse files Browse the repository at this point in the history
  • Loading branch information
@sdrozdsap
sdrozdsap committed Nov 26, 2024
1 parent 72f79bf commit a1a629e
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 101 deletions.
85 changes: 17 additions & 68 deletions projects/storefrontlib/shared/components/carousel/carousel.component.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { ComponentFixture, TestBed, waitForAsync } from '@angular/core/testing';
import { By } from '@angular/platform-browser';
import { RouterTestingModule } from '@angular/router/testing';
import { I18nTestingModule, LoggerService, Product } from '@spartacus/core';
import { ICON_TYPE, SelectFocusUtility } from '@spartacus/storefront';
import { ICON_TYPE } from '@spartacus/storefront';
import { EMPTY, Observable, of } from 'rxjs';
import { CarouselComponent } from './carousel.component';
import { CarouselService } from './carousel.service';
Expand All @@ -18,12 +18,6 @@ class MockCarouselService {
}
}

class MockSelectFocusUtility {
findFocusable(_element: HTMLElement): HTMLElement[] {
return [];
}
}

@Component({
selector: 'cx-icon',
template: '',
Expand Down Expand Up @@ -52,7 +46,6 @@ describe('Carousel Component', () => {
let component: CarouselComponent;
let fixture: ComponentFixture<CarouselComponent>;
let service: CarouselService;
let selectFocusUtil: MockSelectFocusUtility;

let templateFixture: ComponentFixture<MockTemplateComponent>;
let template: TemplateRef<any>;
Expand All @@ -65,20 +58,14 @@ describe('Carousel Component', () => {
MockTemplateComponent,
MockFeatureDirective,
],
providers: [
{ provide: CarouselService, useClass: MockCarouselService },
{ provide: SelectFocusUtility, useClass: MockSelectFocusUtility },
],
providers: [{ provide: CarouselService, useClass: MockCarouselService }],
}).compileComponents();
}));

beforeEach(() => {
fixture = TestBed.createComponent(CarouselComponent);
component = fixture.componentInstance;
service = TestBed.inject(CarouselService);
selectFocusUtil = TestBed.inject(
SelectFocusUtility
) as unknown as MockSelectFocusUtility;

templateFixture = TestBed.createComponent(MockTemplateComponent);
const compiled = templateFixture.debugElement.nativeElement;
Expand Down Expand Up @@ -610,8 +597,8 @@ describe('Carousel Component', () => {
});
});

describe('handleTab', () => {
let nativeElement: HTMLElement;
describe('skipTabForCarouselItems', () => {
let carouselItems: HTMLElement[] = [];
beforeEach(() => {
component.template = template;
nativeElement = fixture.nativeElement;
Expand All @@ -620,64 +607,26 @@ describe('Carousel Component', () => {
const element = document.createElement('div');
element.setAttribute('cxFocusableCarouselItem', '');
nativeElement.appendChild(element);
carouselItems.push(element);
}
fixture.detectChanges();
});

it('should move focus to the next focusable element when Tab is pressed', () => {
const carouselItems: HTMLElement[] = Array.from(
nativeElement.querySelectorAll('[cxFocusableCarouselItem]')
);
const focusableElements = [
document.createElement('div'),
...carouselItems,
document.createElement('div'),
];
spyOn(selectFocusUtil, 'findFocusable').and.returnValue(
focusableElements
);
const event = new KeyboardEvent('keydown', { key: 'Tab' });
spyOn(event, 'preventDefault');
const expectedFocusedElement =
focusableElements[focusableElements.length - 1];
spyOn(expectedFocusedElement, 'focus');
carouselItems[1].dispatchEvent(event);
expect(event.preventDefault).toHaveBeenCalled();
expect(expectedFocusedElement.focus).toHaveBeenCalled();
});

it('should move focus to the previous focusable element when Shift+Tab is pressed', () => {
const carouselItems: HTMLElement[] = Array.from(
nativeElement.querySelectorAll('[cxFocusableCarouselItem]')
);
const focusableElements = [
document.createElement('div'),
...carouselItems,
document.createElement('div'),
];
spyOn(selectFocusUtil, 'findFocusable').and.returnValue(
focusableElements
);
const event = new KeyboardEvent('keydown', {
key: 'Tab',
shiftKey: true,
it('should set tabIndex to -1 for all carousel items', () => {
component['skipTabForCarouselItems']();
carouselItems.forEach((item) => {
expect(item.tabIndex).toBe(-1);
});
spyOn(event, 'preventDefault');
const expectedFocusedElement = focusableElements[0];
spyOn(expectedFocusedElement, 'focus');
carouselItems[0].dispatchEvent(event);
expect(event.preventDefault).toHaveBeenCalled();
expect(expectedFocusedElement.focus).toHaveBeenCalled();
});

it('should not move focus if there are no focusable elements', () => {
spyOn(selectFocusUtil, 'findFocusable').and.returnValue([]);
const event = new KeyboardEvent('keydown', { key: 'Tab' });
spyOn(event, 'preventDefault');

component['handleTab'](event);

expect(event.preventDefault).not.toHaveBeenCalled();
it('should restore tabIndex to 0 after a short delay', (done) => {
component['skipTabForCarouselItems']();
setTimeout(() => {
carouselItems.forEach((item) => {
expect(item.tabIndex).toBe(0);
});
done();
}, 100);
});
});
});
Expand Down
54 changes: 21 additions & 33 deletions projects/storefrontlib/shared/components/carousel/carousel.component.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import {
isDevMode,
OnChanges,
OnInit,
Optional,
Output,
TemplateRef,
} from '@angular/core';
Expand All @@ -22,8 +21,6 @@ import { BehaviorSubject, Observable } from 'rxjs';
import { tap } from 'rxjs/operators';
import { ICON_TYPE } from '../../../cms-components/misc/icon/icon.model';
import { CarouselService } from './carousel.service';
import { SelectFocusUtility } from '../../../layout/a11y/keyboard-focus/services/select-focus.util';
import { DOCUMENT } from '@angular/common';

/**
* Generic carousel component that can be used to render any carousel items,
Expand Down Expand Up @@ -92,10 +89,6 @@ export class CarouselComponent implements OnInit, OnChanges {
size$: Observable<number>;

protected logger = inject(LoggerService);
protected selectFocusUtil = inject(SelectFocusUtility);
@Optional() protected document = inject(DOCUMENT, {
optional: true,
});

constructor(
protected el: ElementRef,
Expand Down Expand Up @@ -129,43 +122,38 @@ export class CarouselComponent implements OnInit, OnChanges {
);
break;
case 'Tab':
this.handleTab(event);
this.skipTabForCarouselItems();
break;
}
}

/**
* Handles "Tab" and "Shift + Tab" keyboard navigation for the carousel component.
* Handles "Tab" navigation within the carousel.
*
* When the "Tab" key is pressed, this method moves the focus to the next focusable element
* outside the carousel, following the normal DOM order. When "Shift + Tab" is pressed,
* it moves the focus to the previous focusable element outside the carousel, following
* the normal DOM order.
* Temporarily removes all `cxFocusableCarouselItem` elements from the tab flow
* and restores them after a short delay. While using `setTimeout` may seem like
* a bad code smell, it is justified here as it ensures natural tabbing flow in
* cases where determining the next focusable element is complex(e.g. if `TrapFocusDirective` is used).
*
* The method uses all focusable elements in the DOM and identifies the boundaries of
* the carousel using elements with the `cxFocusableCarouselItem` directive. This ensures
* that this "Tab/Shift+Tab" override works only if `cxFocusableCarouselItem` is used
*
* @param {KeyboardEvent} event - The keyboard event triggered by the "Tab" or "Shift + Tab" key press.
* The `cxFocusableCarouselItem` selector is used because it identifies carousel
* items that have `ArrowRight`/`ArrowLeft` navigation enabled. These items should not
* use tab navigation according to a11y requirements.
*/
protected handleTab(event: KeyboardEvent): void {
const carouselElements: HTMLElement[] =
this.el.nativeElement.querySelectorAll('[cxFocusableCarouselItem]');
if (!carouselElements.length) {
return;
}
const focusableElements = this.selectFocusUtil.findFocusable(
this.document?.body
);
const index = focusableElements.indexOf(
carouselElements[event.shiftKey ? 0 : carouselElements.length - 1]
protected skipTabForCarouselItems(): void {
const carouselElements: HTMLElement[] = Array.from(
this.el.nativeElement.querySelectorAll('[cxFocusableCarouselItem]')
);
const direction = event.shiftKey ? -1 : 1;
if (!focusableElements[index + direction]) {
if (!carouselElements.length) {
return;
}
event.preventDefault();
focusableElements[index + direction].focus();
carouselElements.forEach((element) => {
element.tabIndex = -1;
});
setTimeout(() => {
carouselElements.forEach((element) => {
element.tabIndex = 0;
});
});
}

/**
Expand Down

0 comments on commit a1a629e

Please sign in to comment.