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

Input field for comments no longer shows warnings about comment length #7414

Closed
danxuliu opened this issue Dec 7, 2017 · 1 comment
Closed

Comments

@danxuliu
Copy link
Member

danxuliu commented Dec 7, 2017

Since the change of the text area to a content editable div the input field for comments no longer shows a warning when nearing or exceeding the maximum allowed comment length.

The problem seems to come from trying to get the div contents still using val(); val() returns the actual contents of the input field only when called on form elements (like input, select or textarea), but not when called on content editable divs. In that later case text() should be used instead.

The unit tests did not catch the problem because even if val() does not return the contents of the element it returns any value previously set through val("whatever"); as the unit tests use val("whatever") and the code under test uses val() the tests passed, masking the undesired use of val() and val("whatever") on a div element.

In other cases (restore original comment when cancelling, do not create a comment with invalid length and do not submit an edited comment with invalid length) the tests failed silently because val("whatever") does not modify the contents of the content editable div. In the first case that makes the test invalid (as nothing was changed and thus the cancellation did nothing regarding the contents), and in the second and third it caused that when the form was submitted the input field was empty, which prevents the comment from being sent.

When fixing the issues above note that the length checks must take into account the length of the message in plain text (that is, with the mention elements replaced by their @ userId equivalent). Calling _commentBodyHTML2Plain each time a new character is typed could be taxing for browsers in low-end systems, so a more advanced approach may be needed (although that is just pure speculation; I have not tested it, and the performance could be fine even in that case).

Finally note that the length checks should be done not only when a character is typed, but also when a mention is autocompleted; this case is probably already supported thanks to the change event being handled too by _onTypeComment, but better verify it to be sure once the other issues are fixed :-)

@danxuliu danxuliu added this to the Nextcloud 13 milestone Dec 7, 2017
rullzer added a commit that referenced this issue Dec 29, 2017
Fixes #7414

Since we no longer use an input field we have to use text instead of
val.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer
Copy link
Member

rullzer commented Dec 29, 2017

PR in #7648

rullzer added a commit that referenced this issue Jan 2, 2018
Fixes #7414

Since we no longer use an input field we have to use text instead of
val.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants