fix(react): nested router outlets will not reanimate entered views #24672

sean-perkins · 2022-01-28T05:18:22Z

Pull request checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
Build (npm run build) was run locally and any changes were pushed
Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

When navigating within a nested router outlet, the page containing the router outlet is transitioning when the view is already visible. The most common instance of this, is when using nested router outlets in combination with IonTabs. When navigating between the tabs, the page containing IonTabs is transitioning, resulting in a flickering display.

Issue Number: #24107

What is the new behavior?

The logic for determining when a page transition should occur has been amended to account for the conditions that apply when you are either within a single router outlet or within a nested router outlet.

Navigating between tabs within a nested router outlet will no longer flicker, as all subsequent routing will not transition the page containing the router outlet.

For example, when navigating between /tabs/tab1 to /tabs/tab2, only the view elements for tab1 and tab2 will transition. The view element containing the /tabs path and router outlet will not transition, as that view is already active in the viewport.

Does this introduce a breaking change?

Yes
No

Other information

This change should have a noticeable performance improvement for React applications using nested router outlets. Prior to this PR, the page element was transitioning and causing multiple repaints, when the view was already active. This likely wasn't caught prior to the custom elements build, as Stencil's hydrated (.hydrated) class was overriding the styling from ion-page-invisible.

Kapture.2022-01-28.at.14.10.44.mp4

sean-perkins · 2022-01-28T05:31:11Z

Looks like I need to address some issues with regards to failing tests. Need to validate that the criteria they are validating is accurate to expected behavior vs. testing the previous incorrect behavior.

I also somehow completely missed that the react router package also has its own test app. I can likely remove all the custom changes to the react test app and just validate changes there.

sean-perkins · 2022-01-28T05:53:14Z

Ok, only one outstanding test is failing, which I will address in the morning.

[1] 2) Nested Outlets 2
[1] /nested-outlet2 >
[1] Go to list from Home IonItem click >
[1] Item#1 IonItem Click >
[1] Item page should be visible >
[1] Back >
[1] Home page should be visible

I believe this test is covering incorrect behavior. Now when clicking the Back > interaction, you will be transitioned to the list view that you clicked Item #1 IonItem Click > from, instead of going back two views to the Home page 🎉

sean-perkins · 2022-01-28T18:15:12Z

Verified in the reproduction app (from the attached issue) and the Ionic React conference app.

sean-perkins · 2022-01-28T18:56:47Z

Performance tests comparing v6.0.4 against this PR.

Navigating between tabs in a nested outlet (navigating from /tab-1, to /tab-2 and back to /tab-1)

v6.0.4	PR	Device/Env
		M1 Macbook Air 16gb ram / Chrome
		iPhone XS Max iOS 15.2.1 / Mobile Safari

Noticeable improvements:

66% decrease in render time in chrome browser
51% decrease in render time in mobile Safari

averyjohnston

Awesome work, and thanks again for the live walkthrough! 🎉

liamdebeasi

Great job! One non-blocking comment.

liamdebeasi · 2022-01-31T15:56:59Z

packages/react-router/src/ReactRouter/StackManager.tsx

+        if (enteringViewItem === leavingViewItem) {
+          /**
+           * If the entering view item is the same as the leaving view item,
+           * we are either transitioning using parameterized routes to the same view
+           * or a parent router outlet is re-rendering as a result of React props changing.
+           *
+           * If the route data does not match the current path, the parent router outlet
+           * is attempting to transition and we cancel the operation.
+           */
+          if (enteringViewItem.routeData.match.url !== routeInfo.pathname) {
+            return;
+          }
+        }


Would it make sense to only check if enteringViewItem.routeData.match.url !== routeInfo.pathname?

If enteringViewItem === leavingViewItem but enteringViewItem.routeData.match.url === routeInfo.pathname then that would mean you went from /page/1 to /page/1 which I don't think is possible.

This specific check is to catch the difference in behavior between changing between tabs vs. pushing to another route that uses the same component.

With tabs and our reliance on React's re-rendering as a result of props changing to trigger route transition, the /tabs component will reach this check when navigating between /tabs/tab1 to /tabs/tab2. But, since the entering view item's rout date is /tabs and not /tabs/tab1, it will return early (skipping the unnecessary transition).

