From 20073e10c934d3704734195c72f4281c9b9658e3 Mon Sep 17 00:00:00 2001 From: Brandy Carney Date: Thu, 8 Aug 2024 11:02:35 -0400 Subject: [PATCH] fix(angular): remove the tabindex set by routerLink from Ionic components (#29744) Issue number: resolves #20632 --------- ## What is the current behavior? When using the `routerLink` directive in Angular, it automatically adds `tabindex="0"` to the element. This creates issues with Ionic components that render native button or anchor elements, as they have their own focus management. As a result, when navigating between list items with `routerLink` using the `Tab` key, you need to press the `Tab` key twice to move to the next item. This problem is illustrated in the following demo: [![Open in StackBlitz](https://developer.stackblitz.com/img/open_in_stackblitz.svg)](https://stackblitz.com/edit/angular-blfa7h?file=src%2Fapp%2Fexample.component.html) Related Angular issue: https://github.com/angular/angular/issues/28345 ## What is the new behavior? Updated our `RouterLinkDelegateDirective` to check if the element using `routerLink` is one of the following Ionic components: `ion-back-button`, `ion-breadcrumb`, `ion-button`, `ion-card`, `ion-fab-button`, `ion-item`, `ion-item-option`, `ion-menu-button`, `ion-segment-button`, or `ion-tab-button`. If so, it removes the `tabindex` attribute from the element. This allows these Ionic components to let the native button or anchor element handle the focus. This solution is demonstrated in the following demo: [![Open in StackBlitz](https://developer.stackblitz.com/img/open_in_stackblitz.svg)](https://stackblitz.com/edit/angular-blfa7h-svmguh?file=src%2Fapp%2Fexample.component.html) > [!NOTE] > I did not include the `ion-router-link` component in the list to remove `tabindex` because [the router link documentation](https://ionicframework.com/docs/api/router-link) does not recommend using it with Angular: >> Note: this component should only be used with vanilla and Stencil JavaScript projects. For Angular projects, use an `` and `routerLink` with the Angular router. ## Does this introduce a breaking change? - [ ] Yes - [x] No ## Other information Dev build: `8.2.7-dev.11722448707.1e8c66e6` --- .../navigation/router-link-delegate.ts | 33 +++++++++++++++++++ .../base/e2e/src/lazy/router-link.spec.ts | 15 +++++++++ .../e2e/src/standalone/router-link.spec.ts | 15 ++++++++- .../router-link/router-link.component.html | 1 + .../router-link/router-link.component.html | 6 ++++ 5 files changed, 69 insertions(+), 1 deletion(-) diff --git a/packages/angular/common/src/directives/navigation/router-link-delegate.ts b/packages/angular/common/src/directives/navigation/router-link-delegate.ts index d996d73942..4391d6ea0d 100644 --- a/packages/angular/common/src/directives/navigation/router-link-delegate.ts +++ b/packages/angular/common/src/directives/navigation/router-link-delegate.ts @@ -31,12 +31,45 @@ export class RouterLinkDelegateDirective implements OnInit, OnChanges { ngOnInit(): void { this.updateTargetUrlAndHref(); + this.updateTabindex(); } ngOnChanges(): void { this.updateTargetUrlAndHref(); } + /** + * The `tabindex` is set to `0` by default on the host element when + * the `routerLink` directive is used. This causes issues with Ionic + * components that wrap an `a` or `button` element, such as `ion-item`. + * See issue https://github.com/angular/angular/issues/28345 + * + * This method removes the `tabindex` attribute from the host element + * to allow the Ionic component to manage the focus state correctly. + */ + private updateTabindex() { + // Ionic components that render a native anchor or button element + const ionicComponents = [ + 'ION-BACK-BUTTON', + 'ION-BREADCRUMB', + 'ION-BUTTON', + 'ION-CARD', + 'ION-FAB-BUTTON', + 'ION-ITEM', + 'ION-ITEM-OPTION', + 'ION-MENU-BUTTON', + 'ION-SEGMENT-BUTTON', + 'ION-TAB-BUTTON', + ]; + const hostElement = this.elementRef.nativeElement; + + if (ionicComponents.includes(hostElement.tagName)) { + if (hostElement.getAttribute('tabindex') === '0') { + hostElement.removeAttribute('tabindex'); + } + } + } + private updateTargetUrlAndHref() { if (this.routerLink?.urlTree) { const href = this.locationStrategy.prepareExternalUrl(this.router.serializeUrl(this.routerLink.urlTree)); diff --git a/packages/angular/test/base/e2e/src/lazy/router-link.spec.ts b/packages/angular/test/base/e2e/src/lazy/router-link.spec.ts index 5550d6326f..d8513540de 100644 --- a/packages/angular/test/base/e2e/src/lazy/router-link.spec.ts +++ b/packages/angular/test/base/e2e/src/lazy/router-link.spec.ts @@ -123,6 +123,21 @@ describe('Router Link', () => { testBack(); }); }); + + // Angular sets the `tabindex` to `"0"` on any element that uses + // the `routerLink` directive. Ionic removes the `tabindex` from + // components that wrap an `a` or `button` element, so we are + // checking here that it is only removed from Ionic components. + // https://github.com/ionic-team/ionic-framework/issues/20632 + describe('tabindex', () => { + it('should have tabindex="0" with a native span', () => { + cy.get('#span').should('have.attr', 'tabindex', '0'); + }); + + it('should not have tabindex set with an ionic button', () => { + cy.get('#routerLink').should('not.have.attr', 'tabindex'); + }); + }); }); function testForward() { diff --git a/packages/angular/test/base/e2e/src/standalone/router-link.spec.ts b/packages/angular/test/base/e2e/src/standalone/router-link.spec.ts index ce8724876a..9550b4f1dc 100644 --- a/packages/angular/test/base/e2e/src/standalone/router-link.spec.ts +++ b/packages/angular/test/base/e2e/src/standalone/router-link.spec.ts @@ -2,7 +2,7 @@ describe('RouterLink', () => { beforeEach(() => { cy.visit('/standalone/router-link'); }); - + it('should mount the root component', () => { cy.ionPageVisible('app-router-link'); @@ -21,4 +21,17 @@ describe('RouterLink', () => { cy.url().should('include', '/standalone/popover'); }); + + // Angular sets the `tabindex` to `"0"` on any element that uses + // the `routerLink` directive. Ionic removes the `tabindex` from + // components that wrap an `a` or `button` element, so we are + // checking here that it is only removed from Ionic components. + // https://github.com/ionic-team/ionic-framework/issues/20632 + it('should have tabindex="0" with a native span', () => { + cy.get('span').should('have.attr', 'tabindex', '0'); + }); + + it('should not have tabindex set with an ionic button', () => { + cy.get('ion-button').should('not.have.attr', 'tabindex'); + }); }); diff --git a/packages/angular/test/base/src/app/lazy/router-link/router-link.component.html b/packages/angular/test/base/src/app/lazy/router-link/router-link.component.html index 791b6a5442..6c9f1f00eb 100644 --- a/packages/angular/test/base/src/app/lazy/router-link/router-link.component.html +++ b/packages/angular/test/base/src/app/lazy/router-link/router-link.component.html @@ -36,4 +36,5 @@

+

span[routerLink]

diff --git a/packages/angular/test/base/src/app/standalone/router-link/router-link.component.html b/packages/angular/test/base/src/app/standalone/router-link/router-link.component.html index 078d4b7c3a..8887626d1c 100644 --- a/packages/angular/test/base/src/app/standalone/router-link/router-link.component.html +++ b/packages/angular/test/base/src/app/standalone/router-link/router-link.component.html @@ -4,3 +4,9 @@ + + I'm a span + + + I'm an ion-button +