blueprintjs removal

[maxphilippov]

This is an attempt to remove BlueprintJS from the project. This is pretty huge in terms of what it changes so I might have missed something, but I believed that the things came out pretty simple and manageable. What I saw during this cleanup is that we've redefined a lot of BP styling etc. making it almost our own implementation.

BlueprintJS is gone from package.json so you have to reinstall node_modules to check this properly.

Things missing:

1. I didn't handle the Escape key bind to close new Dialog. I saw other discussions (#2268, #3868) regarding how we should handle a stack of open windows and close only the latest one.
2. I didn't remove bp4- class overrides (almost all of them done in either dev_rocket or dev_minimal themes), so this is definitely an open discussion (should we make up new names, add these classes to new elements or we can come up with something else). If you worked on these themes, please hit me up.

It also opens up an easier way to update React since we don't have to manage BlueprintJS too.

[WofWca]

About dialogs: not sure if you're aware, but now HTML has a native <dialog> which handles all these dismiss and stack things pretty well.

[farooqkz]

I can't do a review on the PR today(can't concentrate due to bad sleep pattern). But my opinion is mixed. You are doing BP removal which is desired but I am not in favor of big PRs as they need extensive testing and review. IMO, in such a bug codebase, a big PR might do more bad than good unless the review and testing is really extensive. Nevertheless, As I said removal of BP is very much wanted as a major upgrade of React and many other changes are softly dependent on it.

[farooqkz]

BTW, It's good that you haven't touched the Gallery component as there is another PR for it.

[maxphilippov]

I can't do a review on the PR today(can't concentrate due to bad sleep pattern). But my opinion is mixed. You are doing BP removal which is desired but I am not in favor of big PRs as they need extensive testing and review. IMO, in such a bug codebase, a big PR might do more bad than good unless the review and testing is really extensive. Nevertheless, As I said removal of BP is very much wanted as a major upgrade of React and many other changes are softly dependent on it.

Yeah, I don't think it's a good idea to dive into code straight away, I'd rather have people checking all the interfaces they use daily and look at what broke in terms of layout.

I know it looks like a huge change but from what I saw we used BP just for markup, the only thing that has some kind of interactive behavior is Dialog (we lost a lot of interactive things after replacing map with a new one opening in a popup). So it should be pretty obvious if something broke by just the look of it. I'm trying to do a screen by screen comparison, but that still requires some time.

[Simon-Laux]

I think we should forget about using Overlay for dialog and try to use the <dialog> element that the browser already provides, it even already has a ::backdrop css selector. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog

I guess we should first try to use them with the open attribute without showModal, that we don't need to rewrite the dialog manager, but if that doesn't work we might need to rewrite it. also I wonder if the html5 dialog element supports multiple dialogs on top of each other.

If the <dialog> element doesn't work then we need to adjust the overlay component:

• make sure the onClick handler is on the backdrop and renamed to onBackdropClick
• make sure the overlay content is centered in a way that it does not block the backdrop from being clicked.

(thats the bug that prevents me from using the settings dialog)

[farooqkz]

@maxphilippov after you addressed the bugs Simon has found, ping me to do a review by using.

[maxphilippov]

I think we should forget about using Overlay for dialog and try to use the <dialog> element that the browser already provides, it even already has a ::backdrop css selector. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog

I guess we should first try to use them with the open attribute without showModal, that we don't need to rewrite the dialog manager, but if that doesn't work we might need to rewrite it. also I wonder if the html5 dialog element supports multiple dialogs on top of each other.

If the <dialog> element doesn't work then we need to adjust the overlay component:

• make sure the onClick handler is on the backdrop and renamed to onBackdropClick
• make sure the overlay content is centered in a way that it does not block the backdrop from being clicked.

(thats the bug that prevents me from using the settings dialog) also replying to @WofWca

So I've looked into that and I personally find the

very rough and actually bad to use.

I pushed a reference implementation I used to figure things out.

My biggest problem with it is the desing which is kinda orthogonal to React idea of 'you specify element visibility by its presence in the DOM'.

This one is stateful and that makes it do excessive work which we don't even use.

