fix(react): Inline overlays can be conditionally rendered (outdated) #25663

sean-perkins · 2022-07-20T20:18:32Z

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)
- Some docs updates need to be made in the ionic-docs repo, in a separate PR. See the contributing guide for details.
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?

Inline overlays (modal, popover) were never configured with a framework delegate. This resulted in us removing DOM nodes through manual manipulation, instead of relying on React to clear the React component and associated DOM node.

This resulted in two problems:

Developers could not conditionally render an inline modal/popover.
Dynamic sibling nodes rendered before an IonModal/IonPopover that were mutated while the overlay was dismissing, would result in the same exception from 1.

Issue URL: #25590, #25775

What is the new behavior?

Inline overlays are configured with a framework delegate implementation that teleports the inline overlay to the root of the app and back to it's original DOM location (when dismissed)
IonModal and IonPopover can be conditionally rendered.
IonModal and IonPopover will not throw exceptions when sibling content is dynamically rendered before its node.
Overlays are unmounted when history push/pop/replace occurs
Overlays are moved to the top z-index when they are presented
- ActionSheet and Alert internal elements assigned an id based on the z-index are statically computed

Does this introduce a breaking change?

Yes
No

Other information

Open to feedback on naming/folder structure for Github specific issue test-cases.

Dev-build: 6.2.8-dev.11663187107.1a1cbb3f

packages/react/src/components/createInlineOverlayComponent.tsx

averyjohnston · 2022-10-07T17:54:53Z

packages/react/src/components/createInlineOverlayComponent.tsx

