New lines and spaces are miscounted

That's a tough question. Theoretically the maxlength module should try to target more than one WYSIWYG editor. On the other hand, its an edge case usecase probably these days.

Is there ever a use case where HTML, non-breaking-spaces, new-lines or any other invisible characters _should_ be counted?

Also, doesn't the "Safe truncate HTML" option solve this (in some cases - see #2905674: Why is "Safe truncate HTML" option only available when "Force Truncate" is enabled?)?

-mike

This patch does a few things-

1. Accounts for consecutive EOLs (CKEditor adds an additional EOL when pressing return)
2. Replaces hidden return chars with a visible ↩ (for counting purposes)
3. Accounts for multiple instances of  
4. Removes tags with empty spaces (CKEditor does this when adding a new paragraph tag)

Caught an edge case for when there's solely a   in the content area.

#6 works great for me. Let's get it committed!

-mike

Using the patch in #6 with "Force truncate" and "Safe truncate html" enabled, when I hit the maximum number of characters, the "↩"s are displayed and the wysiwyg starts blinking.
If I go back to using "\r" instead, I still get an accurate count without the blinking or hidden character display issues.

I think we might need to introduce a parameter to ml.twochar_lineending that decides whether to replace with `\r` or `\r\n' (as the function originally did) based on field configuration.

`\r` allows the character count to update based on what the user has entered.
`\r\n` ensures that when the maxlength count says that the text is allowed then the PHP validation will also pass.

For example, a plain text field with a maxlength of 5 (in maxlength config and in field storage settings).
If I enter `1234[newline]` and the replacement uses `\r` the maxlength count will say "Content limited to 5 characters, remaining: 0" but PHP validation will fail because it calculates the length as 6 characters.
If I enter `123[newline]` and the replacement uses `\r\n` the maxlength count will say "Content limited to 5 characters, remaining: 0" and PHP validation will pass because they both count the length as 5.

If a decision is to be made one way or the other then I would say it should be `\r\n` because matching the PHP validation is more important. Users seeing an error message "Field: may not be longer than 5 characters." and maxlength saying "Content limited to 5 characters, remaining: 0" is worse than the maxlength count looking wrong and changing by more than 1 after a newline is entered.

This is also what the function name & comment still say even though it doesn't replace with two characters anymore:

  /**
   * Replaces line ending with to chars, because PHP-calculation counts with two chars
   * as two characters.
   *
   * @see http://www.sitepoint.com/blogs/2004/02/16/line-endings-in-javascript/
   */
  ml.twochar_lineending = function(str) {

and was mentioned in rapayanm's comment when this change was introduced: https://www.drupal.org/project/maxlength/issues/2872718#comment-12828366

I've updated the issue title and description for clarity.

re-rolled against 2.0.x#8f09eb0

I've tested this against the current 2.0.x branch and get the following results:

1. Plain text field - works as expected
2. Long Plain text field - works as expected
3. Long Formatted text field - new line counts as 2, but the following character doesn't count so the total count ends up correct
4. Long Formatted text field with summary- summary works as expected, new line counts as 2, but the following character doesn't count so the total count ends up correct

So in summary, I think this solution is acceptable from an end user. It's slightly confusing to have the count jump up by two after a new line. If there is a solution that only increments the count by one upon a new line that's ideal, but not critical (in my opinion). I'm curious what other maintainers say.

As for the question in comment 9, I lean towards agreeing with Mark that replacing lines and spaces with `\r\n`is best, but I won't block the current implementation if other maintainers are ok with it.

Regarding the discussion of \r vs \r\n, you should never use \r, because that doesn't actually represent a new line. You should use either \n or \r\n, and \n is probably better.

https://stackoverflow.com/a/1761086

re-rolled for 2.1.x

Adding the issue that introduced the last replacement to "\r".

What I don't understand, why are we using "\r" as replacement if wherever I check it, should be "\n" or for Win systems "\r\n" pair?

I tested spaces and they are perfectly fine for me over both wysiwyg and none wisywyg as our automated tests show as well, since that part is covered.

I've tested this manually and can confirm that new lines and spaces are being counted properly on CKEditor fields.

Assigning to Heather to do a final code review. If this merge request passes her review, then this can be marked Reviewed and Tested by Community.

@cedewey, @hbrokmeier I think wysiwyg field will be fine because there new lines are often marked by the respective HTML tag <br> or </ br>.
I will have to implement some sort of test for text fields without wsywiyg, so actually the "\r\n" will be used for new lines and line breaks. We have to test the same manually.

Code looks good, and the counting is working correctly for new lines and spaces. There is some inconsistency with how empty lines are counted, but I'm not sure how critical this is. CKE5 counts an empty line as 1 char, CKE4 counts an empty line as 2 chars, and Plain Text fields only count every other empty line.

Hi @joevagyok,

I'm ok with the slight discrepancies in counts between CKEditor 5, CKEditor 4 and plain text. CKEditor 4 is being deprecated and the count difference is marginal.

Marking this RTBC. If any of the maintainers believe this needs more work, please speak up by the end of the week.

Thank you all!