If pushing to the same detail page, the entering and leaving view item will be the same, but the entering view item's route will be /page/1 and the route info will be /page/1, so the transition will occur.

I'm a little worried about opening up this check without the entering/leaving comparison, since this is the only case that is currently attempting to transition when it shouldn't. Ideally all of this goes away, if we move off React's re-rendering/prop changes for handling route transitions and follow closer suite to the Vue implementation.

Makes sense, let's keep it as is 👍

shadowzack · 2022-03-29T19:10:26Z

I have tried using version 6.0.4 but still got the same problem

sean-perkins · 2022-03-29T19:12:23Z

@shadowzack the problem still exists in v6.0.4. The comparison screenshots are comparing the incorrect behavior vs. the corrected behavior.

You need to upgrade to 6.0.5 or later: https://github.com/ionic-team/ionic-framework/blob/main/CHANGELOG.md#605-2022-02-02

shadowzack · 2022-03-29T19:17:38Z

I have just tried version 6.0.5 and the latest 6.0.13, all have the same problem

sean-perkins · 2022-03-29T19:18:42Z

If you want to open an issue with a reproduction starter app, I would be happy to look.

shadowzack · 2022-03-29T19:51:43Z

I have tried to build a test app with the latest ionic core 6.0.13, and the app works as indented with no flickering.
can an inner IonRouterOutlet inside another IonRouterOutlet effect the transition?
i have both tabs and a side menu just like the ionic react conference app, but even after removing the side menu the flickering still exists.
any ideas on why this is happing?

sean-perkins · 2022-03-29T20:03:29Z

If I recall, I verified this issue against the conference app and noticed an improvement to the transition between tab items (nested outlets). The only issue currently that I am aware of this is causing flicker transitions is fallback routes: #24855.

If you are able to pull in the similarities between your application and a new project, to isolate the reproduction, I can give a better recommendation. Nested outlets are unfortunately complex to handle page transitions and mounting/unmounting with how we replicate native page transitions.

shadowzack · 2022-03-29T22:27:30Z

<IonApp>
    <IonReactRouter>
      <IonSplitPane contentId="main">
        <Menu />
      {/*   <IonRouterOutlet id="main"> */}
          <Route path="/" exact>
            <Redirect to="/login" />
          </Route>
          <Route path="/login" exact>
            <Login />
          </Route>
          <IonTabs>
            <IonRouterOutlet>
              <Route path="/notifications" exact>
                <TabNotifications />
              </Route>
              <Route path="/home" exact>
                <TabHome />
              </Route>
              <Route path="/chats" exact>
                <TabChats />
              </Route>
              <Route path="/operations" exact>
                <TabList />
              </Route>
              <Route path="/profile" exact>
                <TabProfile />
              </Route>
              <Route path="/support" exact>
                <TabSupport />
              </Route>
            </IonRouterOutlet>

            <IonTabBar slot="bottom">
              <IonTabButton tab="tabProfile" href="/profile">
                <IonIcon icon={personOutline} />
                <IonLabel>החשבון שלי</IonLabel>
              </IonTabButton>
              <IonTabButton tab="tabNotifications" href="/notifications">
                <IonIcon icon={notificationsOutline} />
                <IonLabel>התראות</IonLabel>
              </IonTabButton>
              <IonTabButton tab="tabHome" href="/home">
                <IonIcon icon={homeOutline} />
                <IonLabel>דף הבית</IonLabel>
              </IonTabButton>
              <IonTabButton tab="tabOperations" href="/operations">
                <IonIcon icon={listOutline} />
                <IonLabel>פעולות</IonLabel>
              </IonTabButton>
              <IonTabButton tab="tabChats" href="/chats">
                <IonIcon icon={chatbubbleOutline} />
                <IonLabel>הודעות</IonLabel>
              </IonTabButton>
            </IonTabBar>
          </IonTabs>
        {/* </IonRouterOutlet> */}
      </IonSplitPane>
    </IonReactRouter>
  </IonApp>

this is the code above, after running the code without the outer IonRouterOutlet the tabs transition without flickering. i think this might be related to the fact that the side menu and the tabs have the same paths(for example home using tabs and also using the side menu)

shadowzack · 2022-03-30T00:44:07Z