+    componentWillUnmount() {
+      if (this.ref.current) {
+        this.ref.current.dismiss();
+      }


While playing around in a small test app, I noticed some errors when doing replace or push operations that I think are due to the timing of this dismiss call.

Repo/branch: https://github.com/amandaejohnston/sandbox-ionic-react/tree/FW-1889
Screencast: https://user-images.githubusercontent.com/90629384/194619453-2bab84cd-bfa6-421f-a9dd-ea68dec8b36c.mp4

My guess is this is happening because the dismiss event is firing and/or being picked up by the Home component after the modal has been removed from the DOM. This would cause the onDidDismiss handler for the modal to try and set the isOpen state on the modal when it's unmounted, throwing the error. Although, the fact that I don't get the error when doing a goBack operation may throw a wrench in that. 🤔

Let me know if this is an implementation issue on my end, of course! The code is mostly copy-pasted from the test app page in this PR but maybe something went screwy in the process.

averyjohnston · 2022-10-07T17:58:09Z

packages/react/test-app/cypress/integration/issues/gh-25590.spec.ts

@@ -0,0 +1,41 @@
+describe('IonModal', () => {


Could we name this file something more descriptive, maybe conditional-render.spec.ts or something? I would rather us not have to dig through the code and/or click through to the GH issue to figure out the context.

liamdebeasi · 2022-10-12T15:12:58Z

core/src/components/action-sheet/action-sheet.tsx

        tabindex="-1"
        {...(htmlAttributes as any)}
        style={{
-          zIndex: `${20000 + this.overlayIndex}`,


Are these changes required for this fix to land? If not, can we move them to a separate PR? Same goes for the changes in alert.tsx.

Yes, these changes are required. In main the id of these internal elements is dependent on the overlayIndex. In main, the overlayIndex is always static after render. In this PR, top overlays are re-assigned a higher overlay index, so that they always display as the top most visible overlay. In doing so, the internal ids are updated to the new z-index value. This is not desired and would be a breaking change and defeat the purpose of these selectors.

With this change, the ids remain constant during re-render, even when the overlay is assigned a new overlayIndex as a result of being presented.

liamdebeasi · 2022-10-12T15:15:16Z

core/src/components/toast/toast.tsx

      clearTimeout(this.durationTimeout);
    }
-    return dismiss(this, data, role, 'toastLeave', iosLeaveAnimation, mdLeaveAnimation, this.position);
+    return dismiss(this, data, role, 'toastLeave', iosLeaveAnimation, mdLeaveAnimation, true, this.position);


Why does toast need the true param but others like alert/action-sheet/etc do not?

The last parameter of the dismiss utility is optional. Alert/ActionSheet/etc. do not use the last parameter to specify additional options, toast does.

The new property introduced to dismiss has a default value of true that matches the base behavior on main (removing the overlay element).

Since Toast specifies a value for the last optional parameter, is has to specify a value for the parameter before it. Alert/ActionSheet/etc. do not, so they do not need to specify the parameter, as it will use the default value.

liamdebeasi · 2022-10-12T15:15:53Z

core/src/components/modal/modal.tsx

+      'modalLeave',
+      iosLeaveAnimation,
+      mdLeaveAnimation,
+      inline === false || delegate === undefined,


Can we extract this to a variable that better defines what it is checking? My understanding is we want to only remove the overlay if the overlay is presented via a controller, right?

Correct, the overlay should only be removed if presented via a controller.

Should it be something like:

const hasController = inline === false || delegate === undefined;

or explicit to the removal of the overlay element?

const removeOverlay = inline === false || delegate === undefined;

liamdebeasi · 2022-10-12T15:17:01Z

packages/react/src/components/createInlineOverlayComponent.tsx

-        }, children) :
-        null
+      const keepContentsMounted =
+        this.props.trigger !== undefined || this.props.keepContentsMounted;


Why do we check for a trigger with keepContentsMounted?

Trigger-based overlays require that the web component is mounted in the DOM in order to attach the event listeners to the trigger element.

This flag, keepContentsMounted, tracks the two states when we should keep the React component mounted; either when the overlay is using a trigger (otherwise it will never open, since it is in a React Portal) or when it explicitly has opted into keepContentsMounted.

liamdebeasi · 2022-10-12T15:20:58Z

packages/react/src/components/createInlineOverlayComponent.tsx

+        this.props.trigger !== undefined || this.props.keepContentsMounted;
+
+      const mountOverlay =
+        this.state.isOpen || this.props.isOpen || this.isDismissing || keepContentsMounted;


Why do we need to check both this.state.isOpen and this.props.isOpen?

I will need to dig back into this decision. It was deliberate to solve an issue with the overlay not mounting when using the isOpen property directly. I will see if I can reproduce the issue that influenced it.

liamdebeasi · 2022-10-12T15:23:34Z

packages/react/src/components/createInlineOverlayComponent.tsx

+        delegate: this.delegate,
+      };
+
+      attachProps(


Why do we need to attach the props here? Shouldn't the render function pass the updated props via newProps?

This behavior exists on main. This change just assigns the React instance of the delegate to the element.

This is also similar to createOverlayComponent: https://github.com/ionic-team/ionic-framework/blob/main/packages/react/src/components/createOverlayComponent.tsx#L90

I am not sure as to why we attach props this way vs. the render function, but I opted for changing as little of the overlay implementation as possible here, unless we are having an issue with the existing pattern.

Edit:

You are correct, this should be refactored to the newProps assignment in the render function.

sean-perkins · 2022-10-14T14:47:53Z

Closing this PR in favor of #26111. I will keep the branch around for reference if we decide to move to React Portals in a future major version.

sean-perkins added 5 commits July 20, 2022 15:54

fix(react): IonModal can be conditionally rendered

ed3e561

chore(): sync IonNav with type changes

a0cf9b4

test(react): sibling contents render when modal is dismissed

14a0aae

test(react): IonModal is conditionally rendered

de20b54

chore(): remove unneeded views

8300f08

github-actions bot added the package: react @ionic/react package label Jul 20, 2022

sean-perkins added 9 commits July 20, 2022 16:30

chore(): lint

e944922

fix(): isOpen state for modal inner

2e9c6eb

fix(): unmounting inner component within modal

97cb2ec

Merge remote-tracking branch 'origin/main' into FW-1889

c34c6a2

fix(): account for keepContentsMounted

4480a7d

Merge branch 'main' into FW-1889

cc3f960

Merge branch 'main' into FW-1889

a47e60c

fix(): keep contents mounted when overlay is dismissed

b78b21e

test(): fix setState for keep contents mounted

e023527

github-actions bot added the package: core @ionic/core package label Aug 12, 2022

sean-perkins added 2 commits August 12, 2022 14:08

fix(): createPortal for detached DOM node

c11ca47

fix(): react works with conditional overlay rendering

da4ff15

sean-perkins changed the title ~~fix(react): IonModal can be conditionally rendered~~ fix(react): Inline overlays can be conditionally rendered Aug 12, 2022

sean-perkins added 6 commits August 12, 2022 15:48

chore(): clean-up

f3fe709

Merge branch 'main' into FW-1889

9c58279

fix(): teleport inline overlay

2380a7f

Merge remote-tracking branch 'origin/FW-1889' into FW-1889

27c7338

chore(): clean-up

8ea0f7e

chore(): lint fix

d615fb7

sean-perkins marked this pull request as ready for review August 12, 2022 21:19

sean-perkins requested review from a team, averyjohnston and liamdebeasi August 12, 2022 21:19

Merge branch 'main' into FW-1889

87fe299

sean-perkins and others added 6 commits August 25, 2022 14:00

fix(): inline overlays usage with triggers

242faa8

Merge branch 'main' into FW-1889

82badba

Merge branch 'main' into FW-1889

f3ba5a1

fix(react): inline trigger overlays always mount contents

022aa06

Merge remote-tracking branch 'origin/FW-1889' into FW-1889

c71bdd4

fix(react): mounting component behavior

ad150ab

sean-perkins commented Sep 12, 2022

View reviewed changes

packages/react/src/components/createInlineOverlayComponent.tsx Outdated Show resolved Hide resolved

sean-perkins added 9 commits September 14, 2022 14:47

fix(overlays): move presented overlays to the top of the visual stack

db2d55a

test(react): inline modal in a modal

bdc773e

Merge branch 'main' into FW-1889

881336e

fix(alert): statically generate child element ids

11d0f89

Merge remote-tracking branch 'origin/FW-1889' into FW-1889

9a0a564

fix(action-sheet): statically generate header id

a1cbb3f

Merge remote-tracking branch 'origin/main' into FW-1889

6e84008

chore(): remove outdated comment

9bd69f7

test(): remove extra cy.visit

86fbd6a

sean-perkins requested a review from a team as a code owner September 27, 2022 20:41

sean-perkins added 3 commits September 29, 2022 20:48

Merge branch 'main' into FW-1889

4b8eff1

Merge branch 'main' into FW-1889

a41944c

Merge remote-tracking branch 'origin/main' into FW-1889

91f7ab8

sean-perkins requested review from averyjohnston and liamdebeasi October 5, 2022 15:48

averyjohnston reviewed Oct 7, 2022

View reviewed changes

liamdebeasi reviewed Oct 12, 2022

View reviewed changes

Merge branch 'main' into FW-1889

ed978c4

sean-perkins mentioned this pull request Oct 12, 2022

fix(react): inline overlays can be conditionally rendered #26111

Merged

13 tasks

sean-perkins changed the title ~~fix(react): Inline overlays can be conditionally rendered~~ fix(react): Inline overlays can be conditionally rendered (outdated) Oct 13, 2022

sean-perkins closed this Oct 14, 2022

sean-perkins deleted the FW-1889 branch March 24, 2023 18:27

fix(react): Inline overlays can be conditionally rendered (outdated) #25663

fix(react): Inline overlays can be conditionally rendered (outdated) #25663

Uh oh!

Conversation

sean-perkins commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sean-perkins Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sean-perkins commented Oct 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sean-perkins commented Jul 20, 2022 •

edited

Loading

sean-perkins Oct 12, 2022 •

edited

Loading