Arrow up and down doesn't work correctly in message field

[kingalg]

Version: 1.1.0-alpha.5 Systems: all

User should be able navigate multi-line message that he is writing with all four arrows. Currently only arrow left and right are working.

[pdurbin]

Related:

• 669

[holmesworcester]

Moving this to current sprint since it's a bugfix and it's done. @josephlacey we can merge this, or you can add a test for it.

[josephlacey]

Moving this to current sprint since it's a bugfix and it's done. @josephlacey we can merge this, or you can add a test for it.

@holmesworcester Best practice is to add a test, but yeah, it is a bug. If the need for a fix outweighs the chance that this will introduce a regression, then we could add a test in the next sprint. Either way.

[holmesworcester]

Okay yeah let's add a test. Sorry!

[kingalg]

Version: 1.2.1-alpha.3

This issue still needs a little polishing. If the user types his message manually arrows are working correctly. However if the message is copy and past to the field up and down arrows move only between first and last letter of the whole message. I've recorded it and what you can see is:

1. At first I'm using up and down arrow wanting to move between lines but as you can see it only moves between the beginning and the end of the message.
2. Then I used left and right arrow to see how up and down arrows will behave if focus is in the middle of the sentence but the result is the same.

https://github.com/TryQuiet/quiet/assets/68909168/18fc4e7b-b2e3-4603-b2c8-ef164c6c54ef

I've checked and on slack it doesn't matter if text is typed or copy-passed, arrows are working correctly so we should aim at the same functionality.

@josephlacey I will move it back to "in progress" column, let me know if you need any additional info from me.

[josephlacey]

This issue still needs a little polishing. If the user types his message manually arrows are working correctly. However if the message is copy and past to the field up and down arrows move only between first and last letter of the whole message. I've recorded it and what you can see is:

1. At first I'm using up and down arrow wanting to move between lines but as you can see it only moves between the beginning and the end of the message.

2. Then I used left and right arrow to see how up and down arrows will behave if focus is in the middle of the sentence but the result is the same.

@kingalg Thanks for the review and feedback! I tracked this copy and paste issue back to the onPaste function here,

packages/desktop/src/renderer/components/widgets/channels/ChannelInput/ChannelInput.tsx#L519C1-L522

The combination of getting plain text from the Clipboard API and then adding that to the DOM with the insertHTML function treats the newline characters from the original text as inline. The traversal code just sees the pasted text as one long string. Adding the text to the DOM with the insertText function unfortunately doesn't help because it adds each line within their own <div>, and the traversal code doesn't see those distinct nodes as a part of the same selection.

Thoughts on what a good solution might be? Could we convert the newlines from the pasted text into <br /> and pass that into the insertHTML function? L520 from the above changed to something like this,

const text = e.clipboardData.getData('text/plain').replace(/(\r\n|\r|\n)/g, '<br />')

[holmesworcester]

My thoughts:

1. That solution sounds fine if it works.
2. How important is it at this point that this is an html area?
3. Maybe we should use a library for this.

GPT has similar suggestions:

There are several ways to address this issue in JavaScript / React. Let's discuss some of them:

1. Manual Parsing and Wrapping: When the text is pasted, you could manually parse the text to identify newline characters and wrap the relevant sections in a block level HTML element, like <p> or <div>. This will allow you to preserve the intended layout of the pasted text. Here's a simple example:

const handlePaste = (event) => {
    event.preventDefault();
    const text = (event.clipboardData || window.clipboardData).getData('text');
    const paragraphs = text.split('\n').map(p => `<p>${p}</p>`).join('');
    document.execCommand('insertHTML', false, paragraphs);
}

2. Use a Rich Text Editor Library: There are several open source libraries available that can handle complex text editing tasks, including handling pasted text in a more sophisticated way. Here are a few options:

   • Draft.js: This is a rich text editor framework for React built by Facebook. It has a lot of flexibility and allows you to build complex text editors with custom behaviors.

   • Quill.js: This is a powerful rich text editor that is built in JavaScript. It has a lot of features out of the box and is highly customizable.

   • Slate.js: Slate is a completely customizable framework for building rich text editors. It's built in React and gives you a high degree of control over the editor's behavior.

