The React CdsModal wrapper does not close a modal on backdrop overlay click / when hitting Esc

[astorije]

Describe the bug

The onCloseChange event handler on CdsModal does not add a listener on the backdrop, so the modal cannot be closed by clicking the backdrop overlay or hitting Esc (but clicking the modal × button does).

Looking at the Storybook doc, it seems we need to do this manually which React doesn’t play well with, and I couldn’t make it work bug-free.

I believe the React CdsModal wrapper should wire that overlay onCloseChange listener itself.

How to reproduce

This StackBlitz should demonstrate the issue: https://stackblitz.com/edit/react-ts-bgirwg?file=App.tsx

Steps to reproduce the behavior:

• Open the modal
• Clicking the × closes it
• Clicking the overlay or hitting Esc does not

Porting from StackBlitz for full context within this issue, we expect the following code to work:

const [open, setOpen] = useState(false);

return (
<>
  <CdsButton onClick={() => setOpen(true)}>Open modal</CdsButton>

  <CdsModal hidden={!open} onCloseChange={() => setOpen(false)}>
    <CdsModalHeader>Title</CdsModalHeader>
    <CdsModalContent>Content</CdsModalContent>
    <CdsModalActions><CdsButton onClick={() => setOpen(false)}>Close</CdsButton></CdsModalActions>
  </CdsModal>
</>
);

Expected behavior

The code above should work exactly like the bare Web Components equivalent does: https://stackblitz.com/edit/typescript-mep2k5?file=index.ts

Versions

Clarity project:

• Clarity Core (with @cds/react)
• Clarity Angular/UI

Clarity version:

• v3.x
• v4.x
• v5.x

Framework:

• Angular
• React
• Vue
• Other:

Framework version:

Tested the issue with latest React v16. Any version of React should have the same issue though.

Additional information

@ashleyryan opened a discussion about framework-specific documentation, and I believe based on the solution for this, this could be a perfect use case for that: #5895

[mathisscott]

@astorije
I'm confused. The stackblitz works for me. As in, if I follow the steps to reproduce the issue, I see no issue. Hitting the escape key or clicking the backdrop. Both close the modal.

[ashleyryan]

Really? The X is closing it for me, but clicking on the backdrop is not. Neither is hitting escape.

Is this a browser issue? I'm on Chrome 90.

Edit: It's also not working for me in Firefox or Safari...

[astorije]

@mathisscott, are you trying on https://stackblitz.com/edit/react-ts-bgirwg?file=App.tsx? (and not the other Stackblitz)
I believe @coryrylan confirmed the issue, and it seems no listeners are installed on the overlay at the moment

[coryrylan]

I don't think its browser specific, it happens to me on Firefox as well as Chrome

[mathisscott]

Sorry for the confusion. I think I clicked on the wrong link. I can see it in @astorije 's link...

[astorije]

Hi everyone! Just following-up on this, is this still on your radar? Anything we can help with?

[coryrylan]

@astorije on our radar, once Shape is over our bandwidth should open back up a bit 👍 We will be updating the react wrapper to the new version from the https://lit.dev project. Ours is an early fork of that version so the update may solve this as well.

[astorije]

Oh that's great, thanks!

[astorije]

FYI, I updated https://stackblitz.com/edit/react-ts-bgirwg to use Clarity Core/React 5.4.0 as I know you've done some work around lit but unfortunately the problem persists :(

[mathisscott]

@astorije

Looking into this, it appears that the esc key and backdrop click share a couple of circumstances.

Both are set up inside the parent superclass of the modal. I don't think that's significant because if React messed with that so much more would be broken in the components.

The other thing they share is how they are defined as event handlers. You'd see in the close button component how the close button binds this. But these two events in the overlay component aren't binding this to anything.

It's my hypothesis that React Hooks are losing track of this. And if we bound those event calls, I think it would work.

I can take a look at this tomorrow AM. If you are still on Europe time and want to get to it before me, please feel free to give it a try. I think what we need to do, though, is just change those bindings in overlay.element.ts from...

window.addEventListener('keydown', this.fireEventOnEscape);

...to...

window.addEventListener('keydown', this.fireEventOnEscape.bind(this));

It may not 100% wind up looking like that but that's the ballpark I'd look into...

[mathisscott]

@astorije
Turns out, it's not a binding issue at all. At least, the main issue causing the problem seems unrelated to binding.

The breakage here is something that we suspected might break in React but we didn't have any hard evidence that it would (or was) breaking. The problem is the focus traps on those two specific items are relying on the real DOM and, at least with React Hooks, the virtual DOM of React is not in sync. More to the point, React isn't allowing access to the real DOM at all in these specific cases.

We had planned a refactor of focus-traps to remove reliance on the DOM and to give Core a sort of singleton functionality to workaround dependency injection concerns. It was not a top priority because we didn't have any concrete issues come up related to our concerns. Now we do.

CC: @coryrylan (This is that thing we've been talking about...)

[astorije]

Thank you for this summary, that's super informative. Let us know if we can help!

[astorije]

Hey @mathisscott and @coryrylan!

Do you have any updates on this? We are still blocked to convert our homegrown modals to Clarity. Thanks! 🙏

[coryrylan]

There was some related work I did that addressed part of this. I need to double check if it had any impact or what is left to do.

[astorije]

I tried to upgrade Clarity versions at https://stackblitz.com/edit/react-ts-bgirwg but somehow Stackblitz broke and I can't seem to get it back.
I opened a similar sandbox at https://codesandbox.io/s/cds-modal-close-ejy2h with v5.5.1 but unfortunately the existing behavior remains.

[coryrylan]

Workaround

The prior PR fixed part of the issue. The last part is related to React not supporting boolean attrs. False can't be used because react will convert it to a string value leaving hidden="false" which is an invalid boolean attr. To get around this you will have to define a ternary expression.

hidden={open ? undefined : true}

https://codesandbox.io/s/cds-modal-close-forked-udvgj?file=/src/App.tsx

Normally the React wrapper can convert boolean attrs/properties automatically. However hidden is built into all HTMLElement instances and the converter only manages custom proeprties not native HTML properties/attrs.

Action Item

I am leaving this open to update the types so we can also set with null since that is a valid boolean value for html boolean attrs. We can update this
@property({ type: Boolean }) hidden = false; to @property({ type: Boolean }) hidden: boolean | null = false;

which allows this:

hidden={open ? null : true}

[astorije]

I just updated https://codesandbox.io/s/cds-modal-close-ejy2h?file=/src/App.tsx to v5.5.4 (just in case...) and to include your workaround, @coryrylan.

Is there any chance @cds/react could get a special treatment to transform any hidden={!open} to hidden={open ? undefined : true} before passing it down to the web components?
We will be using the workaround for now, but our main issue here is that hidden={!open} sort of works if you forget to test clicking on the overlay or hitting Esc, which means that over time we will probably have a lot of broken modals across our codebase and it will be hard to find them or keep track of them...

[coryrylan]

We are working on updating @lit-labs/react dependency and can check to see if there is any improvement for this. We also are keeping a close eye on facebook/react#22184

[astorije]

Sounds good, thank you!

[mathisscott]

I've verified that this works in both props-based react as well as react hooks in the latest code in the dropdowns branch.

This should be available and fixed by the new reactive controllers for focus trap and closable features.

Closing in favor of tracking the popup issue. I will ref this issue back to that issue.

[github-actions]

Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed issues after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.