Skip to content

Commit

Permalink
fix(carousel): fix hidden carousel indicators
Browse files Browse the repository at this point in the history 
- 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
  • Loading branch information
@maxokorokov
maxokorokov committed Feb 22, 2022
1 parent 1425962 commit 0033c78
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 51 deletions.
80 changes: 37 additions & 43 deletions src/carousel/carousel.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,15 @@ import {NgbSlideEventDirection} from './carousel-transition';
const createTestComponent = (html: string, detectChanges = true) =>
createGenericTestComponent(html, TestComponent, detectChanges) as ComponentFixture<TestComponent>;

const getSlideElements = (el: HTMLElement) => Array.from(el.querySelectorAll<HTMLButtonElement>('.carousel-item'));
const getIndicatorElements = (el: HTMLElement) =>
Array.from(el.querySelectorAll<HTMLButtonElement>('.carousel-indicators > button[data-bs-target]'));
const getArrowElements = (el: HTMLElement) =>
Array.from(el.querySelectorAll<HTMLButtonElement>('.carousel-inner ~ button'));

function expectActiveSlides(nativeEl: HTMLDivElement, active: boolean[]) {
const slideElms = nativeEl.querySelectorAll('.carousel-item');
const indicatorElms = nativeEl.querySelectorAll('ol.carousel-indicators > li');
const slideElms = getSlideElements(nativeEl);
const indicatorElms = getIndicatorElements(nativeEl);
const carouselElm = nativeEl.querySelector('ngb-carousel');

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

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

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

discardPeriodicTasks();
}));
Expand All @@ -95,11 +101,8 @@ describe('ngb-carousel', () => {
tick(1001);
fixture.detectChanges();

const carousel = fixture.nativeElement.querySelector('ngb-carousel');
const slides = fixture.nativeElement.querySelectorAll('.carousel-item');

expect(carousel).toBeTruthy();
expect(slides.length).toBe(0);
expect(fixture.nativeElement.querySelector('ngb-carousel')).toBeTruthy();
expect(getSlideElements(fixture.nativeElement).length).toBe(0);

discardPeriodicTasks();
}));
Expand Down Expand Up @@ -217,9 +220,8 @@ describe('ngb-carousel', () => {
const fixture = createTestComponent(html);
const spyCallBack = spyOn(fixture.componentInstance, 'carouselSlideCallBack');
const carouselDebugEl = fixture.debugElement.query(By.directive(NgbCarousel));
const indicatorElms = fixture.nativeElement.querySelectorAll('ol.carousel-indicators > li');
const prevControlElm = fixture.nativeElement.querySelector('.carousel-control-prev');
const nextControlElm = fixture.nativeElement.querySelector('.carousel-control-next');
const indicatorElms = getIndicatorElements(fixture.nativeElement);
const[prevControlElm, nextControlElm] = getArrowElements(fixture.nativeElement);
const next = fixture.nativeElement.querySelector('#next');
const pause = fixture.nativeElement.querySelector('#pause');
const cycle = fixture.nativeElement.querySelector('#cycle');
Expand Down Expand Up @@ -353,7 +355,7 @@ describe('ngb-carousel', () => {
`;

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

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

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

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

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

const fixture = createTestComponent(html);

const prevControlElm = fixture.nativeElement.querySelector('.carousel-control-prev');
const nextControlElm = fixture.nativeElement.querySelector('.carousel-control-next');
const[prevControlElm, nextControlElm] = getArrowElements(fixture.nativeElement);

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

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

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

Expand Down Expand Up @@ -736,9 +735,7 @@ describe('ngb-carousel', () => {
`;

const fixture = createTestComponent(html);

const prevControlElm = fixture.nativeElement.querySelector('.carousel-control-prev');
const nextControlElm = fixture.nativeElement.querySelector('.carousel-control-next');
const[prevControlElm, nextControlElm] = getArrowElements(fixture.nativeElement);

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

Expand Down Expand Up @@ -766,9 +763,7 @@ describe('ngb-carousel', () => {
`;

const fixture = createTestComponent(html);

const prevControlElm = fixture.nativeElement.querySelector('.carousel-control-prev');
const nextControlElm = fixture.nativeElement.querySelector('.carousel-control-next');
const[prevControlElm, nextControlElm] = getArrowElements(fixture.nativeElement);

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

Expand Down Expand Up @@ -852,16 +847,16 @@ describe('ngb-carousel', () => {
`;
const fixture = createTestComponent(html);

const slideElms = fixture.nativeElement.querySelectorAll('.carousel-item');
const slideElms = getSlideElements(fixture.nativeElement);
expect(slideElms.length).toBe(1);
expect(slideElms[0].textContent).toMatch(/foo/);
expect(fixture.nativeElement.querySelectorAll('ol.carousel-indicators.visually-hidden > li').length).toBe(0);
expect(fixture.nativeElement.querySelectorAll('ol.carousel-indicators > li').length).toBe(1);
expect(fixture.nativeElement.querySelectorAll('.carousel-indicators.visually-hidden > button').length).toBe(0);
expect(getIndicatorElements(fixture.nativeElement).length).toBe(1);

fixture.componentInstance.showNavigationIndicators = false;
fixture.detectChanges();
expect(fixture.nativeElement.querySelectorAll('ol.carousel-indicators.visually-hidden > li').length).toBe(1);
expect(fixture.nativeElement.querySelectorAll('ol.carousel-indicators > li').length).toBe(1);
expect(fixture.nativeElement.querySelectorAll('.carousel-indicators.visually-hidden > button').length).toBe(1);
expect(getIndicatorElements(fixture.nativeElement).length).toBe(1);

discardPeriodicTasks();
}));
Expand All @@ -874,13 +869,12 @@ describe('ngb-carousel', () => {
`;
const fixture = createTestComponent(html);

const slideElms = fixture.nativeElement.querySelectorAll('.carousel-item');
expect(slideElms.length).toBe(1);
expect(fixture.nativeElement.querySelectorAll('[role="button"]').length).toBe(2);
expect(getSlideElements(fixture.nativeElement).length).toBe(1);
expect(getArrowElements(fixture.nativeElement).length).toBe(2);

fixture.componentInstance.showNavigationArrows = false;
fixture.detectChanges();
expect(fixture.nativeElement.querySelectorAll('[role="button"]').length).toBe(0);
expect(getArrowElements(fixture.nativeElement).length).toBe(0);

discardPeriodicTasks();
}));
Expand Down Expand Up @@ -983,8 +977,8 @@ if (isBrowserVisible('ngb-carousel animations')) {

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

const[slideOne, slideTwo] = nativeEl.querySelectorAll('.carousel-item');
const indicators = nativeEl.querySelectorAll('ol.carousel-indicators > li');
const[slideOne, slideTwo] = getSlideElements(nativeEl);
const indicators = getIndicatorElements(nativeEl);

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

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

const[slideOne, slideTwo] = nativeEl.querySelectorAll('.carousel-item');
const indicators = nativeEl.querySelectorAll('ol.carousel-indicators > li');
const[slideOne, slideTwo] = getSlideElements(nativeEl);
const indicators = getIndicatorElements(nativeEl);

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

const nativeEl = fixture.nativeElement;
const[slideOne, slideTwo, slideThree] = nativeEl.querySelectorAll('.carousel-item');
const indicators = nativeEl.querySelectorAll('ol.carousel-indicators > li');
const[slideOne, slideTwo, slideThree] = getSlideElements(nativeEl);
const indicators = getIndicatorElements(nativeEl);

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

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

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

expect(slideOne.className).toBe('carousel-item active');
expect(slideTwo.className).toBe('carousel-item');
Expand Down
16 changes: 8 additions & 8 deletions src/carousel/carousel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,12 @@ export class NgbSlide {
'[attr.aria-activedescendant]': `'slide-' + activeId`
},
template: `
<ol class="carousel-indicators" [class.visually-hidden]="!showNavigationIndicators" role="tablist">
<li *ngFor="let slide of slides" [class.active]="slide.id === activeId"
<div class="carousel-indicators" [class.visually-hidden]="!showNavigationIndicators" role="tablist">
<button type="button" data-bs-target *ngFor="let slide of slides" [class.active]="slide.id === activeId"
role="tab" [attr.aria-labelledby]="'slide-' + slide.id" [attr.aria-controls]="'slide-' + slide.id"
[attr.aria-selected]="slide.id === activeId"
(click)="focus();select(slide.id, NgbSlideEventSource.INDICATOR);"></li>
</ol>
(click)="focus();select(slide.id, NgbSlideEventSource.INDICATOR);"></button>
</div>
<div class="carousel-inner">
<div *ngFor="let slide of slides; index as i; count as c" class="carousel-item" [id]="'slide-' + slide.id" role="tabpanel">
<span class="visually-hidden" i18n="Currently selected slide number read by screen reader@@ngb.carousel.slide-number">
Expand All @@ -94,14 +94,14 @@ export class NgbSlide {
<ng-template [ngTemplateOutlet]="slide.tplRef"></ng-template>
</div>
</div>
<a class="carousel-control-prev" role="button" (click)="arrowLeft()" *ngIf="showNavigationArrows">
<button class="carousel-control-prev" type="button" (click)="arrowLeft()" *ngIf="showNavigationArrows">
<span class="carousel-control-prev-icon" aria-hidden="true"></span>
<span class="visually-hidden" i18n="@@ngb.carousel.previous">Previous</span>
</a>
<a class="carousel-control-next" role="button" (click)="arrowRight()" *ngIf="showNavigationArrows">
</button>
<button class="carousel-control-next" type="button" (click)="arrowRight()" *ngIf="showNavigationArrows">
<span class="carousel-control-next-icon" aria-hidden="true"></span>
<span class="visually-hidden" i18n="@@ngb.carousel.next">Next</span>
</a>
</button>
`
})
export class NgbCarousel implements AfterContentChecked,
Expand Down

0 comments on commit 0033c78

Please sign in to comment.