Skip to content

Commit 0033c78

Browse files
maxokorokovmaxokorokov
committed
fix(carousel): fix hidden carousel indicators
- In Bootstrap 5 the CSS selector matching indicators has changed and included `data-bs-target` attribute that we were missing - Recommended carousel markup changed from using `ul`, `li` and `a` to using `button` elements - Had to refactor tests as selectors were not precise enough On Bootstrap side see: - twbs/bootstrap#32661 - twbs/bootstrap#32627 Fixes ng-bootstrap#4200 Fixes ng-bootstrap#4253
1 parent 1425962 commit 0033c78

File tree

2 files changed

+45
-51
lines changed

2 files changed

+45
-51
lines changed

‎src/carousel/carousel.spec.ts

Lines changed: 37 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,15 @@ import {NgbSlideEventDirection} from './carousel-transition';
1414
const createTestComponent = (html: string, detectChanges = true) =>
1515
createGenericTestComponent(html, TestComponent, detectChanges) as ComponentFixture<TestComponent>;
1616

17+
const getSlideElements = (el: HTMLElement) => Array.from(el.querySelectorAll<HTMLButtonElement>('.carousel-item'));
18+
const getIndicatorElements = (el: HTMLElement) =>
19+
Array.from(el.querySelectorAll<HTMLButtonElement>('.carousel-indicators > button[data-bs-target]'));
20+
const getArrowElements = (el: HTMLElement) =>
21+
Array.from(el.querySelectorAll<HTMLButtonElement>('.carousel-inner ~ button'));
22+
1723
function expectActiveSlides(nativeEl: HTMLDivElement, active: boolean[]) {
18-
const slideElms = nativeEl.querySelectorAll('.carousel-item');
19-
const indicatorElms = nativeEl.querySelectorAll('ol.carousel-indicators > li');
24+
const slideElms = getSlideElements(nativeEl);
25+
const indicatorElms = getIndicatorElements(nativeEl);
2026
const carouselElm = nativeEl.querySelector('ngb-carousel');
2127

2228
expect(slideElms.length).toBe(active.length);
@@ -63,13 +69,13 @@ describe('ngb-carousel', () => {
6369
`;
6470
const fixture = createTestComponent(html);
6571

66-
const slideElms = fixture.nativeElement.querySelectorAll('.carousel-item');
72+
const slideElms = getSlideElements(fixture.nativeElement);
6773
expect(slideElms.length).toBe(2);
6874
expect(slideElms[0].textContent).toMatch(/foo/);
6975
expect(slideElms[1].textContent).toMatch(/bar/);
7076

71-
expect(fixture.nativeElement.querySelectorAll('ol.carousel-indicators > li').length).toBe(2);
72-
expect(fixture.nativeElement.querySelectorAll('[role="button"]').length).toBe(2);
77+
expect(getIndicatorElements(fixture.nativeElement).length).toBe(2);
78+
expect(getArrowElements(fixture.nativeElement).length).toBe(2);
7379

7480
discardPeriodicTasks();
7581
}));
@@ -95,11 +101,8 @@ describe('ngb-carousel', () => {
95101
tick(1001);
96102
fixture.detectChanges();
97103

98-
const carousel = fixture.nativeElement.querySelector('ngb-carousel');
99-
const slides = fixture.nativeElement.querySelectorAll('.carousel-item');
100-
101-
expect(carousel).toBeTruthy();
102-
expect(slides.length).toBe(0);
104+
expect(fixture.nativeElement.querySelector('ngb-carousel')).toBeTruthy();
105+
expect(getSlideElements(fixture.nativeElement).length).toBe(0);
103106

104107
discardPeriodicTasks();
105108
}));
@@ -217,9 +220,8 @@ describe('ngb-carousel', () => {
217220
const fixture = createTestComponent(html);
218221
const spyCallBack = spyOn(fixture.componentInstance, 'carouselSlideCallBack');
219222
const carouselDebugEl = fixture.debugElement.query(By.directive(NgbCarousel));
220-
const indicatorElms = fixture.nativeElement.querySelectorAll('ol.carousel-indicators > li');
221-
const prevControlElm = fixture.nativeElement.querySelector('.carousel-control-prev');
222-
const nextControlElm = fixture.nativeElement.querySelector('.carousel-control-next');
223+
const indicatorElms = getIndicatorElements(fixture.nativeElement);
224+
const[prevControlElm, nextControlElm] = getArrowElements(fixture.nativeElement);
223225
const next = fixture.nativeElement.querySelector('#next');
224226
const pause = fixture.nativeElement.querySelector('#pause');
225227
const cycle = fixture.nativeElement.querySelector('#cycle');
@@ -353,7 +355,7 @@ describe('ngb-carousel', () => {
353355
`;
354356

355357
const fixture = createTestComponent(html);
356-
const indicatorElms = fixture.nativeElement.querySelectorAll('ol.carousel-indicators > li');
358+
const indicatorElms = getIndicatorElements(fixture.nativeElement);
357359

358360
expectActiveSlides(fixture.nativeElement, [true, false]);
359361

@@ -374,7 +376,7 @@ describe('ngb-carousel', () => {
374376
`;
375377

376378
const fixture = createTestComponent(html);
377-
const indicatorElms = fixture.nativeElement.querySelectorAll('ol.carousel-indicators > li');
379+
const indicatorElms = getIndicatorElements(fixture.nativeElement);
378380
const spyCallBack = spyOn(fixture.componentInstance, 'carouselSlideCallBack');
379381

380382
indicatorElms[1].click();
@@ -412,9 +414,7 @@ describe('ngb-carousel', () => {
412414
`;
413415

414416
const fixture = createTestComponent(html);
415-
416-
const prevControlElm = fixture.nativeElement.querySelector('.carousel-control-prev');
417-
const nextControlElm = fixture.nativeElement.querySelector('.carousel-control-next');
417+
const[prevControlElm, nextControlElm] = getArrowElements(fixture.nativeElement);
418418

419419
expectActiveSlides(fixture.nativeElement, [true, false]);
420420

@@ -438,8 +438,7 @@ describe('ngb-carousel', () => {
438438
`;
439439

440440
const fixture = createTestComponent(html);
441-
const prevControlElm = fixture.nativeElement.querySelector('.carousel-control-prev');
442-
const nextControlElm = fixture.nativeElement.querySelector('.carousel-control-next');
441+
const[prevControlElm, nextControlElm] = getArrowElements(fixture.nativeElement);
443442
const spyCallBack = spyOn(fixture.componentInstance, 'carouselSlideCallBack');
444443
const spySingleCallBack = spyOn(fixture.componentInstance, 'carouselSingleSlideCallBack');
445444

@@ -736,9 +735,7 @@ describe('ngb-carousel', () => {
736735
`;
737736

738737
const fixture = createTestComponent(html);
739-
740-
const prevControlElm = fixture.nativeElement.querySelector('.carousel-control-prev');
741-
const nextControlElm = fixture.nativeElement.querySelector('.carousel-control-next');
738+
const[prevControlElm, nextControlElm] = getArrowElements(fixture.nativeElement);
742739

743740
expectActiveSlides(fixture.nativeElement, [true, false]);
744741

@@ -766,9 +763,7 @@ describe('ngb-carousel', () => {
766763
`;
767764

768765
const fixture = createTestComponent(html);
769-
770-
const prevControlElm = fixture.nativeElement.querySelector('.carousel-control-prev');
771-
const nextControlElm = fixture.nativeElement.querySelector('.carousel-control-next');
766+
const[prevControlElm, nextControlElm] = getArrowElements(fixture.nativeElement);
772767

773768
expectActiveSlides(fixture.nativeElement, [true, false]);
774769

@@ -852,16 +847,16 @@ describe('ngb-carousel', () => {
852847
`;
853848
const fixture = createTestComponent(html);
854849

855-
const slideElms = fixture.nativeElement.querySelectorAll('.carousel-item');
850+
const slideElms = getSlideElements(fixture.nativeElement);
856851
expect(slideElms.length).toBe(1);
857852
expect(slideElms[0].textContent).toMatch(/foo/);
858-
expect(fixture.nativeElement.querySelectorAll('ol.carousel-indicators.visually-hidden > li').length).toBe(0);
859-
expect(fixture.nativeElement.querySelectorAll('ol.carousel-indicators > li').length).toBe(1);
853+
expect(fixture.nativeElement.querySelectorAll('.carousel-indicators.visually-hidden > button').length).toBe(0);
854+
expect(getIndicatorElements(fixture.nativeElement).length).toBe(1);
860855

861856
fixture.componentInstance.showNavigationIndicators = false;
862857
fixture.detectChanges();
863-
expect(fixture.nativeElement.querySelectorAll('ol.carousel-indicators.visually-hidden > li').length).toBe(1);
864-
expect(fixture.nativeElement.querySelectorAll('ol.carousel-indicators > li').length).toBe(1);
858+
expect(fixture.nativeElement.querySelectorAll('.carousel-indicators.visually-hidden > button').length).toBe(1);
859+
expect(getIndicatorElements(fixture.nativeElement).length).toBe(1);
865860

866861
discardPeriodicTasks();
867862
}));
@@ -874,13 +869,12 @@ describe('ngb-carousel', () => {
874869
`;
875870
const fixture = createTestComponent(html);
876871

877-
const slideElms = fixture.nativeElement.querySelectorAll('.carousel-item');
878-
expect(slideElms.length).toBe(1);
879-
expect(fixture.nativeElement.querySelectorAll('[role="button"]').length).toBe(2);
872+
expect(getSlideElements(fixture.nativeElement).length).toBe(1);
873+
expect(getArrowElements(fixture.nativeElement).length).toBe(2);
880874

881875
fixture.componentInstance.showNavigationArrows = false;
882876
fixture.detectChanges();
883-
expect(fixture.nativeElement.querySelectorAll('[role="button"]').length).toBe(0);
877+
expect(getArrowElements(fixture.nativeElement).length).toBe(0);
884878

885879
discardPeriodicTasks();
886880
}));
@@ -983,8 +977,8 @@ if (isBrowserVisible('ngb-carousel animations')) {
983977

984978
const onSlidSpy = spyOn(fixture.componentInstance, 'onSlid');
985979

986-
const[slideOne, slideTwo] = nativeEl.querySelectorAll('.carousel-item');
987-
const indicators = nativeEl.querySelectorAll('ol.carousel-indicators > li');
980+
const[slideOne, slideTwo] = getSlideElements(nativeEl);
981+
const indicators = getIndicatorElements(nativeEl);
988982

989983
onSlidSpy.and.callFake((payload) => {
990984
expect(slideOne.className).toBe('carousel-item');
@@ -1014,8 +1008,8 @@ if (isBrowserVisible('ngb-carousel animations')) {
10141008

10151009
const onSlidSpy = spyOn(fixture.componentInstance, 'onSlid');
10161010

1017-
const[slideOne, slideTwo] = nativeEl.querySelectorAll('.carousel-item');
1018-
const indicators = nativeEl.querySelectorAll('ol.carousel-indicators > li');
1011+
const[slideOne, slideTwo] = getSlideElements(nativeEl);
1012+
const indicators = getIndicatorElements(nativeEl);
10191013

10201014
expect(slideOne.className).toBe('carousel-item active');
10211015
expect(slideTwo.className).toBe('carousel-item');
@@ -1037,8 +1031,8 @@ if (isBrowserVisible('ngb-carousel animations')) {
10371031
fixture.detectChanges();
10381032

10391033
const nativeEl = fixture.nativeElement;
1040-
const[slideOne, slideTwo, slideThree] = nativeEl.querySelectorAll('.carousel-item');
1041-
const indicators = nativeEl.querySelectorAll('ol.carousel-indicators > li');
1034+
const[slideOne, slideTwo, slideThree] = getSlideElements(nativeEl);
1035+
const indicators = getIndicatorElements(nativeEl);
10421036

10431037
const onSlidSpy = spyOn(fixture.componentInstance, 'onSlid');
10441038
onSlidSpy.and.callFake((payload) => {
@@ -1088,8 +1082,8 @@ if (isBrowserVisible('ngb-carousel animations')) {
10881082

10891083
const onSlidSpy = spyOn(fixture.componentInstance, 'onSlid');
10901084

1091-
const[slideOne, slideTwo, slideThree] = nativeEl.querySelectorAll('.carousel-item');
1092-
const indicators = nativeEl.querySelectorAll('ol.carousel-indicators > li');
1085+
const[slideOne, slideTwo, slideThree] = getSlideElements(nativeEl);
1086+
const indicators = getIndicatorElements(nativeEl);
10931087

10941088
expect(slideOne.className).toBe('carousel-item active');
10951089
expect(slideTwo.className).toBe('carousel-item');

‎src/carousel/carousel.ts

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -80,12 +80,12 @@ export class NgbSlide {
8080
'[attr.aria-activedescendant]': `'slide-' + activeId`
8181
},
8282
template: `
83-
<ol class="carousel-indicators" [class.visually-hidden]="!showNavigationIndicators" role="tablist">
84-
<li *ngFor="let slide of slides" [class.active]="slide.id === activeId"
83+
<div class="carousel-indicators" [class.visually-hidden]="!showNavigationIndicators" role="tablist">
84+
<button type="button" data-bs-target *ngFor="let slide of slides" [class.active]="slide.id === activeId"
8585
role="tab" [attr.aria-labelledby]="'slide-' + slide.id" [attr.aria-controls]="'slide-' + slide.id"
8686
[attr.aria-selected]="slide.id === activeId"
87-
(click)="focus();select(slide.id, NgbSlideEventSource.INDICATOR);"></li>
88-
</ol>
87+
(click)="focus();select(slide.id, NgbSlideEventSource.INDICATOR);"></button>
88+
</div>
8989
<div class="carousel-inner">
9090
<div *ngFor="let slide of slides; index as i; count as c" class="carousel-item" [id]="'slide-' + slide.id" role="tabpanel">
9191
<span class="visually-hidden" i18n="Currently selected slide number read by screen reader@@ngb.carousel.slide-number">
@@ -94,14 +94,14 @@ export class NgbSlide {
9494
<ng-template [ngTemplateOutlet]="slide.tplRef"></ng-template>
9595
</div>
9696
</div>
97-
<a class="carousel-control-prev" role="button" (click)="arrowLeft()" *ngIf="showNavigationArrows">
97+
<button class="carousel-control-prev" type="button" (click)="arrowLeft()" *ngIf="showNavigationArrows">
9898
<span class="carousel-control-prev-icon" aria-hidden="true"></span>
9999
<span class="visually-hidden" i18n="@@ngb.carousel.previous">Previous</span>
100-
</a>
101-
<a class="carousel-control-next" role="button" (click)="arrowRight()" *ngIf="showNavigationArrows">
100+
</button>
101+
<button class="carousel-control-next" type="button" (click)="arrowRight()" *ngIf="showNavigationArrows">
102102
<span class="carousel-control-next-icon" aria-hidden="true"></span>
103103
<span class="visually-hidden" i18n="@@ngb.carousel.next">Next</span>
104-
</a>
104+
</button>
105105
`
106106
})
107107
export class NgbCarousel implements AfterContentChecked,

0 commit comments

Comments
 (0)