UI: Modals no longer update while closing

[Snarling]

Modals were previously updating their contents and were still interactive during the closing animation, allowing e.g. a button that closes the modal to be pressed twice. This PR should fix both of these issues across all modals.

Tested alongside #782 which has some good example modals where it's clear the content is changing after the closing animation starts.

I temporarily set the transition time to 2 full seconds for this test, so it is more clear that the content does not update while the modal is closing. I also removed the special code from that PR that was instant-closing the modal.

Modal transitions were not actually changed to be this slow, the slowness was just a temporary change to make it easier to see that this was working properly.

2023-09-19.19-12-57.mp4

[jjclark1982]

This looks like a good approach. I have verified that it displays appropriately with Corp UIs.

This prevents double clicking on buttons, but it is still possible to double trigger a button with keyboard controls. I have checked the 21 occurrences of

if (event.key === KEY.ENTER)

to see if they have some error checking to prevent this.

CreateGangModal has no error checking and might be able to create multiple Gang objects (although only one would be attached to the Player.)

and there may be others that are keyboard-controllable without an explicit event handler.

[Snarling]

Good catch on the keyboard thing. I think css property inert might be a better approach than setting pointer-events to none, I believe it should stop keyboard events as well.

[Snarling]

@jjclark1982 I changed it to use inert instead of just disabling pointer events. Unfortunately inert in react requires a weird workaround for now. From my local testing this seems to also prevent keyboard interaction during the closing of the modal.

[myCatsName]

@Snarling is inert now available for all components? From your latest commit, I'm guessing non-modal components would continue to get TS complaints? would any other component ever need to be inert, idk (if we introduce Cards, for example). This is the last comment from PR you had linked, if it's helpful.

declare module 'react' {
  interface HTMLAttributes<T> extends AriaAttributes, DOMAttributes<T> {
    inert?: ''; // TODO Treating this totally valid HTML attribute as custom to make React work. Remove after this bug is fixed https://github.com/facebook/react/pull/24730
  }
}

[Snarling]

With the weird workaround (sending empty string instead of true, and undefined/null instead of false), inert should work for any base html element component, and it would probably work on any component that accepts general html attributes.

I'm not a fan of the global type override. Not having the type override means that in order to use it, you must call it out as being abnormal usage by using a ts-ignore or ts-expect-error, where with the global type override you can do the jank workaround somewhere without making a comment about it. If we're going to do something confusing like sending empty string as the truthy value for a boolean html attribute, I think it's better if every usage of that is marked as being abnormal.

[myCatsName]

Cool, that works.

I'm not a fan of the global type override.

I think I understand, and probably there are details I don't understand, consequences of one and the other.

Semi-related - is there a reason BB hasn't updated to React 18? I found updating to 18 as a solution for a dev-mode console error (about aria labels, maybe you've encountered the same), which alone is inconsequential. But we also use ReactDOM.render() , which is deprecated in 18 - reactwg/react-18#5

Other changes seem irrelevant, and again there are details I'll miss, but is update to React 18 worthwhile? facebook/react#24195

[d0sboots]

IIRC React 18 was blocked on MUI, which requires some significant work to upgrade. But I could be mistaken.

[myCatsName]

Sounds like that might be fixed with newer versions of MUI, pretty active topic - https://stackoverflow.com/questions/71713111/material-ui-installation-doesnt-work-with-react-18

[Snarling]

The issue is that there's a lot we would have to change to upgrade to mui 5. It's not a simple package update, it's would be a very extensive refactor. So as with most things, the bottleneck is developers' time and interest.