Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { LocationStrategy } from '@angular/common';
import { ElementRef, OnChanges, OnInit, Directive, HostListener, Input, Optional } from '@angular/core';
import { ElementRef, OnChanges, OnDestroy, OnInit, Directive, HostListener, Input, Optional } from '@angular/core';
import { Router, RouterLink } from '@angular/router';
import type { AnimationBuilder, RouterDirection } from '@ionic/core/components';

Expand All @@ -14,7 +14,7 @@ import { NavController } from '../../providers/nav-controller';
@Directive({
selector: ':not(a):not(area)[routerLink]',
})
export class RouterLinkDelegateDirective implements OnInit, OnChanges {
export class RouterLinkDelegateDirective implements OnInit, OnChanges, OnDestroy {
@Input()
routerDirection: RouterDirection = 'forward';

Expand All @@ -32,12 +32,51 @@ export class RouterLinkDelegateDirective implements OnInit, OnChanges {
ngOnInit(): void {
this.updateTargetUrlAndHref();
this.updateTabindex();

/**
* Ionic components like `ion-item` render a native anchor in their shadow DOM,
* so a modifier click (ctrl/meta/shift/alt) or a non-`_self` target should let
* the browser handle the navigation natively (new tab, new window, download)
* instead of navigating in-app.
*
* We listen in the capture phase so this runs before Angular's `RouterLink`
* handler and our own bubble-phase `onClick`. On a native-navigation intent it
* stops propagation to cancel the in-app navigation, but leaves `preventDefault`
* alone so the native anchor can still act.
*/
this.elementRef.nativeElement.addEventListener('click', this.onCaptureClick, { capture: true });
}

ngOnChanges(): void {
this.updateTargetUrlAndHref();
}

ngOnDestroy(): void {
this.elementRef.nativeElement.removeEventListener('click', this.onCaptureClick, { capture: true });
}

private onCaptureClick = (ev: Event): void => {
if (this.opensNatively(ev)) {
ev.stopImmediatePropagation();
}
};

/**
* True when the browser should handle the click natively instead of routing
* in-app: a modifier was held (ctrl/meta/shift/alt), or the host targets
* something other than `_self`. This mirrors the modifier set Angular's own
* `RouterLink` guards on, so an Ionic `routerLink` behaves like a plain anchor
* for new-tab, new-window, and download intents.
*/
private opensNatively(ev: Event): boolean {
if (ev instanceof MouseEvent && (ev.ctrlKey || ev.metaKey || ev.shiftKey || ev.altKey)) {
return true;
}

const target = this.elementRef.nativeElement.target;
return target != null && target !== '' && target !== '_self';
}

/**
* The `tabindex` is set to `0` by default on the host element when
* the `routerLink` directive is used. This causes issues with Ionic
Expand Down
Comment thread
thetaPC marked this conversation as resolved.
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import { test, expect, type Page } from '@playwright/test';

/**
* Issue #26394: a modifier click (ctrl/meta/shift/alt) or a non-`_self` target on
* a `routerLink` over a non-anchor Ionic component (ion-item, ion-button) must let
* the browser handle the navigation natively (new tab, window, download) instead
* of navigating the current page in-app.
*
* Playwright headless can't dispatch a real modifier-click or observe the
* browser's native new-tab default, so we dispatch a synthetic click on the
* host (not the shadow anchor, which would fire its own default navigation) and
* assert the JS handler chain leaves the page unchanged with `preventDefault`
* uncalled.
*/
test.describe('RouterLink: modifier click', () => {
const dispatchClick = (page: Page, selector: string, modifiers: Record<string, boolean>) =>
page.evaluate(
({ selector, mods }: { selector: string; mods: Record<string, boolean> }) => {
const item = document.querySelector(selector) as HTMLElement;
const ev = new MouseEvent('click', { bubbles: true, composed: true, cancelable: true, button: 0, ...mods });
item.dispatchEvent(ev);
return { defaultPrevented: ev.defaultPrevented };
},
{ selector, mods: modifiers }
);

test.beforeEach(async ({ page }) => {
await page.goto('/standalone/router-link');
});

test('normal click navigates the current page in-app', async ({ page }) => {
const { defaultPrevented } = await dispatchClick(page, '#modifier-item', {});

await expect(page).toHaveURL(/.*\/standalone\/popover/);
// The delegate prevents the default to stop a hard page reload.
expect(defaultPrevented).toBe(true);
});

for (const modifier of ['ctrlKey', 'metaKey', 'shiftKey', 'altKey']) {
test(`${modifier}+click does not navigate the current page and allows native handling`, async ({
page,
}, testInfo) => {
testInfo.annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/26394',
});

const { defaultPrevented } = await dispatchClick(page, '#modifier-item', { [modifier]: true });

// Give any in-app navigation a chance to run before asserting it did not.
await page.waitForTimeout(300);
await expect(page).toHaveURL(/.*\/standalone\/router-link/);
// Default is left intact so the browser can handle the link natively
// (new tab, new window, or download, depending on the browser and OS).
expect(defaultPrevented).toBe(false);
});
}

test('click on a non-_self target does not navigate the current page and allows a native new tab', async ({
page,
}, testInfo) => {
testInfo.annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/26394',
});

const { defaultPrevented } = await dispatchClick(page, '#target-item', {});

// Give any in-app navigation a chance to run before asserting it did not.
await page.waitForTimeout(300);
await expect(page).toHaveURL(/.*\/standalone\/router-link/);
// Default is left intact so the browser can open the link in a new tab.
expect(defaultPrevented).toBe(false);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,9 @@
<ion-button routerLink="/standalone/popover" routerDirection="root">
I'm an ion-button
</ion-button>
<ion-item id="modifier-item" routerLink="/standalone/popover" routerDirection="root">
I'm an ion-item
</ion-item>
<ion-item id="target-item" routerLink="/standalone/popover" routerDirection="root" target="_blank">
I'm an ion-item with a target
</ion-item>
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import { Component } from '@angular/core';
import { RouterLink } from '@angular/router';
import { IonRouterLink, IonRouterLinkWithHref } from '@ionic/angular/standalone';
import { IonItem, IonRouterLink, IonRouterLinkWithHref } from '@ionic/angular/standalone';

@Component({
selector: 'app-router-link',
templateUrl: './router-link.component.html',
standalone: true,
imports: [RouterLink, IonRouterLink, IonRouterLinkWithHref],
imports: [RouterLink, IonItem, IonRouterLink, IonRouterLinkWithHref],
})
export class RouterLinkComponent {}
Loading