From b5ff48d248a3df5d087a30531db363b2a1e8d8fc Mon Sep 17 00:00:00 2001 From: ShaneK Date: Tue, 10 Mar 2026 11:07:51 -0700 Subject: [PATCH] fix(react-router): use location keys for forward detection and prevent overlap in nested outlets --- .../src/ReactRouter/IonRouter.tsx | 29 ++- .../src/ReactRouter/StackManager.tsx | 64 +++++- .../base/tests/e2e/specs/nested-params.cy.js | 205 ++++++++++++++++++ 3 files changed, 282 insertions(+), 16 deletions(-) diff --git a/packages/react-router/src/ReactRouter/IonRouter.tsx b/packages/react-router/src/ReactRouter/IonRouter.tsx index 804d5f9370..94805b1153 100644 --- a/packages/react-router/src/ReactRouter/IonRouter.tsx +++ b/packages/react-router/src/ReactRouter/IonRouter.tsx @@ -83,12 +83,20 @@ export const IonRouter = ({ children, registerHistoryListener }: PropsWithChildr const viewStack = useRef(new ReactRouterViewStack()); const incomingRouteParams = useRef | null>(null); /** - * Tracks URLs (pathname + search) that the user navigated away from via - * browser back. When a POP event's destination matches the top of this - * stack, it's a browser forward navigation. Cleared on PUSH (new - * navigation invalidates forward history, just like in the browser). + * Tracks location keys that the user navigated away from via browser back. + * When a POP event's destination key matches the top of this stack, it's a + * browser forward navigation. Uses React Router's unique location.key + * instead of URLs to correctly handle duplicate URLs in history (e.g., + * navigating to /details, then /settings, then /details via routerLink, + * then pressing back). + * Cleared on PUSH (new navigation invalidates forward history). */ const forwardStack = useRef([]); + /** + * Tracks the current location key so we can push it onto the forward stack + * when navigating back. Updated after each history change. + */ + const currentLocationKeyRef = useRef(location.key); const [routeInfo, setRouteInfo] = useState({ id: generateId('routeInfo'), @@ -203,7 +211,7 @@ export const IonRouter = ({ children, registerHistoryListener }: PropsWithChildr const currentRoute = locationHistory.current.current(); const isForwardNavigation = forwardStack.current.length > 0 && - forwardStack.current[forwardStack.current.length - 1] === location.pathname + location.search; + forwardStack.current[forwardStack.current.length - 1] === location.key; if (isForwardNavigation) { forwardStack.current.pop(); @@ -213,8 +221,8 @@ export const IonRouter = ({ children, registerHistoryListener }: PropsWithChildr tab: tabToUse, }; } else if (currentRoute && currentRoute.pushedByRoute) { - // Back navigation — record current URL for potential forward - forwardStack.current.push(currentRoute.pathname + (currentRoute.search || '')); + // Back navigation. Record current location key for potential forward + forwardStack.current.push(currentLocationKeyRef.current); const prevInfo = locationHistory.current.findLastLocation(currentRoute); incomingRouteParams.current = { ...prevInfo, routeAction: 'pop', routeDirection: 'back' }; } else { @@ -346,6 +354,9 @@ export const IonRouter = ({ children, registerHistoryListener }: PropsWithChildr setRouteInfo(routeInfo); } + // Update the current location key after processing the history change. + // This ensures the forward stack records the correct key when navigating back. + currentLocationKeyRef.current = location.key; incomingRouteParams.current = null; }; @@ -481,8 +492,8 @@ export const IonRouter = ({ children, registerHistoryListener }: PropsWithChildr const condition1 = routeInfo.lastPathname === routeInfo.pushedByRoute; const condition2 = prevInfo.pathname === routeInfo.pushedByRoute && routeInfo.tab === '' && prevInfo.tab === ''; if (condition1 || condition2) { - // Record current URL so browser forward is detectable - forwardStack.current.push(routeInfo.pathname + (routeInfo.search || '')); + // Record the current location key so browser forward is detectable + forwardStack.current.push(currentLocationKeyRef.current); navigate(-1); } else { /** diff --git a/packages/react-router/src/ReactRouter/StackManager.tsx b/packages/react-router/src/ReactRouter/StackManager.tsx index cb5c25f420..9894813764 100644 --- a/packages/react-router/src/ReactRouter/StackManager.tsx +++ b/packages/react-router/src/ReactRouter/StackManager.tsx @@ -33,7 +33,7 @@ interface StackManagerProps { } const isViewVisible = (el: HTMLElement) => - !el.classList.contains('ion-page-invisible') && !el.classList.contains('ion-page-hidden'); + !el.classList.contains('ion-page-invisible') && !el.classList.contains('ion-page-hidden') && el.style.display !== 'none'; const hideIonPageElement = (element: HTMLElement | undefined): void => { if (element) { @@ -44,6 +44,7 @@ const hideIonPageElement = (element: HTMLElement | undefined): void => { const showIonPageElement = (element: HTMLElement | undefined): void => { if (element) { + element.style.removeProperty('display'); element.classList.remove('ion-page-hidden'); element.removeAttribute('aria-hidden'); } @@ -322,8 +323,8 @@ export class StackManager extends React.PureComponent { const enteringEl = enteringViewItem.ionPageElement; if (enteringEl) { - enteringEl.classList.remove('ion-page-hidden', 'ion-page-invisible'); - enteringEl.removeAttribute('aria-hidden'); + showIonPageElement(enteringEl); + enteringEl.classList.remove('ion-page-invisible'); } this.forceUpdate(); @@ -417,7 +418,9 @@ export class StackManager extends React.PureComponent { this.lastTransition = currentTransition; - this.transitionPage(routeInfo, enteringViewItem, leavingViewItem); + const shouldSkipAnimation = this.applySkipAnimationIfNeeded(enteringViewItem, leavingViewItem); + + this.transitionPage(routeInfo, enteringViewItem, leavingViewItem, undefined, false, shouldSkipAnimation); if (shouldUnmountLeavingViewItem && leavingViewItem && enteringViewItem !== leavingViewItem) { leavingViewItem.mount = false; @@ -547,6 +550,36 @@ export class StackManager extends React.PureComponent { } } + /** + * Determines whether to skip the transition animation and, if so, immediately + * hides the leaving view with inline `display:none`. + * + * Skips transitions in outlets nested inside a parent IonPage. These outlets + * render pages inside a parent page's content area. The MD animation shows + * both entering and leaving pages simultaneously, causing text overlap and + * nested scrollbars (each page has its own IonContent). Top-level outlets + * are unaffected and animate normally. + * + * Uses inline display:none rather than ion-page-hidden class because core's + * beforeTransition() removes ion-page-hidden via setPageHidden(). + * Inline display:none survives that removal, keeping the page hidden + * until React unmounts it after ionViewDidLeave fires. + */ + private applySkipAnimationIfNeeded( + enteringViewItem: ViewItem, + leavingViewItem: ViewItem | undefined + ): boolean { + const isNestedOutlet = !!this.routerOutletElement?.closest('.ion-page'); + const shouldSkip = isNestedOutlet && !!leavingViewItem && enteringViewItem !== leavingViewItem; + + if (shouldSkip && leavingViewItem?.ionPageElement) { + leavingViewItem.ionPageElement.style.setProperty('display', 'none'); + leavingViewItem.ionPageElement.setAttribute('aria-hidden', 'true'); + } + + return shouldSkip; + } + /** * Handles entering view with no ion-page element yet (waiting for render). */ @@ -609,7 +642,8 @@ export class StackManager extends React.PureComponent { const latestLeavingView = this.context.findLeavingViewItemByRouteInfo(routeInfo, this.id) ?? leavingViewItem; if (latestEnteringView?.ionPageElement) { - this.transitionPage(routeInfo, latestEnteringView, latestLeavingView ?? undefined); + const shouldSkipAnimation = this.applySkipAnimationIfNeeded(latestEnteringView, latestLeavingView ?? undefined); + this.transitionPage(routeInfo, latestEnteringView, latestLeavingView ?? undefined, undefined, false, shouldSkipAnimation); if (shouldUnmountLeavingViewItem && latestLeavingView && latestEnteringView !== latestLeavingView) { latestLeavingView.mount = false; @@ -863,6 +897,17 @@ export class StackManager extends React.PureComponent { return; } + /** + * Don't let a nested element (e.g., ion-router-outlet with ionPage prop) + * override an existing IonPage registration when the existing element is + * an ancestor of the new one. This ensures ionPageElement always points + * to the outermost IonPage, which is needed to properly hide the entire + * page during back navigation (not just the inner outlet). + */ + if (oldPageElement && oldPageElement !== page && oldPageElement.isConnected && oldPageElement.contains(page)) { + return; + } + foundView.ionPageElement = page; foundView.ionRoute = true; @@ -1008,13 +1053,18 @@ export class StackManager extends React.PureComponent { * @param progressAnimation Indicates if the transition is part of a * gesture controlled animation (e.g., swipe to go back). * Defaults to `false`. + * @param skipAnimation When true, forces `duration: 0` so the page + * swap is instant (no visible animation). Used for ionPage outlets + * and back navigations that unmount the leaving view to prevent + * overlapping content during the transition. Defaults to `false`. */ async transitionPage( routeInfo: RouteInfo, enteringViewItem: ViewItem, leavingViewItem?: ViewItem, direction?: 'forward' | 'back', - progressAnimation = false + progressAnimation = false, + skipAnimation = false ) { const runCommit = async (enteringEl: HTMLElement, leavingEl?: HTMLElement) => { const skipTransition = this.skipTransition; @@ -1055,7 +1105,7 @@ export class StackManager extends React.PureComponent { } await routerOutlet.commit(enteringEl, leavingEl, { - duration: skipTransition || directionToUse === undefined ? 0 : undefined, + duration: skipTransition || skipAnimation || directionToUse === undefined ? 0 : undefined, direction: directionToUse, showGoBack: !!routeInfo.pushedByRoute, progressAnimation, diff --git a/packages/react-router/test/base/tests/e2e/specs/nested-params.cy.js b/packages/react-router/test/base/tests/e2e/specs/nested-params.cy.js index a52483192f..9ac4597487 100644 --- a/packages/react-router/test/base/tests/e2e/specs/nested-params.cy.js +++ b/packages/react-router/test/base/tests/e2e/specs/nested-params.cy.js @@ -49,6 +49,76 @@ describe('Nested Params', () => { cy.get('[data-testid="user-settings-param"]').should('contain', 'Settings view user: 123'); }); + it('/nested-params > Navigate to user then back > No visual overlap during transition', () => { + cy.visit(`http://localhost:${port}/nested-params`); + cy.ionPageVisible('nested-params-landing'); + + // Navigate to user 99 + cy.get('#go-to-user-99').click(); + cy.get('[data-testid="user-layout-param"]').should('contain', 'Layout sees user: 99'); + + // Install an overlap detector that runs every animation frame. + // It checks whether the landing page and user page IonPage elements are + // simultaneously visible — which is the root cause of the overlap bug. + // We check the IonPage elements directly (not descendants) because CSS + // display is not inherited — a child's computed display can be 'block' + // even when a parent has display:none. + cy.window().then((win) => { + win.__overlapDetected = false; + const check = () => { + const landing = win.document.querySelector('[data-pageid="nested-params-landing"]'); + const userPage = win.document.querySelector('[data-pageid^="nested-params-user-"]'); + if (landing && userPage) { + const landingStyle = win.getComputedStyle(landing); + const userStyle = win.getComputedStyle(userPage); + const landingVisible = landingStyle.display !== 'none' && landingStyle.visibility !== 'hidden' && landingStyle.opacity !== '0'; + const userVisible = userStyle.display !== 'none' && userStyle.visibility !== 'hidden' && userStyle.opacity !== '0'; + if (landingVisible && userVisible) { + win.__overlapDetected = true; + } + } + if (!win.__overlapCheckDone) { + requestAnimationFrame(check); + } + }; + requestAnimationFrame(check); + }); + + // Go back to landing + cy.go('back'); + cy.ionPageVisible('nested-params-landing'); + cy.get('[data-testid="user-layout-param"]').should('not.exist'); + + // Stop the observer and verify no overlap was detected + cy.window().then((win) => { + win.__overlapCheckDone = true; + expect(win.__overlapDetected).to.be.false; + }); + }); + + it('/nested-params > Navigate to user, back, forward, back > Pages should not persist', () => { + cy.visit(`http://localhost:${port}/nested-params`); + cy.ionPageVisible('nested-params-landing'); + + // Navigate to user 99 + cy.get('#go-to-user-99').click(); + cy.get('[data-testid="user-layout-param"]').should('contain', 'Layout sees user: 99'); + + // Go back + cy.go('back'); + cy.ionPageVisible('nested-params-landing'); + + // Go forward — wait for async POP event processing and transition to settle + cy.go('forward'); + cy.wait(500); + cy.get('[data-testid="user-layout-param"]').should('contain', 'Layout sees user: 99'); + + // Go back again + cy.go('back'); + cy.ionPageVisible('nested-params-landing'); + cy.get('[data-testid="user-layout-param"]').should('not.exist'); + }); + it('/nested-params > Different users should have different params', () => { cy.visit(`http://localhost:${port}/nested-params`); cy.ionPageVisible('nested-params-landing'); @@ -67,4 +137,139 @@ describe('Nested Params', () => { cy.get('[data-testid="user-layout-param"]').should('contain', 'Layout sees user: 99'); cy.get('[data-testid="user-details-param"]').should('contain', 'Details view user: 99'); }); + + it('/nested-params > Full back navigation from sibling routes to root > Root page should show', () => { + // Start at root + cy.visit(`http://localhost:${port}/`); + cy.ionPageVisible('home'); + + // Navigate to nested-params + cy.contains('ion-item', 'Nested Params').click(); + cy.ionPageVisible('nested-params-landing'); + + // Navigate to user 99 details + cy.get('#go-to-user-99').click(); + cy.get('[data-testid="user-layout-param"]').should('contain', 'Layout sees user: 99'); + cy.get('[data-testid="user-details-param"]').should('contain', 'Details view user: 99'); + + // Go to settings + cy.get('#go-to-settings').click(); + cy.get('[data-testid="user-settings-param"]').should('contain', 'Settings view user: 99'); + + // Hit "Back to Details" (this is a forward push via routerLink) + cy.contains('ion-button', 'Back to Details').click(); + cy.get('[data-testid="user-details-param"]').should('contain', 'Details view user: 99'); + + // Browser back repeatedly to root + // Back 1: details -> settings + cy.go('back'); + cy.get('[data-testid="user-settings-param"]').should('contain', 'Settings view user: 99'); + + // Back 2: settings -> details + cy.go('back'); + cy.get('[data-testid="user-details-param"]').should('contain', 'Details view user: 99'); + + // Back 3: details -> nested-params landing + cy.go('back'); + cy.ionPageVisible('nested-params-landing'); + + // Back 4: nested-params -> root + cy.go('back'); + cy.ionPageVisible('home'); + cy.get('[data-testid="user-layout-param"]').should('not.exist'); + }); + + it('/nested-params > Sibling route transitions (details <-> settings) > No visual overlap', () => { + // Start directly on the details page + cy.visit(`http://localhost:${port}/nested-params/user/99/details`); + cy.get('[data-testid="user-details-param"]').should('contain', 'Details view user: 99'); + + // Install overlap detector that checks whether details and settings IonPage + // elements are simultaneously visible inside the nested outlet. + cy.window().then((win) => { + win.__siblingOverlapDetected = false; + const check = () => { + const details = win.document.querySelector('[data-pageid="nested-params-details"]'); + const settings = win.document.querySelector('[data-pageid="nested-params-settings"]'); + if (details && settings) { + const detailsStyle = win.getComputedStyle(details); + const settingsStyle = win.getComputedStyle(settings); + const detailsVisible = detailsStyle.display !== 'none' && detailsStyle.visibility !== 'hidden' && detailsStyle.opacity !== '0'; + const settingsVisible = settingsStyle.display !== 'none' && settingsStyle.visibility !== 'hidden' && settingsStyle.opacity !== '0'; + if (detailsVisible && settingsVisible) { + win.__siblingOverlapDetected = true; + } + } + if (!win.__siblingOverlapCheckDone) { + requestAnimationFrame(check); + } + }; + requestAnimationFrame(check); + }); + + // Navigate details -> settings + cy.get('#go-to-settings').click(); + cy.get('[data-testid="user-settings-param"]').should('contain', 'Settings view user: 99'); + + // Navigate settings -> details + cy.contains('ion-button', 'Back to Details').click(); + cy.get('[data-testid="user-details-param"]').should('contain', 'Details view user: 99'); + + // Stop the observer and verify no overlap was detected in either direction + cy.window().then((win) => { + win.__siblingOverlapCheckDone = true; + expect(win.__siblingOverlapDetected).to.be.false; + }); + }); + + it('/nested-params > Sibling route transitions > No nested scrollbars during transition', () => { + // Start directly on the details page + cy.visit(`http://localhost:${port}/nested-params/user/99/details`); + cy.get('[data-testid="user-details-param"]').should('contain', 'Details view user: 99'); + + // Install a detector that checks for multiple visible ion-content elements + // inside the nested outlet. Multiple visible ion-content elements cause + // nested scrollbars during transitions. + cy.window().then((win) => { + win.__nestedScrollbarsDetected = false; + const check = () => { + const outlet = win.document.querySelector('#nested-params-user-outlet'); + if (outlet) { + const contents = outlet.querySelectorAll('ion-content'); + let visibleCount = 0; + contents.forEach((content) => { + // Check the parent IonPage element's visibility + const page = content.closest('.ion-page'); + if (page) { + const style = win.getComputedStyle(page); + if (style.display !== 'none' && style.visibility !== 'hidden' && style.opacity !== '0') { + visibleCount++; + } + } + }); + if (visibleCount > 1) { + win.__nestedScrollbarsDetected = true; + } + } + if (!win.__nestedScrollbarsCheckDone) { + requestAnimationFrame(check); + } + }; + requestAnimationFrame(check); + }); + + // Navigate details -> settings + cy.get('#go-to-settings').click(); + cy.get('[data-testid="user-settings-param"]').should('contain', 'Settings view user: 99'); + + // Navigate settings -> details + cy.contains('ion-button', 'Back to Details').click(); + cy.get('[data-testid="user-details-param"]').should('contain', 'Details view user: 99'); + + // Stop the observer and verify no nested scrollbars were detected + cy.window().then((win) => { + win.__nestedScrollbarsCheckDone = true; + expect(win.__nestedScrollbarsDetected).to.be.false; + }); + }); });