I have downloaded the ionic-react-conference-app, even without adding anything just running the code you can see the flickering when moving between tabs. can you try to fix it I think fixing it will help me and the anyone stuck in this problem.

shadowzack · 2022-03-31T16:56:10Z

I have migrated back to ionic 5 until this issue is resolved, TIP - you can migrate back using this command:-
npm install @ionic/react@v5-lts @ionic/react-router@v5-lts

fix(react): nested router outlets will not reanimate entered views

4e8ec36

sean-perkins requested a review from a team as a code owner January 28, 2022 05:18

github-actions bot added the package: react @ionic/react package label Jan 28, 2022

fix(): failing tests for view not visible (minus nested-outlet tests)

f78c589

fix(): react-router integration with nested outlets

29a4e7f

sean-perkins force-pushed the FW-274 branch from 2c2decf to 29a4e7f Compare January 28, 2022 17:21

fix(): allow parameterized routes to transition without tab flicker

ef0c659

averyjohnston approved these changes Jan 31, 2022

View reviewed changes

liamdebeasi approved these changes Jan 31, 2022

View reviewed changes

sean-perkins merged commit 43aa6c1 into main Jan 31, 2022

sean-perkins deleted the FW-274 branch January 31, 2022 16:55

averyjohnston mentioned this pull request Jan 31, 2022

bug: Ionic Routing on currently active IonPage triggers transition and re-rendering, although it is rendered already #22426

Closed

This was referenced Feb 2, 2022

Navigation flickering ionic-team/ionic-react-conference-app#79

Closed

Mark Tabs component as ionPage ionic-team/ionic-react-conference-app#80

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(react): nested router outlets will not reanimate entered views #24672

fix(react): nested router outlets will not reanimate entered views #24672

sean-perkins commented Jan 28, 2022 •

edited

Loading

sean-perkins commented Jan 28, 2022 •

edited

Loading

sean-perkins commented Jan 28, 2022 •

edited

Loading

sean-perkins commented Jan 28, 2022

sean-perkins commented Jan 28, 2022 •

edited

Loading

averyjohnston left a comment

liamdebeasi left a comment

liamdebeasi Jan 31, 2022

sean-perkins Jan 31, 2022

liamdebeasi Jan 31, 2022

shadowzack commented Mar 29, 2022

sean-perkins commented Mar 29, 2022

shadowzack commented Mar 29, 2022

sean-perkins commented Mar 29, 2022

shadowzack commented Mar 29, 2022

sean-perkins commented Mar 29, 2022

shadowzack commented Mar 29, 2022 •

edited

Loading

shadowzack commented Mar 30, 2022

shadowzack commented Mar 31, 2022 •

edited

Loading

fix(react): nested router outlets will not reanimate entered views #24672

fix(react): nested router outlets will not reanimate entered views #24672

Conversation

sean-perkins commented Jan 28, 2022 • edited Loading

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

sean-perkins commented Jan 28, 2022 • edited Loading

sean-perkins commented Jan 28, 2022 • edited Loading

sean-perkins commented Jan 28, 2022

sean-perkins commented Jan 28, 2022 • edited Loading

averyjohnston left a comment

Choose a reason for hiding this comment

liamdebeasi left a comment

Choose a reason for hiding this comment

liamdebeasi Jan 31, 2022

Choose a reason for hiding this comment

sean-perkins Jan 31, 2022

Choose a reason for hiding this comment

liamdebeasi Jan 31, 2022

Choose a reason for hiding this comment

shadowzack commented Mar 29, 2022

sean-perkins commented Mar 29, 2022

shadowzack commented Mar 29, 2022

sean-perkins commented Mar 29, 2022

shadowzack commented Mar 29, 2022

sean-perkins commented Mar 29, 2022

shadowzack commented Mar 29, 2022 • edited Loading

shadowzack commented Mar 30, 2022

shadowzack commented Mar 31, 2022 • edited Loading

sean-perkins commented Jan 28, 2022 •

edited

Loading

sean-perkins commented Jan 28, 2022 •

edited

Loading

sean-perkins commented Jan 28, 2022 •

edited

Loading

sean-perkins commented Jan 28, 2022 •

edited

Loading

shadowzack commented Mar 29, 2022 •

edited

Loading

shadowzack commented Mar 31, 2022 •

edited

Loading