Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editing a message that contains just a space throws an error #10358

Open
bwindels opened this issue Jul 17, 2019 · 4 comments

Comments

@bwindels
Copy link
Contributor

commented Jul 17, 2019

part is undefined to be precise

@bwindels bwindels changed the title editing a message that contains just a space throws an error Editing a message that contains just a space throws an error Jul 17, 2019

@zacharystenger

This comment has been minimized.

Copy link

commented Aug 3, 2019

I looked into this issue and found the problem is when you send a message consisting of spaces with Markdown enabled, it creates a message consisting of an empty string. The error can be traced back to the parseAtRoomMentions method in deserialize.js which returns an empty 'parts' array when dealing with an empty string. The findNodeInLineForPart method in caret.js tries to access the first element of this empty array which throws the error.

I would think the best course of action would be to not create the message at all, like what happens if the user tries to submit an empty string. In the handleReturn method of MessageComposerInput.js we could add
if (contentText === '') return true;
underneath the line
contentText = mdWithPills.toPlaintext();
Alternatively, it could ignore the Markdown and create a message consisting of spaces. I've tried out both options and they both work.

It would probably be good to also have more robust error handling in caret.js when it tries to access elements in an array that is empty. I've just begun looking at this code base so any feedback would be great.

@bwindels

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Thank you for your investigation @zacharystenger ! As any client, and not just Riot, could create empty messages, it would probably be good to make the editor more robust. We should probably handle an empty parts array when setting the caret.

Also note that at the moment, we have two different implementations of our message composer. The main one, to send messages, and the one to edit them after they have been sent.

The latter one is quite recent, and I'm about the start replacing the old main composer with the new one, also using it for sending, so a lot of things will be changing in MessageComposerInput.js in the short term.

@bwindels bwindels self-assigned this Aug 6, 2019

@bwindels

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

Changing line 83 in editor/model.js to return new DocumentPosition(-1, 0); fixes it, which makes sense as we shouldn't return invalid positive indices. Fixing this as part of another PR.

@bwindels

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.