chore(Portal): Listen for Escape keypress using addEventListener instead of event-stack

[dominikdosoudil]

trafficstars

It is only one removed EventStack instance from Portal. I wanna go step by step with it just to know that I am not going some wrong direction.

There are few decisions I would like to explain:

• I've put the event handling into the PortalInner. It seems to me like the right place as PortalInner is only mounted when Portal is open (-> listeners only added when portal is open as well as with EventStack component).
• I've made onClose and closeOnEscape required in PortalInner because it seems to me that PortalInner is and should only be used inside the Portal itself. Therefor there should not be option for SUIR developer to forget prop drill it.

Overall, I looked into it a bit and I think that most of the listeners should be in the PortalInner as it has access to the ref which will be needed for other handlers. I should even make Portal itself a bit lightweight when closed.

I hope that it makes sense and does not disrespect some SUIR architecture.

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
semantic-ui-react	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 21, 2024 11:12am

[dominikdosoudil]

Btw there is some behaviour change in the "Multiple Modals" example.

Prod: when I click "Open first Modal", then "Proceed", then hit Escape both modals close. When I open the first one again, the second one cannot be opened. The dimmer is also darker so it seems that something opens twice.

This PR: Still both modals close with 1 Escape hit, however second opening of second modal works.

I believe that the production behaviour is a bug tho.

[dominikdosoudil]

Test error is done() called multiple times in hook <"after each" hook> (of root suite); however that does not happen to me locally. Is that really some problem in the code or just test environment having a bad mood?

[layershifter]

Btw there is some behaviour change in the "Multiple Modals" example.

Prod: when I click "Open first Modal", then "Proceed", then hit Escape both modals close. When I open the first one again, the second one cannot be opened. The dimmer is also darker so it seems that something opens twice.

This PR: Still both modals close with 1 Escape hit, however second opening of second modal works.

I believe that the production behaviour is a bug tho.

Yeah, that's something that is ugly about EventStack. Can you please check https://github.com/Semantic-Org/Semantic-UI-React/issues/4409#issuecomment-2493216774?

[dominikdosoudil]

Btw there is some behaviour change in the "Multiple Modals" example. Prod: when I click "Open first Modal", then "Proceed", then hit Escape both modals close. When I open the first one again, the second one cannot be opened. The dimmer is also darker so it seems that something opens twice. This PR: Still both modals close with 1 Escape hit, however second opening of second modal works. I believe that the production behaviour is a bug tho.

Yeah, that's something that is ugly about EventStack. Can you please check #4409 (comment)?

@layershifter I've been exploring behaviour of the fluentui Popover and I've noticed that the nested popover closes on Escape only when something inside the Popover has focus because there is the contains condition

Is that intentional? At first I thought that the escape closing one by one somehow starts working with the virtual parents but as I see it now, these are 2 different things.

If it is intentional and I am right that escape has nothing to do with Virtual Parents (there is nothing about escape in the VP PR) implementation of VP and escape handling should be 2 separate PRs IMO.