1026397 - Should not truncate input string between a surrogate pair at handling maxlength

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Got a cold so that using sick leaves)]

This is a part of bug 670837. At least, we shouldn't separate a surrogate pair at handling maxlength in editor.

Note that the goal of this bug isn't an ideal goal of this issue. For example, coming patch won't care IVS.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Got a cold so that using sick leaves)]

Attachment: part.1 Add tests for inputting surrogate pair character

Simon, this tests must represent what you mentioned in bug 670837 comment 8.

This drops high surrogate if the max length boundary causes separating a surrogate pair.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Got a cold so that using sick leaves)]

Attachment: part.2 Shouldn't separate input string between a surrogate pair at handling maxlength

And the actual logic to drop high surrogate which is remaining by truncating input string in editor.

I'll request review to Ehsan if Simon agree with this behavior.

Comment 3

[(no longer active)]

I think this behavior is fine.

Comment 4

[Simon Montagu :smontagu]

Comment on attachment 8441466 [details] [diff] [review]
part.1 Add tests for inputting surrogate pair character

Review of attachment 8441466 [details] [diff] [review]:
-----------------------------------------------------------------

feedback+ on the behaviour, but I would like the tests better if you used a helper function instead of repeating so much.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Got a cold so that using sick leaves)]

(In reply to Simon Montagu :smontagu from comment #4)
> Comment on attachment 8441466 [details] [diff] [review]
> part.1 Add tests for inputting surrogate pair character
> 
> Review of attachment 8441466 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> feedback+ on the behaviour, but I would like the tests better if you used a
> helper function instead of repeating so much.

Thanks, I'll recreate it.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Got a cold so that using sick leaves)]

Attachment: part.1 Add tests for inputting surrogate pair character

Updated the tests and add some more tests.

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=b349f0e6abf7

Comment 7

[(no longer active)]

Comment on attachment 8441469 [details] [diff] [review]
part.2 Shouldn't separate input string between a surrogate pair at handling maxlength

Review of attachment 8441469 [details] [diff] [review]:
-----------------------------------------------------------------

Also, please update the comment earlier in this function.

::: editor/libeditor/text/nsTextEditRules.cpp
@@ +1249,5 @@
>      {
> +      int32_t oldLength = aOutString->Length();
> +      if (oldLength + resultingDocLength > aMaxLength) {
> +        int32_t newLength = aMaxLength - resultingDocLength;
> +        if (newLength > 0) {

Based on the if condition above, this should always be true, so you can just MOZ_ASSERT it.

@@ +1260,5 @@
> +          }
> +          // XXX What should we do if we're removing IVS and its preceding
> +          //     character won't be removed?
> +        }
> +        MOZ_ASSERT(newLength >= 0);

And then you can remove this MOZ_ASSERT.

Comment 8

[(no longer active)]

Comment on attachment 8442835 [details] [diff] [review]
part.1 Add tests for inputting surrogate pair character

Review of attachment 8442835 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/text/tests/test_bug1026397.html
@@ +18,5 @@
> +<pre id="test">
> +<script type="application/javascript">
> +
> +/** Test for Bug 1026397 **/
> +SimpleTest.expectAssertions(10);

Please annotate this with a bug # about the assertions.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Got a cold so that using sick leaves)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be72c3a82c8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd91cb6f8ee3

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #8)
> Comment on attachment 8442835 [details] [diff] [review]
> part.1 Add tests for inputting surrogate pair character
> 
> Review of attachment 8442835 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: editor/libeditor/text/tests/test_bug1026397.html
> @@ +18,5 @@
> > +<pre id="test">
> > +<script type="application/javascript">
> > +
> > +/** Test for Bug 1026397 **/
> > +SimpleTest.expectAssertions(10);
> 
> Please annotate this with a bug # about the assertions.

Filed bug 1028485. Although, I already have a patch for it, I'd like to create automated tests for it.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/be72c3a82c8b
https://hg.mozilla.org/mozilla-central/rev/bd91cb6f8ee3