Our current system (and I would argue it's a sane one) has a simple principle:

• there's no 'closed' state for dialog, if it's in DOM it's open

1. There's only one way to open
   through markup and it's the only property 'open': that gives us neither backdrop nor 'close on escape' So we have to do useEffect and call showModal() every time, cause we don't have a 'closed' state.

Do we need a 'closed' state? I don't think so. That would require either to have all dialogs present in DOM or to have a manager to cleanup unused dialogs. I don't see any of these being useful.

We also need to have CSS-display: flex on the dialog, which conflicts with how

manages display.

2. We already have a stack of opened dialogs in Dialog Context and it's implemented as natural as possible with array. So
   storing it's own order is again excessive work.

What I'm doing in reference impl. is using it's onClose callback to send a message to our DialogContext it's time to delete this one right after it get's hidden.

3. I'm not an expert on accessibility but it seems like it doesn't even set aria-modal="true"

[maxphilippov]

Bugs from testing:

Most of the dialog things should be fixed, but there's also a clear discrepancy between our styles. I didn't have a problem with flex dialog footer/header. But I had issues with basic text color in dialog. Can you re-check those please?

I didn't want to touch Gallery since @farooqkz is working on this, maybe we can handle it the same you @Simon-Laux did the follow-up PR to this one?

[nicodh]

@maxphilippov could you please go through the comments and answer Simons in detail (unresolved conversations) For example which of the reported bugs are fixed. Would be helpful for reviews...

[Simon-Laux]

can you check the todo boxes in my comment for the issues that you have addressed?

[farooqkz]

@maxphilippov I think it's a good idea that I finish my Gallery BP removal and that you base your PR on it? Because one of the bugs @Simon-Laux has found directly is part of Gallery. Plus I'm doing some refactors there and introducing "Tabs" component which you could use.

[Simon-Laux]

max pr is bigger so I guess the other way around would make more sense? (rebase #3977 to this pr and change it so it should merge into maxph/blueprintjs_removal_part2)

[maxphilippov]

Rebased towards current main (to the monorepo version). Got some minor issues with colors, gonna look at these later and post here an update.

[Simon-Laux]

create chat dialog / dialog footer actions - should be space between the items.

Will review the rest later

[nicodh]

@maxphilippov ist this final now for review? You mentioned on monday you will fix some issues?

[maxphilippov]

@maxphilippov ist this final now for review? You mentioned on monday you will fix some issues?

@Simon-Laux yeah, this is probably not ready to test yet, I had some things messed up during rebase, I did force-push some fixes but there are couple issues left. I'll change it from draft when I'm done. Hope to do that by the end of Friday/before Monday morning

[maxphilippov]

So, that's my fault of not investigating

enough. It introduces us to the world of 'top-layer', where there's no way to bypass dialogs other than Popover API, Fullscreen element, and some specific animations. Fullscreen doesn't make sense for elements I'm gonna talk about later and Popover API isn't officially in React type-system (it's supposedly coming in v19).

Here's the chain effect of introducing <dialog>:

it immediately broke FullscreenMedia and FullscreenAvatar (when you open your profile picture it opens behind profile dialog) so I had to implement these using

too.

Which in turn broke ContextMenu (again it always gonna be behind either dialog or FullscreenMedia) so it's implemented using

too.

I still decided to go for the

for the promise of accessibility.

But there's a catch again:

Dialog picks an element to focus on when it's opened. Looks like it cannot be disabled and the only thing left is to either unfocus or disable styling here's a relevant discussion. As a keyboard user I'm pretty fine with that, the only questionable thing for me is how it's rendered in Settings.

So the bigger question is "can we integrate focus organically in UI or that's confusing?" Maybe with a reasonably contrast background highlight somewhere like in this example in Settings.

[WofWca]

I had to implement these using too

What does this mean?

it immediately broke FullscreenMedia and FullscreenAvatar (when you open your profile picture it opens behind profile dialog)

Shouldn't this be fixed by putting FullscreenMedia and FullscreenAvatar inside the dialog element?

Dialog picks an element to focus on when it's opened

How is this a problem? Why do we need to unfocus?

[maxphilippov]

I had to implement these using too

What does this mean? Shouldn't this be fixed by putting FullscreenMedia and FullscreenAvatar inside the dialog element?

Yeah, these are now wrapped inside a special "fullscreen transparent" dialog.

How is this a problem? Why do we need to unfocus?

I'm talking about this image for example.

[maxphilippov]

• [x] tabs are invisible in dark theme

I've actually missed this one, gonna fix a bit later.

[Simon-Laux]

About dialog focus: I think this is also what operating systems and other programs do: they focus a dialog button on confirmation and alert dialogs.

But that is not what we want for dialogs like fullscreenmedia view and settings dialog. There we probably want to focus the entire dialog element instead.

See https://github.com/whatwg/html/wiki/dialog--initial-focus,-a-proposal#use-case-analysis

[maxphilippov]

operating systems and other programs do: they focus a dialog button on confirmation and alert dialogs.

But that is not what we want for dialogs like fullscreenmedia view and settings dialog. There we probably want to focus the entire dialog element instead.

Yeah, but the issue I guess is how to display that. That's why I'm asking if we are willing to integrate that in an UI, cause I can see how surrounding a close button with a thick orange rectangle might not be appealing.

Shouldn't we though focus "close" button in fullscreenmedia if there's only one image and "next/prev" if there are multiple?