Please note that while these libraries can provide a solution to the problem you're facing, they may come with additional complexity and might be overkill if this is the only issue you're trying to solve.

3. Customize the Behavior of the insertHTML function: You could create a customized version of the insertHTML function that has the specific behavior you want. You could do this by creating a function that takes the pasted text, processes it in a specific way (e.g., replacing newline characters with a specific HTML element), and then calls the insertHTML function with the processed text.

4. Textarea Autogrow: Another approach is to use a textarea field instead of a contentEditable field. You can then make the textarea automatically grow and shrink to fit its content, giving a similar user experience to a contentEditable field. There are various libraries available for this, like "react-textarea-autosize".

Remember, each solution has its own trade-offs and you should consider what fits your specific use case best.

[holmesworcester]

Given that the text entry field has given us trouble (including with security issues like #744) and at some point will have complex stuff like @ mentions and autocompletes, it might make sense to use a library here.

[josephlacey]

Given that the text entry field has given us trouble (including with security issues like #744) and at some point will have complex stuff like @ mentions and autocompletes, it might make sense to use a library here.

@holmesworcester It could, definitely. I can't speak to the original choice for using a generic div for the input field. Was closing this issue intentional and related to this comment? If so, cool. If not, I can reopen and push my local changes.

[holmesworcester]

I'm sorry, closing was not intentional!

[holmesworcester]

The original reason for making it an html area was to be able to have @ mentions with some highlight style around that.

Switching to a plain text area is probably best for now if that fixes it.

[josephlacey]

@holmesworcester @kingalg I also found a bug with the handling of empty lines. Fixes pushed for the pasted code problem and the empty lines one.

[holmesworcester]

Oops, just seeing this! In the future if stuff is done you can move it to "waiting for review"

[kingalg]

Hi @josephlacey ! I wasn't here last week. I will check it as soon as changes will get to QA version of the app. Good job with finding a bug with empty lines. What was happening exactly?

[josephlacey]

Oops, just seeing this! In the future if stuff is done you can move it to "waiting for review"

Cool, I keep that in mind in the future.

[josephlacey]

Hi @josephlacey ! I wasn't here last week. I will check it as soon as changes will get to QA version of the app. Good job with finding a bug with empty lines. What was happening exactly?

The simplest way to describe it would be erratic and unexpected up and down navigation. The caret would jump over the empty line, and then place you in a weird location in the DOM that made your next up or down even stranger.

Technically line breaks are DOM nodes, so the original iteration to skip over those nodes would place the caret next to the second line break when there was an empty line. It appeared like you were at the end of a text line, but you were in fact at the beginning line break node. Up or down again, and then you were at the beginning of the previous or next node, which could be text or line break depending on the context. The code needed to check the content of the next two nodes and act accordingly.

I need to submit a new PR with these changes since the original was merged and closed.

[kingalg]

Version: 1.4.0-alpha.1

If the text is copy and passed up and down arrows doesn't work at all for me (MacOS). For the text that is written manually up and down arrow work but in a very unpredictable way. For example they jump to the next paragraph instead of next line.

I'm moving this back to "In progress column".

[josephlacey]

Version: 1.4.0-alpha.1

If the text is copy and passed up and down arrows doesn't work at all for me (MacOS). For the text that is written manually up and down arrow work but in a very unpredictable way. For example they jump to the next paragraph instead of next line.

I'm moving this back to "In progress column".

@kingalg Thanks for the report. Unfortunately I don't have access to MacOS, so I don't have a way to test this directly. In the abstract, I suspect it's one of two things.

It could be MacOS encoded line ending characters, but I thought I'd accounted for with this line.

Or it could be the how the nodeValue empty lines are translated on MacOS.

I've pushed a potential fix for the latter problem, but I have no way to test if this is correct. If not, could you paste the raw HTML here of how the pasted text is handled? That would either eliminate or confirm the issue with the first possibility above.

[holmesworcester]

@josephlacey if it's helpful we have a cloud macOS account you can use

[josephlacey]

@josephlacey if it's helpful we have a cloud macOS account you can use

@holmesworcester If that's a cloud-hosted MacOS version I could test with, I could try it out, yeah.

[holmesworcester]

@holmesworcester If that's a cloud-hosted MacOS version I could test with, I could try it out, yeah.

Sent on Signal. Let me know if it's helpful.

[josephlacey]

I've gotten to the bottom of this most recent issue. It's not just MacOS and seems specific to empty lines, which I spent a good amount of time accommodating. But Typescript's linting is requiring additional narrowing that's causing the code to not work with empty lines. I need to triangulate these two.

[holmesworcester]

Well that's good news you can reproduce this without macOS.

Random question: right now, is the message entry field a contentEditable or a text area? If the former, is there any current functionality that depends on it being contentEditable?

[josephlacey]

Random question: right now, is the message entry field a contentEditable or a text area? If the former, is there any current functionality that depends on it being contentEditable?

contentEditable, and that's what I've been mulling over. I wonder if lots of this custom code and the original problem would go away by switching the containing element to a text area.

[holmesworcester]

I think that's a good question. We originally used contenteditable to highlight mentions with special html, but it was very finicky, we never got it working quite right, and we should probably be using some kind of library for this anyway.

Let's switch to textarea if there aren't any features that depend on contenteditable right now. We could also go looking for a React and React Native library for this field. My memory is that there are some Wysiwyg libraries that handle mentions with auto-completion.

[holmesworcester]

@josephlacey we just discussed this and confirmed that switching this to a textarea is totally appropriate.

One note from the team is that we might need more test coverage before doing this refactor, for things like files, emojis, etc. So go heavy on self-QA and be imaginative about writing more tests to catch any issues here.

[josephlacey]

@holmesworcester I've been working on the replacement here but have narrowed in a strange issue. The up and down arrows also don't work with an HTML textarea (technically MUI's TextField with the multiline attribute), but when I render this same textarea in another generic dev context, they do. I've removed a bunch functionality, like the @ Mention selection, that might be interfering here inadvertently, but still nothing. Is there some other functionality, perhaps higher up in the tree, that might be causing an issue?

[holmesworcester]

At some point we used up-arrow for scrolling up with the keyboard. Were you involved in fixing that?

What's the best way to search a codebase for anything capturing keystrokes?

[josephlacey]

At some point we used up-arrow for scrolling up with the keyboard. Were you involved in fixing that?

What's the best way to search a codebase for anything capturing keystrokes?

Ah, I remember that. There was still code in the related file capturing the up and down arrows, but removing it doesn't solve the problem. I also searched through the rest of the code for 'ArrowUp', 'ArrowDown' and their corresponding codes, 38 and 40, and only found one other place, src/renderer/containers/hooks.ts, but removing the related code doesn't solve the problem either. I'll see if there's other debugging that might be helpful here, but other advice is welcome as well.

[holmesworcester]

@Kacper-RF any ideas? the search bar?

[Kacper-RF]

Maybe custom hook that is responsible for handling keyboard interaction. useCyclingFocus in hooks.ts file

[holmesworcester]

@josephlacey had a chance to try this?

[EmiM]

@josephlacey you can test that easily using devtools. Open EventsListeners section and look at list of listeners for 'keydown'. You can remove listeners one by one and check when arrows start to work again. It seems like (on current develop) the problem is caused by hooks.ts (as Kacper correctly suggested in the comment above).

Commenting line document.addEventListener('keydown', handleKeyDown, false) confirms that.

Note: I probably fixed that here: https://github.com/TryQuiet/quiet/pull/1986/commits/445ad622c09ef07123bd34ec2f2f4a7cbaf897cb

[josephlacey]

@EmiM Very nice, yeah, I'm seeing the copy and paste up and down arrows bug as fixed on Linux.

It also looks like you switch the input field to a textarea rather than a ContextEditable div, which means we can yank out the custom code I wrote. I'll submit a PR for that.

[kingalg]

quiet@2.0.3-alpha.0 System: Windows, Linux, MacOS

With changes made by @EmiM everything seams to be working fine on all systems.