67P / hyperchannel

Kosmos Chat for the Web
Mozilla Public License 2.0
20 stars 3 forks source link

UI for joining channels #254

Closed raucao closed 2 years ago

raucao commented 2 years ago

Adds a basic new channel-join dialog with an account switcher and different components based on the chat protocol of the account.

Left to do:

closes #229

raucao commented 2 years ago

OK, we have working join dialogs now (and they're obviously already much nicer than our placeholder prompt).

There's an issue, at least in FF, with the ember modal library preventing tabbing between inputs. I checked the focus-trap lib they use, and I don't see why our other inputs shouldn't be tabbable, especially when specifically adding tabindex attributes.

Other than that, there's one failing integration test, which looks to me like it should be green. And there are no integration tests for the actual join components at the moment.

@galfert Maybe you could have a look at the first two issues with a fresh set of eyes.

galfert commented 2 years ago

There's an issue, at least in FF, with the ember modal library preventing tabbing between inputs. I checked the focus-trap lib they use, and I don't see why our other inputs shouldn't be tabbable, especially when specifically adding tabindex attributes.

That's some weird behaviour. I can only cycle backwards with shift+tab, but not forwards. And only between the select and the input, but not the submit button. When I remove the tabindex attributes, I can also tab to the button, but still only backwards.

raucao commented 2 years ago

That's some weird behaviour. I can only cycle backwards with shift+tab, but not forwards. And only between the select and the input, but not the submit button. When I remove the tabindex attributes, I can also tab to the button, but still only backwards.

Yup, same here. Very unusual. The library is basically destroying perfectly fine browser behavior, by trying to trap focus in the modal window.

galfert commented 2 years ago

There is some discussion about making it configurable to disable focus trap at https://github.com/simplabs/ember-promise-modals/issues/435.

One option would be to fork the library and use the patch from https://github.com/simplabs/ember-promise-modals/issues/435#issuecomment-961771874 until it's supported officially.

raucao commented 2 years ago

One option would be to fork the library and use the patch from simplabs/ember-promise-modals#435 (comment) until it's supported officially.

Yeah, I guess that's the best solution for now. :+1:

raucao commented 2 years ago

So, I forked the library, applied the patch, and removed all tabindex attributes. It's a simple change, and the code is executed as expected. But the result is even weirder now:

comeon

galfert commented 2 years ago

That was an interesting one :)

I added the ember-promise-modal to another project to see how it behaves there. Turns out that even with the focus-trap activated all was working fine and as expected.

So there had to be something different in Hyperchannel itself. After some digging I found the tab keyboard listener for the message field that we use for the username completion.

After changing it to only activate when the message field is focused, all is working fine and dandy now.

I reverted the ember-promise-modals dependency back to the official release and also introduced a {{autofocus}} modifier to focus on the channel name field when opening the dialog.

EDIT: I'll tackle the integration test for the join-channel-irc component next.

raucao commented 2 years ago

Nice catch!

And we can likely make use of the focus trap with the chat input actually, as we will have several more inputs grouped in that component, for attachments and such.

raucao commented 2 years ago

EDIT: I'll tackle the integration test for the join-channel-irc component next.

Maybe do it as a new PR against this one, because the kredits medium label is starting to become a bit small, but I don't feel like it's going to be a large overall...

galfert commented 2 years ago

Maybe do it as a new PR against this one, because the kredits medium label is starting to become a bit small, but I don't feel like it's going to be a large overall...

ok